[PATCH v1 0/3] x86/vvmx: fixes for mov-ss and shadow vmcs handling

[PATCH v1 0/3] x86/vvmx: fixes for mov-ss and shadow vmcs handling

[Sergey Dyasli]

This series includes 2 more checks for nested vmentry and a fix for
handling a nested shadow vmcs.

Sergey Dyasli (3):
  x86/vvmx: add mov-ss blocking check to vmentry
  x86/vvmx: correct nested shadow VMCS handling
  x86/vvmx: add a shadow vmcs check to vmlaunch

 xen/arch/x86/hvm/vmx/vvmx.c        | 45 ++++++++++++++++++++++++++++++++++----
 xen/include/asm-x86/hvm/vmx/vmcs.h |  1 +
 xen/include/asm-x86/hvm/vmx/vvmx.h |  1 +
 3 files changed, 43 insertions(+), 4 deletions(-)

-- 
2.9.3


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

[PATCH v1 1/3] x86/vvmx: add mov-ss blocking check to vmentry

[Sergey Dyasli]

Intel SDM states that if there is a current VMCS and there is MOV-SS
blocking, VMFailValid occurs and control passes to the next instruction.

Implement such behaviour for nested vmlaunch and vmresume.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c        | 16 ++++++++++++++++
 xen/include/asm-x86/hvm/vmx/vmcs.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index e2c0951..09e4250 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1572,6 +1572,7 @@ int nvmx_handle_vmresume(struct cpu_user_regs *regs)
     bool_t launched;
     struct vcpu *v = current;
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+    unsigned long intr_shadow;
     int rc = vmx_inst_check_privilege(regs, 0);
 
     if ( rc != X86EMUL_OKAY )
@@ -1583,6 +1584,13 @@ int nvmx_handle_vmresume(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;        
     }
 
+    __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
+    if ( intr_shadow & VMX_INTR_SHADOW_MOV_SS )
+    {
+        vmfail_valid(regs, VMX_INSN_VMENTRY_BLOCKED_BY_MOV_SS);
+        return X86EMUL_OKAY;
+    }
+
     launched = vvmcs_launched(&nvmx->launched_list,
                               PFN_DOWN(v->arch.hvm_vmx.vmcs_shadow_maddr));
     if ( !launched )
@@ -1598,6 +1606,7 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
     bool_t launched;
     struct vcpu *v = current;
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+    unsigned long intr_shadow;
     int rc = vmx_inst_check_privilege(regs, 0);
 
     if ( rc != X86EMUL_OKAY )
@@ -1609,6 +1618,13 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;
     }
 
+    __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
+    if ( intr_shadow & VMX_INTR_SHADOW_MOV_SS )
+    {
+        vmfail_valid(regs, VMX_INSN_VMENTRY_BLOCKED_BY_MOV_SS);
+        return X86EMUL_OKAY;
+    }
+
     launched = vvmcs_launched(&nvmx->launched_list,
                               PFN_DOWN(v->arch.hvm_vmx.vmcs_shadow_maddr));
     if ( launched )
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index f465fff..dc5d91f 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -515,6 +515,7 @@ enum vmx_insn_errno
     VMX_INSN_VMPTRLD_INCORRECT_VMCS_ID     = 11,
     VMX_INSN_UNSUPPORTED_VMCS_COMPONENT    = 12,
     VMX_INSN_VMXON_IN_VMX_ROOT             = 15,
+    VMX_INSN_VMENTRY_BLOCKED_BY_MOV_SS     = 26,
     VMX_INSN_FAIL_INVALID                  = ~0,
 };
 
-- 
2.9.3


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

[PATCH v1 2/3] x86/vvmx: correct nested shadow VMCS handling

[Sergey Dyasli]

Currently xen always sets the shadow VMCS-indicator bit on nested
vmptrld and always clears it on nested vmclear.  This behavior is
wrong when the guest loads a shadow VMCS: shadow bit will be lost
on nested vmclear.

