Re: [Qemu-devel] [PATCH 7/7] disas: arm: Use target_disas impl for monitor

[Peter Crosthwaite]

On Tue, May 5, 2015 at 7:38 AM, Claudio Fontana
<address@hidden> wrote:
> Hello Peter,
>
> On 05.05.2015 06:45, Peter Crosthwaite wrote:
>> As it is more fully featured. It has multi-endian, thumb and AArch64
>> support whereas the existing monitor disas support only has vanilla
>> AA32 support.
>>
>> E.G. Running an AA64 linux kernel the follow -d in_asm disas happens
>> (taget_disas()):
>>
>> IN:
>> 0x0000000040000000:  580000c0      ldr x0, pc+24 (addr 0x40000018)
>> 0x0000000040000004:  aa1f03e1      mov x1, xzr
>>
>> However before this patch, disasing the same from the monitor:
>>
>> (qemu) xp/i 0x40000000
>> 0x0000000040000000:  580000c0      stmdapl  r0, {r6, r7}
>>
>> After this patch:
>> (qemu) xp/i 0x40000000
>> 0x0000000040000000:  580000c0      ldr x0, pc+24 (addr 0x40000018)
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>>  disas.c | 48 +++++++++++++++++++++++-------------------------
>>  1 file changed, 23 insertions(+), 25 deletions(-)
>>
>> diff --git a/disas.c b/disas.c
>> index 498b05f..e1da40d 100644
>> --- a/disas.c
>> +++ b/disas.c
>> @@ -208,6 +208,27 @@ target_disas_set_info(int (**print_insn)(bfd_vma pc, 
>> disassemble_info *info),
>>          s->info.mach = bfd_mach_i386_i386;
>>      }
>>      *print_insn = print_insn_i386;
>> +#elif defined(TARGET_ARM)
>> +    if (flags & 4) {
>> +        /* We might not be compiled with the A64 disassembler
>> +        * because it needs a C++ compiler; in that case we will
>
> nit: misaligned * (add one space)
>

Will fix.

> for all the rest, I didn't notice any problems.
>

Thanks for the test.

> Regarding the libvixl output, I still have a lot of unimplemented 
> instructions..
> is it possible to improve libvixl to dissect system instructions?
> I guess this question is more for the libvixl project itself.
>

So my project (and coding time) is currently going is a different
direction. My main motivation here is to QOMify the disas code path to
remove the target specific compilation of the disaser. This series is
the minimal cleanup and functional changes before diving into the
bigger refactor.

Is the GSOC event happening again and should we list this as a project?

Regards,
Peter

> Looking at my disassembly I just tested I have:
>
> 0x00000000400d08a0:   d50342df      unimplemented (System)
> 0x00000000400d08a4:   d5033fdf      isb
> 0x00000000400d08a8:   d503207f      unimplemented (System)
> 0x00000000400d08ac:   d503207f      unimplemented (System)
>
> Tested-by: Claudio Fontana <address@hidden>
> Reviewed-by: Claudio Fontana <address@hidden>
>
>> +         * fall through to the default print_insn_od case.
>> +         */
>> +#if defined(CONFIG_ARM_A64_DIS)
>> +        *print_insn = print_insn_arm_a64;
>> +#endif
>> +    } else if (flags & 1) {
>> +        *print_insn = print_insn_thumb1;
>> +    } else {
>> +        *print_insn = print_insn_arm;
>> +    }
>> +    if (flags & 2) {
>> +#ifdef TARGET_WORDS_BIGENDIAN
>> +        s->info.endian = BFD_ENDIAN_LITTLE;
>> +#else
>> +        s->info.endian = BFD_ENDIAN_BIG;
>> +#endif
>> +    }
>>  #elif defined(TARGET_SPARC)
>>      *print_insn = print_insn_sparc;
>>  #ifdef TARGET_SPARC64
>> @@ -271,28 +292,7 @@ void target_disas(FILE *out, CPUArchState *env, 
>> target_ulong code,
>>      s.info.buffer_vma = code;
>>      s.info.buffer_length = size;
>>
>> -#if defined(TARGET_ARM)
>> -    if (flags & 4) {
>> -        /* We might not be compiled with the A64 disassembler
>> -         * because it needs a C++ compiler; in that case we will
>> -         * fall through to the default print_insn_od case.
>> -         */
>> -#if defined(CONFIG_ARM_A64_DIS)
>> -        print_insn = print_insn_arm_a64;
>> -#endif
>> -    } else if (flags & 1) {
>> -        print_insn = print_insn_thumb1;
>> -    } else {
>> -        print_insn = print_insn_arm;
>> -    }
>> -    if (flags & 2) {
>> -#ifdef TARGET_WORDS_BIGENDIAN
>> -        s.info.endian = BFD_ENDIAN_LITTLE;
>> -#else
>> -        s.info.endian = BFD_ENDIAN_BIG;
>> -#endif
>> -    }
>> -#elif defined(TARGET_PPC)
>> +#if defined(TARGET_PPC)
>>      if ((flags >> 16) & 1) {
>>          s.info.endian = BFD_ENDIAN_LITTLE;
>>      }
>> @@ -475,9 +475,7 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
>>
>>      s.info.buffer_vma = pc;
>>
>> -#if defined(TARGET_ARM)
>> -    print_insn = print_insn_arm;
>> -#elif defined(TARGET_ALPHA)
>> +#if defined(TARGET_ALPHA)
>>      print_insn = print_insn_alpha;
>>  #elif defined(TARGET_PPC)
>>      if (flags & 0xFFFF) {
>>
>
>
> --
> Claudio Fontana
> Server Virtualization Architect
> Huawei Technologies Duesseldorf GmbH
> Riesstraße 25 - 80992 München
>
>