[PATCH v1 0/6] x86/vvmx: various fixes

[PATCH v1 0/6] x86/vvmx: various fixes

[Sergey Dyasli]

These were found by running nested VMX tests from kvm-unit-tests.

Sergey Dyasli (6):
  x86/vvmx: introduce vvmcx_valid()
  x86/vvmx: correct vmfail() usage for vmptrld and vmclear
  x86/vvmx: add VMX_INSN_VMPTRLD_WITH_VMXON_PTR errno
  x86/vvmx: add VMX_INSN_VMCLEAR_WITH_VMXON_PTR errno
  x86/vvmx: correctly report vvmcs size
  x86/vvmx: fix I/O and MSR bitmaps mapping

 xen/arch/x86/hvm/vmx/vvmx.c         | 164 +++++++++++++++++++---------
 xen/include/asm-x86/hvm/nestedhvm.h |   5 +
 xen/include/asm-x86/hvm/vmx/vmcs.h  |   2 +
 3 files changed, 117 insertions(+), 54 deletions(-)

-- 
2.17.1


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

[PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()

[Sergey Dyasli]

As a convenient helper function and refactor the code to use it.

No functional change.

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

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 0e45db83e5..8eee6d0ea8 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -521,8 +521,7 @@ static void vmfail(struct cpu_user_regs *regs, enum vmx_insn_errno errno)
     if ( errno == VMX_INSN_SUCCEED )
         return;
 
-    if ( vcpu_nestedhvm(current).nv_vvmcxaddr != INVALID_PADDR &&
-         errno != VMX_INSN_FAIL_INVALID )
+    if ( vvmcx_valid(current) && errno != VMX_INSN_FAIL_INVALID )
         vmfail_valid(regs, errno);
     else
         vmfail_invalid(regs);
@@ -805,7 +804,7 @@ static void nvmx_purge_vvmcs(struct vcpu *v)
     int i;
 
     __clear_current_vvmcs(v);
-    if ( nvcpu->nv_vvmcxaddr != INVALID_PADDR )
+    if ( vvmcx_valid(v) )
         hvm_unmap_guest_frame(nvcpu->nv_vvmcx, 1);
     nvcpu->nv_vvmcx = NULL;
     nvcpu->nv_vvmcxaddr = INVALID_PADDR;
@@ -1601,7 +1600,7 @@ static int nvmx_vmresume(struct vcpu *v, struct cpu_user_regs *regs)
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
 
     /* check VMCS is valid and IO BITMAP is set */
-    if ( (nvcpu->nv_vvmcxaddr != INVALID_PADDR) &&
+    if ( vvmcx_valid(v) &&
             ((nvmx->iobitmap[0] && nvmx->iobitmap[1]) ||
             !(__n2_exec_control(v) & CPU_BASED_ACTIVATE_IO_BITMAP) ) )
         nvcpu->nv_vmentry_pending = 1;
@@ -1622,7 +1621,7 @@ int nvmx_handle_vmresume(struct cpu_user_regs *regs)
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
+    if ( !vvmcx_valid(v) )
     {
         vmfail_invalid(regs);
         return X86EMUL_OKAY;        
@@ -1656,7 +1655,7 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
+    if ( !vvmcx_valid(v) )
     {
         vmfail_invalid(regs);
         return X86EMUL_OKAY;
@@ -1709,7 +1708,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
     if ( nvcpu->nv_vvmcxaddr != gpa )
         nvmx_purge_vvmcs(v);
 
-    if ( nvcpu->nv_vvmcxaddr == INVALID_PADDR )
+    if ( !vvmcx_valid(v) )
     {
         bool_t writable;
         void *vvmcx = hvm_map_guest_frame_rw(paddr_to_pfn(gpa), 1, &writable);
@@ -1848,7 +1847,7 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
+    if ( !vvmcx_valid(v) )
     {
         vmfail_invalid(regs);
         return X86EMUL_OKAY;
@@ -1891,7 +1890,7 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
              != X86EMUL_OKAY )
         return X86EMUL_EXCEPTION;
 
-    if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
+    if ( !vvmcx_valid(v) )
     {
         vmfail_invalid(regs);
         return X86EMUL_OKAY;
diff --git a/xen/include/asm-x86/hvm/nestedhvm.h b/xen/include/asm-x86/hvm/nestedhvm.h
index 9d1c2742b5..e09fa9d47d 100644
--- a/xen/include/asm-x86/hvm/nestedhvm.h
+++ b/xen/include/asm-x86/hvm/nestedhvm.h
@@ -92,4 +92,9 @@ static inline void nestedhvm_set_cr(struct vcpu *v, unsigned int cr,
         v->arch.hvm.nvcpu.guest_cr[cr] = value;
 }
 
+static inline bool vvmcx_valid(const struct vcpu *v)
+{
+    return vcpu_nestedhvm(v).nv_vvmcxaddr != INVALID_PADDR;
+}
+
 #endif /* _HVM_NESTEDHVM_H */
-- 
2.17.1


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

[PATCH v1 2/6] x86/vvmx: correct vmfail() usage for vmptrld and vmclear

[Sergey Dyasli]

Calling vmfail_valid() is correct only if vvmcx is valid. Modify
functions to use vmfail() instead which performs the necessary check.

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

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 8eee6d0ea8..26b7d72660 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1744,7 +1744,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
              !map_io_bitmap_all(v) ||
              !_map_msr_bitmap(v) )
         {
-            vmfail_valid(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
+            vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
             goto out;
         }
     }
@@ -1828,7 +1828,7 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
     if ( rc == VMSUCCEED )
         vmsucceed(regs);
     else if ( rc == VMFAIL_VALID )
-        vmfail_valid(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
+        vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
     else
         vmfail_invalid(regs);
 
-- 
2.17.1


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

[PATCH v1 3/6] x86/vvmx: add VMX_INSN_VMPTRLD_WITH_VMXON_PTR errno

[Sergey Dyasli]

And make nvmx_handle_vmptrld() return the new errno in case the provided
address is the same as vmxon region address.

While at it, correct the return value for not-4KB-aligned case.

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

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 26b7d72660..4caa5811a1 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1699,9 +1699,15 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    if ( gpa == vcpu_2_nvmx(v).vmxon_region_pa || gpa & 0xfff )
+    if ( gpa & 0xfff )
     {
-        vmfail_invalid(regs);
+        vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
+        goto out;
+    }
+
+    if ( gpa == vcpu_2_nvmx(v).vmxon_region_pa )
+    {
+        vmfail(regs, VMX_INSN_VMPTRLD_WITH_VMXON_PTR);
         goto out;
     }
 
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 76dd04a72d..cae1861610 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -534,6 +534,7 @@ enum vmx_insn_errno
     VMX_INSN_INVALID_CONTROL_STATE         = 7,
     VMX_INSN_INVALID_HOST_STATE            = 8,
     VMX_INSN_VMPTRLD_INVALID_PHYADDR       = 9,
+    VMX_INSN_VMPTRLD_WITH_VMXON_PTR        = 10,
     VMX_INSN_VMPTRLD_INCORRECT_VMCS_ID     = 11,
     VMX_INSN_UNSUPPORTED_VMCS_COMPONENT    = 12,
     VMX_INSN_VMXON_IN_VMX_ROOT             = 15,
-- 
2.17.1


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

[PATCH v1 4/6] x86/vvmx: add VMX_INSN_VMCLEAR_WITH_VMXON_PTR errno

[Sergey Dyasli]

And make nvmx_handle_vmclear() return the new errno in case the provided
address is the same as vmxon region address.

While at it, correct the return value for not-4KB-aligned case and for
invalid physaddr.

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

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 4caa5811a1..8b691bfc04 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1804,9 +1804,20 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
         return rc;
 
     BUILD_BUG_ON(X86EMUL_OKAY != VMSUCCEED); /* rc = VMSUCCEED; */
+
+    if ( gpa == vcpu_2_nvmx(v).vmxon_region_pa )
+    {
+        vmfail(regs, VMX_INSN_VMCLEAR_WITH_VMXON_PTR);
+        goto out;
+    }
+
     if ( gpa & 0xfff )
-        rc = VMFAIL_INVALID;
-    else if ( gpa == nvcpu->nv_vvmcxaddr )
+    {
+        vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
+        goto out;
+    }
+
+    if ( gpa == nvcpu->nv_vvmcxaddr )
     {
         if ( cpu_has_vmx_vmcs_shadowing )
             nvmx_clear_vmcs_pointer(v, nvcpu->nv_vvmcx);
@@ -1820,7 +1831,10 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
         bool_t writable;
 
         vvmcs = hvm_map_guest_frame_rw(paddr_to_pfn(gpa), 0, &writable);
-        if ( vvmcs ) 
+
+        if ( !vvmcs )
+            rc = VMFAIL_VALID;
+        else
         {
             if ( writable )
                 clear_vvmcs_launched(&nvmx->launched_list,
@@ -1835,9 +1849,8 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
         vmsucceed(regs);
     else if ( rc == VMFAIL_VALID )
         vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
-    else
-        vmfail_invalid(regs);
 
+out:
     return X86EMUL_OKAY;
 }
 
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index cae1861610..e84d2e482b 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -529,6 +529,7 @@ enum vmx_insn_errno
 {
     VMX_INSN_SUCCEED                       = 0,
     VMX_INSN_VMCLEAR_INVALID_PHYADDR       = 2,
+    VMX_INSN_VMCLEAR_WITH_VMXON_PTR        = 3,
     VMX_INSN_VMLAUNCH_NONCLEAR_VMCS        = 4,
     VMX_INSN_VMRESUME_NONLAUNCHED_VMCS     = 5,
     VMX_INSN_INVALID_CONTROL_STATE         = 7,
-- 
2.17.1


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

[PATCH v1 5/6] x86/vvmx: correctly report vvmcs size

[Sergey Dyasli]

The size of Xen's virtual vmcs region is 4096 bytes. Correctly report
it to the guest in case when VMCS shadowing is not available instead of
providing H/W value (which is usually smaller).

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

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 8b691bfc04..2c2ba36d94 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2064,6 +2064,14 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
         data = (host_data & (~0ul << 32)) |
                (vmcs->vmcs_revision_id & 0x7fffffff);
         unmap_domain_page(vmcs);
+
+        if ( !cpu_has_vmx_vmcs_shadowing )
+        {
+            /* Report vmcs_region_size as 4096 */
+            data &= ~VMX_BASIC_VMCS_SIZE_MASK;
+            data |= 1ULL << 44;
+        }
+
         break;
     }
     case MSR_IA32_VMX_PINBASED_CTLS:
-- 
2.17.1


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

[PATCH v1 6/6] x86/vvmx: fix I/O and MSR bitmaps mapping

[Sergey Dyasli]

Currently Xen tries to map bitmaps during emulation of vmptrld and
vmwrite. This is wrong: the guest can store arbitrary values in those
fields.

Make bitmaps mapping happen only during a nested vmentry and only if
the appropriate execution controls are turned on by L1 hypervisor.
Mapping happens only:

    1. During the first nested vmentry
    2. After L1 has changed an appropriate vmcs field
    3. After nvmx_purge_vvmcs() was previously called

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

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 2c2ba36d94..e10ed79f66 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -750,6 +750,17 @@ static void __clear_current_vvmcs(struct vcpu *v)
         __vmpclear(nvcpu->nv_n2vmcx_pa);
 }
 
+static void unmap_msr_bitmap(struct vcpu *v)
+{
+    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+
+    if ( nvmx->msrbitmap )
+    {
+        hvm_unmap_guest_frame(nvmx->msrbitmap, 1);
+        nvmx->msrbitmap = NULL;
+    }
+}
+
 /*
  * Refreshes the MSR bitmap mapping for the current nested vcpu.  Returns true
  * for a successful mapping, and returns false for MSR_BITMAP parameter errors
@@ -760,12 +771,7 @@ static bool __must_check _map_msr_bitmap(struct vcpu *v)
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
     uint64_t gpa;
 
-    if ( nvmx->msrbitmap )
-    {
-        hvm_unmap_guest_frame(nvmx->msrbitmap, 1);
-        nvmx->msrbitmap = NULL;
-    }
-
+    unmap_msr_bitmap(v);
     gpa = get_vvmcs(v, MSR_BITMAP);
 
     if ( !IS_ALIGNED(gpa, PAGE_SIZE) )
@@ -776,6 +782,17 @@ static bool __must_check _map_msr_bitmap(struct vcpu *v)
     return nvmx->msrbitmap != NULL;
 }
 
+static void unmap_io_bitmap(struct vcpu *v, unsigned int idx)
+{
+    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+
+    if ( nvmx->iobitmap[idx] )
+    {
+        hvm_unmap_guest_frame(nvmx->iobitmap[idx], 1);
+        nvmx->iobitmap[idx] = NULL;
+    }
+}
+
 static bool_t __must_check _map_io_bitmap(struct vcpu *v, u64 vmcs_reg)
 {
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
@@ -783,8 +800,7 @@ static bool_t __must_check _map_io_bitmap(struct vcpu *v, u64 vmcs_reg)
     int index;
 
     index = vmcs_reg == IO_BITMAP_A ? 0 : 1;
-    if (nvmx->iobitmap[index])
-        hvm_unmap_guest_frame(nvmx->iobitmap[index], 1);
+    unmap_io_bitmap(v, index);
     gpa = get_vvmcs(v, vmcs_reg);
     nvmx->iobitmap[index] = hvm_map_guest_frame_ro(gpa >> PAGE_SHIFT, 1);
 
@@ -799,7 +815,6 @@ static inline bool_t __must_check map_io_bitmap_all(struct vcpu *v)
 
 static void nvmx_purge_vvmcs(struct vcpu *v)
 {
-    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
     int i;
 
@@ -809,16 +824,11 @@ static void nvmx_purge_vvmcs(struct vcpu *v)
     nvcpu->nv_vvmcx = NULL;
     nvcpu->nv_vvmcxaddr = INVALID_PADDR;
     v->arch.hvm.vmx.vmcs_shadow_maddr = 0;
-    for (i=0; i<2; i++) {
-        if ( nvmx->iobitmap[i] ) {
-            hvm_unmap_guest_frame(nvmx->iobitmap[i], 1);
-            nvmx->iobitmap[i] = NULL;
-        }
-    }
-    if ( nvmx->msrbitmap ) {
-        hvm_unmap_guest_frame(nvmx->msrbitmap, 1);
-        nvmx->msrbitmap = NULL;
-    }
+
+    for ( i = 0; i < 2; i++ )
+        unmap_io_bitmap(v, i);
+
+    unmap_msr_bitmap(v);
 }
 
 u64 nvmx_get_tsc_offset(struct vcpu *v)
@@ -1594,20 +1604,34 @@ static void clear_vvmcs_launched(struct list_head *launched_list,
     }
 }
 
-static int nvmx_vmresume(struct vcpu *v, struct cpu_user_regs *regs)
+static enum vmx_insn_errno nvmx_vmresume(struct vcpu *v)
 {
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
+    unsigned int exec_ctrl;
 
-    /* check VMCS is valid and IO BITMAP is set */
-    if ( vvmcx_valid(v) &&
-            ((nvmx->iobitmap[0] && nvmx->iobitmap[1]) ||
-            !(__n2_exec_control(v) & CPU_BASED_ACTIVATE_IO_BITMAP) ) )
-        nvcpu->nv_vmentry_pending = 1;
-    else
-        vmfail_invalid(regs);
+    ASSERT(vvmcx_valid(v));
+    exec_ctrl = __n2_exec_control(v);
 
-    return X86EMUL_OKAY;
+    if ( exec_ctrl & CPU_BASED_ACTIVATE_IO_BITMAP )
+    {
+        if ( (nvmx->iobitmap[0] == NULL || nvmx->iobitmap[1] == NULL) &&
+             !map_io_bitmap_all(v) )
+            goto invalid_control_state;
+    }
+
+    if ( exec_ctrl & CPU_BASED_ACTIVATE_MSR_BITMAP )
+    {
+        if ( nvmx->msrbitmap == NULL && !_map_msr_bitmap(v) )
+            goto invalid_control_state;
+    }
+
+    nvcpu->nv_vmentry_pending = 1;
+
+    return VMX_INSN_SUCCEED;
+
+invalid_control_state:
+    return VMX_INSN_INVALID_CONTROL_STATE;
 }
 
 int nvmx_handle_vmresume(struct cpu_user_regs *regs)
@@ -1641,7 +1665,12 @@ int nvmx_handle_vmresume(struct cpu_user_regs *regs)
         vmfail_valid(regs, VMX_INSN_VMRESUME_NONLAUNCHED_VMCS);
         return X86EMUL_OKAY;
     }
-    return nvmx_vmresume(v,regs);
+
+    rc = nvmx_vmresume(v);
+    if ( rc )
+        vmfail_valid(regs, rc);
+
+    return X86EMUL_OKAY;
 }
 
 int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
@@ -1676,13 +1705,16 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;
     }
     else {
-        rc = nvmx_vmresume(v,regs);
-        if ( rc == X86EMUL_OKAY )
+        rc = nvmx_vmresume(v);
+        if ( rc )
+            vmfail_valid(regs, rc);
+        else
         {
             if ( set_vvmcs_launched(&nvmx->launched_list,
                                     PFN_DOWN(v->arch.hvm.vmx.vmcs_shadow_maddr)) < 0 )
                 return X86EMUL_UNHANDLEABLE;
         }
+        rc = X86EMUL_OKAY;
     }
     return rc;
 }
@@ -1746,9 +1778,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
                 vvmcx = NULL;
             }
         }
-        if ( !vvmcx ||
-             !map_io_bitmap_all(v) ||
-             !_map_msr_bitmap(v) )
+        else
         {
             vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
             goto out;
@@ -1926,13 +1956,13 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
     switch ( vmcs_encoding & ~VMCS_HIGH(0) )
     {
     case IO_BITMAP_A:
-        okay = _map_io_bitmap(v, IO_BITMAP_A);
+        unmap_io_bitmap(v, 0);
         break;
     case IO_BITMAP_B:
-        okay = _map_io_bitmap(v, IO_BITMAP_B);
+        unmap_io_bitmap(v, 1);
         break;
     case MSR_BITMAP:
-        okay = _map_msr_bitmap(v);
+        unmap_msr_bitmap(v);
         break;
     }
 
-- 
2.17.1


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

Re: [PATCH v1 2/6] x86/vvmx: correct vmfail() usage for vmptrld and vmclear

[Wei Liu]

On Fri, Oct 12, 2018 at 04:27:55PM +0100, Sergey Dyasli wrote:
> Calling vmfail_valid() is correct only if vvmcx is valid. Modify
> functions to use vmfail() instead which performs the necessary check.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

I think the code is a bit convoluted because what fields get access is
hidden behind layers of macros and functions. In order to catch future
issues, may I suggest you add some assertions to vmfail_{in,}valid?

Like

   ASSERT(!cpu_has_vmx_vmcs_shadowing && vvmcx_invalid(v));

in vmfail_invalid. Something similar can be added in vmfail_valid as
well.

> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 8eee6d0ea8..26b7d72660 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1744,7 +1744,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
>               !map_io_bitmap_all(v) ||
>               !_map_msr_bitmap(v) )
>          {
> -            vmfail_valid(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
> +            vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
>              goto out;
>          }
>      }
> @@ -1828,7 +1828,7 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
>      if ( rc == VMSUCCEED )
>          vmsucceed(regs);
>      else if ( rc == VMFAIL_VALID )
> -        vmfail_valid(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
> +        vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
>      else
>          vmfail_invalid(regs);
>  
> -- 
> 2.17.1
> 

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

Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()

[Wei Liu]

On Fri, Oct 12, 2018 at 04:27:54PM +0100, Sergey Dyasli wrote:
> As a convenient helper function and refactor the code to use it.
> 
> No functional change.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Friday, October 12, 2018 11:28 PM
> 
> As a convenient helper function and refactor the code to use it.
> 
> No functional change.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

since vmcx is hvm abstracted term, what about using this
helper in svm side too?

> ---
>  xen/arch/x86/hvm/vmx/vvmx.c         | 17 ++++++++---------
>  xen/include/asm-x86/hvm/nestedhvm.h |  5 +++++
>  2 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
> b/xen/arch/x86/hvm/vmx/vvmx.c
> index 0e45db83e5..8eee6d0ea8 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -521,8 +521,7 @@ static void vmfail(struct cpu_user_regs *regs, enum
> vmx_insn_errno errno)
>      if ( errno == VMX_INSN_SUCCEED )
>          return;
> 
> -    if ( vcpu_nestedhvm(current).nv_vvmcxaddr != INVALID_PADDR &&
> -         errno != VMX_INSN_FAIL_INVALID )
> +    if ( vvmcx_valid(current) && errno != VMX_INSN_FAIL_INVALID )
>          vmfail_valid(regs, errno);
>      else
>          vmfail_invalid(regs);
> @@ -805,7 +804,7 @@ static void nvmx_purge_vvmcs(struct vcpu *v)
>      int i;
> 
>      __clear_current_vvmcs(v);
> -    if ( nvcpu->nv_vvmcxaddr != INVALID_PADDR )
> +    if ( vvmcx_valid(v) )
>          hvm_unmap_guest_frame(nvcpu->nv_vvmcx, 1);
>      nvcpu->nv_vvmcx = NULL;
>      nvcpu->nv_vvmcxaddr = INVALID_PADDR;
> @@ -1601,7 +1600,7 @@ static int nvmx_vmresume(struct vcpu *v, struct
> cpu_user_regs *regs)
>      struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
> 
>      /* check VMCS is valid and IO BITMAP is set */
> -    if ( (nvcpu->nv_vvmcxaddr != INVALID_PADDR) &&
> +    if ( vvmcx_valid(v) &&
>              ((nvmx->iobitmap[0] && nvmx->iobitmap[1]) ||
>              !(__n2_exec_control(v) & CPU_BASED_ACTIVATE_IO_BITMAP) ) )
>          nvcpu->nv_vmentry_pending = 1;
> @@ -1622,7 +1621,7 @@ int nvmx_handle_vmresume(struct
> cpu_user_regs *regs)
>      if ( rc != X86EMUL_OKAY )
>          return rc;
> 
> -    if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
> +    if ( !vvmcx_valid(v) )
>      {
>          vmfail_invalid(regs);
>          return X86EMUL_OKAY;
> @@ -1656,7 +1655,7 @@ int nvmx_handle_vmlaunch(struct cpu_user_regs
> *regs)
>      if ( rc != X86EMUL_OKAY )
>          return rc;
> 
> -    if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
> +    if ( !vvmcx_valid(v) )
>      {
>          vmfail_invalid(regs);
>          return X86EMUL_OKAY;
> @@ -1709,7 +1708,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs
> *regs)
>      if ( nvcpu->nv_vvmcxaddr != gpa )
>          nvmx_purge_vvmcs(v);
> 
> -    if ( nvcpu->nv_vvmcxaddr == INVALID_PADDR )
> +    if ( !vvmcx_valid(v) )
>      {
>          bool_t writable;
>          void *vvmcx = hvm_map_guest_frame_rw(paddr_to_pfn(gpa), 1,
> &writable);
> @@ -1848,7 +1847,7 @@ int nvmx_handle_vmread(struct cpu_user_regs
> *regs)
>      if ( rc != X86EMUL_OKAY )
>          return rc;
> 
> -    if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
> +    if ( !vvmcx_valid(v) )
>      {
>          vmfail_invalid(regs);
>          return X86EMUL_OKAY;
> @@ -1891,7 +1890,7 @@ int nvmx_handle_vmwrite(struct cpu_user_regs
> *regs)
>               != X86EMUL_OKAY )
>          return X86EMUL_EXCEPTION;
> 
> -    if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
> +    if ( !vvmcx_valid(v) )
>      {
>          vmfail_invalid(regs);
>          return X86EMUL_OKAY;
> diff --git a/xen/include/asm-x86/hvm/nestedhvm.h b/xen/include/asm-
> x86/hvm/nestedhvm.h
> index 9d1c2742b5..e09fa9d47d 100644
> --- a/xen/include/asm-x86/hvm/nestedhvm.h
> +++ b/xen/include/asm-x86/hvm/nestedhvm.h
> @@ -92,4 +92,9 @@ static inline void nestedhvm_set_cr(struct vcpu *v,
> unsigned int cr,
>          v->arch.hvm.nvcpu.guest_cr[cr] = value;
>  }
> 
> +static inline bool vvmcx_valid(const struct vcpu *v)
> +{
> +    return vcpu_nestedhvm(v).nv_vvmcxaddr != INVALID_PADDR;
> +}
> +
>  #endif /* _HVM_NESTEDHVM_H */
> --
> 2.17.1


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

Re: [PATCH v1 2/6] x86/vvmx: correct vmfail() usage for vmptrld and vmclear

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Friday, October 12, 2018 11:28 PM
> 
> Calling vmfail_valid() is correct only if vvmcx is valid. Modify
> functions to use vmfail() instead which performs the necessary check.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>. btw Wei's suggestion
on adding assert makes sense to me

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

Re: [PATCH v1 3/6] x86/vvmx: add VMX_INSN_VMPTRLD_WITH_VMXON_PTR errno

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Friday, October 12, 2018 11:28 PM
> 
> And make nvmx_handle_vmptrld() return the new errno in case the
> provided
> address is the same as vmxon region address.
> 
> While at it, correct the return value for not-4KB-aligned case.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

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

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

Re: [PATCH v1 4/6] x86/vvmx: add VMX_INSN_VMCLEAR_WITH_VMXON_PTR errno

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Friday, October 12, 2018 11:28 PM
> 
> And make nvmx_handle_vmclear() return the new errno in case the
> provided
> address is the same as vmxon region address.
> 
> While at it, correct the return value for not-4KB-aligned case and for
> invalid physaddr.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c        | 23 ++++++++++++++++++-----
>  xen/include/asm-x86/hvm/vmx/vmcs.h |  1 +
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
> b/xen/arch/x86/hvm/vmx/vvmx.c
> index 4caa5811a1..8b691bfc04 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1804,9 +1804,20 @@ int nvmx_handle_vmclear(struct cpu_user_regs
> *regs)
>          return rc;
> 
>      BUILD_BUG_ON(X86EMUL_OKAY != VMSUCCEED); /* rc = VMSUCCEED;
> */
> +
> +    if ( gpa == vcpu_2_nvmx(v).vmxon_region_pa )
> +    {
> +        vmfail(regs, VMX_INSN_VMCLEAR_WITH_VMXON_PTR);
> +        goto out;
> +    }
> +
>      if ( gpa & 0xfff )
> -        rc = VMFAIL_INVALID;
> -    else if ( gpa == nvcpu->nv_vvmcxaddr )
> +    {
> +        vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
> +        goto out;
> +    }
> +
> +    if ( gpa == nvcpu->nv_vvmcxaddr )
>      {
>          if ( cpu_has_vmx_vmcs_shadowing )
>              nvmx_clear_vmcs_pointer(v, nvcpu->nv_vvmcx);
> @@ -1820,7 +1831,10 @@ int nvmx_handle_vmclear(struct cpu_user_regs
> *regs)
>          bool_t writable;
> 
>          vvmcs = hvm_map_guest_frame_rw(paddr_to_pfn(gpa), 0, &writable);
> -        if ( vvmcs )
> +
> +        if ( !vvmcs )
> +            rc = VMFAIL_VALID;
> +        else
>          {
>              if ( writable )
>                  clear_vvmcs_launched(&nvmx->launched_list,
> @@ -1835,9 +1849,8 @@ int nvmx_handle_vmclear(struct cpu_user_regs
> *regs)
>          vmsucceed(regs);
>      else if ( rc == VMFAIL_VALID )
>          vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
> -    else
> -        vmfail_invalid(regs);
> 

there is only one place poking rc now. clearer to replace rc with direct
vmfail too. then above rc checks can be removed.

> +out:
>      return X86EMUL_OKAY;
>  }
> 
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-
> x86/hvm/vmx/vmcs.h
> index cae1861610..e84d2e482b 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -529,6 +529,7 @@ enum vmx_insn_errno
>  {
>      VMX_INSN_SUCCEED                       = 0,
>      VMX_INSN_VMCLEAR_INVALID_PHYADDR       = 2,
> +    VMX_INSN_VMCLEAR_WITH_VMXON_PTR        = 3,
>      VMX_INSN_VMLAUNCH_NONCLEAR_VMCS        = 4,
>      VMX_INSN_VMRESUME_NONLAUNCHED_VMCS     = 5,
>      VMX_INSN_INVALID_CONTROL_STATE         = 7,
> --
> 2.17.1


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

Re: [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Friday, October 12, 2018 11:28 PM
> 
> The size of Xen's virtual vmcs region is 4096 bytes. Correctly report
> it to the guest in case when VMCS shadowing is not available instead of
> providing H/W value (which is usually smaller).

what is the problem of reporting smaller size even when actual
size is 4096? is L1 expected to access the portion beyond h/w
reported size?

> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
> b/xen/arch/x86/hvm/vmx/vvmx.c
> index 8b691bfc04..2c2ba36d94 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -2064,6 +2064,14 @@ int nvmx_msr_read_intercept(unsigned int msr,
> u64 *msr_content)
>          data = (host_data & (~0ul << 32)) |
>                 (vmcs->vmcs_revision_id & 0x7fffffff);
>          unmap_domain_page(vmcs);
> +
> +        if ( !cpu_has_vmx_vmcs_shadowing )
> +        {
> +            /* Report vmcs_region_size as 4096 */
> +            data &= ~VMX_BASIC_VMCS_SIZE_MASK;
> +            data |= 1ULL << 44;
> +        }
> +
>          break;
>      }
>      case MSR_IA32_VMX_PINBASED_CTLS:
> --
> 2.17.1


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

Re: [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size

[Sergey Dyasli]

On 30/10/2018 08:06, Tian, Kevin wrote:
>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>> Sent: Friday, October 12, 2018 11:28 PM
>>
>> The size of Xen's virtual vmcs region is 4096 bytes. Correctly report
>> it to the guest in case when VMCS shadowing is not available instead of
>> providing H/W value (which is usually smaller).
> 
> what is the problem of reporting smaller size even when actual
> size is 4096? is L1 expected to access the portion beyond h/w
> reported size?
> 

Here's the code snippet from kvm-unit-tests:

	vmcs[0]->hdr.revision_id = basic.revision;
	assert(!vmcs_clear(vmcs[0]));
	assert(!make_vmcs_current(vmcs[0]));
	set_all_vmcs_fields(0x86);

	assert(!vmcs_clear(vmcs[0]));
	memcpy(vmcs[1], vmcs[0], basic.size);
	assert(!make_vmcs_current(vmcs[1]));
	report("test vmclear flush (current VMCS)", check_all_vmcs_fields(0x86));

set_all_vmcs_fields() vmwrites almost 4k, but memcpy() copies only 1024
bytes and vmreads get incorrect values.

>>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>> ---
>>  xen/arch/x86/hvm/vmx/vvmx.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
>> b/xen/arch/x86/hvm/vmx/vvmx.c
>> index 8b691bfc04..2c2ba36d94 100644
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -2064,6 +2064,14 @@ int nvmx_msr_read_intercept(unsigned int msr,
>> u64 *msr_content)
>>          data = (host_data & (~0ul << 32)) |
>>                 (vmcs->vmcs_revision_id & 0x7fffffff);
>>          unmap_domain_page(vmcs);
>> +
>> +        if ( !cpu_has_vmx_vmcs_shadowing )
>> +        {
>> +            /* Report vmcs_region_size as 4096 */
>> +            data &= ~VMX_BASIC_VMCS_SIZE_MASK;
>> +            data |= 1ULL << 44;
>> +        }
>> +
>>          break;
>>      }
>>      case MSR_IA32_VMX_PINBASED_CTLS:
>> --
>> 2.17.1
> 

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

Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()

[Sergey Dyasli]

On 30/10/2018 07:41, Tian, Kevin wrote:
>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>> Sent: Friday, October 12, 2018 11:28 PM
>>
>> As a convenient helper function and refactor the code to use it.
>>
>> No functional change.
>>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> 
> since vmcx is hvm abstracted term, what about using this
> helper in svm side too?

I couldn't find any code in nestedsvm.c that would benefit from this new
helper.

--
Thanks,
Sergey

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

Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Tuesday, October 30, 2018 8:41 PM
> 
> On 30/10/2018 07:41, Tian, Kevin wrote:
> >> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> >> Sent: Friday, October 12, 2018 11:28 PM
> >>
> >> As a convenient helper function and refactor the code to use it.
> >>
> >> No functional change.
> >>
> >> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> >
> > since vmcx is hvm abstracted term, what about using this
> > helper in svm side too?
> 
> I couldn't find any code in nestedsvm.c that would benefit from this new
> helper.
> 

just a quick check:

    if (nv->nv_vvmcx != NULL && nv->nv_vvmcxaddr != vmcbaddr) {
        ASSERT(nv->nv_vvmcxaddr != INVALID_PADDR);
        hvm_unmap_guest_frame(nv->nv_vvmcx, 1);
        nv->nv_vvmcx = NULL;
        nv->nv_vvmcxaddr = INVALID_PADDR;
    }

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

Re: [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Tuesday, October 30, 2018 8:36 PM
> 
> On 30/10/2018 08:06, Tian, Kevin wrote:
> >> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> >> Sent: Friday, October 12, 2018 11:28 PM
> >>
> >> The size of Xen's virtual vmcs region is 4096 bytes. Correctly report
> >> it to the guest in case when VMCS shadowing is not available instead of
> >> providing H/W value (which is usually smaller).
> >
> > what is the problem of reporting smaller size even when actual
> > size is 4096? is L1 expected to access the portion beyond h/w
> > reported size?
> >
> 
> Here's the code snippet from kvm-unit-tests:
> 
> 	vmcs[0]->hdr.revision_id = basic.revision;
> 	assert(!vmcs_clear(vmcs[0]));
> 	assert(!make_vmcs_current(vmcs[0]));
> 	set_all_vmcs_fields(0x86);
> 
> 	assert(!vmcs_clear(vmcs[0]));
> 	memcpy(vmcs[1], vmcs[0], basic.size);
> 	assert(!make_vmcs_current(vmcs[1]));
> 	report("test vmclear flush (current VMCS)",
> check_all_vmcs_fields(0x86));
> 
> set_all_vmcs_fields() vmwrites almost 4k, but memcpy() copies only 1024
> bytes and vmreads get incorrect values.
> 

I didn't understand why set_all_vmcs_fields blindly touch 4k instead of
following reported size. Also I didn't get the reason of this patch - whatever
size reported, xen just needs to emulate hw behavior according to spec,
i.e. do proper emulation if offset < size, otherwise just vmfail. Guest is
not aware of shadow vmcs. why do we want to report different vmcs
size based on presence of shadow vmcs?

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

Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()

[Sergey Dyasli]

On 01/11/2018 02:22, Tian, Kevin wrote:
>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>> Sent: Tuesday, October 30, 2018 8:41 PM
>>
>> On 30/10/2018 07:41, Tian, Kevin wrote:
>>>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>>>> Sent: Friday, October 12, 2018 11:28 PM
>>>>
>>>> As a convenient helper function and refactor the code to use it.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>>>
>>> since vmcx is hvm abstracted term, what about using this
>>> helper in svm side too?
>>
>> I couldn't find any code in nestedsvm.c that would benefit from this new
>> helper.
>>
> 
> just a quick check:
> 
>     if (nv->nv_vvmcx != NULL && nv->nv_vvmcxaddr != vmcbaddr) {
>         ASSERT(nv->nv_vvmcxaddr != INVALID_PADDR);
>         hvm_unmap_guest_frame(nv->nv_vvmcx, 1);
>         nv->nv_vvmcx = NULL;
>         nv->nv_vvmcxaddr = INVALID_PADDR;
>     }

Yes, that's the only usage and it looks better open coded IMHO. But I
can use the new helper here also, if you insist.

Thanks,
Sergey

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

Re: [PATCH v1 5/6] x86/vvmx: correctly report vvmcs size

[Sergey Dyasli]

On 01/11/2018 02:29, Tian, Kevin wrote:
>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>> Sent: Tuesday, October 30, 2018 8:36 PM
>>
>> On 30/10/2018 08:06, Tian, Kevin wrote:
>>>> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
>>>> Sent: Friday, October 12, 2018 11:28 PM
>>>>
>>>> The size of Xen's virtual vmcs region is 4096 bytes. Correctly report
>>>> it to the guest in case when VMCS shadowing is not available instead of
>>>> providing H/W value (which is usually smaller).
>>>
>>> what is the problem of reporting smaller size even when actual
>>> size is 4096? is L1 expected to access the portion beyond h/w
>>> reported size?
>>>
>>
>> Here's the code snippet from kvm-unit-tests:
>>
>> 	vmcs[0]->hdr.revision_id = basic.revision;
>> 	assert(!vmcs_clear(vmcs[0]));
>> 	assert(!make_vmcs_current(vmcs[0]));
>> 	set_all_vmcs_fields(0x86);
>>
>> 	assert(!vmcs_clear(vmcs[0]));
>> 	memcpy(vmcs[1], vmcs[0], basic.size);
>> 	assert(!make_vmcs_current(vmcs[1]));
>> 	report("test vmclear flush (current VMCS)",
>> check_all_vmcs_fields(0x86));
>>
>> set_all_vmcs_fields() vmwrites almost 4k, but memcpy() copies only 1024
>> bytes and vmreads get incorrect values.
>>
> 
> I didn't understand why set_all_vmcs_fields blindly touch 4k instead of
> following reported size. Also I didn't get the reason of this patch - whatever
> size reported, xen just needs to emulate hw behavior according to spec,
> i.e. do proper emulation if offset < size, otherwise just vmfail. Guest is
> not aware of shadow vmcs. why do we want to report different vmcs
> size based on presence of shadow vmcs?

Here's the detailed explanation (for when vmcs shadowing is not
available in H/W):

1. Guest reads vmcs region size as 1024 (from H/W), allocates it and
does vmptrld

2. Xen maps provided guest's memory and uses it as virtual vmcs (vmcs12)

3. Guest uses vmwrites to set up the vmcs

4. During emulation (set_vvmcs_virtual()), Xen writes values into
virtual vmcs but the resulting offset can be larger than 1024 (when
multiplied by sizeof(u64)).

There is even a comment in include/asm-x86/hvm/vmx/vvmx.h:

/*
 * Virtual VMCS layout
 *
 * Since physical VMCS layout is unknown, a custom layout is used
 * for virtual VMCS seen by guest. It occupies a 4k page, and the
 * field is offset by an 9-bit offset into u64[], The offset is as
 * follow, which means every <width, type> pair has a max of 32
 * fields available.
 *
 *             9       7      5               0
 *             --------------------------------
 *     offset: | width | type |     index     |
 *             --------------------------------
 *
 * Also, since the lower range <width=0, type={0,1}> has only one
 * field: VPID, it is moved to a higher offset (63), and leaves the
 * lower range to non-indexed field like VMCS revision.
 *
 */

--
Thanks,
Sergey

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

Re: [PATCH v1 1/6] x86/vvmx: introduce vvmcx_valid()

[Andrew Cooper]

On 12/10/18 16:27, Sergey Dyasli wrote:
> diff --git a/xen/include/asm-x86/hvm/nestedhvm.h b/xen/include/asm-x86/hvm/nestedhvm.h
> index 9d1c2742b5..e09fa9d47d 100644
> --- a/xen/include/asm-x86/hvm/nestedhvm.h
> +++ b/xen/include/asm-x86/hvm/nestedhvm.h
> @@ -92,4 +92,9 @@ static inline void nestedhvm_set_cr(struct vcpu *v, unsigned int cr,
>          v->arch.hvm.nvcpu.guest_cr[cr] = value;
>  }
>  
> +static inline bool vvmcx_valid(const struct vcpu *v)
> +{
> +    return vcpu_nestedhvm(v).nv_vvmcxaddr != INVALID_PADDR;
> +}
> +

Now that XSA-278 is public, I can explain why this predicate is a little
broken.

nv_vvmcxaddr doesn't get initialised until HVM_PARAM_NESTED_VIRT is set
to 1.

As with c/s 18cef4df8, please adjust the vcpu construction path to make
this predicate unconditionally safe to use.

~Andrew

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