[PATCH v2 for-4.7 0/5] Fixes for invlpg handling for HVM guests

[PATCH v2 for-4.7 0/5] Fixes for invlpg handling for HVM guests

[Andrew Cooper]

Turns out there are a lot of broken corner cases.

Changes in v2:
 * Some improvements to commit messages
 * Split part of the original patch 4 out, to make the new patch 5 clearer
 * Add vcpu parameter to new invlpg() function, and avoid assuming 'current'
 * Modify paging_invlpg() to be void, and issue the PV TLB flush as well

Andrew Cooper (5):
  x86/hvm: Always return the linear address from
    hvm_virtual_to_linear_addr()
  x86/hvm: Raise #SS faults for %ss-based segmentation violations
  x86/hvm: Correct the emulated interaction of invlpg with segments
  x86/svm: Don't unconditionally use a new ASID in
    svm_invlpg_intercept()
  x86/hvm: Fix invalidation for emulated invlpg instructions

 xen/arch/x86/hvm/emulate.c      | 20 ++++++++++++++++++--
 xen/arch/x86/hvm/hvm.c          | 37 +++++++++++++++++++++++--------------
 xen/arch/x86/hvm/svm/svm.c      | 11 +++++++----
 xen/arch/x86/hvm/vmx/vmx.c      | 14 +++++++++-----
 xen/arch/x86/mm.c               | 24 ++++++++++++++++++------
 xen/arch/x86/mm/hap/hap.c       | 23 ++++++++++-------------
 xen/arch/x86/mm/shadow/common.c |  3 ++-
 xen/include/asm-x86/hvm/hvm.h   |  4 ++--
 xen/include/asm-x86/paging.h    | 11 ++---------
 9 files changed, 91 insertions(+), 56 deletions(-)

-- 
2.1.4


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

[PATCH v2 for-4.7 1/5] x86/hvm: Always return the linear address from hvm_virtual_to_linear_addr()

[Andrew Cooper]

Some callers need the linear address (with appropriate segment base), whether
or not the limit or canonical check succeeds.

While modifying the function, change the return type to bool_t to match its
semantics.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/hvm.c        | 37 +++++++++++++++++++++++--------------
 xen/include/asm-x86/hvm/hvm.h |  2 +-
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 82e2ed1..863d134 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2411,7 +2411,7 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
     return X86EMUL_EXCEPTION;
 }
 
