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

[Richard Henderson <rth@twiddle.net>]

On 05/05/2015 11:57 PM, Peter Crosthwaite wrote:
> 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.

Hmm.  I don't think that I've ever noticed the monitor disassembly could do
that.  If I were going to do that kind of debugging I certainly wouldn't use
the monitor -- I'd use gdb.  ;-)

If someone thinks we ought to keep that feature, speak now...

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

It's not intended to be.  Since I couldn't find where wsize was initialized, I
pulled the tests used by target-i386/translator.c, for dc->code32 and
dc->code64, since I knew where to find them right away.  ;-)

Without going back to the manuals, I don't know the difference between CS64 and
LMA; from the code it appears only the behaviour of sysret, which seems surprising.

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

Sounds good.


r~