Fix this by checking if the guest has provided a shadow VMCS.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c        | 22 ++++++++++++++++++----
 xen/include/asm-x86/hvm/vmx/vvmx.h |  1 +
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 09e4250..3017849 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1119,10 +1119,19 @@ static bool_t nvmx_vpid_enabled(const struct vcpu *v)
 
 static void nvmx_set_vmcs_pointer(struct vcpu *v, struct vmcs_struct *vvmcs)
 {
+    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
     paddr_t vvmcs_maddr = v->arch.hvm_vmx.vmcs_shadow_maddr;
 
     __vmpclear(vvmcs_maddr);
-    vvmcs->vmcs_revision_id |= VMCS_RID_TYPE_MASK;
+    if ( !nvmx->shadow_vmcs )
+    {
+        /*
+         * We must set the shadow VMCS-indicator in order for the next vmentry
+         * to succeed with a newly set up link pointer in vmcs01.
+         * Note: guest can see that this bit was set.
+         */
+        vvmcs->vmcs_revision_id |= VMCS_RID_TYPE_MASK;
+    }
     __vmwrite(VMCS_LINK_POINTER, vvmcs_maddr);
     __vmwrite(VMREAD_BITMAP, page_to_maddr(v->arch.hvm_vmx.vmread_bitmap));
     __vmwrite(VMWRITE_BITMAP, page_to_maddr(v->arch.hvm_vmx.vmwrite_bitmap));
@@ -1130,10 +1139,13 @@ static void nvmx_set_vmcs_pointer(struct vcpu *v, struct vmcs_struct *vvmcs)
 
 static void nvmx_clear_vmcs_pointer(struct vcpu *v, struct vmcs_struct *vvmcs)
 {
+    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
     paddr_t vvmcs_maddr = v->arch.hvm_vmx.vmcs_shadow_maddr;
 
     __vmpclear(vvmcs_maddr);
-    vvmcs->vmcs_revision_id &= ~VMCS_RID_TYPE_MASK;
+    if ( !nvmx->shadow_vmcs )
+        vvmcs->vmcs_revision_id &= ~VMCS_RID_TYPE_MASK;
+    nvmx->shadow_vmcs = false;
     __vmwrite(VMCS_LINK_POINTER, ~0ul);
     __vmwrite(VMREAD_BITMAP, 0);
     __vmwrite(VMWRITE_BITMAP, 0);
@@ -1674,12 +1686,14 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
         {
             if ( writable )
             {
+                struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
                 struct vmcs_struct *vvmcs = vvmcx;
 
+                nvmx->shadow_vmcs =
+                    vvmcs->vmcs_revision_id & ~VMX_BASIC_REVISION_MASK;
                 if ( ((vvmcs->vmcs_revision_id ^ vmx_basic_msr) &
                                          VMX_BASIC_REVISION_MASK) ||
-                     (!cpu_has_vmx_vmcs_shadowing &&
-                      (vvmcs->vmcs_revision_id & ~VMX_BASIC_REVISION_MASK)) )
+                     (!cpu_has_vmx_vmcs_shadowing && nvmx->shadow_vmcs) )
                 {
                     hvm_unmap_guest_frame(vvmcx, 1);
                     vmfail(regs, VMX_INSN_VMPTRLD_INCORRECT_VMCS_ID);
diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h
index ca2fb25..9a65218 100644
--- a/xen/include/asm-x86/hvm/vmx/vvmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
@@ -51,6 +51,7 @@ struct nestedvmx {
     } ept;
     uint32_t guest_vpid;
     struct list_head launched_list;
+    bool shadow_vmcs;
 };
 
 #define vcpu_2_nvmx(v)	(vcpu_nestedhvm(v).u.nvmx)
-- 
2.9.3


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

[PATCH v1 3/3] x86/vvmx: add a shadow vmcs check to vmlaunch

[Sergey Dyasli]

Intel SDM states that if the current VMCS is a shadow VMCS,
VMFailInvalid occurs and control passes to the next instruction.

Implement such behaviour for nested vmlaunch.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 3017849..173ec74 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1630,6 +1630,13 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;
     }
 
+    /* Check that guest is not using a shadow vmcs for vmentry */
+    if ( nvmx->shadow_vmcs )
+    {
+        vmfail_invalid(regs);
+        return X86EMUL_OKAY;
+    }
+
     __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
     if ( intr_shadow & VMX_INTR_SHADOW_MOV_SS )
     {
-- 
2.9.3


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

Re: [PATCH v1 1/3] x86/vvmx: add mov-ss blocking check to vmentry

[Andrew Cooper]

On 13/03/17 10:51, Sergey Dyasli wrote:
> Intel SDM states that if there is a current VMCS and there is MOV-SS
> blocking, VMFailValid occurs and control passes to the next instruction.
>
> Implement such behaviour for nested vmlaunch and vmresume.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

The content here looks correct, so Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

I am wondering however whether we can start introducing transparent
unions and bitfields for the controls, like I did with ept_qual_t

~Andrew

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

Re: [PATCH v1 1/3] x86/vvmx: add mov-ss blocking check to vmentry

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Monday, March 13, 2017 6:52 PM
> 
> Intel SDM states that if there is a current VMCS and there is MOV-SS blocking,
> VMFailValid occurs and control passes to the next instruction.
> 
> Implement such behaviour for nested vmlaunch and vmresume.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH v1 2/3] x86/vvmx: correct nested shadow VMCS handling

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Monday, March 13, 2017 6:52 PM
> 
> Currently xen always sets the shadow VMCS-indicator bit on nested vmptrld
> and always clears it on nested vmclear.  This behavior is wrong when the
> guest loads a shadow VMCS: shadow bit will be lost on nested vmclear.
> 
> Fix this by checking if the guest has provided a shadow VMCS.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH v1 3/3] x86/vvmx: add a shadow vmcs check to vmlaunch

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Monday, March 13, 2017 6:52 PM
> 
> Intel SDM states that if the current VMCS is a shadow VMCS, VMFailInvalid
> occurs and control passes to the next instruction.
> 
> Implement such behaviour for nested vmlaunch.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH v1 1/3] x86/vvmx: add mov-ss blocking check to vmentry

