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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/08/16 17:07, Jan Beulich wrote:
>>>> 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.

Ok - with a comment, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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