Re: [Qemu-devel] [PATCH v2 41/60] target-i386: Create gen_lea_v_seg

[Peter Maydell]

On 27 December 2013 14:49, Richard Henderson <address@hidden> wrote:
> On 12/26/2013 01:27 PM, Peter Maydell wrote:
>>> The only time s->addseg will be false in 16-bit mode is during translation 
>>> of
>>> LEA.  I do need the addseg check there for LEA cleanup, but this change 
>>> should
>>> not affect gen_string_movl.
>>
>> Oh, is that the bit that does:
>>
>>         val = s->addseg;
>>         s->addseg = 0;
>>         gen_lea_modrm(env, s, modrm);
>>         s->addseg = val;
>>
>> ? I think we should get rid of that -- s->addseg should always
>> mean "we can do the segment-base-is-zero optimization",
>> it shouldn't be a secret hidden parameter to the gen_lea functions
>> saying "don't do this addition".
>
> Perhaps.  I'd rather do that as follow-up, since lea_v_seg isn't the top-level
> function called for LEA.  And if we clean up addseg, we might as well clean up
> popl_esp_hack as well.
>
> How about a comment for now?

Yes, a comment is fine for now.

>> More generally, it's pretty unclear to me why we're handling
>> "use default segment register for instruction" (ie ovr_seg == -1)
>> differently for the three cases.
>
> Because the handling of segments is fundamentally different in each of the
> three cpu modes?

Well, yes, but I've been unable to match up what the
code is doing differently in the three cases with what the
Intel manual says about them. For instance the manual says
that segmentation is mostly disabled in 64 bit mode, but although
the function has a conditional on CODE64 it doesn't seem to
be suppressing the add for CS/DS/ES/SS base as I would
expect it to. Perhaps an explanatory comment with a reference
to the relevant sections of the intel docs would help?

Some of this seems to be because the code is tangling up a
bunch of things at once (like the lea backdooring of addseg
for 16 bit mode).

>> Why is it OK to skip the addition of the base address for ES
>> (in the movl_A0_EDI case) when the comment for addseg says
>> it only applies to CS/DS/ES?
>
> Err... when are we skipping the addition in gen_string_movl_A0_EDI?  We pass 
> in
> R_ES as the segment register to use...

Yeah, that was a confusion on my part.

thanks
-- PMM