Re: [Xen-devel] [PATCH 1/3] x86emul: use DstEax also for {, I}{MUL, DIV}

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.08.16 at 17:23, <JBeulich@xxxxxxxx> wrote:
>>>> On 16.08.16 at 17:11, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 16/08/16 15:57, Jan Beulich wrote:
>>>>>> On 16.08.16 at 16:08, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 16/08/16 10:32, Jan Beulich wrote:
>>>>> Just like said in commit c0bc0adf24 ("x86emul: use DstEax where
>>>>> possible"): While it avoids just a few instructions, we should
>>>>> nevertheless make use of generic code as much as possible.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> This does reduce the amount of code, but it isn't strictly true.  The
>>>> mul and div instructions are DstEaxEdx, as are a number of other
>>>> instructions.
>>>>
>>>> We shouldn't end up with special casing the eax part because we have an
>>>> easy literal for it, but leaving the edx hard coded because that is
>>>> easier to express in the current code.
>>> I think the code reduction is nevertheless worth it, and reduction
>>> here can only help readability imo. Would you be okay if I added
>>> a comment to the place where the DstEax gets set here? (Note
>>> that DstEdxEax wouldn't be true for 8-bit operations, so I'd rather
>>> not use this as another alias or even a completely new operand
>>> kind description. And please also remember that the tables don't
>>> express all operands in all cases anyway - just consider
>>> SHLD/SHRD.)
>> 
>> The other option would be to use DstNone and explicitly fill in
>> _regs.eax, which avoids all the code to play with dst, and matches how
>> rdtsc/rdmsr/wrmsr currently work.
> 
> Well, that would be more code, but not less of a lie. Or maybe, if
> it we stayed with DstImplicit (as it is without this patch) instead of
> making it DstNone. Let me see how that ends up looking.

Actually - no, we can't do that: The imul case has other imul cases
funneled into it (via the imul: label), and I wouldn't want the mul,
div, and idiv cases be different from the imul one. So I'd really like
to ask you to reconsider whether the patch in its current form
(perhaps with some comment added) isn't acceptable.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel