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~
