| Message ID | 57B2F9E402000078001063FD@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@suse.com> 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. ~Andrew
>>> On 16.08.16 at 16:08, <andrew.cooper3@citrix.com> 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@suse.com> > > 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.) Jan
On 16/08/16 15:57, Jan Beulich wrote: >>>> On 16.08.16 at 16:08, <andrew.cooper3@citrix.com> 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@suse.com> >> 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. ~Andrew
>>> On 16.08.16 at 17:11, <andrew.cooper3@citrix.com> wrote: > On 16/08/16 15:57, Jan Beulich wrote: >>>>> On 16.08.16 at 16:08, <andrew.cooper3@citrix.com> 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@suse.com> >>> 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. Jan
>>> On 16.08.16 at 17:23, <JBeulich@suse.com> wrote: >>>> On 16.08.16 at 17:11, <andrew.cooper3@citrix.com> wrote: >> On 16/08/16 15:57, Jan Beulich wrote: >>>>>> On 16.08.16 at 16:08, <andrew.cooper3@citrix.com> 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@suse.com> >>>> 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
On 16/08/16 17:07, Jan Beulich wrote: >>>> On 16.08.16 at 17:23, <JBeulich@suse.com> wrote: >>>>> On 16.08.16 at 17:11, <andrew.cooper3@citrix.com> wrote: >>> On 16/08/16 15:57, Jan Beulich wrote: >>>>>>> On 16.08.16 at 16:08, <andrew.cooper3@citrix.com> 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@suse.com> >>>>> 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@citrix.com>
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1738,7 +1738,7 @@ x86_emulate( case 5: /* imul */ case 6: /* div */ case 7: /* idiv */ - d = (d & (ByteOp | ModRM)) | DstImplicit | SrcMem; + d = (d & (ByteOp | ModRM)) | DstEax | SrcMem; break; } break; @@ -3554,11 +3554,8 @@ x86_emulate( emulate_1op("neg", dst, _regs.eflags); break; case 4: /* mul */ - dst.type = OP_REG; - dst.reg = (unsigned long *)&_regs.eax; - dst.val = *dst.reg; _regs.eflags &= ~(EFLG_OF|EFLG_CF); - switch ( dst.bytes = src.bytes ) + switch ( dst.bytes ) { case 1: dst.val = (uint8_t)dst.val; @@ -3594,10 +3591,6 @@ x86_emulate( } break; case 5: /* imul */ - dst.type = OP_REG; - dst.reg = (unsigned long *)&_regs.eax; - dst.val = *dst.reg; - dst.bytes = src.bytes; imul: _regs.eflags &= ~(EFLG_OF|EFLG_CF); switch ( dst.bytes ) @@ -3639,9 +3632,7 @@ x86_emulate( } break; case 6: /* div */ - dst.type = OP_REG; - dst.reg = (unsigned long *)&_regs.eax; - switch ( dst.bytes = src.bytes ) + switch ( src.bytes ) { case 1: u[0] = (uint16_t)_regs.eax; @@ -3686,9 +3677,7 @@ x86_emulate( } break; case 7: /* idiv */ - dst.type = OP_REG; - dst.reg = (unsigned long *)&_regs.eax; - switch ( dst.bytes = src.bytes ) + switch ( src.bytes ) { case 1: u[0] = (int16_t)_regs.eax;
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@suse.com> x86emul: use DstEax also for {,I}{MUL,DIV} 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@suse.com> --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1738,7 +1738,7 @@ x86_emulate( case 5: /* imul */ case 6: /* div */ case 7: /* idiv */ - d = (d & (ByteOp | ModRM)) | DstImplicit | SrcMem; + d = (d & (ByteOp | ModRM)) | DstEax | SrcMem; break; } break; @@ -3554,11 +3554,8 @@ x86_emulate( emulate_1op("neg", dst, _regs.eflags); break; case 4: /* mul */ - dst.type = OP_REG; - dst.reg = (unsigned long *)&_regs.eax; - dst.val = *dst.reg; _regs.eflags &= ~(EFLG_OF|EFLG_CF); - switch ( dst.bytes = src.bytes ) + switch ( dst.bytes ) { case 1: dst.val = (uint8_t)dst.val; @@ -3594,10 +3591,6 @@ x86_emulate( } break; case 5: /* imul */ - dst.type = OP_REG; - dst.reg = (unsigned long *)&_regs.eax; - dst.val = *dst.reg; - dst.bytes = src.bytes; imul: _regs.eflags &= ~(EFLG_OF|EFLG_CF); switch ( dst.bytes ) @@ -3639,9 +3632,7 @@ x86_emulate( } break; case 6: /* div */ - dst.type = OP_REG; - dst.reg = (unsigned long *)&_regs.eax; - switch ( dst.bytes = src.bytes ) + switch ( src.bytes ) { case 1: u[0] = (uint16_t)_regs.eax; @@ -3686,9 +3677,7 @@ x86_emulate( } break; case 7: /* idiv */ - dst.type = OP_REG; - dst.reg = (unsigned long *)&_regs.eax; - switch ( dst.bytes = src.bytes ) + switch ( src.bytes ) { case 1: u[0] = (int16_t)_regs.eax;