Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics

[Richard Henderson <rth@twiddle.net>]

On 05/05/2015 10:19 AM, Peter Maydell wrote:
> On 5 May 2015 at 05:45, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
>> Add the ARM specific disassembly flags setup, so ARM can be correctly
>> disassembled from the monitor.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>>  monitor.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index d831d98..9d9f1e2 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1217,6 +1217,17 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>          int flags;
>>          flags = 0;
>>          env = mon_get_cpu();
>> +#ifdef TARGET_ARM
>> +        if (env->thumb) {
>> +            flags |= 1;
>> +        }
>> +        if (env->bswap_code) {
>> +            flags |= 2;
>> +        }
>> +        if (env->aarch64) {
>> +            flags |= 4;
>> +        }
>> +#endif
> 
> monitor.c has no business poking around in the CPU state
> internals like this... You probably want a CPU method
> get_disas_flags() or something.
> 
> -- PMM
> 

While this patch set does improve the current dismal state of affairs, I think
the ideal solution is a cpu method that takes care of all the disassembly info
setup.

Indeed, the flags setup becomes less obscure when, instead of

#ifdef TARGET_I386
        if (wsize == 2) {
            flags = 1;
        } else if (wsize == 4) {
            flags = 0;
        } else {
            /* as default we use the current CS size */
            flags = 0;
            if (env) {
#ifdef TARGET_X86_64
                if ((env->efer & MSR_EFER_LMA) &&
                    (env->segs[R_CS].flags & DESC_L_MASK))
                    flags = 2;
                else
#endif
                if (!(env->segs[R_CS].flags & DESC_B_MASK))
                    flags = 1;
            }
        }

in one place and

#if defined(TARGET_I386)
    if (flags == 2) {
        s.info.mach = bfd_mach_x86_64;
    } else if (flags == 1) {
        s.info.mach = bfd_mach_i386_i8086;
    } else {
        s.info.mach = bfd_mach_i386_i386;
    }
    print_insn = print_insn_i386;

in another, we merge the two so that we get

    s.info.mach = bfd_mach_i386_i8086;
    if (env->hflags & (1U << HF_CS32_SHIFT)) {
        s.info.mach = bfd_mach_i386_i386;
    }
#ifdef TARGET_X86_64
    if (env->hflags & (1U << HF_CS64_SHIFT)) {
        s.info.mach = bfd_mach_x86_64;
    }
#endif


I'm not sure what the right interface for this is, exactly.  But I'd think that
the CPUDebug structure would be initialized in the caller -- target or monitor
-- while the mach and whatnot get filled in by the cpu hook.  Maybe even have
the cpu hook return the print_insn function to use.



r~