x86emul: correct LFS et al handling for 64-bit mode

Commit Message

Jan Beulich Dec. 11, 2019, 9:28 a.m. UTC

AMD and friends explicitly specify that 64-bit operands aren't possible
for these insns. Nevertheless REX.W isn't fully ignored: It still
cancels a possible operand size override (0x66). Intel otoh explicitly
provides for 64-bit operands on the respective insn page of the SDM.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Andrew Cooper Dec. 11, 2019, 8:57 p.m. UTC | #1

On 11/12/2019 09:28, Jan Beulich wrote:
> AMD and friends explicitly specify that 64-bit operands aren't possible
> for these insns. Nevertheless REX.W isn't fully ignored: It still
> cancels a possible operand size override (0x66). Intel otoh explicitly
> provides for 64-bit operands on the respective insn page of the SDM.
>
> Signed-off-by: Jan Beulich <[e-mail]>

It is definitely more than just these.  Near jumps have per-vendor
behaviour on how long the instruction is, whereas far jump/calls are in
the same category as these by the looks of things.

~Andrew

>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -2640,6 +2640,15 @@ x86_decode_twobyte(
>          }
>          break;
>  
> +    case 0xb2: /* lss */
> +    case 0xb4: /* lfs */
> +    case 0xb5: /* lgs */
> +        /* REX.W ignored on a vendor-dependent basis. */
> +        if ( op_bytes == 8 &&
> +             (ctxt->cpuid->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
> +            op_bytes = 4;
> +        break;
> +
>      case 0xb8: /* jmpe / popcnt */
>          if ( rep_prefix() )
>              ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);

Jan Beulich Dec. 12, 2019, 10:11 a.m. UTC | #2

On 11.12.2019 21:57, Andrew Cooper wrote:
> On 11/12/2019 09:28, Jan Beulich wrote:
>> AMD and friends explicitly specify that 64-bit operands aren't possible
>> for these insns. Nevertheless REX.W isn't fully ignored: It still
>> cancels a possible operand size override (0x66). Intel otoh explicitly
>> provides for 64-bit operands on the respective insn page of the SDM.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
> 
> It is definitely more than just these.  Near jumps have per-vendor
> behaviour on how long the instruction is, whereas far jump/calls are in
> the same category as these by the looks of things.

But you don't expect me to fold all of these into one patch, do
you? We have _some_ vendor dependent behavior already, and I'm
slowly adding to it. Our far call/jmp support is rather
incomplete in other ways anyway. (As an aside, I'm trying to do
the binutils side of some of these before wanting to do the
emulator parts. I'm only about to get to the far call/jmp issues
there, as reported by you in bug 24546.)

I'd appreciate if you could clarify what exactly your reply
means for the patch at hand.

Jan

Andrew Cooper Dec. 12, 2019, 11:37 a.m. UTC | #3

On 12/12/2019 10:11, Jan Beulich wrote:
> On 11.12.2019 21:57, Andrew Cooper wrote:
>> On 11/12/2019 09:28, Jan Beulich wrote:
>>> AMD and friends explicitly specify that 64-bit operands aren't possible
>>> for these insns. Nevertheless REX.W isn't fully ignored: It still
>>> cancels a possible operand size override (0x66). Intel otoh explicitly
>>> provides for 64-bit operands on the respective insn page of the SDM.
>>>
>>> Signed-off-by: Jan Beulich <[e-mail]>
>> It is definitely more than just these.  Near jumps have per-vendor
>> behaviour on how long the instruction is, whereas far jump/calls are in
>> the same category as these by the looks of things.
> But you don't expect me to fold all of these into one patch, do
> you?

short jmp certainly not, but far jmp/call is just two extra case
statements in this new code block, no?

> We have _some_ vendor dependent behavior already, and I'm
> slowly adding to it. Our far call/jmp support is rather
> incomplete in other ways anyway.

There is different truncation behaviour for %rip which needs altering,
but that is a separate area of code.  Anything else?

~Andrew

Jan Beulich Dec. 12, 2019, 1:05 p.m. UTC | #4

On 12.12.2019 12:37, Andrew Cooper wrote:
> On 12/12/2019 10:11, Jan Beulich wrote:
>> On 11.12.2019 21:57, Andrew Cooper wrote:
>>> On 11/12/2019 09:28, Jan Beulich wrote:
>>>> AMD and friends explicitly specify that 64-bit operands aren't possible
>>>> for these insns. Nevertheless REX.W isn't fully ignored: It still
>>>> cancels a possible operand size override (0x66). Intel otoh explicitly
>>>> provides for 64-bit operands on the respective insn page of the SDM.
>>>>
>>>> Signed-off-by: Jan Beulich <[e-mail]>
>>> It is definitely more than just these.  Near jumps have per-vendor
>>> behaviour on how long the instruction is, whereas far jump/calls are in
>>> the same category as these by the looks of things.
>> But you don't expect me to fold all of these into one patch, do
>> you?
> 
> short jmp certainly not, but far jmp/call is just two extra case
> statements in this new code block, no?

Not exactly (the other change would need to be in
x86_decode_onebyte() and depend on ModRM.reg), but yes, I can
do this. Yet then it would feel odd to not also deal with the
near counterparts at the same time. Which in turn would make
is desirable to also deal with near RET as well. At which
point we're about to also discuss CALL/JMP with displacement
operands and Jcc.

>> We have _some_ vendor dependent behavior already, and I'm
>> slowly adding to it. Our far call/jmp support is rather
>> incomplete in other ways anyway.
> 
> There is different truncation behaviour for %rip which needs altering,
> but that is a separate area of code.  Anything else?

protmode_load_seg() and MOVSXD already have vendor dependent
code, if that was your question. For things needing doing see
above plus LOOP, J[ER]CXZ, SYSENTER, and SYSEXIT as far as I'm
currently aware.

Jan

Andrew Cooper Dec. 12, 2019, 2:20 p.m. UTC | #5

On 12/12/2019 13:05, Jan Beulich wrote:
> On 12.12.2019 12:37, Andrew Cooper wrote:
>> On 12/12/2019 10:11, Jan Beulich wrote:
>>> On 11.12.2019 21:57, Andrew Cooper wrote:
>>>> On 11/12/2019 09:28, Jan Beulich wrote:
>>>>> AMD and friends explicitly specify that 64-bit operands aren't possible
>>>>> for these insns. Nevertheless REX.W isn't fully ignored: It still
>>>>> cancels a possible operand size override (0x66). Intel otoh explicitly
>>>>> provides for 64-bit operands on the respective insn page of the SDM.
>>>>>
>>>>> Signed-off-by: Jan Beulich <[e-mail]>
>>>> It is definitely more than just these.  Near jumps have per-vendor
>>>> behaviour on how long the instruction is, whereas far jump/calls are in
>>>> the same category as these by the looks of things.
>>> But you don't expect me to fold all of these into one patch, do
>>> you?
>> short jmp certainly not, but far jmp/call is just two extra case
>> statements in this new code block, no?
> Not exactly (the other change would need to be in
> x86_decode_onebyte() and depend on ModRM.reg), but yes, I can
> do this. Yet then it would feel odd to not also deal with the
> near counterparts at the same time. Which in turn would make
> is desirable to also deal with near RET as well. At which
> point we're about to also discuss CALL/JMP with displacement
> operands and Jcc.
>
>>> We have _some_ vendor dependent behavior already, and I'm
>>> slowly adding to it. Our far call/jmp support is rather
>>> incomplete in other ways anyway.
>> There is different truncation behaviour for %rip which needs altering,
>> but that is a separate area of code.  Anything else?
> protmode_load_seg() and MOVSXD already have vendor dependent
> code, if that was your question.

I was actually just asking about far jmp/call.

If you're sure that far jmp/call is more complicated than just tweaking
this patch, then fine.  Acked-by: Andrew Cooper <[e-mail]>

> For things needing doing see
> above plus LOOP, J[ER]CXZ, SYSENTER, and SYSEXIT as far as I'm
> currently aware.

SYSCALL and SYSRET as well.  The way they handle MSR_STAR is vendor
specific, as well as #UD conditions.

I've just noticed that I've still got an XSA-204 followup patch still
outstanding from 2016...

~Andrew

Jan Beulich Dec. 12, 2019, 3:55 p.m. UTC | #6

On 12.12.2019 15:20, Andrew Cooper wrote:
> On 12/12/2019 13:05, Jan Beulich wrote:
>> On 12.12.2019 12:37, Andrew Cooper wrote:
>>> On 12/12/2019 10:11, Jan Beulich wrote:
>>>> On 11.12.2019 21:57, Andrew Cooper wrote:
>>>>> On 11/12/2019 09:28, Jan Beulich wrote:
>>>>>> AMD and friends explicitly specify that 64-bit operands aren't possible
>>>>>> for these insns. Nevertheless REX.W isn't fully ignored: It still
>>>>>> cancels a possible operand size override (0x66). Intel otoh explicitly
>>>>>> provides for 64-bit operands on the respective insn page of the SDM.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <[e-mail]>
>>>>> It is definitely more than just these.  Near jumps have per-vendor
>>>>> behaviour on how long the instruction is, whereas far jump/calls are in
>>>>> the same category as these by the looks of things.
>>>> But you don't expect me to fold all of these into one patch, do
>>>> you?
>>> short jmp certainly not, but far jmp/call is just two extra case
>>> statements in this new code block, no?
>> Not exactly (the other change would need to be in
>> x86_decode_onebyte() and depend on ModRM.reg), but yes, I can
>> do this. Yet then it would feel odd to not also deal with the
>> near counterparts at the same time. Which in turn would make
>> is desirable to also deal with near RET as well. At which
>> point we're about to also discuss CALL/JMP with displacement
>> operands and Jcc.
>>
>>>> We have _some_ vendor dependent behavior already, and I'm
>>>> slowly adding to it. Our far call/jmp support is rather
>>>> incomplete in other ways anyway.
>>> There is different truncation behaviour for %rip which needs altering,
>>> but that is a separate area of code.  Anything else?
>> protmode_load_seg() and MOVSXD already have vendor dependent
>> code, if that was your question.
> 
> I was actually just asking about far jmp/call.
> 
> If you're sure that far jmp/call is more complicated than just tweaking
> this patch, then fine.  Acked-by: Andrew Cooper <[e-mail]>

Thanks much. I'll see to put together the far branch counterpart
soon. Perhaps I can also do the near branch parts then.

>> For things needing doing see
>> above plus LOOP, J[ER]CXZ, SYSENTER, and SYSEXIT as far as I'm
>> currently aware.
> 
> SYSCALL and SYSRET as well.  The way they handle MSR_STAR is vendor
> specific, as well as #UD conditions.
> 
> I've just noticed that I've still got an XSA-204 followup patch still
> outstanding from 2016...

Oh. Looking forward to see it.

Jan

Patch

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2640,6 +2640,15 @@  x86_decode_twobyte(
         }
         break;
 
+    case 0xb2: /* lss */
+    case 0xb4: /* lfs */
+    case 0xb5: /* lgs */
+        /* REX.W ignored on a vendor-dependent basis. */
+        if ( op_bytes == 8 &&
+             (ctxt->cpuid->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
+            op_bytes = 4;
+        break;
+
     case 0xb8: /* jmpe / popcnt */
         if ( rep_prefix() )
             ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);