[PATCH 0/3] x86emul: VME/PVI mode fixes

[PATCH 0/3] x86emul: VME/PVI mode fixes

[Jan Beulich]

1: VME and PVI modes require a #GP(0) check first thing
2: raise #GP(0) in VME mode for POPF with TF set in new value
3: consolidate CR4 handling

Jan



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

[PATCH 1/3] x86emul: VME and PVI modes require a #GP(0) check first thing

[Jan Beulich]

As explicitly spelled out by the SDM, EFLAGS.VIF and EFLAGS.VIP both set
at the start of an instruction trigger #GP(0) independent of actual
instruction.

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
@@ -3247,6 +3247,11 @@ x86_emulate(
 
     ASSERT(ops->read);
 
+    generate_exception_if((mode_vif() &&
+                           (_regs.eflags & X86_EFLAGS_VIF) &&
+                           (_regs.eflags & X86_EFLAGS_VIP)),
+                          EXC_GP, 0);
+
     rc = x86_decode(&state, ctxt, ops);
     if ( rc != X86EMUL_OKAY )
         return rc;



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

Re: [PATCH 1/3] x86emul: VME and PVI modes require a #GP(0) check first thing

[Andrew Cooper]

On 02/11/18 16:45, Jan Beulich wrote:
> As explicitly spelled out by the SDM, EFLAGS.VIF and EFLAGS.VIP both set
> at the start of an instruction trigger #GP(0) independent of actual
> instruction.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[PATCH 2/3] x86emul: raise #GP(0) in VME mode for POPF with TF set in new value

[Jan Beulich]

This is a check explicitly listed by the instruction page in the SDM.

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
@@ -4049,6 +4049,7 @@ x86_emulate(
             dst.val = (uint16_t)dst.val | (_regs.eflags & 0xffff0000u);
             if ( cr4 & X86_CR4_VME )
             {
+                generate_exception_if(dst.val & X86_EFLAGS_TF, EXC_GP, 0);
                 if ( dst.val & X86_EFLAGS_IF )
                 {
                     generate_exception_if(_regs.eflags & X86_EFLAGS_VIP,



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

Re: [PATCH 2/3] x86emul: raise #GP(0) in VME mode for POPF with TF set in new value

[Andrew Cooper]

On 02/11/18 16:45, Jan Beulich wrote:
> This is a check explicitly listed by the instruction page in the SDM.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Hmm - looking at the listing, I think we've got an IOPL mismatch.  This
behaviour, as well as the VIP adjustment in context, only apply at IOPL < 3.

~Andrew

>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -4049,6 +4049,7 @@ x86_emulate(
>              dst.val = (uint16_t)dst.val | (_regs.eflags & 0xffff0000u);
>              if ( cr4 & X86_CR4_VME )
>              {
> +                generate_exception_if(dst.val & X86_EFLAGS_TF, EXC_GP, 0);
>                  if ( dst.val & X86_EFLAGS_IF )
>                  {
>                      generate_exception_if(_regs.eflags & X86_EFLAGS_VIP,
>
>


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

Re: [PATCH 2/3] x86emul: raise #GP(0) in VME mode for POPF with TF set in new value

[Jan Beulich]

>>> On 02.11.18 at 19:14, <andrew.cooper3@citrix.com> wrote:
> On 02/11/18 16:45, Jan Beulich wrote:
>> This is a check explicitly listed by the instruction page in the SDM.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Hmm - looking at the listing, I think we've got an IOPL mismatch.  This
> behaviour, as well as the VIP adjustment in context, only apply at IOPL < 3.

Ah, yes, but that'll be another (prereq) patch.

Jan



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

[PATCH 3/3] x86emul: consolidate CR4 handling

[Jan Beulich]

Now that there's an almost unconditional CR4 read right at the beginning
of x86_emulate(), centralize its reading there and use result and value
everywhere else without further invoking the hook.

Subsequently we may want to consider having the callers provide
whichever value they deem appropriate in their contexts, to avoid
invoking the hook altogether for this purpose.

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
@@ -1163,10 +1163,19 @@ do {
     ops->write_segment(x86_seg_cs, cs, ctxt);                           \
 })
 
+#define check_cr4() ({                      \
+    if ( unlikely(cr4_rc != X86EMUL_OKAY) ) \
+    {                                       \
+        rc = cr4_rc;                        \
+        goto done;                          \
+    }                                       \
+})
+
 static int _get_fpu(
     enum x86_emulate_fpu_type type,
     struct x86_emulate_ctxt *ctxt,
-    const struct x86_emulate_ops *ops)
+    const struct x86_emulate_ops *ops,
+    unsigned long cr4, int cr4_rc)
 {
     uint64_t xcr0;
     int rc;
@@ -1208,11 +1217,7 @@ static int _get_fpu(
         fail_if(!ops->read_cr);
         if ( type >= X86EMUL_FPU_xmm )
         {
-            unsigned long cr4;
-
-            rc = ops->read_cr(4, &cr4, ctxt);
-            if ( rc != X86EMUL_OKAY )
-                return rc;
+            check_cr4();
             generate_exception_if(!(cr4 & ((type == X86EMUL_FPU_xmm)
                                            ? X86_CR4_OSFXSR : X86_CR4_OSXSAVE)),
                                   EXC_UD);
@@ -1243,7 +1248,7 @@ static int _get_fpu(
 
 #define get_fpu(type)                                           \
 do {                                                            \
-    rc = _get_fpu(fpu_type = (type), ctxt, ops);                \
+    rc = _get_fpu(fpu_type = (type), ctxt, ops, cr4, cr4_rc);   \
     if ( rc ) goto done;                                        \
 } while (0)
 
@@ -1603,13 +1608,9 @@ _mode_iopl(
     _iopl;                                      \
 })
 #define mode_vif() ({                                        \
-    cr4 = 0;                                                 \
-    if ( ops->read_cr && get_cpl(ctxt, ops) == 3 )           \
-    {                                                        \
-        rc = ops->read_cr(4, &cr4, ctxt);                    \
-        if ( rc != X86EMUL_OKAY ) goto done;                 \
-    }                                                        \
-    !!(cr4 & (_regs.eflags & X86_EFLAGS_VM ? X86_CR4_VME : X86_CR4_PVI)); \
+    check_cr4();                                             \
+    get_cpl(ctxt, ops) == 3 &&                               \
+    (cr4 & (_regs.eflags & X86_EFLAGS_VM ? X86_CR4_VME : X86_CR4_PVI)); \
 })
 
 static int ioport_access_check(
@@ -2185,14 +2186,11 @@ static bool is_branch_step(struct x86_em
 }
 
 static bool umip_active(struct x86_emulate_ctxt *ctxt,
-                        const struct x86_emulate_ops *ops)
+                        const struct x86_emulate_ops *ops,
+                        unsigned long cr4)
 {
-    unsigned long cr4;
-
     /* Intentionally not using mode_ring0() here to avoid its fail_if(). */
-    return get_cpl(ctxt, ops) > 0 &&
-           ops->read_cr && ops->read_cr(4, &cr4, ctxt) == X86EMUL_OKAY &&
-           (cr4 & X86_CR4_UMIP);
+    return get_cpl(ctxt, ops) > 0 && (cr4 & X86_CR4_UMIP);
 }
 
 static void adjust_bnd(struct x86_emulate_ctxt *ctxt,
@@ -3226,7 +3224,7 @@ x86_emulate(
     /* Shadow copy of register state. Committed on successful emulation. */
     struct cpu_user_regs _regs = *ctxt->regs;
     struct x86_emulate_state state;
-    int rc;
+    int rc, cr4_rc;
     uint8_t b, d, *opc = NULL;
     unsigned int first_byte = 0, insn_bytes = 0;
     bool singlestep = (_regs.eflags & X86_EFLAGS_TF) &&
@@ -3234,7 +3232,7 @@ x86_emulate(
     bool sfence = false;
     struct operand src = { .reg = PTR_POISON };
     struct operand dst = { .reg = PTR_POISON };
-    unsigned long cr4;
+    unsigned long cr4 = 0;
     enum x86_emulate_fpu_type fpu_type = X86EMUL_FPU_none;
     struct x86_emulate_stub stub = {};
     DECLARE_ALIGNED(mmval_t, mmval);
@@ -3247,6 +3245,8 @@ x86_emulate(
 
     ASSERT(ops->read);
 
+    cr4_rc = ops->read_cr ? ops->read_cr(4, &cr4, ctxt) : X86EMUL_OKAY;
+
     generate_exception_if((mode_vif() &&
                            (_regs.eflags & X86_EFLAGS_VIF) &&
                            (_regs.eflags & X86_EFLAGS_VIP)),
@@ -4000,13 +4000,8 @@ x86_emulate(
         if ( (_regs.eflags & X86_EFLAGS_VM) &&
              MASK_EXTR(_regs.eflags, X86_EFLAGS_IOPL) != 3 )
         {
-            cr4 = 0;
-            if ( op_bytes == 2 && ops->read_cr )
-            {
-                rc = ops->read_cr(4, &cr4, ctxt);
-                if ( rc != X86EMUL_OKAY )
-                    goto done;
-            }
+            if ( op_bytes == 2 )
+                check_cr4();
             generate_exception_if(!(cr4 & X86_CR4_VME), EXC_GP, 0);
             src.val = (_regs.flags & ~X86_EFLAGS_IF) | X86_EFLAGS_IOPL;
             if ( _regs.eflags & X86_EFLAGS_VIF )
@@ -4019,17 +4014,12 @@ x86_emulate(
     case 0x9d: /* popf */ {
         uint32_t mask = X86_EFLAGS_VIP | X86_EFLAGS_VIF | X86_EFLAGS_VM;
 
-        cr4 = 0;
         if ( !mode_ring0() )
         {
             if ( _regs.eflags & X86_EFLAGS_VM )
             {
-                if ( op_bytes == 2 && ops->read_cr )
-                {
-                    rc = ops->read_cr(4, &cr4, ctxt);
-                    if ( rc != X86EMUL_OKAY )
-                        goto done;
-                }
+                if ( op_bytes == 2 )
+                    check_cr4();
                 generate_exception_if(!(cr4 & X86_CR4_VME) &&
                                       MASK_EXTR(_regs.eflags, X86_EFLAGS_IOPL) != 3,
                                       EXC_GP, 0);
@@ -5113,7 +5103,7 @@ x86_emulate(
         switch ( modrm_reg & 6 )
         {
         case 0: /* sldt / str */
-            generate_exception_if(umip_active(ctxt, ops), EXC_GP, 0);
+            generate_exception_if(umip_active(ctxt, ops, cr4), EXC_GP, 0);
             goto store_selector;
         case 2: /* lldt / ltr */
             generate_exception_if(!mode_ring0(), EXC_GP, 0);
@@ -5168,11 +5158,10 @@ x86_emulate(
             break;
 
         case 0xd0: /* xgetbv */
-            generate_exception_if(vex.pfx, EXC_UD);
-            if ( !ops->read_cr || !ops->read_xcr ||
-                 ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
-                cr4 = 0;
-            generate_exception_if(!(cr4 & X86_CR4_OSXSAVE), EXC_UD);
+            generate_exception_if((vex.pfx || !ops->read_xcr ||
+                                   cr4_rc != X86EMUL_OKAY ||
+                                   !(cr4 & X86_CR4_OSXSAVE)),
+                                  EXC_UD);
             rc = ops->read_xcr(_regs.ecx, &msr_val, ctxt);
             if ( rc != X86EMUL_OKAY )
                 goto done;
@@ -5181,11 +5170,10 @@ x86_emulate(
             break;
 
         case 0xd1: /* xsetbv */
-            generate_exception_if(vex.pfx, EXC_UD);
-            if ( !ops->read_cr || !ops->write_xcr ||
-                 ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
-                cr4 = 0;
-            generate_exception_if(!(cr4 & X86_CR4_OSXSAVE), EXC_UD);
+            generate_exception_if((vex.pfx || !ops->write_xcr ||
+                                   cr4_rc != X86EMUL_OKAY ||
+                                   !(cr4 & X86_CR4_OSXSAVE)),
+                                  EXC_UD);
             generate_exception_if(!mode_ring0(), EXC_GP, 0);
             rc = ops->write_xcr(_regs.ecx,
                                 _regs.eax | ((uint64_t)_regs.edx << 32), ctxt);
@@ -5309,7 +5297,7 @@ x86_emulate(
         case GRP7_MEM(0): /* sgdt */
         case GRP7_MEM(1): /* sidt */
             ASSERT(ea.type == OP_MEM);
-            generate_exception_if(umip_active(ctxt, ops), EXC_GP, 0);
+            generate_exception_if(umip_active(ctxt, ops, cr4), EXC_GP, 0);
             fail_if(!ops->read_segment || !ops->write);
             if ( (rc = ops->read_segment(seg, &sreg, ctxt)) )
                 goto done;
@@ -5348,7 +5336,7 @@ x86_emulate(
             break;
 
         case GRP7_ALL(4): /* smsw */
-            generate_exception_if(umip_active(ctxt, ops), EXC_GP, 0);
+            generate_exception_if(umip_active(ctxt, ops, cr4), EXC_GP, 0);
             if ( ea.type == OP_MEM )
             {
                 fail_if(!ops->write);
@@ -5931,8 +5919,7 @@ x86_emulate(
         if ( !mode_ring0() )
         {
             fail_if(ops->read_cr == NULL);
-            if ( (rc = ops->read_cr(4, &cr4, ctxt)) )
-                goto done;
+            check_cr4();
             generate_exception_if(cr4 & X86_CR4_TSD, EXC_GP, 0);
         }
         fail_if(ops->read_msr == NULL);
@@ -6968,8 +6955,6 @@ x86_emulate(
         fail_if(modrm_mod != 3);
         generate_exception_if((modrm_reg & 4) || !mode_64bit(), EXC_UD);
         fail_if(!ops->read_cr);
-        if ( (rc = ops->read_cr(4, &cr4, ctxt)) != X86EMUL_OKAY )
-            goto done;
         generate_exception_if(!(cr4 & X86_CR4_FSGSBASE), EXC_UD);
         seg = modrm_reg & 1 ? x86_seg_gs : x86_seg_fs;
         fail_if(!ops->read_segment);
@@ -8960,13 +8945,8 @@ x86_emulate(
  emulation_stub_failure:
     generate_exception_if(stub_exn.info.fields.trapnr == EXC_MF, EXC_MF);
     if ( stub_exn.info.fields.trapnr == EXC_XM )
-    {
-        unsigned long cr4;
-
-        if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
-            cr4 = X86_CR4_OSXMMEXCPT;
-        generate_exception(cr4 & X86_CR4_OSXMMEXCPT ? EXC_XM : EXC_UD);
-    }
+        generate_exception(!ops->read_cr || cr4_rc != X86EMUL_OKAY ||
+                           cr4 & X86_CR4_OSXMMEXCPT ? EXC_XM : EXC_UD);
     gprintk(XENLOG_WARNING,
             "exception %u (ec=%04x) in emulation stub (line %u)\n",
             stub_exn.info.fields.trapnr, stub_exn.info.fields.ec,




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

Re: [PATCH 3/3] x86emul: consolidate CR4 handling

[Andrew Cooper]

On 02/11/18 16:46, Jan Beulich wrote:
> Now that there's an almost unconditional CR4 read right at the beginning
> of x86_emulate(), centralize its reading there and use result and value
> everywhere else without further invoking the hook.
>
> Subsequently we may want to consider having the callers provide
> whichever value they deem appropriate in their contexts, to avoid
> invoking the hook altogether for this purpose.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I've got most of a series doing this for cpuid, which drops ~4k of .text
volume from x86_emulate() alone.

My plan was to get all the architectural state in a directly readable
form, to reduce the complexity and boilerplate.  On that subject...

> @@ -3247,6 +3245,8 @@ x86_emulate(
>  
>      ASSERT(ops->read);
>  
> +    cr4_rc = ops->read_cr ? ops->read_cr(4, &cr4, ctxt) : X86EMUL_OKAY;

... why not make read_cr() mandatory, or put cr4 into ctxt?  Plumbing
cr4_rc around still feels like a lot of boilerplate.

~Andrew

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

Re: [PATCH 3/3] x86emul: consolidate CR4 handling

[Jan Beulich]

>>> On 02.11.18 at 19:24, <andrew.cooper3@citrix.com> wrote:
> On 02/11/18 16:46, Jan Beulich wrote:
>> Now that there's an almost unconditional CR4 read right at the beginning
>> of x86_emulate(), centralize its reading there and use result and value
>> everywhere else without further invoking the hook.
>>
>> Subsequently we may want to consider having the callers provide
>> whichever value they deem appropriate in their contexts, to avoid
>> invoking the hook altogether for this purpose.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I've got most of a series doing this for cpuid, which drops ~4k of .text
> volume from x86_emulate() alone.
> 
> My plan was to get all the architectural state in a directly readable
> form, to reduce the complexity and boilerplate.

I'm not convinced, the more that I don't think you really mean "all"
when you say "all": Surely not the whole set of MSRs or any of
XSTATE?

>  On that subject...
> 
>> @@ -3247,6 +3245,8 @@ x86_emulate(
>>  
>>      ASSERT(ops->read);
>>  
>> +    cr4_rc = ops->read_cr ? ops->read_cr(4, &cr4, ctxt) : X86EMUL_OKAY;
> 
> ... why not make read_cr() mandatory, or put cr4 into ctxt?  Plumbing
> cr4_rc around still feels like a lot of boilerplate.

read_cr() is not currently populated by all paths; see
hvm_emulate_one_mmio() for one example. I don't want to make
the hook mandatory, at least not yet.

Jan



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

[PATCH v2 0/3] x86emul: VME/PVI mode fixes

[Jan Beulich]

1: skip VIF processing in VME mode for 16-bit POPF at IOPL 3
2: raise #GP(0) in VME mode for POPF with TF set in new value
3: consolidate CR4 handling

Jan




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

[PATCH v2 1/3] x86emul: skip VIF processing in VME mode for 16-bit POPF at IOPL 3

[Jan Beulich]

At IOPL 3 CR4.VME is irrelevant.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -4047,7 +4047,8 @@ x86_emulate(
         if ( op_bytes == 2 )
         {
             dst.val = (uint16_t)dst.val | (_regs.eflags & 0xffff0000u);
-            if ( cr4 & X86_CR4_VME )
+            if ( (cr4 & X86_CR4_VME) &&
+                 MASK_EXTR(_regs.eflags, X86_EFLAGS_IOPL) != 3 )
             {
                 if ( dst.val & X86_EFLAGS_IF )
                 {



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

Re: [PATCH v2 1/3] x86emul: skip VIF processing in VME mode for 16-bit POPF at IOPL 3

[Andrew Cooper]

On 06/11/2018 13:44, Jan Beulich wrote:
> At IOPL 3 CR4.VME is irrelevant.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[PATCH v2 2/3] x86emul: raise #GP(0) in VME mode for POPF with TF set in new value

[Jan Beulich]

This is a check explicitly listed by the instruction page in the SDM.

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
@@ -4050,6 +4050,7 @@ x86_emulate(
             if ( (cr4 & X86_CR4_VME) &&
                  MASK_EXTR(_regs.eflags, X86_EFLAGS_IOPL) != 3 )
             {
+                generate_exception_if(dst.val & X86_EFLAGS_TF, EXC_GP, 0);
                 if ( dst.val & X86_EFLAGS_IF )
                 {
                     generate_exception_if(_regs.eflags & X86_EFLAGS_VIP,



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

Re: [PATCH v2 2/3] x86emul: raise #GP(0) in VME mode for POPF with TF set in new value

[Andrew Cooper]

On 06/11/2018 13:45, Jan Beulich wrote:
> This is a check explicitly listed by the instruction page in the SDM.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[PATCH v2 3/3] x86emul: consolidate CR4 handling

[Jan Beulich]

Now that there's an almost unconditional CR4 read right at the beginning
of x86_emulate(), centralize its reading there and use result and value
everywhere else without further invoking the hook.

Subsequently we may want to consider having the callers provide
whichever value they deem appropriate in their contexts, to avoid
invoking the hook altogether for this purpose.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Don't allow 32-bit PUSHF/POPF in VME mode to get away without
    #GP(0).

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1163,10 +1163,19 @@ do {
     ops->write_segment(x86_seg_cs, cs, ctxt);                           \
 })
 
+#define check_cr4() ({                      \
+    if ( unlikely(cr4_rc != X86EMUL_OKAY) ) \
+    {                                       \
+        rc = cr4_rc;                        \
+        goto done;                          \
+    }                                       \
+})
+
 static int _get_fpu(
     enum x86_emulate_fpu_type type,
     struct x86_emulate_ctxt *ctxt,
-    const struct x86_emulate_ops *ops)
+    const struct x86_emulate_ops *ops,
+    unsigned long cr4, int cr4_rc)
 {
     uint64_t xcr0;
     int rc;
@@ -1208,11 +1217,7 @@ static int _get_fpu(
         fail_if(!ops->read_cr);
         if ( type >= X86EMUL_FPU_xmm )
         {
-            unsigned long cr4;
-
-            rc = ops->read_cr(4, &cr4, ctxt);
-            if ( rc != X86EMUL_OKAY )
-                return rc;
+            check_cr4();
             generate_exception_if(!(cr4 & ((type == X86EMUL_FPU_xmm)
                                            ? X86_CR4_OSFXSR : X86_CR4_OSXSAVE)),
                                   EXC_UD);
@@ -1243,7 +1248,7 @@ static int _get_fpu(
 
 #define get_fpu(type)                                           \
 do {                                                            \
-    rc = _get_fpu(fpu_type = (type), ctxt, ops);                \
+    rc = _get_fpu(fpu_type = (type), ctxt, ops, cr4, cr4_rc);   \
     if ( rc ) goto done;                                        \
 } while (0)
 
@@ -1603,13 +1608,9 @@ _mode_iopl(
     _iopl;                                      \
 })
 #define mode_vif() ({                                        \
-    cr4 = 0;                                                 \
-    if ( ops->read_cr && get_cpl(ctxt, ops) == 3 )           \
-    {                                                        \
-        rc = ops->read_cr(4, &cr4, ctxt);                    \
-        if ( rc != X86EMUL_OKAY ) goto done;                 \
-    }                                                        \
-    !!(cr4 & (_regs.eflags & X86_EFLAGS_VM ? X86_CR4_VME : X86_CR4_PVI)); \
+    check_cr4();                                             \
+    get_cpl(ctxt, ops) == 3 &&                               \
+    (cr4 & (_regs.eflags & X86_EFLAGS_VM ? X86_CR4_VME : X86_CR4_PVI)); \
 })
 
 static int ioport_access_check(
@@ -2185,14 +2186,11 @@ static bool is_branch_step(struct x86_em
 }
 
 static bool umip_active(struct x86_emulate_ctxt *ctxt,
-                        const struct x86_emulate_ops *ops)
+                        const struct x86_emulate_ops *ops,
+                        unsigned long cr4)
 {
-    unsigned long cr4;
-
     /* Intentionally not using mode_ring0() here to avoid its fail_if(). */
-    return get_cpl(ctxt, ops) > 0 &&
-           ops->read_cr && ops->read_cr(4, &cr4, ctxt) == X86EMUL_OKAY &&
-           (cr4 & X86_CR4_UMIP);
+    return get_cpl(ctxt, ops) > 0 && (cr4 & X86_CR4_UMIP);
 }
 
 static void adjust_bnd(struct x86_emulate_ctxt *ctxt,
@@ -3226,7 +3224,7 @@ x86_emulate(
     /* Shadow copy of register state. Committed on successful emulation. */
     struct cpu_user_regs _regs = *ctxt->regs;
     struct x86_emulate_state state;
-    int rc;
+    int rc, cr4_rc;
     uint8_t b, d, *opc = NULL;
     unsigned int first_byte = 0, insn_bytes = 0;
     bool singlestep = (_regs.eflags & X86_EFLAGS_TF) &&
@@ -3234,7 +3232,7 @@ x86_emulate(
     bool sfence = false;
     struct operand src = { .reg = PTR_POISON };
     struct operand dst = { .reg = PTR_POISON };
-    unsigned long cr4;
+    unsigned long cr4 = 0;
     enum x86_emulate_fpu_type fpu_type = X86EMUL_FPU_none;
     struct x86_emulate_stub stub = {};
     DECLARE_ALIGNED(mmval_t, mmval);
@@ -3247,6 +3245,8 @@ x86_emulate(
 
     ASSERT(ops->read);
 
+    cr4_rc = ops->read_cr ? ops->read_cr(4, &cr4, ctxt) : X86EMUL_OKAY;
+
     generate_exception_if((mode_vif() &&
                            (_regs.eflags & X86_EFLAGS_VIF) &&
                            (_regs.eflags & X86_EFLAGS_VIP)),
@@ -4000,13 +4000,10 @@ x86_emulate(
         if ( (_regs.eflags & X86_EFLAGS_VM) &&
              MASK_EXTR(_regs.eflags, X86_EFLAGS_IOPL) != 3 )
         {
-            cr4 = 0;
-            if ( op_bytes == 2 && ops->read_cr )
-            {
-                rc = ops->read_cr(4, &cr4, ctxt);
-                if ( rc != X86EMUL_OKAY )
-                    goto done;
-            }
+            if ( op_bytes == 2 )
+                check_cr4();
+            else
+                cr4 = 0;
             generate_exception_if(!(cr4 & X86_CR4_VME), EXC_GP, 0);
             src.val = (_regs.flags & ~X86_EFLAGS_IF) | X86_EFLAGS_IOPL;
             if ( _regs.eflags & X86_EFLAGS_VIF )
@@ -4019,17 +4016,14 @@ x86_emulate(
     case 0x9d: /* popf */ {
         uint32_t mask = X86_EFLAGS_VIP | X86_EFLAGS_VIF | X86_EFLAGS_VM;
 
-        cr4 = 0;
         if ( !mode_ring0() )
         {
             if ( _regs.eflags & X86_EFLAGS_VM )
             {
-                if ( op_bytes == 2 && ops->read_cr )
-                {
-                    rc = ops->read_cr(4, &cr4, ctxt);
-                    if ( rc != X86EMUL_OKAY )
-                        goto done;
-                }
+                if ( op_bytes == 2 )
+                    check_cr4();
+                else
+                    cr4 = 0;
                 generate_exception_if(!(cr4 & X86_CR4_VME) &&
                                       MASK_EXTR(_regs.eflags, X86_EFLAGS_IOPL) != 3,
                                       EXC_GP, 0);
@@ -5114,7 +5108,7 @@ x86_emulate(
         switch ( modrm_reg & 6 )
         {
         case 0: /* sldt / str */
-            generate_exception_if(umip_active(ctxt, ops), EXC_GP, 0);
+            generate_exception_if(umip_active(ctxt, ops, cr4), EXC_GP, 0);
             goto store_selector;
         case 2: /* lldt / ltr */
             generate_exception_if(!mode_ring0(), EXC_GP, 0);
@@ -5169,11 +5163,10 @@ x86_emulate(
             break;
 
         case 0xd0: /* xgetbv */
-            generate_exception_if(vex.pfx, EXC_UD);
-            if ( !ops->read_cr || !ops->read_xcr ||
-                 ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
-                cr4 = 0;
-            generate_exception_if(!(cr4 & X86_CR4_OSXSAVE), EXC_UD);
+            generate_exception_if((vex.pfx || !ops->read_xcr ||
+                                   cr4_rc != X86EMUL_OKAY ||
+                                   !(cr4 & X86_CR4_OSXSAVE)),
+                                  EXC_UD);
             rc = ops->read_xcr(_regs.ecx, &msr_val, ctxt);
             if ( rc != X86EMUL_OKAY )
                 goto done;
@@ -5182,11 +5175,10 @@ x86_emulate(
             break;
 
         case 0xd1: /* xsetbv */
-            generate_exception_if(vex.pfx, EXC_UD);
-            if ( !ops->read_cr || !ops->write_xcr ||
-                 ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
-                cr4 = 0;
-            generate_exception_if(!(cr4 & X86_CR4_OSXSAVE), EXC_UD);
+            generate_exception_if((vex.pfx || !ops->write_xcr ||
+                                   cr4_rc != X86EMUL_OKAY ||
+                                   !(cr4 & X86_CR4_OSXSAVE)),
+                                  EXC_UD);
             generate_exception_if(!mode_ring0(), EXC_GP, 0);
             rc = ops->write_xcr(_regs.ecx,
                                 _regs.eax | ((uint64_t)_regs.edx << 32), ctxt);
@@ -5310,7 +5302,7 @@ x86_emulate(
         case GRP7_MEM(0): /* sgdt */
         case GRP7_MEM(1): /* sidt */
             ASSERT(ea.type == OP_MEM);
-            generate_exception_if(umip_active(ctxt, ops), EXC_GP, 0);
+            generate_exception_if(umip_active(ctxt, ops, cr4), EXC_GP, 0);
             fail_if(!ops->read_segment || !ops->write);
             if ( (rc = ops->read_segment(seg, &sreg, ctxt)) )
                 goto done;
@@ -5349,7 +5341,7 @@ x86_emulate(
             break;
 
         case GRP7_ALL(4): /* smsw */
-            generate_exception_if(umip_active(ctxt, ops), EXC_GP, 0);
+            generate_exception_if(umip_active(ctxt, ops, cr4), EXC_GP, 0);
             if ( ea.type == OP_MEM )
             {
                 fail_if(!ops->write);
@@ -5932,8 +5924,7 @@ x86_emulate(
         if ( !mode_ring0() )
         {
             fail_if(ops->read_cr == NULL);
-            if ( (rc = ops->read_cr(4, &cr4, ctxt)) )
-                goto done;
+            check_cr4();
             generate_exception_if(cr4 & X86_CR4_TSD, EXC_GP, 0);
         }
         fail_if(ops->read_msr == NULL);
@@ -6969,8 +6960,6 @@ x86_emulate(
         fail_if(modrm_mod != 3);
         generate_exception_if((modrm_reg & 4) || !mode_64bit(), EXC_UD);
         fail_if(!ops->read_cr);
-        if ( (rc = ops->read_cr(4, &cr4, ctxt)) != X86EMUL_OKAY )
-            goto done;
         generate_exception_if(!(cr4 & X86_CR4_FSGSBASE), EXC_UD);
         seg = modrm_reg & 1 ? x86_seg_gs : x86_seg_fs;
         fail_if(!ops->read_segment);
@@ -8961,13 +8950,8 @@ x86_emulate(
  emulation_stub_failure:
     generate_exception_if(stub_exn.info.fields.trapnr == EXC_MF, EXC_MF);
     if ( stub_exn.info.fields.trapnr == EXC_XM )
-    {
-        unsigned long cr4;
-
-        if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
-            cr4 = X86_CR4_OSXMMEXCPT;
-        generate_exception(cr4 & X86_CR4_OSXMMEXCPT ? EXC_XM : EXC_UD);
-    }
+        generate_exception(!ops->read_cr || cr4_rc != X86EMUL_OKAY ||
+                           cr4 & X86_CR4_OSXMMEXCPT ? EXC_XM : EXC_UD);
     gprintk(XENLOG_WARNING,
             "exception %u (ec=%04x) in emulation stub (line %u)\n",
             stub_exn.info.fields.trapnr, stub_exn.info.fields.ec,




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

Re: [PATCH v2 3/3] x86emul: consolidate CR4 handling

[Andrew Cooper]

On 06/11/2018 13:45, Jan Beulich wrote:
> Now that there's an almost unconditional CR4 read right at the beginning
> of x86_emulate(), centralize its reading there and use result and value
> everywhere else without further invoking the hook.
>
> Subsequently we may want to consider having the callers provide
> whichever value they deem appropriate in their contexts, to avoid
> invoking the hook altogether for this purpose.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I'm afraid that I am still unconvinced that cr4_rc is a clever move.

It would be far more simple to require callers to provide it in
x86_emulate_ctxt.

> @@ -4000,13 +4000,10 @@ x86_emulate(
>          if ( (_regs.eflags & X86_EFLAGS_VM) &&
>               MASK_EXTR(_regs.eflags, X86_EFLAGS_IOPL) != 3 )
>          {
> -            cr4 = 0;
> -            if ( op_bytes == 2 && ops->read_cr )
> -            {
> -                rc = ops->read_cr(4, &cr4, ctxt);
> -                if ( rc != X86EMUL_OKAY )
> -                    goto done;
> -            }
> +            if ( op_bytes == 2 )
> +                check_cr4();
> +            else
> +                cr4 = 0;

This clobbers cr4, which is a latent bug if any of the retire logic
wants to start using the value.

This code is only like this because you've overloaded the value in CR4
to include an implicit "opsize == 16", and results in exceedingly
complicated logic to follow.

~Andrew

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

Re: [PATCH v2 3/3] x86emul: consolidate CR4 handling

[Jan Beulich]

>>> On 03.12.18 at 20:37, <andrew.cooper3@citrix.com> wrote:
> On 06/11/2018 13:45, Jan Beulich wrote:
>> Now that there's an almost unconditional CR4 read right at the beginning
>> of x86_emulate(), centralize its reading there and use result and value
>> everywhere else without further invoking the hook.
>>
>> Subsequently we may want to consider having the callers provide
>> whichever value they deem appropriate in their contexts, to avoid
>> invoking the hook altogether for this purpose.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I'm afraid that I am still unconvinced that cr4_rc is a clever move.
> 
> It would be far more simple to require callers to provide it in
> x86_emulate_ctxt.

I'll look into this another time, but I'm afraid this will result in a
behavioral change in particular for the callers which currently
don't supply a read_cr hook. Furthermore I'd then question
the purpose of that hook altogether: To be consistent, we then
should pass in all CRs. At which point the question arises
whether DRs and MSRs and XCRn shouldn't also be passed
in, instead of getting obtained via callbacks.

>> @@ -4000,13 +4000,10 @@ x86_emulate(
>>          if ( (_regs.eflags & X86_EFLAGS_VM) &&
>>               MASK_EXTR(_regs.eflags, X86_EFLAGS_IOPL) != 3 )
>>          {
>> -            cr4 = 0;
>> -            if ( op_bytes == 2 && ops->read_cr )
>> -            {
>> -                rc = ops->read_cr(4, &cr4, ctxt);
>> -                if ( rc != X86EMUL_OKAY )
>> -                    goto done;
>> -            }
>> +            if ( op_bytes == 2 )
>> +                check_cr4();
>> +            else
>> +                cr4 = 0;
> 
> This clobbers cr4, which is a latent bug if any of the retire logic
> wants to start using the value.

Yes, I did realize this when writing this code, but at this point
I can't see any such case (at least for the particular instructions
where such clobbering does happen), even if taking a purely
abstract perspective. Hence I'm considering this acceptable.

> This code is only like this because you've overloaded the value in CR4
> to include an implicit "opsize == 16", and results in exceedingly
> complicated logic to follow.

"Exceedingly complicated" is a matter of perception - adding
several more op_bytes checks is what looked to me to
complicate / obscure things more than would be desirable.

Jan



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

Re: [PATCH v2 0/3] x86emul: VME/PVI mode fixes

[Andrew Cooper]

On 06/11/18 13:33, Jan Beulich wrote:
> 1: skip VIF processing in VME mode for 16-bit POPF at IOPL 3
> 2: raise #GP(0) in VME mode for POPF with TF set in new value

I'm afraid that I think there is still a bug with PVI emulation.

We only ever read cr4 when operating in vm86, which means that the later
logic checking VME doesn't apply to 16bit protected mode execution.

~Andrew

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

Re: [PATCH v2 0/3] x86emul: VME/PVI mode fixes

[Jan Beulich]

>>> On 06.11.18 at 15:21, <andrew.cooper3@citrix.com> wrote:
> On 06/11/18 13:33, Jan Beulich wrote:
>> 1: skip VIF processing in VME mode for 16-bit POPF at IOPL 3
>> 2: raise #GP(0) in VME mode for POPF with TF set in new value
> 
> I'm afraid that I think there is still a bug with PVI emulation.
> 
> We only ever read cr4 when operating in vm86, which means that the later
> logic checking VME doesn't apply to 16bit protected mode execution.

And where on the instruction page in the SDM do you see CR4.PVI
mattering for POPF? Quite the opposite - there is:

"(The protected-mode virtual-interrupt feature — enabled by
 setting CR4.PVI — affects the CLI and STI instructions in the
 same manner as the virtual-8086 mode extensions. POPF,
 however, is not affected by CR4.PVI.)"

Even if there was a further issue, it would be orthogonal to the
changes here, i.e. you'd only make me add yet another patch.
IOW I don't think the comment affects the patches currently in
this series.

Jan


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