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

[Peter Crosthwaite]

On Tue, May 5, 2015 at 10:43 AM, Richard Henderson <address@hidden> wrote:
> On 05/05/2015 10:19 AM, Peter Maydell wrote:
>> On 5 May 2015 at 05:45, Peter Crosthwaite <address@hidden> wrote:
>>> Add the ARM specific disassembly flags setup, so ARM can be correctly
>>> disassembled from the monitor.
>>>
>>> Signed-off-by: Peter Crosthwaite <address@hidden>
>>> ---
>>>  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.
>

So I have made a start on this. The ARM, MB and CRIS in this patch
series is rather easy. Its X86 im having trouble with but your example
here looks like most of the work ...

> 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 {

So here the monitor is actually using the argument memory-dump size to
dictate the flags. Is this flawed and should we delete this wsize
if-else and rely on the CPU-state driven logic for correct disas info
selection? This wsize reliance seems unique to x86. I think we would
have to give this up in a QOMified approach.

>             /* 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))

This uses env->efer and segs to drive the flags...

>                     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)) {

But your new implementation uses hflags. Are they the same state? I
couldnt find easy correltation between MSR_EFER_LMA and HF_CSXX_SHIFT
(although I do see that map to hflags HF_LMA?).

Is your code a functional change?

>         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.
>

I went for adding print_insn to disassembly_info and passing just that
to the hook. Patches soon! I might leave X86 out for the first spin.

Regards,
Peter

>
>
> r~
>