[Krish Sadhukhan]

The Intel SDM also mentions POP-SS. Are you planning to do it via 
another patch ?

Also, I was wondering if it makes more sense to rename the new enum code as

     VMX_INSN_VMENTRY_BLOCKED

since it can then also be used for POP-SS.


-Krish

On 03/13/2017 03:51 AM, Sergey Dyasli wrote:
> Intel SDM states that if there is a current VMCS and there is MOV-SS
> blocking, VMFailValid occurs and control passes to the next instruction.
>
> Implement such behaviour for nested vmlaunch and vmresume.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>   xen/arch/x86/hvm/vmx/vvmx.c        | 16 ++++++++++++++++
>   xen/include/asm-x86/hvm/vmx/vmcs.h |  1 +
>   2 files changed, 17 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index e2c0951..09e4250 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1572,6 +1572,7 @@ int nvmx_handle_vmresume(struct cpu_user_regs *regs)
>       bool_t launched;
>       struct vcpu *v = current;
>       struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
> +    unsigned long intr_shadow;
>       int rc = vmx_inst_check_privilege(regs, 0);
>   
>       if ( rc != X86EMUL_OKAY )
> @@ -1583,6 +1584,13 @@ int nvmx_handle_vmresume(struct cpu_user_regs *regs)
>           return X86EMUL_OKAY;
>       }
>   
> +    __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
> +    if ( intr_shadow & VMX_INTR_SHADOW_MOV_SS )
> +    {
> +        vmfail_valid(regs, VMX_INSN_VMENTRY_BLOCKED_BY_MOV_SS);
> +        return X86EMUL_OKAY;
> +    }
> +
>       launched = vvmcs_launched(&nvmx->launched_list,
>                                 PFN_DOWN(v->arch.hvm_vmx.vmcs_shadow_maddr));
>       if ( !launched )
> @@ -1598,6 +1606,7 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
>       bool_t launched;
>       struct vcpu *v = current;
>       struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
> +    unsigned long intr_shadow;
>       int rc = vmx_inst_check_privilege(regs, 0);
>   
>       if ( rc != X86EMUL_OKAY )
> @@ -1609,6 +1618,13 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
>           return X86EMUL_OKAY;
>       }
>   
> +    __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
> +    if ( intr_shadow & VMX_INTR_SHADOW_MOV_SS )
> +    {
> +        vmfail_valid(regs, VMX_INSN_VMENTRY_BLOCKED_BY_MOV_SS);
> +        return X86EMUL_OKAY;
> +    }
> +
>       launched = vvmcs_launched(&nvmx->launched_list,
>                                 PFN_DOWN(v->arch.hvm_vmx.vmcs_shadow_maddr));
>       if ( launched )
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index f465fff..dc5d91f 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -515,6 +515,7 @@ enum vmx_insn_errno
>       VMX_INSN_VMPTRLD_INCORRECT_VMCS_ID     = 11,
>       VMX_INSN_UNSUPPORTED_VMCS_COMPONENT    = 12,
>       VMX_INSN_VMXON_IN_VMX_ROOT             = 15,
> +    VMX_INSN_VMENTRY_BLOCKED_BY_MOV_SS     = 26,
>       VMX_INSN_FAIL_INVALID                  = ~0,
>   };
>   


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