-int hvm_virtual_to_linear_addr(
+bool_t hvm_virtual_to_linear_addr(
     enum x86_segment seg,
     struct segment_register *reg,
     unsigned long offset,
@@ -2421,6 +2421,7 @@ int hvm_virtual_to_linear_addr(
     unsigned long *linear_addr)
 {
     unsigned long addr = offset, last_byte;
+    bool_t okay = 0;
 
     if ( !(current->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE) )
     {
@@ -2431,7 +2432,7 @@ int hvm_virtual_to_linear_addr(
         addr = (uint32_t)(addr + reg->base);
         last_byte = (uint32_t)addr + bytes - !!bytes;
         if ( last_byte < addr )
-            return 0;
+            goto out;
     }
     else if ( addr_size != 64 )
     {
@@ -2439,15 +2440,21 @@ int hvm_virtual_to_linear_addr(
          * COMPATIBILITY MODE: Apply segment checks and add base.
          */
 
+        /*
+         * Hardware truncates to 32 bits in compatibility mode.
+         * It does not truncate to 16 bits in 16-bit address-size mode.
+         */
+        addr = (uint32_t)(addr + reg->base);
+
         switch ( access_type )
         {
         case hvm_access_read:
             if ( (reg->attr.fields.type & 0xa) == 0x8 )
-                return 0; /* execute-only code segment */
+                goto out; /* execute-only code segment */
             break;
         case hvm_access_write:
             if ( (reg->attr.fields.type & 0xa) != 0x2 )
-                return 0; /* not a writable data segment */
+                goto out; /* not a writable data segment */
             break;
         default:
             break;
@@ -2464,16 +2471,10 @@ int hvm_virtual_to_linear_addr(
 
             /* Check first byte and last byte against respective bounds. */
             if ( (offset <= reg->limit) || (last_byte < offset) )
-                return 0;
+                goto out;
         }
         else if ( (last_byte > reg->limit) || (last_byte < offset) )
-            return 0; /* last byte is beyond limit or wraps 0xFFFFFFFF */
-
-        /*
-         * Hardware truncates to 32 bits in compatibility mode.
-         * It does not truncate to 16 bits in 16-bit address-size mode.
-         */
-        addr = (uint32_t)(addr + reg->base);
+            goto out; /* last byte is beyond limit or wraps 0xFFFFFFFF */
     }
     else
     {
@@ -2487,11 +2488,19 @@ int hvm_virtual_to_linear_addr(
         last_byte = addr + bytes - !!bytes;
         if ( !is_canonical_address(addr) || last_byte < addr ||
              !is_canonical_address(last_byte) )
-            return 0;
+            goto out;
     }
 
+    /* All checks ok. */
+    okay = 1;
+
+ out:
+    /*
+     * Always return the correct linear address, even if a permission check
+     * failed.  The permissions failure is not relevant to some callers.
+     */
     *linear_addr = addr;
-    return 1;
+    return okay;
 }
 
 struct hvm_write_map {
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 7b7ff3f..add6ee8 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -471,7 +471,7 @@ enum hvm_access_type {
     hvm_access_read,
     hvm_access_write
 };
-int hvm_virtual_to_linear_addr(
+bool_t hvm_virtual_to_linear_addr(
     enum x86_segment seg,
     struct segment_register *reg,
     unsigned long offset,
-- 
2.1.4


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

[PATCH v2 for-4.7 2/5] x86/hvm: Raise #SS faults for %ss-based segmentation violations

[Andrew Cooper]

Raising #GP under such circumstances is architecturally wrong.  (Refer
to the Intel or AMD manuals describing the conditions under which the

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Tim Deegan <tim@xen.org>
---
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

v2:
 * Clarified the commit message.
---
 xen/arch/x86/hvm/emulate.c      | 3 ++-
 xen/arch/x86/mm/shadow/common.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index be1e7c2..ee5cf1f 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -566,7 +566,8 @@ static int hvmemul_virtual_to_linear(
 
     /* This is a singleton operation: fail it with an exception. */
     hvmemul_ctxt->exn_pending = 1;
-    hvmemul_ctxt->trap.vector = TRAP_gp_fault;
+    hvmemul_ctxt->trap.vector =
+        (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault;
     hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
     hvmemul_ctxt->trap.error_code = 0;
     hvmemul_ctxt->trap.insn_len = 0;
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 559d4a4..226e32d 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -148,7 +148,8 @@ static int hvm_translate_linear_addr(
 
     if ( !okay )
     {
-        hvm_inject_hw_exception(TRAP_gp_fault, 0);
+        hvm_inject_hw_exception(
+            (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault, 0);
         return X86EMUL_EXCEPTION;
     }
 
-- 
2.1.4


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

[PATCH v2 for-4.7 3/5] x86/hvm: Correct the emulated interaction of invlpg with segments

[Andrew Cooper]

The `invlpg` instruction is documented to take a memory address, and is not
documented to suffer faults from segmentation violations.  It is also
explicitly documented to be a NOP when issued on a non-canonical address.

Experimentally, and subsequently confirmed by both Intel and AMD, the
instruction does take into account segment bases, but will happily invalidate
a TLB entry for a mapping beyond the segment limit.

The emulation logic will currently raise #GP/#SS faults for segment limit
violations, or non-canonical addresses, which doesn't match hardware's
behaviour.  Instead, squash exceptions generated by
hvmemul_virtual_to_linear() and proceed with invalidation.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/emulate.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index ee5cf1f..e6316be 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1608,7 +1608,22 @@ static int hvmemul_invlpg(
     rc = hvmemul_virtual_to_linear(
         seg, offset, 1, &reps, hvm_access_none, hvmemul_ctxt, &addr);
 
-    if ( rc == X86EMUL_OKAY )
+    if ( rc == X86EMUL_EXCEPTION )
+    {
+        /*
+         * `invlpg` takes segment bases into account, but is not subject to
+         * faults from segment type/limit checks, and is specified as a NOP
+         * when issued on non-canonical addresses.
+         *
+         * hvmemul_virtual_to_linear() raises exceptions for type/limit
+         * violations, so squash them.
+         */
+        hvmemul_ctxt->exn_pending = 0;
+        hvmemul_ctxt->trap = (struct hvm_trap){};
+        rc = X86EMUL_OKAY;
+    }
+
+    if ( rc == X86EMUL_OKAY && is_canonical_address(addr) )
         hvm_funcs.invlpg_intercept(addr);
 
     return rc;
-- 
2.1.4


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

[PATCH v2 for-4.7 4/5] x86/svm: Don't unconditionally use a new ASID in svm_invlpg_intercept()

[Andrew Cooper]

paging_invlpg() already returns a boolean indicating whether an invalidation
is necessary or not.  A return value of 0 indicates that the specified virtual
address wasn't shadowed (or has already been flushed), cannot currently be
cached in the TLB.

This is a performance optimisation.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

While being a performance optimisation, the main purpose of splitting this
patch out is to separate the functional change.  The following patch performs
some function shuffling, and this patch makes the following one more obviously
correct.

v2:
 * Newly split out
---
 xen/arch/x86/hvm/svm/svm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 7634c3f..081a5d8 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2224,8 +2224,8 @@ static void svm_invlpg_intercept(unsigned long vaddr)
 {
     struct vcpu *curr = current;
     HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(vaddr));
-    paging_invlpg(curr, vaddr);
-    svm_asid_g_invlpg(curr, vaddr);
+    if ( paging_invlpg(curr, vaddr) )
+        svm_asid_g_invlpg(curr, vaddr);
 }
 
 static struct hvm_function_table __initdata svm_function_table = {
-- 
2.1.4


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

[PATCH v2 for-4.7 5/5] x86/hvm: Fix invalidation for emulated invlpg instructions

[Andrew Cooper]

hap_invlpg() is reachable from the instruction emulator, which means
introspection and tests using hvm_fep can end up here.  As such, crashing the
domain is not an appropriate action to take.

Fixing this involves rearranging the callgraph.

paging_invlpg() is now the central entry point.  It first checks for the
non-canonical NOP case, and calls ino the paging subsystem.  If a real flush
is needed, it will call the appropriate handler for the vcpu.  This allows the
PV callsites of paging_invlpg() to be simplified.

The sole user of hvm_funcs.invlpg_intercept() is altered to use
paging_invlpg() instead, allowing the .invlpg_intercept() hook to be removed.

For both VMX and SVM, the existing $VENDOR_invlpg_intercept() is split in
half.  $VENDOR_invlpg_intercept() stays as the intercept handler only (which
just calls paging_invlpg()), and new $VENDOR_invlpg() functions do the
ASID/VPID management.  These later functions are made available in hvm_funcs
for paging_invlpg() to use.

As a result, correct ASID/VPID management occurs for the hvmemul path, even if
it did not originate from an real hardware intercept.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Tim Deegan <tim@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Wei Liu <wei.liu2@citrix.com>

v2:
 * Split performance improvement for AMD into previous patch
 * Add vcpu parameter to new invlpg() function, and avoid assuming 'current'
 * Modify paging_invlpg() to be void, and issue the PV TLB flush as well
---
 xen/arch/x86/hvm/emulate.c    |  4 ++--
 xen/arch/x86/hvm/svm/svm.c    | 11 +++++++----
 xen/arch/x86/hvm/vmx/vmx.c    | 14 +++++++++-----
 xen/arch/x86/mm.c             | 24 ++++++++++++++++++------
 xen/arch/x86/mm/hap/hap.c     | 23 ++++++++++-------------
 xen/include/asm-x86/hvm/hvm.h |  2 +-
 xen/include/asm-x86/paging.h  | 11 ++---------
 7 files changed, 49 insertions(+), 40 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index e6316be..b9cac8e 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1623,8 +1623,8 @@ static int hvmemul_invlpg(
         rc = X86EMUL_OKAY;
     }
 
-    if ( rc == X86EMUL_OKAY && is_canonical_address(addr) )
-        hvm_funcs.invlpg_intercept(addr);
+    if ( rc == X86EMUL_OKAY )
+        paging_invlpg(current, addr);
 
     return rc;
 }
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 081a5d8..679e615 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2222,10 +2222,13 @@ static void svm_invlpga_intercept(
 
 static void svm_invlpg_intercept(unsigned long vaddr)
 {
-    struct vcpu *curr = current;
     HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(vaddr));
-    if ( paging_invlpg(curr, vaddr) )
-        svm_asid_g_invlpg(curr, vaddr);
+    paging_invlpg(current, vaddr);
+}
+
+static void svm_invlpg(struct vcpu *v, unsigned long vaddr)
+{
+    svm_asid_g_invlpg(v, vaddr);
 }
 
 static struct hvm_function_table __initdata svm_function_table = {
@@ -2258,12 +2261,12 @@ static struct hvm_function_table __initdata svm_function_table = {
     .inject_trap          = svm_inject_trap,
     .init_hypercall_page  = svm_init_hypercall_page,
     .event_pending        = svm_event_pending,
+    .invlpg               = svm_invlpg,
     .cpuid_intercept      = svm_cpuid_intercept,
     .wbinvd_intercept     = svm_wbinvd_intercept,
     .fpu_dirty_intercept  = svm_fpu_dirty_intercept,
     .msr_read_intercept   = svm_msr_read_intercept,
     .msr_write_intercept  = svm_msr_write_intercept,
-    .invlpg_intercept     = svm_invlpg_intercept,
     .set_rdtsc_exiting    = svm_set_rdtsc_exiting,
     .get_insn_bytes       = svm_get_insn_bytes,
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index bc4410f..3acf1ab 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -81,7 +81,7 @@ static void vmx_wbinvd_intercept(void);
 static void vmx_fpu_dirty_intercept(void);
 static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
 static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content);
-static void vmx_invlpg_intercept(unsigned long vaddr);
+static void vmx_invlpg(struct vcpu *v, unsigned long vaddr);
 static int vmx_vmfunc_intercept(struct cpu_user_regs *regs);
 
 struct vmx_pi_blocking_vcpu {
@@ -2137,6 +2137,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
     .inject_trap          = vmx_inject_trap,
     .init_hypercall_page  = vmx_init_hypercall_page,
     .event_pending        = vmx_event_pending,
+    .invlpg               = vmx_invlpg,
     .cpu_up               = vmx_cpu_up,
     .cpu_down             = vmx_cpu_down,
     .cpuid_intercept      = vmx_cpuid_intercept,
@@ -2144,7 +2145,6 @@ static struct hvm_function_table __initdata vmx_function_table = {
     .fpu_dirty_intercept  = vmx_fpu_dirty_intercept,
     .msr_read_intercept   = vmx_msr_read_intercept,
     .msr_write_intercept  = vmx_msr_write_intercept,
-    .invlpg_intercept     = vmx_invlpg_intercept,
     .vmfunc_intercept     = vmx_vmfunc_intercept,
     .handle_cd            = vmx_handle_cd,
     .set_info_guest       = vmx_set_info_guest,
@@ -2432,10 +2432,14 @@ static void vmx_dr_access(unsigned long exit_qualification,
 
 static void vmx_invlpg_intercept(unsigned long vaddr)
 {
-    struct vcpu *curr = current;
     HVMTRACE_LONG_2D(INVLPG, /*invlpga=*/ 0, TRC_PAR_LONG(vaddr));
-    if ( paging_invlpg(curr, vaddr) && cpu_has_vmx_vpid )
-        vpid_sync_vcpu_gva(curr, vaddr);
+    paging_invlpg(current, vaddr);
+}
+
+static void vmx_invlpg(struct vcpu *v, unsigned long vaddr)
+{
+    if ( cpu_has_vmx_vpid )
+        vpid_sync_vcpu_gva(v, vaddr);
 }
 
 static int vmx_vmfunc_intercept(struct cpu_user_regs *regs)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 2bb920b..03a4d5b 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3386,10 +3386,8 @@ long do_mmuext_op(
         case MMUEXT_INVLPG_LOCAL:
             if ( unlikely(d != pg_owner) )
                 rc = -EPERM;
-            else if ( !paging_mode_enabled(d)
-                      ? __addr_ok(op.arg1.linear_addr)
-                      : paging_invlpg(curr, op.arg1.linear_addr) )
-                flush_tlb_one_local(op.arg1.linear_addr);
+            else
+                paging_invlpg(curr, op.arg1.linear_addr);
             break;
 
         case MMUEXT_TLB_FLUSH_MULTI:
@@ -4510,8 +4508,7 @@ static int __do_update_va_mapping(
         switch ( (bmap_ptr = flags & ~UVMF_FLUSHTYPE_MASK) )
         {
         case UVMF_LOCAL:
-            if ( !paging_mode_enabled(d) || paging_invlpg(v, va) )
-                flush_tlb_one_local(va);
+            paging_invlpg(v, va);
             break;
         case UVMF_ALL:
             flush_tlb_one_mask(d->domain_dirty_cpumask, va);
@@ -6478,6 +6475,21 @@ const unsigned long *__init get_platform_badpages(unsigned int *array_size)
     return bad_pages;
 }
 
+void paging_invlpg(struct vcpu *v, unsigned long va)
+{
+    if ( !is_canonical_address(va) )
+        return;
+
+    if ( paging_mode_enabled(v->domain) &&
+         !paging_get_hostmode(v)->invlpg(v, va) )
+        return;
+
+    if ( is_pv_vcpu(v) )
+        flush_tlb_one_local(va);
+    else
+        hvm_funcs.invlpg(v, va);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 4ab99bb..9c2cd49 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -680,23 +680,20 @@ static int hap_page_fault(struct vcpu *v, unsigned long va,
 
 /*
  * HAP guests can handle invlpg without needing any action from Xen, so
- * should not be intercepting it.
+ * should not be intercepting it.  However, we need to correctly handle
+ * getting here from instruction emulation.
  */
 static bool_t hap_invlpg(struct vcpu *v, unsigned long va)
 {
-    if (nestedhvm_enabled(v->domain)) {
-        /* Emulate INVLPGA:
-         * Must perform the flush right now or an other vcpu may
-         * use it when we use the next VMRUN emulation, otherwise.
-         */
-        if ( vcpu_nestedhvm(v).nv_p2m )
-            p2m_flush(v, vcpu_nestedhvm(v).nv_p2m);
-        return 1;
-    }
+    /*
+     * Emulate INVLPGA:
+     * Must perform the flush right now or an other vcpu may
+     * use it when we use the next VMRUN emulation, otherwise.
+     */
+    if ( nestedhvm_enabled(v->domain) && vcpu_nestedhvm(v).nv_p2m )
+        p2m_flush(v, vcpu_nestedhvm(v).nv_p2m);
 
-    HAP_ERROR("Intercepted a guest INVLPG (%pv) with HAP enabled\n", v);
-    domain_crash(v->domain);
-    return 0;
+    return 1;
 }
 
 static void hap_update_cr3(struct vcpu *v, int do_locking)
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index add6ee8..ddbcbe8 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -154,6 +154,7 @@ struct hvm_function_table {
     void (*init_hypercall_page)(struct domain *d, void *hypercall_page);
 
     int  (*event_pending)(struct vcpu *v);
+    void (*invlpg)(struct vcpu *v, unsigned long vaddr);
 
     int  (*cpu_up_prepare)(unsigned int cpu);
     void (*cpu_dead)(unsigned int cpu);
@@ -172,7 +173,6 @@ struct hvm_function_table {
     void (*fpu_dirty_intercept)(void);
     int (*msr_read_intercept)(unsigned int msr, uint64_t *msr_content);
     int (*msr_write_intercept)(unsigned int msr, uint64_t msr_content);
-    void (*invlpg_intercept)(unsigned long vaddr);
     int (*vmfunc_intercept)(struct cpu_user_regs *regs);
     void (*handle_cd)(struct vcpu *v, unsigned long value);
     void (*set_info_guest)(struct vcpu *v);
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index ab131cc..a1401ab 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -237,15 +237,8 @@ paging_fault(unsigned long va, struct cpu_user_regs *regs)
     return paging_get_hostmode(v)->page_fault(v, va, regs);
 }
 
-/* Handle invlpg requests on vcpus.
- * Returns 1 if the invlpg instruction should be issued on the hardware,
- * or 0 if it's safe not to do so. */
-static inline bool_t paging_invlpg(struct vcpu *v, unsigned long va)
-{
-    return (paging_mode_external(v->domain) ? is_canonical_address(va)
-                                            : __addr_ok(va)) &&
-           paging_get_hostmode(v)->invlpg(v, va);
-}
+/* Handle invlpg requests on vcpus. */
+void paging_invlpg(struct vcpu *v, unsigned long va);
 
 /* Translate a guest virtual address to the frame number that the
  * *guest* pagetables would map it to.  Returns INVALID_GFN if the guest
-- 
2.1.4


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

Re: [PATCH v2 for-4.7 4/5] x86/svm: Don't unconditionally use a new ASID in svm_invlpg_intercept()

[Boris Ostrovsky]

On 05/09/2016 02:27 PM, Andrew Cooper wrote:
> paging_invlpg() already returns a boolean indicating whether an invalidation
> is necessary or not.  A return value of 0 indicates that the specified virtual
> address wasn't shadowed (or has already been flushed), cannot currently be
> cached in the TLB.
>
> This is a performance optimisation.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> While being a performance optimisation, the main purpose of splitting this
> patch out is to separate the functional change.  The following patch performs
> some function shuffling, and this patch makes the following one more obviously
> correct.
>
> v2:
>  * Newly split out
> ---
>  xen/arch/x86/hvm/svm/svm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 7634c3f..081a5d8 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2224,8 +2224,8 @@ static void svm_invlpg_intercept(unsigned long vaddr)
>  {
>      struct vcpu *curr = current;
>      HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(vaddr));
> -    paging_invlpg(curr, vaddr);
> -    svm_asid_g_invlpg(curr, vaddr);
> +    if ( paging_invlpg(curr, vaddr) )
> +        svm_asid_g_invlpg(curr, vaddr);
>  }
>  
>  static struct hvm_function_table __initdata svm_function_table = {


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

Re: [PATCH v2 for-4.7 5/5] x86/hvm: Fix invalidation for emulated invlpg instructions

[Boris Ostrovsky]

On 05/09/2016 02:27 PM, Andrew Cooper wrote:
> hap_invlpg() is reachable from the instruction emulator, which means
> introspection and tests using hvm_fep can end up here.  As such, crashing the
> domain is not an appropriate action to take.
>
> Fixing this involves rearranging the callgraph.
>
> paging_invlpg() is now the central entry point.  It first checks for the
> non-canonical NOP case, and calls ino the paging subsystem.  If a real flush
> is needed, it will call the appropriate handler for the vcpu.  This allows the
> PV callsites of paging_invlpg() to be simplified.
>
> The sole user of hvm_funcs.invlpg_intercept() is altered to use
> paging_invlpg() instead, allowing the .invlpg_intercept() hook to be removed.
>
> For both VMX and SVM, the existing $VENDOR_invlpg_intercept() is split in
> half.  $VENDOR_invlpg_intercept() stays as the intercept handler only (which
> just calls paging_invlpg()), and new $VENDOR_invlpg() functions do the
> ASID/VPID management.  These later functions are made available in hvm_funcs
> for paging_invlpg() to use.
>
> As a result, correct ASID/VPID management occurs for the hvmemul path, even if
> it did not originate from an real hardware intercept.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


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

Re: [PATCH v2 for-4.7 2/5] x86/hvm: Raise #SS faults for %ss-based segmentation violations

[Jan Beulich]

>>> On 09.05.16 at 20:27, <andrew.cooper3@citrix.com> wrote:
> Raising #GP under such circumstances is architecturally wrong.  (Refer
> to the Intel or AMD manuals describing the conditions under which the

This sentence is pretty clearly unfinished.

Jan


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

Re: [PATCH v2 for-4.7 2/5] x86/hvm: Raise #SS faults for %ss-based segmentation violations

[Andrew Cooper]

On 10/05/2016 08:39, Jan Beulich wrote:
>>>> On 09.05.16 at 20:27, <andrew.cooper3@citrix.com> wrote:
>> Raising #GP under such circumstances is architecturally wrong.  (Refer
>> to the Intel or AMD manuals describing the conditions under which the
> This sentence is pretty clearly unfinished.

So it is.  It started #SS and Git helpfully stripped the comment out of
the final committed message.  I will fix it up.

~Andrew

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

Re: [PATCH v2 for-4.7 5/5] x86/hvm: Fix invalidation for emulated invlpg instructions

[Jan Beulich]

>>> On 09.05.16 at 20:27, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2222,10 +2222,13 @@ static void svm_invlpga_intercept(
>  
>  static void svm_invlpg_intercept(unsigned long vaddr)
>  {
> -    struct vcpu *curr = current;
>      HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(vaddr));
> -    if ( paging_invlpg(curr, vaddr) )
> -        svm_asid_g_invlpg(curr, vaddr);
> +    paging_invlpg(current, vaddr);
> +}
> +
> +static void svm_invlpg(struct vcpu *v, unsigned long vaddr)
> +{
> +    svm_asid_g_invlpg(v, vaddr);
>  }

I don't see the need for the wrapper: svm_asid_g_invlpg() is itself
suitable to be used ...

> @@ -2258,12 +2261,12 @@ static struct hvm_function_table __initdata svm_function_table = {
>      .inject_trap          = svm_inject_trap,
>      .init_hypercall_page  = svm_init_hypercall_page,
>      .event_pending        = svm_event_pending,
> +    .invlpg               = svm_invlpg,

... here.

> @@ -2137,6 +2137,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
>      .inject_trap          = vmx_inject_trap,
>      .init_hypercall_page  = vmx_init_hypercall_page,
>      .event_pending        = vmx_event_pending,
> +    .invlpg               = vmx_invlpg,

Similarly here, except that the pointer would need zapping if
!cpu_has_vmx_vpid, requiring ...

> +void paging_invlpg(struct vcpu *v, unsigned long va)
> +{
> +    if ( !is_canonical_address(va) )
> +        return;
> +
> +    if ( paging_mode_enabled(v->domain) &&
> +         !paging_get_hostmode(v)->invlpg(v, va) )
> +        return;
> +
> +    if ( is_pv_vcpu(v) )
> +        flush_tlb_one_local(va);
> +    else
> +        hvm_funcs.invlpg(v, va);

... this call to become conditional.

Nevertheless, since functionally the patch is fine, with or without
said cleanup
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH v2 for-4.7 5/5] x86/hvm: Fix invalidation for emulated invlpg instructions

[George Dunlap]

On 09/05/16 19:27, Andrew Cooper wrote:
> hap_invlpg() is reachable from the instruction emulator, which means
> introspection and tests using hvm_fep can end up here.  As such, crashing the
> domain is not an appropriate action to take.
> 
> Fixing this involves rearranging the callgraph.
> 
> paging_invlpg() is now the central entry point.  It first checks for the
> non-canonical NOP case, and calls ino the paging subsystem.  If a real flush
> is needed, it will call the appropriate handler for the vcpu.  This allows the
> PV callsites of paging_invlpg() to be simplified.
> 
> The sole user of hvm_funcs.invlpg_intercept() is altered to use
> paging_invlpg() instead, allowing the .invlpg_intercept() hook to be removed.
> 
> For both VMX and SVM, the existing $VENDOR_invlpg_intercept() is split in
> half.  $VENDOR_invlpg_intercept() stays as the intercept handler only (which
> just calls paging_invlpg()), and new $VENDOR_invlpg() functions do the
> ASID/VPID management.  These later functions are made available in hvm_funcs
> for paging_invlpg() to use.
> 
> As a result, correct ASID/VPID management occurs for the hvmemul path, even if
> it did not originate from an real hardware intercept.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I haven't reviewed it in detail but it looks like a good idea; and based
on the other Reviewed-bys (and with or without Jan's suggested clean-up):

Acked-by: George Dunlap <george.dunlap@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> v2:
>  * Split performance improvement for AMD into previous patch
>  * Add vcpu parameter to new invlpg() function, and avoid assuming 'current'
>  * Modify paging_invlpg() to be void, and issue the PV TLB flush as well
> ---
>  xen/arch/x86/hvm/emulate.c    |  4 ++--
>  xen/arch/x86/hvm/svm/svm.c    | 11 +++++++----
>  xen/arch/x86/hvm/vmx/vmx.c    | 14 +++++++++-----
>  xen/arch/x86/mm.c             | 24 ++++++++++++++++++------
>  xen/arch/x86/mm/hap/hap.c     | 23 ++++++++++-------------
>  xen/include/asm-x86/hvm/hvm.h |  2 +-
>  xen/include/asm-x86/paging.h  | 11 ++---------
>  7 files changed, 49 insertions(+), 40 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index e6316be..b9cac8e 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1623,8 +1623,8 @@ static int hvmemul_invlpg(
>          rc = X86EMUL_OKAY;
>      }
>  
> -    if ( rc == X86EMUL_OKAY && is_canonical_address(addr) )
> -        hvm_funcs.invlpg_intercept(addr);
> +    if ( rc == X86EMUL_OKAY )
> +        paging_invlpg(current, addr);
>  
>      return rc;
>  }
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 081a5d8..679e615 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2222,10 +2222,13 @@ static void svm_invlpga_intercept(
>  
>  static void svm_invlpg_intercept(unsigned long vaddr)
>  {
> -    struct vcpu *curr = current;
>      HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(vaddr));
> -    if ( paging_invlpg(curr, vaddr) )
> -        svm_asid_g_invlpg(curr, vaddr);
> +    paging_invlpg(current, vaddr);
> +}
> +
> +static void svm_invlpg(struct vcpu *v, unsigned long vaddr)
> +{
> +    svm_asid_g_invlpg(v, vaddr);
>  }
>  
>  static struct hvm_function_table __initdata svm_function_table = {
> @@ -2258,12 +2261,12 @@ static struct hvm_function_table __initdata svm_function_table = {
>      .inject_trap          = svm_inject_trap,
>      .init_hypercall_page  = svm_init_hypercall_page,
>      .event_pending        = svm_event_pending,
> +    .invlpg               = svm_invlpg,
>      .cpuid_intercept      = svm_cpuid_intercept,
>      .wbinvd_intercept     = svm_wbinvd_intercept,
>      .fpu_dirty_intercept  = svm_fpu_dirty_intercept,
>      .msr_read_intercept   = svm_msr_read_intercept,
>      .msr_write_intercept  = svm_msr_write_intercept,
> -    .invlpg_intercept     = svm_invlpg_intercept,
>      .set_rdtsc_exiting    = svm_set_rdtsc_exiting,
>      .get_insn_bytes       = svm_get_insn_bytes,
>  
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index bc4410f..3acf1ab 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -81,7 +81,7 @@ static void vmx_wbinvd_intercept(void);
>  static void vmx_fpu_dirty_intercept(void);
>  static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
>  static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content);
> -static void vmx_invlpg_intercept(unsigned long vaddr);
> +static void vmx_invlpg(struct vcpu *v, unsigned long vaddr);
>  static int vmx_vmfunc_intercept(struct cpu_user_regs *regs);
>  
>  struct vmx_pi_blocking_vcpu {
> @@ -2137,6 +2137,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
>      .inject_trap          = vmx_inject_trap,
>      .init_hypercall_page  = vmx_init_hypercall_page,
>      .event_pending        = vmx_event_pending,
> +    .invlpg               = vmx_invlpg,
>      .cpu_up               = vmx_cpu_up,
>      .cpu_down             = vmx_cpu_down,
>      .cpuid_intercept      = vmx_cpuid_intercept,
> @@ -2144,7 +2145,6 @@ static struct hvm_function_table __initdata vmx_function_table = {
>      .fpu_dirty_intercept  = vmx_fpu_dirty_intercept,
>      .msr_read_intercept   = vmx_msr_read_intercept,
>      .msr_write_intercept  = vmx_msr_write_intercept,
> -    .invlpg_intercept     = vmx_invlpg_intercept,
>      .vmfunc_intercept     = vmx_vmfunc_intercept,
>      .handle_cd            = vmx_handle_cd,
>      .set_info_guest       = vmx_set_info_guest,
> @@ -2432,10 +2432,14 @@ static void vmx_dr_access(unsigned long exit_qualification,
>  
>  static void vmx_invlpg_intercept(unsigned long vaddr)
>  {
> -    struct vcpu *curr = current;
>      HVMTRACE_LONG_2D(INVLPG, /*invlpga=*/ 0, TRC_PAR_LONG(vaddr));
> -    if ( paging_invlpg(curr, vaddr) && cpu_has_vmx_vpid )
> -        vpid_sync_vcpu_gva(curr, vaddr);
> +    paging_invlpg(current, vaddr);
> +}
> +
> +static void vmx_invlpg(struct vcpu *v, unsigned long vaddr)
> +{
> +    if ( cpu_has_vmx_vpid )
> +        vpid_sync_vcpu_gva(v, vaddr);
>  }
>  
>  static int vmx_vmfunc_intercept(struct cpu_user_regs *regs)
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 2bb920b..03a4d5b 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3386,10 +3386,8 @@ long do_mmuext_op(
>          case MMUEXT_INVLPG_LOCAL:
>              if ( unlikely(d != pg_owner) )
>                  rc = -EPERM;
> -            else if ( !paging_mode_enabled(d)
> -                      ? __addr_ok(op.arg1.linear_addr)
> -                      : paging_invlpg(curr, op.arg1.linear_addr) )
> -                flush_tlb_one_local(op.arg1.linear_addr);
> +            else
> +                paging_invlpg(curr, op.arg1.linear_addr);
>              break;
>  
>          case MMUEXT_TLB_FLUSH_MULTI:
> @@ -4510,8 +4508,7 @@ static int __do_update_va_mapping(
>          switch ( (bmap_ptr = flags & ~UVMF_FLUSHTYPE_MASK) )
>          {
>          case UVMF_LOCAL:
> -            if ( !paging_mode_enabled(d) || paging_invlpg(v, va) )
> -                flush_tlb_one_local(va);
> +            paging_invlpg(v, va);
>              break;
>          case UVMF_ALL:
>              flush_tlb_one_mask(d->domain_dirty_cpumask, va);
> @@ -6478,6 +6475,21 @@ const unsigned long *__init get_platform_badpages(unsigned int *array_size)
>      return bad_pages;
>  }
>  
> +void paging_invlpg(struct vcpu *v, unsigned long va)
> +{
> +    if ( !is_canonical_address(va) )
> +        return;
> +
> +    if ( paging_mode_enabled(v->domain) &&
> +         !paging_get_hostmode(v)->invlpg(v, va) )
> +        return;
> +
> +    if ( is_pv_vcpu(v) )
> +        flush_tlb_one_local(va);
> +    else
> +        hvm_funcs.invlpg(v, va);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 4ab99bb..9c2cd49 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -680,23 +680,20 @@ static int hap_page_fault(struct vcpu *v, unsigned long va,
>  
>  /*
>   * HAP guests can handle invlpg without needing any action from Xen, so
> - * should not be intercepting it.
> + * should not be intercepting it.  However, we need to correctly handle
> + * getting here from instruction emulation.
>   */
>  static bool_t hap_invlpg(struct vcpu *v, unsigned long va)
>  {
> -    if (nestedhvm_enabled(v->domain)) {
> -        /* Emulate INVLPGA:
> -         * Must perform the flush right now or an other vcpu may
> -         * use it when we use the next VMRUN emulation, otherwise.
> -         */
> -        if ( vcpu_nestedhvm(v).nv_p2m )
> -            p2m_flush(v, vcpu_nestedhvm(v).nv_p2m);
> -        return 1;
> -    }
> +    /*
> +     * Emulate INVLPGA:
> +     * Must perform the flush right now or an other vcpu may
> +     * use it when we use the next VMRUN emulation, otherwise.
> +     */
> +    if ( nestedhvm_enabled(v->domain) && vcpu_nestedhvm(v).nv_p2m )
> +        p2m_flush(v, vcpu_nestedhvm(v).nv_p2m);
>  
> -    HAP_ERROR("Intercepted a guest INVLPG (%pv) with HAP enabled\n", v);
> -    domain_crash(v->domain);
> -    return 0;
> +    return 1;
>  }
>  
>  static void hap_update_cr3(struct vcpu *v, int do_locking)
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index add6ee8..ddbcbe8 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -154,6 +154,7 @@ struct hvm_function_table {
>      void (*init_hypercall_page)(struct domain *d, void *hypercall_page);
>  
>      int  (*event_pending)(struct vcpu *v);
> +    void (*invlpg)(struct vcpu *v, unsigned long vaddr);
>  
>      int  (*cpu_up_prepare)(unsigned int cpu);
>      void (*cpu_dead)(unsigned int cpu);
> @@ -172,7 +173,6 @@ struct hvm_function_table {
>      void (*fpu_dirty_intercept)(void);
>      int (*msr_read_intercept)(unsigned int msr, uint64_t *msr_content);
>      int (*msr_write_intercept)(unsigned int msr, uint64_t msr_content);
> -    void (*invlpg_intercept)(unsigned long vaddr);
>      int (*vmfunc_intercept)(struct cpu_user_regs *regs);
>      void (*handle_cd)(struct vcpu *v, unsigned long value);
>      void (*set_info_guest)(struct vcpu *v);
> diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
> index ab131cc..a1401ab 100644
> --- a/xen/include/asm-x86/paging.h
> +++ b/xen/include/asm-x86/paging.h
> @@ -237,15 +237,8 @@ paging_fault(unsigned long va, struct cpu_user_regs *regs)
>      return paging_get_hostmode(v)->page_fault(v, va, regs);
>  }
>  
> -/* Handle invlpg requests on vcpus.
> - * Returns 1 if the invlpg instruction should be issued on the hardware,
> - * or 0 if it's safe not to do so. */
> -static inline bool_t paging_invlpg(struct vcpu *v, unsigned long va)
> -{
> -    return (paging_mode_external(v->domain) ? is_canonical_address(va)
> -                                            : __addr_ok(va)) &&
> -           paging_get_hostmode(v)->invlpg(v, va);
> -}
> +/* Handle invlpg requests on vcpus. */
> +void paging_invlpg(struct vcpu *v, unsigned long va);
>  
>  /* Translate a guest virtual address to the frame number that the
>   * *guest* pagetables would map it to.  Returns INVALID_GFN if the guest
> 


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

Re: [PATCH v2 for-4.7 0/5] Fixes for invlpg handling for HVM guests

[Wei Liu]

On Mon, May 09, 2016 at 07:27:47PM +0100, Andrew Cooper wrote:
> Turns out there are a lot of broken corner cases.
> 
> Changes in v2:
>  * Some improvements to commit messages
>  * Split part of the original patch 4 out, to make the new patch 5 clearer
>  * Add vcpu parameter to new invlpg() function, and avoid assuming 'current'
>  * Modify paging_invlpg() to be void, and issue the PV TLB flush as well
> 
> Andrew Cooper (5):
>   x86/hvm: Always return the linear address from
>     hvm_virtual_to_linear_addr()
>   x86/hvm: Raise #SS faults for %ss-based segmentation violations
>   x86/hvm: Correct the emulated interaction of invlpg with segments
>   x86/svm: Don't unconditionally use a new ASID in
>     svm_invlpg_intercept()
>   x86/hvm: Fix invalidation for emulated invlpg instructions
> 

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

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

Re: [PATCH v2 for-4.7 5/5] x86/hvm: Fix invalidation for emulated invlpg instructions

[Andrew Cooper]

On 10/05/16 08:51, Jan Beulich wrote:
>>>> On 09.05.16 at 20:27, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2222,10 +2222,13 @@ static void svm_invlpga_intercept(
>>  
>>  static void svm_invlpg_intercept(unsigned long vaddr)
>>  {
>> -    struct vcpu *curr = current;
>>      HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(vaddr));
>> -    if ( paging_invlpg(curr, vaddr) )
>> -        svm_asid_g_invlpg(curr, vaddr);
>> +    paging_invlpg(current, vaddr);
>> +}
>> +
>> +static void svm_invlpg(struct vcpu *v, unsigned long vaddr)
>> +{
>> +    svm_asid_g_invlpg(v, vaddr);
>>  }
> I don't see the need for the wrapper: svm_asid_g_invlpg() is itself
> suitable to be used ...
>
>> @@ -2258,12 +2261,12 @@ static struct hvm_function_table __initdata svm_function_table = {
>>      .inject_trap          = svm_inject_trap,
>>      .init_hypercall_page  = svm_init_hypercall_page,
>>      .event_pending        = svm_event_pending,
>> +    .invlpg               = svm_invlpg,
> ... here.

So it can.  I will do this...

>
>> @@ -2137,6 +2137,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
>>      .inject_trap          = vmx_inject_trap,
>>      .init_hypercall_page  = vmx_init_hypercall_page,
>>      .event_pending        = vmx_event_pending,
>> +    .invlpg               = vmx_invlpg,
> Similarly here, except that the pointer would need zapping if
> !cpu_has_vmx_vpid, requiring ...

but not this.  For testing "older" configurations, I have a plan to make
options like vpid configurable per domain, at which point the function
pointer necessarily needs following.

~Andrew

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

Re: [PATCH v2 for-4.7 5/5] x86/hvm: Fix invalidation for emulated invlpg instructions

[Andrew Cooper]

On 10/05/16 11:15, Andrew Cooper wrote:
> On 10/05/16 08:51, Jan Beulich wrote:
>>>>> On 09.05.16 at 20:27, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -2222,10 +2222,13 @@ static void svm_invlpga_intercept(
>>>  
>>>  static void svm_invlpg_intercept(unsigned long vaddr)
>>>  {
>>> -    struct vcpu *curr = current;
>>>      HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(vaddr));
>>> -    if ( paging_invlpg(curr, vaddr) )
>>> -        svm_asid_g_invlpg(curr, vaddr);
>>> +    paging_invlpg(current, vaddr);
>>> +}
>>> +
>>> +static void svm_invlpg(struct vcpu *v, unsigned long vaddr)
>>> +{
>>> +    svm_asid_g_invlpg(v, vaddr);
>>>  }
>> I don't see the need for the wrapper: svm_asid_g_invlpg() is itself
>> suitable to be used ...
>>
>>> @@ -2258,12 +2261,12 @@ static struct hvm_function_table __initdata svm_function_table = {
>>>      .inject_trap          = svm_inject_trap,
>>>      .init_hypercall_page  = svm_init_hypercall_page,
>>>      .event_pending        = svm_event_pending,
>>> +    .invlpg               = svm_invlpg,
>> ... here.
> So it can.  I will do this...

Actually, this isn't trivial.  svm_asid_g_invlpg() is a static inline in
a header file, and svm_invlpg() is its sole user.

I will fold the static inline into svm_invlpg(), which also makes the
VT-x and SVM code symmetric.

~Andrew

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

Re: [PATCH v2 for-4.7 5/5] x86/hvm: Fix invalidation for emulated invlpg instructions

[Jan Beulich]

>>> On 10.05.16 at 12:41, <andrew.cooper3@citrix.com> wrote:
> On 10/05/16 11:15, Andrew Cooper wrote:
>> On 10/05/16 08:51, Jan Beulich wrote:
>>>>>> On 09.05.16 at 20:27, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>> @@ -2222,10 +2222,13 @@ static void svm_invlpga_intercept(
>>>>  
>>>>  static void svm_invlpg_intercept(unsigned long vaddr)
>>>>  {
>>>> -    struct vcpu *curr = current;
>>>>      HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(vaddr));
>>>> -    if ( paging_invlpg(curr, vaddr) )
>>>> -        svm_asid_g_invlpg(curr, vaddr);
>>>> +    paging_invlpg(current, vaddr);
>>>> +}
>>>> +
>>>> +static void svm_invlpg(struct vcpu *v, unsigned long vaddr)
>>>> +{
>>>> +    svm_asid_g_invlpg(v, vaddr);
>>>>  }
>>> I don't see the need for the wrapper: svm_asid_g_invlpg() is itself
>>> suitable to be used ...
>>>
>>>> @@ -2258,12 +2261,12 @@ static struct hvm_function_table __initdata svm_function_table = {
>>>>      .inject_trap          = svm_inject_trap,
>>>>      .init_hypercall_page  = svm_init_hypercall_page,
>>>>      .event_pending        = svm_event_pending,
>>>> +    .invlpg               = svm_invlpg,
>>> ... here.
>> So it can.  I will do this...
> 
> Actually, this isn't trivial.  svm_asid_g_invlpg() is a static inline in
> a header file, and svm_invlpg() is its sole user.
> 
> I will fold the static inline into svm_invlpg(), which also makes the
> VT-x and SVM code symmetric.

That's an option (which I'm fine with), but I don't think a requirement:
Marked inline or not the compiler should produce an out of line instance
when its address is taken.

Jan


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

Re: [PATCH v2 for-4.7 5/5] x86/hvm: Fix invalidation for emulated invlpg instructions

[Andrew Cooper]

On 10/05/16 12:02, Jan Beulich wrote:
>>>> On 10.05.16 at 12:41, <andrew.cooper3@citrix.com> wrote:
>> On 10/05/16 11:15, Andrew Cooper wrote:
>>> On 10/05/16 08:51, Jan Beulich wrote:
>>>>>>> On 09.05.16 at 20:27, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>>> @@ -2222,10 +2222,13 @@ static void svm_invlpga_intercept(
>>>>>  
>>>>>  static void svm_invlpg_intercept(unsigned long vaddr)
>>>>>  {
>>>>> -    struct vcpu *curr = current;
>>>>>      HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(vaddr));
>>>>> -    if ( paging_invlpg(curr, vaddr) )
>>>>> -        svm_asid_g_invlpg(curr, vaddr);
>>>>> +    paging_invlpg(current, vaddr);
>>>>> +}
>>>>> +
>>>>> +static void svm_invlpg(struct vcpu *v, unsigned long vaddr)
>>>>> +{
>>>>> +    svm_asid_g_invlpg(v, vaddr);
>>>>>  }
>>>> I don't see the need for the wrapper: svm_asid_g_invlpg() is itself
>>>> suitable to be used ...
>>>>
>>>>> @@ -2258,12 +2261,12 @@ static struct hvm_function_table __initdata svm_function_table = {
>>>>>      .inject_trap          = svm_inject_trap,
>>>>>      .init_hypercall_page  = svm_init_hypercall_page,
>>>>>      .event_pending        = svm_event_pending,
>>>>> +    .invlpg               = svm_invlpg,
>>>> ... here.
>>> So it can.  I will do this...
>> Actually, this isn't trivial.  svm_asid_g_invlpg() is a static inline in
>> a header file, and svm_invlpg() is its sole user.
>>
>> I will fold the static inline into svm_invlpg(), which also makes the
>> VT-x and SVM code symmetric.
> That's an option (which I'm fine with), but I don't think a requirement:
> Marked inline or not the compiler should produce an out of line instance
> when its address is taken.

I am making a 6/5 patch which does some cleanup to the SVM/ASID code. 
There is more than just this to fix.

~Andrew

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

Re: [PATCH v2 for-4.7 5/5] x86/hvm: Fix invalidation for emulated invlpg instructions

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, May 10, 2016 2:28 AM
> 
> hap_invlpg() is reachable from the instruction emulator, which means
> introspection and tests using hvm_fep can end up here.  As such, crashing the
> domain is not an appropriate action to take.
> 
> Fixing this involves rearranging the callgraph.
> 
> paging_invlpg() is now the central entry point.  It first checks for the
> non-canonical NOP case, and calls ino the paging subsystem.  If a real flush
> is needed, it will call the appropriate handler for the vcpu.  This allows the
> PV callsites of paging_invlpg() to be simplified.
> 
> The sole user of hvm_funcs.invlpg_intercept() is altered to use
> paging_invlpg() instead, allowing the .invlpg_intercept() hook to be removed.
> 
> For both VMX and SVM, the existing $VENDOR_invlpg_intercept() is split in
> half.  $VENDOR_invlpg_intercept() stays as the intercept handler only (which
> just calls paging_invlpg()), and new $VENDOR_invlpg() functions do the
> ASID/VPID management.  These later functions are made available in hvm_funcs
> for paging_invlpg() to use.
> 
> As a result, correct ASID/VPID management occurs for the hvmemul path, even if
> it did not originate from an real hardware intercept.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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