Re: [Patch] x86emul: simplify prefix handling for VMFUNC

Re: [Patch] x86emul: simplify prefix handling for VMFUNC

[Lai, Paul]

On Tue, Sep 20, 2016 at 09:50:15AM -0600, Jan Beulich wrote:
> 
> Paul, there's been no reply to
> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00380.html
> 
> Jan
> 

Jan:

The refered to patch, commit a1b1572833, adds a check for vmfunc.
I look a little time to look at the SDM and finally found the reference.
The vmfunc can be found in Table A-6 "Opcode Extensions for One- and Two-
byte Opcodes by Group Number" on page A-18 Vol. 2D of the
 e64-ia-32-architectures-software-developer-manual-325462.pdf.
The values for vmfunc match the values in the code.
I also took the liberty of looking at the other existing cases in the
switch statement, and can find RDTSCP and INVLPG. The CLZERO extension
value is a mystery to me.

I also tried to find the original email referred to by the URL above, but
could not in my mail archives.  Somehow it did not get to me, so I'm replying
to this email (with modified subject line) and Cc'ing Andy, Ravi, and xen-devel.

-Paul

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

Re: [Patch] x86emul: simplify prefix handling for VMFUNC

[Jan Beulich]

>>> On 21.09.16 at 00:35, <paul.c.lai@intel.com> wrote:
> On Tue, Sep 20, 2016 at 09:50:15AM -0600, Jan Beulich wrote:
>> 
>> Paul, there's been no reply to
>> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00380.html 
> 
> The refered to patch, commit a1b1572833, adds a check for vmfunc.
> I look a little time to look at the SDM and finally found the reference.
> The vmfunc can be found in Table A-6 "Opcode Extensions for One- and Two-
> byte Opcodes by Group Number" on page A-18 Vol. 2D of the
>  e64-ia-32-architectures-software-developer-manual-325462.pdf.
> The values for vmfunc match the values in the code.
> I also took the liberty of looking at the other existing cases in the
> switch statement, and can find RDTSCP and INVLPG. The CLZERO extension
> value is a mystery to me.

Well - the question raised was whether the documentation is
perhaps wrong. VMFUNC allowing 66, F2, and F3 prefixes when
other opcodes in its neighborhood (e.g. xsetbv, xtest, xend)
don't seems at least suspicious. Extensions originating from AMD
(rdtscp, clzero) can't be reasonably taken for reference.

Jan


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

Re: [Patch] x86emul: simplify prefix handling for VMFUNC

[Lai, Paul]

On Wed, Sep 21, 2016 at 02:39:58AM -0600, Jan Beulich wrote:
> >>> On 21.09.16 at 00:35, <paul.c.lai@intel.com> wrote:
> > On Tue, Sep 20, 2016 at 09:50:15AM -0600, Jan Beulich wrote:
> >> 
> >> Paul, there's been no reply to
> >> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00380.html 
> > 
> > The refered to patch, commit a1b1572833, adds a check for vmfunc.
> > I look a little time to look at the SDM and finally found the reference.
> > The vmfunc can be found in Table A-6 "Opcode Extensions for One- and Two-
> > byte Opcodes by Group Number" on page A-18 Vol. 2D of the
> >  e64-ia-32-architectures-software-developer-manual-325462.pdf.
> > The values for vmfunc match the values in the code.
> > I also took the liberty of looking at the other existing cases in the
> > switch statement, and can find RDTSCP and INVLPG. The CLZERO extension
> > value is a mystery to me.
> 
> Well - the question raised was whether the documentation is
> perhaps wrong.

VMFUNC allowing 66, F2, and F3 prefixes when
> other opcodes in its neighborhood (e.g. xsetbv, xtest, xend)
> don't seems at least suspicious. 

Thanks for the clearer problem statement. 

> Extensions originating from AMD
> (rdtscp, clzero) can't be reasonably taken for reference.
> 
> Jan
> 

I'll check....


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

