(removing Paolo from CC as agreed with him)
On 24.05.2013 10:51, Claudio Fontana wrote:
> On 23.05.2013 18:39, Peter Maydell wrote:
>> On 23 May 2013 09:18, Claudio Fontana <claudio.font...@huawei.com> wrote:
>>>
>>> add preliminary support for TCG target aarch64.
>>
>> Richard's handling the technical bits of the review, so
>> just some minor style nits here.
>>
>> I tested this on the foundation model and was able to boot
>> a 32-bit-ARM kernel.
>>
>>> +static inline void reloc_pc19(void *code_ptr, tcg_target_long target)
>>> +{
>>> + tcg_target_long offset; uint32_t insn;
>>> + offset = (target - (tcg_target_long)code_ptr) / 4;
>>> + offset &= 0x07ffff;
>>> + /* read instruction, mask away previous PC_REL19 parameter contents,
>>> + set the proper offset, then write back the instruction. */
>>> + insn = *(uint32_t *)code_ptr;
>>> + insn = (insn & 0xff00001f) | offset << 5; /* lower 5 bits = condition
>>> */
>>
>> You can say
>> insn = deposit32(insn, 5, 19, offset);
>> here rather than doing
>> offset &= 0x07ffff;
>> insn = (insn & 0xff00001f) | offset << 5;
>>
>> (might as well also use deposit32 for consistency in the pc26 function.)
>
> Ok, I'll make use of it.
>
>>> +static inline enum aarch64_ldst_op_data
>>> +aarch64_ldst_get_data(TCGOpcode tcg_op)
>>> +{
>>> + switch (tcg_op) {
>>> + case INDEX_op_ld8u_i32: case INDEX_op_ld8s_i32:
>>> + case INDEX_op_ld8u_i64: case INDEX_op_ld8s_i64:
>>> + case INDEX_op_st8_i32: case INDEX_op_st8_i64:
>>
>> One case per line, please (here and elsewhere).
>
> Will comply.
>
>>> +static inline void tcg_out_call(TCGContext *s, tcg_target_long target)
>>> +{
>>> + tcg_target_long offset;
>>> +
>>> + offset = (target - (tcg_target_long)s->code_ptr) / 4;
>>> +
>>> + if (offset <= -0x02000000 || offset >= 0x02000000) { /* out of 26bit
>>> rng */
>>> + tcg_out_movi64(s, TCG_REG_TMP, target);
>>> + tcg_out_callr(s, TCG_REG_TMP);
>>> +
>>
>> Stray blank line.
>
> I should remove this \n I assume. Ok.
>
>>> + case INDEX_op_mov_i64: ext = 1;
>>
>> Please don't put code on the same line as a case statement.
>> Also fall-through cases should have an explicit /* fall through */
>> comment (except in the case where there is no code at all
>> between one case statement and the next).
Would it be acceptable to put a comment at the beginning of the function
describing ext use, to avoiding a series of /* fall through */ comments?
Like this:
/* ext will be set in the switch below, which will fall through
to the common code. It triggers the use of extended registers
where appropriate. */
and then going:
case INDEX_op_something_64:
ext = 1;
case INDEX_op_something_32:
the_actual_meat(s, ext, ...);
break;
>
> Will change for the next version.
>
>> thanks
>> -- PMM
>>
Claudio