Re: [PATCH v1 3/3] x86/vvmx: add a shadow vmcs check to vmlaunch

[Krish Sadhukhan]

This one looks good to me.

-Krish

On 03/13/2017 03:51 AM, Sergey Dyasli wrote:
> Intel SDM states that if the current VMCS is a shadow VMCS,
> VMFailInvalid occurs and control passes to the next instruction.
>
> Implement such behaviour for nested vmlaunch.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>   xen/arch/x86/hvm/vmx/vvmx.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 3017849..173ec74 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1630,6 +1630,13 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
>           return X86EMUL_OKAY;
>       }
>   
> +    /* Check that guest is not using a shadow vmcs for vmentry */
> +    if ( nvmx->shadow_vmcs )
> +    {
> +        vmfail_invalid(regs);
> +        return X86EMUL_OKAY;
> +    }
> +
>       __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
>       if ( intr_shadow & VMX_INTR_SHADOW_MOV_SS )
>       {


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

Re: [PATCH v1 3/3] x86/vvmx: add a shadow vmcs check to vmlaunch

[Krish Sadhukhan]

Acknowledging it formally...

Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

The review was based on Intel SDM chapters 24 and 30.

-Krish

On 03/16/2017 11:24 AM, Krish Sadhukhan wrote:
> This one looks good to me.
>
> -Krish
>
> On 03/13/2017 03:51 AM, Sergey Dyasli wrote:
>> Intel SDM states that if the current VMCS is a shadow VMCS,
>> VMFailInvalid occurs and control passes to the next instruction.
>>
>> Implement such behaviour for nested vmlaunch.
>>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>> ---
>>   xen/arch/x86/hvm/vmx/vvmx.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
>> index 3017849..173ec74 100644
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -1630,6 +1630,13 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs 
>> *regs)
>>           return X86EMUL_OKAY;
>>       }
>>   +    /* Check that guest is not using a shadow vmcs for vmentry */
>> +    if ( nvmx->shadow_vmcs )
>> +    {
>> +        vmfail_invalid(regs);
>> +        return X86EMUL_OKAY;
>> +    }
>> +
>>       __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
>>       if ( intr_shadow & VMX_INTR_SHADOW_MOV_SS )
>>       {
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel


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

Re: [PATCH v1 1/3] x86/vvmx: add mov-ss blocking check to vmentry

[Sergey Dyasli]

On Thu, 2017-03-16 at 11:23 -0700, Krish Sadhukhan wrote:
> The Intel SDM also mentions POP-SS. Are you planning to do it via 
> another patch ?

(SDM from Dec 2016) In "Table 24-3. Format of Interruptibility State"
it reads "This document uses the term “blocking by MOV SS,” but it
applies equally to POP SS."

> 
> Also, I was wondering if it makes more sense to rename the new enum code as
> 
>      VMX_INSN_VMENTRY_BLOCKED
> 
> since it can then also be used for POP-SS.

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