Re: [Patch] x86emul: simplify prefix handling for VMFUNC

[Lai, Paul]

On Wed, Sep 21, 2016 at 10:22:32AM -0700, Lai, Paul wrote:
> On Wed, Sep 21, 2016 at 02:39:58AM -0600, Jan Beulich wrote:
> > >>> On 21.09.16 at 00:35, <paul.c.lai@intel.com> wrote:
> > > On Tue, Sep 20, 2016 at 09:50:15AM -0600, Jan Beulich wrote:
> > >> 
> > >> Paul, there's been no reply to
> > >> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00380.html 
> > > 
> > > The refered to patch, commit a1b1572833, adds a check for vmfunc.
> > > I look a little time to look at the SDM and finally found the reference.
> > > The vmfunc can be found in Table A-6 "Opcode Extensions for One- and Two-
> > > byte Opcodes by Group Number" on page A-18 Vol. 2D of the
> > >  e64-ia-32-architectures-software-developer-manual-325462.pdf.
> > > The values for vmfunc match the values in the code.
> > > I also took the liberty of looking at the other existing cases in the
> > > switch statement, and can find RDTSCP and INVLPG. The CLZERO extension
> > > value is a mystery to me.
> > 
> > Well - the question raised was whether the documentation is
> > perhaps wrong.
> 
> VMFUNC allowing 66, F2, and F3 prefixes when
> > other opcodes in its neighborhood (e.g. xsetbv, xtest, xend)
> > don't seems at least suspicious. 
> 
> Thanks for the clearer problem statement. 
> 
> > Extensions originating from AMD
> > (rdtscp, clzero) can't be reasonably taken for reference.
> > 
> > Jan
> > 
> 
> I'll check....
> 
At the bottom of the A-6 Table ("Opcode Extensions for One- and Two- byte
Opcodes by Group Number") there's a footnote that states
   All blanks in all opcode maps are reserved and must not be used.
   Do not depend on the operation of undefined or reserved locations
So, since the "pfx" value for Group 7 opcodes are "blank", none are allowed,
and an "#UD" is expected if a "pfx" is used.

I also checked the narrative descriptions of vmfunc (and similar opcodes,
in particular the list in 25.1.3 "Instructions That Cause VM Exits
Conditionally").  None of the descriptions seem to state explicitly the
expected values of "pfx", nor the behavior of a "bad pfx".  That said, the
table and footnote seem to be the most explicit, cleanest way of communicating
the information.

-Paul


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

Re: [Patch] x86emul: simplify prefix handling for VMFUNC

[Jan Beulich]

>>> On 26.09.16 at 20:13, <paul.c.lai@intel.com> wrote:
> On Wed, Sep 21, 2016 at 10:22:32AM -0700, Lai, Paul wrote:
>> On Wed, Sep 21, 2016 at 02:39:58AM -0600, Jan Beulich wrote:
>> > >>> On 21.09.16 at 00:35, <paul.c.lai@intel.com> wrote:
>> > > On Tue, Sep 20, 2016 at 09:50:15AM -0600, Jan Beulich wrote:
>> > >> 
>> > >> Paul, there's been no reply to
>> > >> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00380.html 
>> > > 
>> > > The refered to patch, commit a1b1572833, adds a check for vmfunc.
>> > > I look a little time to look at the SDM and finally found the reference.
>> > > The vmfunc can be found in Table A-6 "Opcode Extensions for One- and Two-
>> > > byte Opcodes by Group Number" on page A-18 Vol. 2D of the
>> > >  e64-ia-32-architectures-software-developer-manual-325462.pdf.
>> > > The values for vmfunc match the values in the code.
>> > > I also took the liberty of looking at the other existing cases in the
>> > > switch statement, and can find RDTSCP and INVLPG. The CLZERO extension
>> > > value is a mystery to me.
>> > 
>> > Well - the question raised was whether the documentation is
>> > perhaps wrong.
>> 
>> VMFUNC allowing 66, F2, and F3 prefixes when
>> > other opcodes in its neighborhood (e.g. xsetbv, xtest, xend)
>> > don't seems at least suspicious. 
>> 
>> Thanks for the clearer problem statement. 
>> 
>> > Extensions originating from AMD
>> > (rdtscp, clzero) can't be reasonably taken for reference.
>> > 
>> > Jan
>> > 
>> 
>> I'll check....
>> 
> At the bottom of the A-6 Table ("Opcode Extensions for One- and Two- byte
> Opcodes by Group Number") there's a footnote that states
>    All blanks in all opcode maps are reserved and must not be used.
>    Do not depend on the operation of undefined or reserved locations
> So, since the "pfx" value for Group 7 opcodes are "blank", none are allowed,
> and an "#UD" is expected if a "pfx" is used.

This foot note can't possibly apply to the pfx column: Blank entries
there mean at best "no prefix", but certainly not "reserved". Plus
most opcodes allow namely the 66 prefix (as operand size override),
and I'm also sure the F2 and F3 prefixes get ignored for many of
the table entries.

> I also checked the narrative descriptions of vmfunc (and similar opcodes,
> in particular the list in 25.1.3 "Instructions That Cause VM Exits
> Conditionally").  None of the descriptions seem to state explicitly the
> expected values of "pfx", nor the behavior of a "bad pfx".  That said, the
> table and footnote seem to be the most explicit, cleanest way of 
> communicating the information.

As mentioned elsewhere, the examples of other instructions (xsetbv,
xgetbv, xend, xtest) were given because their instruction pages all
mention 66, F2, and F3 as invalid, and they're all in the same group 7
(and all in the same table column). enclu (documented in vol 3) btw
also lists these prefixes as invalid.

Jan


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

Re: [Patch] x86emul: simplify prefix handling for VMFUNC

[Lai, Paul]

On Tue, Sep 27, 2016 at 02:26:00AM -0600, Jan Beulich wrote:
> >>> On 26.09.16 at 20:13, <paul.c.lai@intel.com> wrote:
> > On Wed, Sep 21, 2016 at 10:22:32AM -0700, Lai, Paul wrote:
> >> On Wed, Sep 21, 2016 at 02:39:58AM -0600, Jan Beulich wrote:
> >> > >>> On 21.09.16 at 00:35, <paul.c.lai@intel.com> wrote:
> >> > > On Tue, Sep 20, 2016 at 09:50:15AM -0600, Jan Beulich wrote:
> >> > >> 
> >> > >> Paul, there's been no reply to
> >> > >> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00380.html 
> >> > > 
> >> > > The refered to patch, commit a1b1572833, adds a check for vmfunc.
> >> > > I look a little time to look at the SDM and finally found the reference.
> >> > > The vmfunc can be found in Table A-6 "Opcode Extensions for One- and Two-
> >> > > byte Opcodes by Group Number" on page A-18 Vol. 2D of the
> >> > >  e64-ia-32-architectures-software-developer-manual-325462.pdf.
> >> > > The values for vmfunc match the values in the code.
> >> > > I also took the liberty of looking at the other existing cases in the
> >> > > switch statement, and can find RDTSCP and INVLPG. The CLZERO extension
> >> > > value is a mystery to me.
> >> > 
> >> > Well - the question raised was whether the documentation is
> >> > perhaps wrong.
> >> 
> >> VMFUNC allowing 66, F2, and F3 prefixes when
> >> > other opcodes in its neighborhood (e.g. xsetbv, xtest, xend)
> >> > don't seems at least suspicious. 
> >> 
> >> Thanks for the clearer problem statement. 
> >> 
> >> > Extensions originating from AMD
> >> > (rdtscp, clzero) can't be reasonably taken for reference.
> >> > 
> >> > Jan
> >> > 
> >> 
> >> I'll check....
> >> 
> > At the bottom of the A-6 Table ("Opcode Extensions for One- and Two- byte
> > Opcodes by Group Number") there's a footnote that states
> >    All blanks in all opcode maps are reserved and must not be used.
> >    Do not depend on the operation of undefined or reserved locations
> > So, since the "pfx" value for Group 7 opcodes are "blank", none are allowed,
> > and an "#UD" is expected if a "pfx" is used.
> 
> This foot note can't possibly apply to the pfx column: Blank entries
> there mean at best "no prefix", but certainly not "reserved". Plus
> most opcodes allow namely the 66 prefix (as operand size override),
> and I'm also sure the F2 and F3 prefixes get ignored for many of
> the table entries.

Hi Jan:

My interpretation of the footnote comes from discussions with key people.
This is their understanding of the footnote.

> 
> > I also checked the narrative descriptions of vmfunc (and similar opcodes,
> > in particular the list in 25.1.3 "Instructions That Cause VM Exits
> > Conditionally").  None of the descriptions seem to state explicitly the
> > expected values of "pfx", nor the behavior of a "bad pfx".  That said, the
> > table and footnote seem to be the most explicit, cleanest way of 
> > communicating the information.
> 
> As mentioned elsewhere, the examples of other instructions (xsetbv,
> xgetbv, xend, xtest) were given because their instruction pages all
> mention 66, F2, and F3 as invalid, and they're all in the same group 7
> (and all in the same table column). enclu (documented in vol 3) btw
> also lists these prefixes as invalid.
> 
> Jan

Thanks for the reminder of xsetbv, xgetbv, xend, xtest.  I finally located their
opcode pages in Vol 2 5.2.  The descriptions of these opcodes disallows for
"pfx", which is consistent with the footnote.

Finally found the vmfunc opcode page in Vol 3 30.3, VMX Instruction Reference.
Agreed, there's no mention of prefixes, "pfx", on this page.  It appears
that the other VMX instructions in this section don't mention prefixes either.
Looking at Table A-6 "Opcode Extensions for One- and Two-Byte Opcodes":
vmcall, vmlaunch, vmresume, and vmxoff would need similar updating.

I can ask for updating of the VMX instuctions to include mention of prefixes.
Anything else as I gather requirements?

-Paul

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

Re: [Patch] x86emul: simplify prefix handling for VMFUNC

[Jan Beulich]

>>> On 27.09.16 at 19:43, <paul.c.lai@intel.com> wrote:
> Finally found the vmfunc opcode page in Vol 3 30.3, VMX Instruction Reference.
> Agreed, there's no mention of prefixes, "pfx", on this page.  It appears
> that the other VMX instructions in this section don't mention prefixes either.
> Looking at Table A-6 "Opcode Extensions for One- and Two-Byte Opcodes":
> vmcall, vmlaunch, vmresume, and vmxoff would need similar updating.

Indeed.

> I can ask for updating of the VMX instuctions to include mention of prefixes.
> Anything else as I gather requirements?

I don't think so, but of course if would be nice to get confirmed (ahead
of the SDM update going public, which presumably will take some time)
that for all of those presence of said prefixes will cause #UD, as that
goes beyond stating that "they're reserved" (which is all the footnote
says).

Jan


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

Re: [Patch] x86emul: simplify prefix handling for VMFUNC

[Lai, Paul]

On Wed, Sep 28, 2016 at 01:47:56AM -0600, Jan Beulich wrote:
> >>> On 27.09.16 at 19:43, <paul.c.lai@intel.com> wrote:
> > Finally found the vmfunc opcode page in Vol 3 30.3, VMX Instruction Reference.
> > Agreed, there's no mention of prefixes, "pfx", on this page.  It appears
> > that the other VMX instructions in this section don't mention prefixes either.
> > Looking at Table A-6 "Opcode Extensions for One- and Two-Byte Opcodes":
> > vmcall, vmlaunch, vmresume, and vmxoff would need similar updating.
> 
> Indeed.
> 
> > I can ask for updating of the VMX instuctions to include mention of prefixes.
> > Anything else as I gather requirements?
> 
> I don't think so, but of course if would be nice to get confirmed (ahead
> of the SDM update going public, which presumably will take some time)
> that for all of those presence of said prefixes will cause #UD, as that
> goes beyond stating that "they're reserved" (which is all the footnote
> says).
> 
> Jan
> 
Got word that there's an effort to clean up the SDM in general, and this
section in particular.  Also, the word is that it'll be early next year (2017)
before something comes out.  No expectation was set of when an early draft
would be available.

-Paul

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

[PATCH] x86emul: simplify prefix handling for VMFUNC

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

LOCK prefixes get dealt with elsewhere and 66, F2, and F3 can all be
checked for in one go by looking at vex.pfx.

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
@@ -3942,8 +3942,8 @@ x86_emulate(
             goto rdtsc;
         }
         case 0xd4: /* vmfunc */
-            generate_exception_if(lock_prefix | rep_prefix() | (vex.pfx == vex_66),
-                                  EXC_UD, -1);
+            if ( vex.pfx )
+                break;
             fail_if(ops->vmfunc == NULL);
             if ( (rc = ops->vmfunc(ctxt) != X86EMUL_OKAY) )
                 goto done;




[-- Attachment #2: x86emul-VMFUNC-prefixes.patch --]
[-- Type: text/plain, Size: 740 bytes --]

x86emul: simplify prefix handling for VMFUNC

LOCK prefixes get dealt with elsewhere and 66, F2, and F3 can all be
checked for in one go by looking at vex.pfx.

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
@@ -3942,8 +3942,8 @@ x86_emulate(
             goto rdtsc;
         }
         case 0xd4: /* vmfunc */
-            generate_exception_if(lock_prefix | rep_prefix() | (vex.pfx == vex_66),
-                                  EXC_UD, -1);
+            if ( vex.pfx )
+                break;
             fail_if(ops->vmfunc == NULL);
             if ( (rc = ops->vmfunc(ctxt) != X86EMUL_OKAY) )
                 goto done;

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] x86emul: simplify prefix handling for VMFUNC

[Andrew Cooper]

On 05/09/16 10:13, Jan Beulich wrote:
> LOCK prefixes get dealt with elsewhere and 66, F2, and F3 can all be
> checked for in one go by looking at vex.pfx.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

As far as subsuming the checks goes, this is fine.  However, is the code
actually correct?  The manual makes no indication that the use of these
prefixes is prohibited.

~Andrew

>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -3942,8 +3942,8 @@ x86_emulate(
>              goto rdtsc;
>          }
>          case 0xd4: /* vmfunc */
> -            generate_exception_if(lock_prefix | rep_prefix() | (vex.pfx == vex_66),
> -                                  EXC_UD, -1);
> +            if ( vex.pfx )
> +                break;
>              fail_if(ops->vmfunc == NULL);
>              if ( (rc = ops->vmfunc(ctxt) != X86EMUL_OKAY) )
>                  goto done;
>
>
>


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

Re: [PATCH] x86emul: simplify prefix handling for VMFUNC

[Jan Beulich]

>>> On 05.09.16 at 11:52, <andrew.cooper3@citrix.com> wrote:
> On 05/09/16 10:13, Jan Beulich wrote:
>> LOCK prefixes get dealt with elsewhere and 66, F2, and F3 can all be
>> checked for in one go by looking at vex.pfx.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> As far as subsuming the checks goes, this is fine.  However, is the code
> actually correct?  The manual makes no indication that the use of these
> prefixes is prohibited.

Hmm, indeed (and I probably should have checked). Looking at e.g.
XEND and XTEST (which is how I came here) I wonder though
whether the manual is wrong. Let's see if we can get this figured
out. Ravi, Paul: Said checks originate from commit a1b1572833
("VMX: add VMFUNC leaf 0 (EPTP switching) to emulator"). Can you
please clarify whether they got added in error, or whether you
have knowledge the SDM doesn't expose?

Thanks, Jan


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