[PATCH 0/2][4.17] x86emul: string insn corrections

[PATCH 0/2][4.17] x86emul: string insn corrections

[Jan Beulich]

Both partly RFC - see the remarks in the patches themselves.

1: further correct 64-bit mode zero count repeated string insn handling
2: pull permission check ahead for REP INS/OUTS

Jan

[PATCH 1/2][4.17] x86emul: further correct 64-bit mode zero count repeated string insn handling

[Jan Beulich]

In an entirely different context I came across Linux commit 428e3d08574b
("KVM: x86: Fix zero iterations REP-string"), which points out that
we're still doing things wrong: For one, there's no zero-extension at
all on AMD. And then while RCX is zero-extended from 32 bits uniformly
for all string instructions on newer hardware, RSI/RDI are only for MOVS
and STOS on the systems I have access to. (On an old family 0xf system
I've further found that for REP LODS even RCX is not zero-extended.)

Fixes: 79e996a89f69 ("x86emul: correct 64-bit mode repeated string insn handling with zero count")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Partly RFC for none of this being documented anywhere (and it partly
being model specific); inquiry pending.

If I was asked, I would have claimed to have tested all string insns and
for both vendors back at the time. But pretty clearly I didn't, and
instead I did derive uniform behavior from just the MOVS and STOS
observations on just Intel hardware; I'm sorry for that.

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1589,7 +1589,7 @@ static inline void put_loop_count(
         regs->r(cx) = ad_bytes == 4 ? (uint32_t)count : count;
 }
 
-#define get_rep_prefix(using_si, using_di) ({                           \
+#define get_rep_prefix(extend_si, extend_di) ({                         \
     unsigned long max_reps = 1;                                         \
     if ( rep_prefix() )                                                 \
         max_reps = get_loop_count(&_regs, ad_bytes);                    \
@@ -1597,14 +1597,14 @@ static inline void put_loop_count(
     {                                                                   \
         /*                                                              \
          * Skip the instruction if no repetitions are required, but     \
-         * zero extend involved registers first when using 32-bit       \
+         * zero extend relevant registers first when using 32-bit       \
          * addressing in 64-bit mode.                                   \
          */                                                             \
-        if ( mode_64bit() && ad_bytes == 4 )                            \
+        if ( !amd_like(ctxt) && mode_64bit() && ad_bytes == 4 )         \
         {                                                               \
             _regs.r(cx) = 0;                                            \
-            if ( using_si ) _regs.r(si) = _regs.esi;                    \
-            if ( using_di ) _regs.r(di) = _regs.edi;                    \
+            if ( extend_si ) _regs.r(si) = _regs.esi;                   \
+            if ( extend_di ) _regs.r(di) = _regs.edi;                   \
         }                                                               \
         goto complete_insn;                                             \
     }                                                                   \
@@ -4248,7 +4248,7 @@ x86_emulate(
         goto imul;
 
     case 0x6c ... 0x6d: /* ins %dx,%es:%edi */ {
-        unsigned long nr_reps = get_rep_prefix(false, true);
+        unsigned long nr_reps = get_rep_prefix(false, false);
         unsigned int port = _regs.dx;
 
         dst.bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
@@ -4289,7 +4289,7 @@ x86_emulate(
     }
 
     case 0x6e ... 0x6f: /* outs %esi,%dx */ {
-        unsigned long nr_reps = get_rep_prefix(true, false);
+        unsigned long nr_reps = get_rep_prefix(false, false);
         unsigned int port = _regs.dx;
 
         dst.bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
@@ -4630,7 +4630,7 @@ x86_emulate(
     case 0xa6 ... 0xa7: /* cmps */ {
         unsigned long next_eip = _regs.r(ip);
 
-        get_rep_prefix(true, true);
+        get_rep_prefix(false, false);
         src.bytes = dst.bytes = (d & ByteOp) ? 1 : op_bytes;
         if ( (rc = read_ulong(ea.mem.seg, truncate_ea(_regs.r(si)),
                               &dst.val, dst.bytes, ctxt, ops)) ||
@@ -4672,7 +4672,7 @@ x86_emulate(
     }
 
     case 0xac ... 0xad: /* lods */
-        get_rep_prefix(true, false);
+        get_rep_prefix(false, false);
         if ( (rc = read_ulong(ea.mem.seg, truncate_ea(_regs.r(si)),
                               &dst.val, dst.bytes, ctxt, ops)) != 0 )
             goto done;
@@ -4683,7 +4683,7 @@ x86_emulate(
     case 0xae ... 0xaf: /* scas */ {
         unsigned long next_eip = _regs.r(ip);
 
-        get_rep_prefix(false, true);
+        get_rep_prefix(false, false);
         if ( (rc = read_ulong(x86_seg_es, truncate_ea(_regs.r(di)),
                               &dst.val, src.bytes, ctxt, ops)) != 0 )
             goto done;

[PATCH 2/2][4.17] x86emul: pull permission check ahead for REP INS/OUTS

[Jan Beulich]

Based on observations on a fair range of hardware from both primary
vendors even zero-iteration-count instances of these insns perform the
port related permission checking first.

Fixes: fe300600464c ("x86: Fix emulation of REP prefix")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Partly RFC for this not being documented anywhere; inquiry pending.

The referenced commit is still not really the one, but before it REP
handling was so broken that I didn't want to go hunt further.

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -4248,14 +4248,15 @@ x86_emulate(
         goto imul;
 
     case 0x6c ... 0x6d: /* ins %dx,%es:%edi */ {
-        unsigned long nr_reps = get_rep_prefix(false, false);
+        unsigned long nr_reps;
         unsigned int port = _regs.dx;
 
         dst.bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
-        dst.mem.seg = x86_seg_es;
-        dst.mem.off = truncate_ea_and_reps(_regs.r(di), nr_reps, dst.bytes);
         if ( (rc = ioport_access_check(port, dst.bytes, ctxt, ops)) != 0 )
             goto done;
+        nr_reps = get_rep_prefix(false, false);
+        dst.mem.off = truncate_ea_and_reps(_regs.r(di), nr_reps, dst.bytes);
+        dst.mem.seg = x86_seg_es;
         /* Try the presumably most efficient approach first. */
         if ( !ops->rep_ins )
             nr_reps = 1;
@@ -4289,13 +4290,14 @@ x86_emulate(
     }
 
     case 0x6e ... 0x6f: /* outs %esi,%dx */ {
-        unsigned long nr_reps = get_rep_prefix(false, false);
+        unsigned long nr_reps;
         unsigned int port = _regs.dx;
 
         dst.bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
-        ea.mem.off = truncate_ea_and_reps(_regs.r(si), nr_reps, dst.bytes);
         if ( (rc = ioport_access_check(port, dst.bytes, ctxt, ops)) != 0 )
             goto done;
+        nr_reps = get_rep_prefix(false, false);
+        ea.mem.off = truncate_ea_and_reps(_regs.r(si), nr_reps, dst.bytes);
         /* Try the presumably most efficient approach first. */
         if ( !ops->rep_outs )
             nr_reps = 1;

Re: [PATCH 2/2][4.17] x86emul: pull permission check ahead for REP INS/OUTS

[Andrew Cooper]

On 06/10/2022 14:11, Jan Beulich wrote:
> Based on observations on a fair range of hardware from both primary
> vendors even zero-iteration-count instances of these insns perform the
> port related permission checking first.
>
> Fixes: fe300600464c ("x86: Fix emulation of REP prefix")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Partly RFC for this not being documented anywhere; inquiry pending.

Intel do actually document this in two roundabout ways.

1) The order of checks in the pseudocode.  Multiple times in the past,
Intel have said that the order of checks in pseudocode is authoritative.

2) This paragraph I've just found at the end of the INS description.

"These instructions may read from the I/O port without writing to the
memory location if an exception or VM exit occurs due to the write (e.g.
#PF). If this would be problematic, for example because the I/O port
read has side-effects, software should ensure the write to the memory
location does not cause an exception or VM exit."

This makes it clear that the IO port is read before the memory operand
is interpreted.  (As a tangent, while the SDM statement is all true,
it's entirely useless advice for e.g. a migrating VM.)


Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, preferably with
some of ^ discussed in the commit message.

>
> The referenced commit is still not really the one, but before it REP
> handling was so broken that I didn't want to go hunt further.
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -4248,14 +4248,15 @@ x86_emulate(
>          goto imul;
>  
>      case 0x6c ... 0x6d: /* ins %dx,%es:%edi */ {
> -        unsigned long nr_reps = get_rep_prefix(false, false);
> +        unsigned long nr_reps;
>          unsigned int port = _regs.dx;
>  
>          dst.bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
> -        dst.mem.seg = x86_seg_es;
> -        dst.mem.off = truncate_ea_and_reps(_regs.r(di), nr_reps, dst.bytes);
>          if ( (rc = ioport_access_check(port, dst.bytes, ctxt, ops)) != 0 )
>              goto done;
> +        nr_reps = get_rep_prefix(false, false);
> +        dst.mem.off = truncate_ea_and_reps(_regs.r(di), nr_reps, dst.bytes);
> +        dst.mem.seg = x86_seg_es;

As a further observation, both the Intel and AMD manuals elude to the
use of unsegmented memory space for the 64bit forms of these.

However, as both %ds (outs) and %es (ins) ignore their bases in 64bit
mode, I can't think of any practical consequences of conditionally not
using x86_seg_none here.

~Andrew

Re: [PATCH 1/2][4.17] x86emul: further correct 64-bit mode zero count repeated string insn handling

[Andrew Cooper]

On 06/10/2022 14:11, Jan Beulich wrote:
> In an entirely different context I came across Linux commit 428e3d08574b
> ("KVM: x86: Fix zero iterations REP-string"), which points out that
> we're still doing things wrong: For one, there's no zero-extension at
> all on AMD. And then while RCX is zero-extended from 32 bits uniformly
> for all string instructions on newer hardware, RSI/RDI are only for MOVS
> and STOS on the systems I have access to. (On an old family 0xf system
> I've further found that for REP LODS even RCX is not zero-extended.)
>
> Fixes: 79e996a89f69 ("x86emul: correct 64-bit mode repeated string insn handling with zero count")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Partly RFC for none of this being documented anywhere (and it partly
> being model specific); inquiry pending.

None of this surprises me.  The rep instructions have always been
microcoded, and 0 reps is a special case which has been largely ignored
until recently.

I wouldn't be surprised if the behaviour changes with
MISC_ENABLE.FAST_STRINGS (given the KVM commit message) and I also
wouldn't be surprised if it's different between Core and Atom too (given
the Fam 0xf observation).

It's almost worth executing a zero-length rep stub, except that may
potentially go very wrong in certain ecx/rcx cases.

I'm not sure how important these cases are to cover.  Given that they do
differ between vendors and generation, and that their use in compiled
code is not going to consider the registers live after use, is the
complexity really worth it?

~Andrew

Re: [PATCH 2/2][4.17] x86emul: pull permission check ahead for REP INS/OUTS

[Jan Beulich]

On 10.10.2022 20:08, Andrew Cooper wrote:
> On 06/10/2022 14:11, Jan Beulich wrote:
>> Based on observations on a fair range of hardware from both primary
>> vendors even zero-iteration-count instances of these insns perform the
>> port related permission checking first.
>>
>> Fixes: fe300600464c ("x86: Fix emulation of REP prefix")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Partly RFC for this not being documented anywhere; inquiry pending.
> 
> Intel do actually document this in two roundabout ways.
> 
> 1) The order of checks in the pseudocode.  Multiple times in the past,
> Intel have said that the order of checks in pseudocode is authoritative.

Which pseudo code are you referring to here? There's none I could find
for REP, and the INS and OUTS pages doesn't describe this specific
behavior for REP INS / REP OUTS. Instead, if the description of REP
was authoritative, then

WHILE CountReg ≠ 0
DO
...
OD;

would mean the entire INS/OUTS operation is contained in the body of
that loop, leading to no possible exceptions when the count is zero.

> 2) This paragraph I've just found at the end of the INS description.
> 
> "These instructions may read from the I/O port without writing to the
> memory location if an exception or VM exit occurs due to the write (e.g.
> #PF). If this would be problematic, for example because the I/O port
> read has side-effects, software should ensure the write to the memory
> location does not cause an exception or VM exit."
> 
> This makes it clear that the IO port is read before the memory operand
> is interpreted.  (As a tangent, while the SDM statement is all true,
> it's entirely useless advice for e.g. a migrating VM.)

I, too, had noticed that paragraph. But as above it adds no clarity
whatsoever for the count == 0 case.

> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, preferably with
> some of ^ discussed in the commit message.

Thanks, I'll apply this provisionally as I'll need to wait for an ack
from Henry anyway. In the meantime you might clarify whether my
responses above (which mean no further discussion in the description
for there being nothing to refer to) don't find your agreement.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -4248,14 +4248,15 @@ x86_emulate(
>>          goto imul;
>>  
>>      case 0x6c ... 0x6d: /* ins %dx,%es:%edi */ {
>> -        unsigned long nr_reps = get_rep_prefix(false, false);
>> +        unsigned long nr_reps;
>>          unsigned int port = _regs.dx;
>>  
>>          dst.bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
>> -        dst.mem.seg = x86_seg_es;
>> -        dst.mem.off = truncate_ea_and_reps(_regs.r(di), nr_reps, dst.bytes);
>>          if ( (rc = ioport_access_check(port, dst.bytes, ctxt, ops)) != 0 )
>>              goto done;
>> +        nr_reps = get_rep_prefix(false, false);
>> +        dst.mem.off = truncate_ea_and_reps(_regs.r(di), nr_reps, dst.bytes);
>> +        dst.mem.seg = x86_seg_es;
> 
> As a further observation, both the Intel and AMD manuals elude to the
> use of unsegmented memory space for the 64bit forms of these.
> 
> However, as both %ds (outs) and %es (ins) ignore their bases in 64bit
> mode, I can't think of any practical consequences of conditionally not
> using x86_seg_none here.

I find "not using" irritating, but perhaps I'm simply not reading this
the way it was meant. I'm convinced the memory accesses by these insns
are normal ones, so using ES: means linear address unconditionally (for
INS) in 64-bit mode. For OUTS, however, I don't think an FS: or GS:
override would be ignored. The SDM text also doesn't read as if it
would, to me at least. What is it that you have derived your reply
from? "In 64-bit mode, ..., and 64-bit address is specified using RSI
by default" (for OUTS) doesn't say anything about the segment override
being ignored, and earlier text actually talks about the possibility of
an override, without restricting that to any subset of modes.

Jan

Re: [PATCH 1/2][4.17] x86emul: further correct 64-bit mode zero count repeated string insn handling

[Jan Beulich]

On 10.10.2022 20:56, Andrew Cooper wrote:
> On 06/10/2022 14:11, Jan Beulich wrote:
>> In an entirely different context I came across Linux commit 428e3d08574b
>> ("KVM: x86: Fix zero iterations REP-string"), which points out that
>> we're still doing things wrong: For one, there's no zero-extension at
>> all on AMD. And then while RCX is zero-extended from 32 bits uniformly
>> for all string instructions on newer hardware, RSI/RDI are only for MOVS
>> and STOS on the systems I have access to. (On an old family 0xf system
>> I've further found that for REP LODS even RCX is not zero-extended.)
>>
>> Fixes: 79e996a89f69 ("x86emul: correct 64-bit mode repeated string insn handling with zero count")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Partly RFC for none of this being documented anywhere (and it partly
>> being model specific); inquiry pending.
> 
> None of this surprises me.  The rep instructions have always been
> microcoded, and 0 reps is a special case which has been largely ignored
> until recently.
> 
> I wouldn't be surprised if the behaviour changes with
> MISC_ENABLE.FAST_STRINGS (given the KVM commit message) and I also
> wouldn't be surprised if it's different between Core and Atom too (given
> the Fam 0xf observation).
> 
> It's almost worth executing a zero-length rep stub, except that may
> potentially go very wrong in certain ecx/rcx cases.
> 
> I'm not sure how important these cases are to cover.  Given that they do
> differ between vendors and generation, and that their use in compiled
> code is not going to consider the registers live after use, is the
> complexity really worth it?

By "complexity", what do you mean? The patch doesn't add new complexity,
it only converts "true" to "false" in several places, plus it updates a
comment. I don't think we can legitimately simplify things (by removing
logic), so the only thing I can think of is your thought towards
executing a zero-length REP stub (which you say may be problematic in
certain cases). Patch 2 makes clear why this wouldn't be a good idea
for INS and OUTS. It also cannot possibly be got right when emulating
16-bit code (without switching to a 16-bit code segment), and it's
uncertain whether a 32-bit address size override would actually yield
the same behavior as a native address size operation in 32-bit code.
Of course, if limiting this (the way we currently do) to just 32-bit
addressing in 64-bit mode, then this ought to be representative (with
the INS/OUTS caveat remaining), but - as you say - adding complexity
for likely little gain.

Jan

RE: [PATCH 2/2][4.17] x86emul: pull permission check ahead for REP INS/OUTS

[Henry Wang]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Subject: Re: [PATCH 2/2][4.17] x86emul: pull permission check ahead for REP
> INS/OUTS
> 
> On 10.10.2022 20:08, Andrew Cooper wrote:
> > On 06/10/2022 14:11, Jan Beulich wrote:
> >> Based on observations on a fair range of hardware from both primary
> >> vendors even zero-iteration-count instances of these insns perform the
> >> port related permission checking first.
> >>
> >> Fixes: fe300600464c ("x86: Fix emulation of REP prefix")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, preferably
> with
> > some of ^ discussed in the commit message.
> 
> Thanks, I'll apply this provisionally as I'll need to wait for an ack
> from Henry anyway.

Sorry I was actually waiting for the review/ack in the Patch#1 of this series
so that I can ack them together after they are properly reviewed...

> In the meantime you might clarify whether my
> responses above (which mean no further discussion in the description
> for there being nothing to refer to) don't find your agreement.

...Since IIUC this patch is also trying to harden the code, so as long as you
and Andrew reach the agreement of this patch, you can have my:

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

Re: [PATCH 1/2][4.17] x86emul: further correct 64-bit mode zero count repeated string insn handling

[Jan Beulich]

On 10.10.2022 20:56, Andrew Cooper wrote:
> On 06/10/2022 14:11, Jan Beulich wrote:
>> In an entirely different context I came across Linux commit 428e3d08574b
>> ("KVM: x86: Fix zero iterations REP-string"), which points out that
>> we're still doing things wrong: For one, there's no zero-extension at
>> all on AMD. And then while RCX is zero-extended from 32 bits uniformly
>> for all string instructions on newer hardware, RSI/RDI are only for MOVS
>> and STOS on the systems I have access to. (On an old family 0xf system
>> I've further found that for REP LODS even RCX is not zero-extended.)
>>
>> Fixes: 79e996a89f69 ("x86emul: correct 64-bit mode repeated string insn handling with zero count")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Partly RFC for none of this being documented anywhere (and it partly
>> being model specific); inquiry pending.
> 
> None of this surprises me.  The rep instructions have always been
> microcoded, and 0 reps is a special case which has been largely ignored
> until recently.
> 
> I wouldn't be surprised if the behaviour changes with
> MISC_ENABLE.FAST_STRINGS (given the KVM commit message)

I've tried this on a Skylake, and things don't change there when forcing
the MSR bit off.

Jan

> and I also
> wouldn't be surprised if it's different between Core and Atom too (given
> the Fam 0xf observation).
> 
> It's almost worth executing a zero-length rep stub, except that may
> potentially go very wrong in certain ecx/rcx cases.
> 
> I'm not sure how important these cases are to cover.  Given that they do
> differ between vendors and generation, and that their use in compiled
> code is not going to consider the registers live after use, is the
> complexity really worth it?
> 
> ~Andrew