[PATCH 0/5] Improvements to HVM Hypercall dispatching

[PATCH 0/5] Improvements to HVM Hypercall dispatching

[Andrew Cooper]

Andrew Cooper (5):
  x86/hvm: Rework HVM_HCALL_invalidate handling
  x86/hvm: Split the hypercall dispatching infrastructure out of hvm.c
  x86/hvm: Improve memory_op hypercall dispatching
  x86/hvm: Improve grant_table_op hypercall dispatching
  x86/hvm: Improve physdev_op hypercall dispatching

 xen/arch/x86/hvm/Makefile         |   1 +
 xen/arch/x86/hvm/hvm.c            | 299 --------------------------------------
 xen/arch/x86/hvm/hypercall.c      | 293 +++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/svm.c        |   8 +-
 xen/arch/x86/hvm/vmx/vmx.c        |  13 +-
 xen/include/asm-x86/hvm/support.h |   3 +-
 6 files changed, 301 insertions(+), 316 deletions(-)
 create mode 100644 xen/arch/x86/hvm/hypercall.c

-- 
2.1.4


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

[PATCH 1/5] x86/hvm: Rework HVM_HCALL_invalidate handling

[Andrew Cooper]

Sending an invalidation to the device model is an internal detail of
completing the hypercall; callers should not need to be responsible for it.
Drop HVM_HCALL_invalidate entirely and call send_invalidate_req() when
appropriate.

This makes the function boolean in nature, although the existing
HVM_HCALL_{completed,preempted} to aid code clarity.  While updating the
return type, drop _do from the name, as it is redundant.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
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>
---
 xen/arch/x86/hvm/hvm.c            |  7 +++----
 xen/arch/x86/hvm/svm/svm.c        |  8 ++------
 xen/arch/x86/hvm/vmx/vmx.c        | 13 ++++---------
 xen/include/asm-x86/hvm/support.h |  3 +--
 4 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 5f72758..e164f57 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3874,7 +3874,7 @@ static const hypercall_table_t hvm_hypercall_table[] = {
 #undef HYPERCALL
 #undef COMPAT_CALL
 
-int hvm_do_hypercall(struct cpu_user_regs *regs)
+bool hvm_hypercall(struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
     struct domain *currd = curr->domain;
@@ -4011,9 +4011,8 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
         return HVM_HCALL_preempted;
 
     if ( unlikely(currd->arch.hvm_domain.qemu_mapcache_invalidate) &&
-         test_and_clear_bool(currd->arch.hvm_domain.
-                             qemu_mapcache_invalidate) )
-        return HVM_HCALL_invalidate;
+         test_and_clear_bool(currd->arch.hvm_domain.qemu_mapcache_invalidate) )
+        send_invalidate_req();
 
     return HVM_HCALL_completed;
 }
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 01c7b58..ca2785c 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2542,13 +2542,9 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
             break;
         BUG_ON(vcpu_guestmode);
         HVMTRACE_1D(VMMCALL, regs->_eax);
-        rc = hvm_do_hypercall(regs);
-        if ( rc != HVM_HCALL_preempted )
-        {
+
+        if ( hvm_hypercall(regs) == HVM_HCALL_completed )
             __update_guest_eip(regs, inst_len);
-            if ( rc == HVM_HCALL_invalidate )
-                send_invalidate_req();
-        }
         break;
 
     case VMEXIT_DR0_READ ... VMEXIT_DR7_READ:
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index d3d98da..42f4fbd 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3629,19 +3629,14 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         update_guest_eip(); /* Safe: RDTSC, RDTSCP */
         hvm_rdtsc_intercept(regs);
         break;
+
     case EXIT_REASON_VMCALL:
-    {
-        int rc;
         HVMTRACE_1D(VMMCALL, regs->_eax);
-        rc = hvm_do_hypercall(regs);
-        if ( rc != HVM_HCALL_preempted )
-        {
+
+        if ( hvm_hypercall(regs) == HVM_HCALL_completed )
             update_guest_eip(); /* Safe: VMCALL */
-            if ( rc == HVM_HCALL_invalidate )
-                send_invalidate_req();
-        }
         break;
-    }
+
     case EXIT_REASON_CR_ACCESS:
     {
         __vmread(EXIT_QUALIFICATION, &exit_qualification);
diff --git a/xen/include/asm-x86/hvm/support.h b/xen/include/asm-x86/hvm/support.h
index 16550c5..dc257c5 100644
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -105,8 +105,7 @@ enum hvm_copy_result hvm_fetch_from_guest_linear(
 
 #define HVM_HCALL_completed  0 /* hypercall completed - no further action */
 #define HVM_HCALL_preempted  1 /* hypercall preempted - re-execute VMCALL */
-#define HVM_HCALL_invalidate 2 /* invalidate ioemu-dm memory cache        */
-int hvm_do_hypercall(struct cpu_user_regs *pregs);
+bool hvm_hypercall(struct cpu_user_regs *regs);
 
 void hvm_hlt(unsigned int eflags);
 void hvm_triple_fault(void);
-- 
2.1.4


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

[PATCH 2/5] x86/hvm: Split the hypercall dispatching infrastructure out of hvm.c

[Andrew Cooper]

Into a new hypercall.c.  This is purely code motion.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hvm/Makefile    |   1 +
 xen/arch/x86/hvm/hvm.c       | 298 --------------------------------------
 xen/arch/x86/hvm/hypercall.c | 332 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 333 insertions(+), 298 deletions(-)
 create mode 100644 xen/arch/x86/hvm/hypercall.c

diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 5869d1b..ec0daae 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -6,6 +6,7 @@ obj-y += dm.o
 obj-y += emulate.o
 obj-y += hpet.o
 obj-y += hvm.o
+obj-y += hypercall.o
 obj-y += i8254.o
 obj-y += intercept.o
 obj-y += io.o
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e164f57..266f708 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3719,304 +3719,6 @@ enum hvm_intblk hvm_interrupt_blocked(struct vcpu *v, struct hvm_intack intack)
     return hvm_intblk_none;
 }
 
-static int grant_table_op_is_allowed(unsigned int cmd)
-{
-    switch (cmd) {
-    case GNTTABOP_query_size:
-    case GNTTABOP_setup_table:
-    case GNTTABOP_set_version:
-    case GNTTABOP_get_version:
-    case GNTTABOP_copy:
-    case GNTTABOP_map_grant_ref:
-    case GNTTABOP_unmap_grant_ref:
-    case GNTTABOP_swap_grant_ref:
-        return 1;
-    default:
-        /* all other commands need auditing */
-        return 0;
-    }
-}
-
-static long hvm_grant_table_op(
-    unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
-{
-    if ( !grant_table_op_is_allowed(cmd) )
-        return -ENOSYS; /* all other commands need auditing */
-    return do_grant_table_op(cmd, uop, count);
-}
-
-static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
-    long rc;
-
-    switch ( cmd & MEMOP_CMD_MASK )
-    {
-    case XENMEM_machine_memory_map:
-    case XENMEM_machphys_mapping:
-        return -ENOSYS;
-    case XENMEM_decrease_reservation:
-        rc = do_memory_op(cmd, arg);
-        current->domain->arch.hvm_domain.qemu_mapcache_invalidate = 1;
-        return rc;
-    }
-    return do_memory_op(cmd, arg);
-}
-
-static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
-    switch ( cmd )
-    {
-    default:
-        if ( !is_pvh_vcpu(current) || !is_hardware_domain(current->domain) )
-            return -ENOSYS;
-        /* fall through */
-    case PHYSDEVOP_map_pirq:
-    case PHYSDEVOP_unmap_pirq:
-    case PHYSDEVOP_eoi:
-    case PHYSDEVOP_irq_status_query:
-    case PHYSDEVOP_get_free_pirq:
-        return do_physdev_op(cmd, arg);
-    }
-}
-
-static long hvm_grant_table_op_compat32(unsigned int cmd,
-                                        XEN_GUEST_HANDLE_PARAM(void) uop,
-                                        unsigned int count)
-{
-    if ( !grant_table_op_is_allowed(cmd) )
-        return -ENOSYS;
-    return compat_grant_table_op(cmd, uop, count);
-}
-
-static long hvm_memory_op_compat32(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
-    int rc;
-
-    switch ( cmd & MEMOP_CMD_MASK )
-    {
-    case XENMEM_machine_memory_map:
-    case XENMEM_machphys_mapping:
-        return -ENOSYS;
-    case XENMEM_decrease_reservation:
-        rc = compat_memory_op(cmd, arg);
-        current->domain->arch.hvm_domain.qemu_mapcache_invalidate = 1;
-        return rc;
-    }
-    return compat_memory_op(cmd, arg);
-}
-
-static long hvm_physdev_op_compat32(
-    int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
-    switch ( cmd )
-    {
-        case PHYSDEVOP_map_pirq:
-        case PHYSDEVOP_unmap_pirq:
-        case PHYSDEVOP_eoi:
-        case PHYSDEVOP_irq_status_query:
-        case PHYSDEVOP_get_free_pirq:
-            return compat_physdev_op(cmd, arg);
-        break;
-    default:
-            return -ENOSYS;
-        break;
-    }
-}
-
-#define HYPERCALL(x)                                         \
-    [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x,  \
-                               (hypercall_fn_t *) do_ ## x }
-
-#define COMPAT_CALL(x)                                       \
-    [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x,  \
-                               (hypercall_fn_t *) compat_ ## x }
-
-#define do_memory_op          hvm_memory_op
-#define compat_memory_op      hvm_memory_op_compat32
-#define do_physdev_op         hvm_physdev_op
-#define compat_physdev_op     hvm_physdev_op_compat32
-#define do_grant_table_op     hvm_grant_table_op
-#define compat_grant_table_op hvm_grant_table_op_compat32
-#define do_arch_1             paging_domctl_continuation
-
-static const hypercall_table_t hvm_hypercall_table[] = {
-    COMPAT_CALL(memory_op),
-    COMPAT_CALL(grant_table_op),
-    COMPAT_CALL(vcpu_op),
-    COMPAT_CALL(physdev_op),
-    COMPAT_CALL(xen_version),
-    HYPERCALL(console_io),
-    HYPERCALL(event_channel_op),
-    COMPAT_CALL(sched_op),
-    COMPAT_CALL(set_timer_op),
-    HYPERCALL(xsm_op),
-    HYPERCALL(hvm_op),
-    HYPERCALL(sysctl),
-    HYPERCALL(domctl),
-#ifdef CONFIG_TMEM
-    HYPERCALL(tmem_op),
-#endif
-    COMPAT_CALL(platform_op),
-    COMPAT_CALL(mmuext_op),
-    HYPERCALL(xenpmu_op),
-    COMPAT_CALL(dm_op),
-    HYPERCALL(arch_1)
-};
-
-#undef do_memory_op
-#undef compat_memory_op
-#undef do_physdev_op
-#undef compat_physdev_op
-#undef do_grant_table_op
-#undef compat_grant_table_op
-#undef do_arch_1
-
-#undef HYPERCALL
-#undef COMPAT_CALL
-
-bool hvm_hypercall(struct cpu_user_regs *regs)
-{
-    struct vcpu *curr = current;
-    struct domain *currd = curr->domain;
-    int mode = hvm_guest_x86_mode(curr);
-    unsigned long eax = regs->_eax;
-
-    switch ( mode )
-    {
-    case 8:
-        eax = regs->rax;
-        /* Fallthrough to permission check. */
-    case 4:
-    case 2:
-        if ( unlikely(hvm_get_cpl(curr)) )
-        {
-    default:
-            regs->rax = -EPERM;
-            return HVM_HCALL_completed;
-        }
-    case 0:
-        break;
-    }
-
-    if ( (eax & 0x80000000) && is_viridian_domain(currd) )
-        return viridian_hypercall(regs);
-
-    BUILD_BUG_ON(ARRAY_SIZE(hvm_hypercall_table) >
-                 ARRAY_SIZE(hypercall_args_table));
-
-    if ( (eax >= ARRAY_SIZE(hvm_hypercall_table)) ||
-         !hvm_hypercall_table[eax].native )
-    {
-        regs->rax = -ENOSYS;
-        return HVM_HCALL_completed;
-    }
-
-    curr->arch.hvm_vcpu.hcall_preempted = 0;
-
-    if ( mode == 8 )
-    {
-        unsigned long rdi = regs->rdi;
-        unsigned long rsi = regs->rsi;
-        unsigned long rdx = regs->rdx;
-        unsigned long r10 = regs->r10;
-        unsigned long r8 = regs->r8;
-        unsigned long r9 = regs->r9;
-
-        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%lx, %lx, %lx, %lx, %lx, %lx)",
-                    eax, rdi, rsi, rdx, r10, r8, r9);
-
-#ifndef NDEBUG
-        /* Deliberately corrupt parameter regs not used by this hypercall. */
-        switch ( hypercall_args_table[eax].native )
-        {
-        case 0: rdi = 0xdeadbeefdeadf00dUL;
-        case 1: rsi = 0xdeadbeefdeadf00dUL;
-        case 2: rdx = 0xdeadbeefdeadf00dUL;
-        case 3: r10 = 0xdeadbeefdeadf00dUL;
-        case 4: r8 = 0xdeadbeefdeadf00dUL;
-        case 5: r9 = 0xdeadbeefdeadf00dUL;
-        }
-#endif
-
-        curr->arch.hvm_vcpu.hcall_64bit = 1;
-        regs->rax = hvm_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8,
-                                                    r9);
-
-        curr->arch.hvm_vcpu.hcall_64bit = 0;
-
-#ifndef NDEBUG
-        if ( !curr->arch.hvm_vcpu.hcall_preempted )
-        {
-            /* Deliberately corrupt parameter regs used by this hypercall. */
-            switch ( hypercall_args_table[eax].native )
-            {
-            case 6: regs->r9  = 0xdeadbeefdeadf00dUL;
-            case 5: regs->r8  = 0xdeadbeefdeadf00dUL;
-            case 4: regs->r10 = 0xdeadbeefdeadf00dUL;
-            case 3: regs->rdx = 0xdeadbeefdeadf00dUL;
-            case 2: regs->rsi = 0xdeadbeefdeadf00dUL;
-            case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
-            }
-        }
-#endif
-    }
-    else
-    {
-        unsigned int ebx = regs->_ebx;
-        unsigned int ecx = regs->_ecx;
-        unsigned int edx = regs->_edx;
-        unsigned int esi = regs->_esi;
-        unsigned int edi = regs->_edi;
-        unsigned int ebp = regs->_ebp;
-
-        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%x, %x, %x, %x, %x, %x)", eax,
-                    ebx, ecx, edx, esi, edi, ebp);
-
-#ifndef NDEBUG
-        /* Deliberately corrupt parameter regs not used by this hypercall. */
-        switch ( hypercall_args_table[eax].compat )
-        {
-        case 0: ebx = 0xdeadf00d;
-        case 1: ecx = 0xdeadf00d;
-        case 2: edx = 0xdeadf00d;
-        case 3: esi = 0xdeadf00d;
-        case 4: edi = 0xdeadf00d;
-        case 5: ebp = 0xdeadf00d;
-        }
-#endif
-
-        regs->rax = hvm_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi,
-                                                    ebp);
-
-#ifndef NDEBUG
-        if ( !curr->arch.hvm_vcpu.hcall_preempted )
-        {
-            /* Deliberately corrupt parameter regs used by this hypercall. */
-            switch ( hypercall_args_table[eax].compat )
-            {
-            case 6: regs->rbp = 0xdeadf00d;
-            case 5: regs->rdi = 0xdeadf00d;
-            case 4: regs->rsi = 0xdeadf00d;
-            case 3: regs->rdx = 0xdeadf00d;
-            case 2: regs->rcx = 0xdeadf00d;
-            case 1: regs->rbx = 0xdeadf00d;
-            }
-        }
-#endif
-    }
-
-    HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu -> %lx", eax, regs->rax);
-
-    if ( curr->arch.hvm_vcpu.hcall_preempted )
-        return HVM_HCALL_preempted;
-
-    if ( unlikely(currd->arch.hvm_domain.qemu_mapcache_invalidate) &&
-         test_and_clear_bool(currd->arch.hvm_domain.qemu_mapcache_invalidate) )
-        send_invalidate_req();
-
-    return HVM_HCALL_completed;
-}
-
 static void hvm_latch_shinfo_size(struct domain *d)
 {
     /*
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
new file mode 100644
index 0000000..11bd16d
--- /dev/null
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -0,0 +1,332 @@
+/******************************************************************************
+ * arch/hvm/hypercall.c
+ *
+ * HVM hypercall dispatching routines
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2017 Citrix Systems Ltd.
+ */
+#include <xen/lib.h>
+#include <xen/hypercall.h>
+
+#include <asm/hvm/support.h>
+
+static int grant_table_op_is_allowed(unsigned int cmd)
+{
+    switch (cmd) {
+    case GNTTABOP_query_size:
+    case GNTTABOP_setup_table:
+    case GNTTABOP_set_version:
+    case GNTTABOP_get_version:
+    case GNTTABOP_copy:
+    case GNTTABOP_map_grant_ref:
+    case GNTTABOP_unmap_grant_ref:
+    case GNTTABOP_swap_grant_ref:
+        return 1;
+    default:
+        /* all other commands need auditing */
+        return 0;
+    }
+}
+
+static long hvm_grant_table_op(
+    unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
+{
+    if ( !grant_table_op_is_allowed(cmd) )
+        return -ENOSYS; /* all other commands need auditing */
+    return do_grant_table_op(cmd, uop, count);
+}
+
+static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+    long rc;
+
+    switch ( cmd & MEMOP_CMD_MASK )
+    {
+    case XENMEM_machine_memory_map:
+    case XENMEM_machphys_mapping:
+        return -ENOSYS;
+    case XENMEM_decrease_reservation:
+        rc = do_memory_op(cmd, arg);
+        current->domain->arch.hvm_domain.qemu_mapcache_invalidate = 1;
+        return rc;
+    }
+    return do_memory_op(cmd, arg);
+}
+
+static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+    switch ( cmd )
+    {
+    default:
+        if ( !is_pvh_vcpu(current) || !is_hardware_domain(current->domain) )
+            return -ENOSYS;
+        /* fall through */
+    case PHYSDEVOP_map_pirq:
+    case PHYSDEVOP_unmap_pirq:
+    case PHYSDEVOP_eoi:
+    case PHYSDEVOP_irq_status_query:
+    case PHYSDEVOP_get_free_pirq:
+        return do_physdev_op(cmd, arg);
+    }
+}
+
+static long hvm_grant_table_op_compat32(unsigned int cmd,
+                                        XEN_GUEST_HANDLE_PARAM(void) uop,
+                                        unsigned int count)
+{
+    if ( !grant_table_op_is_allowed(cmd) )
+        return -ENOSYS;
+    return compat_grant_table_op(cmd, uop, count);
+}
+
+static long hvm_memory_op_compat32(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+    int rc;
+
+    switch ( cmd & MEMOP_CMD_MASK )
+    {
+    case XENMEM_machine_memory_map:
+    case XENMEM_machphys_mapping:
+        return -ENOSYS;
+    case XENMEM_decrease_reservation:
+        rc = compat_memory_op(cmd, arg);
+        current->domain->arch.hvm_domain.qemu_mapcache_invalidate = 1;
+        return rc;
+    }
+    return compat_memory_op(cmd, arg);
+}
+
+static long hvm_physdev_op_compat32(
+    int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+    switch ( cmd )
+    {
+        case PHYSDEVOP_map_pirq:
+        case PHYSDEVOP_unmap_pirq:
+        case PHYSDEVOP_eoi:
+        case PHYSDEVOP_irq_status_query:
+        case PHYSDEVOP_get_free_pirq:
+            return compat_physdev_op(cmd, arg);
+        break;
+    default:
+            return -ENOSYS;
+        break;
+    }
+}
+
+#define HYPERCALL(x)                                         \
+    [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x,  \
+                               (hypercall_fn_t *) do_ ## x }
+
+#define COMPAT_CALL(x)                                       \
+    [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x,  \
+                               (hypercall_fn_t *) compat_ ## x }
+
+#define do_memory_op          hvm_memory_op
+#define compat_memory_op      hvm_memory_op_compat32
+#define do_physdev_op         hvm_physdev_op
+#define compat_physdev_op     hvm_physdev_op_compat32
+#define do_grant_table_op     hvm_grant_table_op
+#define compat_grant_table_op hvm_grant_table_op_compat32
+#define do_arch_1             paging_domctl_continuation
+
+static const hypercall_table_t hvm_hypercall_table[] = {
+    COMPAT_CALL(memory_op),
+    COMPAT_CALL(grant_table_op),
+    COMPAT_CALL(vcpu_op),
+    COMPAT_CALL(physdev_op),
+    COMPAT_CALL(xen_version),
+    HYPERCALL(console_io),
+    HYPERCALL(event_channel_op),
+    COMPAT_CALL(sched_op),
+    COMPAT_CALL(set_timer_op),
+    HYPERCALL(xsm_op),
+    HYPERCALL(hvm_op),
+    HYPERCALL(sysctl),
+    HYPERCALL(domctl),
+#ifdef CONFIG_TMEM
+    HYPERCALL(tmem_op),
+#endif
+    COMPAT_CALL(platform_op),
+    COMPAT_CALL(mmuext_op),
+    HYPERCALL(xenpmu_op),
+    COMPAT_CALL(dm_op),
+    HYPERCALL(arch_1)
+};
+
+#undef do_memory_op
+#undef compat_memory_op
+#undef do_physdev_op
+#undef compat_physdev_op
+#undef do_grant_table_op
+#undef compat_grant_table_op
+#undef do_arch_1
+
+#undef HYPERCALL
+#undef COMPAT_CALL
+
+bool hvm_hypercall(struct cpu_user_regs *regs)
+{
+    struct vcpu *curr = current;
+    struct domain *currd = curr->domain;
+    int mode = hvm_guest_x86_mode(curr);
+    unsigned long eax = regs->_eax;
+
+    switch ( mode )
+    {
+    case 8:
+        eax = regs->rax;
+        /* Fallthrough to permission check. */
+    case 4:
+    case 2:
+        if ( unlikely(hvm_get_cpl(curr)) )
+        {
+    default:
+            regs->rax = -EPERM;
+            return HVM_HCALL_completed;
+        }
+    case 0:
+        break;
+    }
+
+    if ( (eax & 0x80000000) && is_viridian_domain(currd) )
+        return viridian_hypercall(regs);
+
+    BUILD_BUG_ON(ARRAY_SIZE(hvm_hypercall_table) >
+                 ARRAY_SIZE(hypercall_args_table));
+
+    if ( (eax >= ARRAY_SIZE(hvm_hypercall_table)) ||
+         !hvm_hypercall_table[eax].native )
+    {
+        regs->rax = -ENOSYS;
+        return HVM_HCALL_completed;
+    }
+
+    curr->arch.hvm_vcpu.hcall_preempted = 0;
+
+    if ( mode == 8 )
+    {
+        unsigned long rdi = regs->rdi;
+        unsigned long rsi = regs->rsi;
+        unsigned long rdx = regs->rdx;
+        unsigned long r10 = regs->r10;
+        unsigned long r8 = regs->r8;
+        unsigned long r9 = regs->r9;
+
+        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%lx, %lx, %lx, %lx, %lx, %lx)",
+                    eax, rdi, rsi, rdx, r10, r8, r9);
+
+#ifndef NDEBUG
+        /* Deliberately corrupt parameter regs not used by this hypercall. */
+        switch ( hypercall_args_table[eax].native )
+        {
+        case 0: rdi = 0xdeadbeefdeadf00dUL;
+        case 1: rsi = 0xdeadbeefdeadf00dUL;
+        case 2: rdx = 0xdeadbeefdeadf00dUL;
+        case 3: r10 = 0xdeadbeefdeadf00dUL;
+        case 4: r8 = 0xdeadbeefdeadf00dUL;
+        case 5: r9 = 0xdeadbeefdeadf00dUL;
+        }
+#endif
+
+        curr->arch.hvm_vcpu.hcall_64bit = 1;
+        regs->rax = hvm_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8,
+                                                    r9);
+
+        curr->arch.hvm_vcpu.hcall_64bit = 0;
+
+#ifndef NDEBUG
+        if ( !curr->arch.hvm_vcpu.hcall_preempted )
+        {
+            /* Deliberately corrupt parameter regs used by this hypercall. */
+            switch ( hypercall_args_table[eax].native )
+            {
+            case 6: regs->r9  = 0xdeadbeefdeadf00dUL;
+            case 5: regs->r8  = 0xdeadbeefdeadf00dUL;
+            case 4: regs->r10 = 0xdeadbeefdeadf00dUL;
+            case 3: regs->rdx = 0xdeadbeefdeadf00dUL;
+            case 2: regs->rsi = 0xdeadbeefdeadf00dUL;
+            case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
+            }
+        }
+#endif
+    }
+    else
+    {
+        unsigned int ebx = regs->_ebx;
+        unsigned int ecx = regs->_ecx;
+        unsigned int edx = regs->_edx;
+        unsigned int esi = regs->_esi;
+        unsigned int edi = regs->_edi;
+        unsigned int ebp = regs->_ebp;
+
+        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%x, %x, %x, %x, %x, %x)", eax,
+                    ebx, ecx, edx, esi, edi, ebp);
+
+#ifndef NDEBUG
+        /* Deliberately corrupt parameter regs not used by this hypercall. */
+        switch ( hypercall_args_table[eax].compat )
+        {
+        case 0: ebx = 0xdeadf00d;
+        case 1: ecx = 0xdeadf00d;
+        case 2: edx = 0xdeadf00d;
+        case 3: esi = 0xdeadf00d;
+        case 4: edi = 0xdeadf00d;
+        case 5: ebp = 0xdeadf00d;
+        }
+#endif
+
+        regs->rax = hvm_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi,
+                                                    ebp);
+
+#ifndef NDEBUG
+        if ( !curr->arch.hvm_vcpu.hcall_preempted )
+        {
+            /* Deliberately corrupt parameter regs used by this hypercall. */
+            switch ( hypercall_args_table[eax].compat )
+            {
+            case 6: regs->rbp = 0xdeadf00d;
+            case 5: regs->rdi = 0xdeadf00d;
+            case 4: regs->rsi = 0xdeadf00d;
+            case 3: regs->rdx = 0xdeadf00d;
+            case 2: regs->rcx = 0xdeadf00d;
+            case 1: regs->rbx = 0xdeadf00d;
+            }
+        }
+#endif
+    }
+
+    HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu -> %lx", eax, regs->rax);
+
+    if ( curr->arch.hvm_vcpu.hcall_preempted )
+        return HVM_HCALL_preempted;
+
+    if ( unlikely(currd->arch.hvm_domain.qemu_mapcache_invalidate) &&
+         test_and_clear_bool(currd->arch.hvm_domain.qemu_mapcache_invalidate) )
+        send_invalidate_req();
+
+    return HVM_HCALL_completed;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.1.4


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

[PATCH 3/5] x86/hvm: Improve memory_op hypercall dispatching

[Andrew Cooper]

hvm_memory_op() and hvm_memory_op_compat32() are almost identical, but there
is no need to have two functions instantiated at the end of different function
pointers.

Combine the two into single hvm_memory_op() which dispatches to
{do,compat}_memory_op() based on the hcall_64bit setting.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hvm/hypercall.c | 68 +++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 39 deletions(-)

diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 11bd16d..0f3df4e 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -23,6 +23,29 @@
 
 #include <asm/hvm/support.h>
 
+static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+    struct vcpu *curr = current;
+    long rc;
+
+    switch ( cmd & MEMOP_CMD_MASK )
+    {
+    case XENMEM_machine_memory_map:
+    case XENMEM_machphys_mapping:
+        return -ENOSYS;
+    }
+
+    if ( curr->arch.hvm_vcpu.hcall_64bit )
+        rc = do_memory_op(cmd, arg);
+    else
+        rc = compat_memory_op(cmd, arg);
+
+    if ( (cmd & MEMOP_CMD_MASK) == XENMEM_decrease_reservation )
+        curr->domain->arch.hvm_domain.qemu_mapcache_invalidate = true;
+
+    return rc;
+}
+
 static int grant_table_op_is_allowed(unsigned int cmd)
 {
     switch (cmd) {
@@ -49,23 +72,6 @@ static long hvm_grant_table_op(
     return do_grant_table_op(cmd, uop, count);
 }
 
-static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
-    long rc;
-
-    switch ( cmd & MEMOP_CMD_MASK )
-    {
-    case XENMEM_machine_memory_map:
-    case XENMEM_machphys_mapping:
-        return -ENOSYS;
-    case XENMEM_decrease_reservation:
-        rc = do_memory_op(cmd, arg);
-        current->domain->arch.hvm_domain.qemu_mapcache_invalidate = 1;
-        return rc;
-    }
-    return do_memory_op(cmd, arg);
-}
-
 static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     switch ( cmd )
@@ -92,23 +98,6 @@ static long hvm_grant_table_op_compat32(unsigned int cmd,
     return compat_grant_table_op(cmd, uop, count);
 }
 
-static long hvm_memory_op_compat32(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
-    int rc;
-
-    switch ( cmd & MEMOP_CMD_MASK )
-    {
-    case XENMEM_machine_memory_map:
-    case XENMEM_machphys_mapping:
-        return -ENOSYS;
-    case XENMEM_decrease_reservation:
-        rc = compat_memory_op(cmd, arg);
-        current->domain->arch.hvm_domain.qemu_mapcache_invalidate = 1;
-        return rc;
-    }
-    return compat_memory_op(cmd, arg);
-}
-
 static long hvm_physdev_op_compat32(
     int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
@@ -131,12 +120,14 @@ static long hvm_physdev_op_compat32(
     [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x,  \
                                (hypercall_fn_t *) do_ ## x }
 
+#define HVM_CALL(x)                                          \
+    [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) hvm_ ## x, \
+                               (hypercall_fn_t *) hvm_ ## x }
+
 #define COMPAT_CALL(x)                                       \
     [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x,  \
                                (hypercall_fn_t *) compat_ ## x }
 
-#define do_memory_op          hvm_memory_op
-#define compat_memory_op      hvm_memory_op_compat32
 #define do_physdev_op         hvm_physdev_op
 #define compat_physdev_op     hvm_physdev_op_compat32
 #define do_grant_table_op     hvm_grant_table_op
@@ -144,7 +135,7 @@ static long hvm_physdev_op_compat32(
 #define do_arch_1             paging_domctl_continuation
 
 static const hypercall_table_t hvm_hypercall_table[] = {
-    COMPAT_CALL(memory_op),
+    HVM_CALL(memory_op),
     COMPAT_CALL(grant_table_op),
     COMPAT_CALL(vcpu_op),
     COMPAT_CALL(physdev_op),
@@ -167,8 +158,6 @@ static const hypercall_table_t hvm_hypercall_table[] = {
     HYPERCALL(arch_1)
 };
 
-#undef do_memory_op
-#undef compat_memory_op
 #undef do_physdev_op
 #undef compat_physdev_op
 #undef do_grant_table_op
@@ -176,6 +165,7 @@ static const hypercall_table_t hvm_hypercall_table[] = {
 #undef do_arch_1
 
 #undef HYPERCALL
+#undef HVM_CALL
 #undef COMPAT_CALL
 
 bool hvm_hypercall(struct cpu_user_regs *regs)
-- 
2.1.4


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

[PATCH 4/5] x86/hvm: Improve grant_table_op hypercall dispatching

[Andrew Cooper]

hvm_grant_table_op() and hvm_grant_table_op_compat32() are almost identical,
but there is no need to have two functions instantiated at the end of
different function pointers.

Combine the two into a single hvm_grant_table_op() (folding
grant_table_op_is_allowed() into is now-single caller) and dispatch to
{do,compat}_grant_table_op() based on the hcall_64bit setting.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hvm/hypercall.c | 40 +++++++++++++---------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 0f3df4e..140676d 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -46,9 +46,11 @@ static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     return rc;
 }
 
-static int grant_table_op_is_allowed(unsigned int cmd)
+static long hvm_grant_table_op(
+    unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
 {
-    switch (cmd) {
+    switch ( cmd )
+    {
     case GNTTABOP_query_size:
     case GNTTABOP_setup_table:
     case GNTTABOP_set_version:
@@ -57,19 +59,16 @@ static int grant_table_op_is_allowed(unsigned int cmd)
     case GNTTABOP_map_grant_ref:
     case GNTTABOP_unmap_grant_ref:
     case GNTTABOP_swap_grant_ref:
-        return 1;
-    default:
-        /* all other commands need auditing */
-        return 0;
+        break;
+
+    default: /* All other commands need auditing. */
+        return -ENOSYS;
     }
-}
 
-static long hvm_grant_table_op(
-    unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
-{
-    if ( !grant_table_op_is_allowed(cmd) )
-        return -ENOSYS; /* all other commands need auditing */
-    return do_grant_table_op(cmd, uop, count);
+    if ( current->arch.hvm_vcpu.hcall_64bit )
+        return do_grant_table_op(cmd, uop, count);
+    else
+        return compat_grant_table_op(cmd, uop, count);
 }
 
 static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
@@ -89,15 +88,6 @@ static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     }
 }
 
-static long hvm_grant_table_op_compat32(unsigned int cmd,
-                                        XEN_GUEST_HANDLE_PARAM(void) uop,
-                                        unsigned int count)
-{
-    if ( !grant_table_op_is_allowed(cmd) )
-        return -ENOSYS;
-    return compat_grant_table_op(cmd, uop, count);
-}
-
 static long hvm_physdev_op_compat32(
     int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
@@ -130,13 +120,11 @@ static long hvm_physdev_op_compat32(
 
 #define do_physdev_op         hvm_physdev_op
 #define compat_physdev_op     hvm_physdev_op_compat32
-#define do_grant_table_op     hvm_grant_table_op
-#define compat_grant_table_op hvm_grant_table_op_compat32
 #define do_arch_1             paging_domctl_continuation
 
 static const hypercall_table_t hvm_hypercall_table[] = {
     HVM_CALL(memory_op),
-    COMPAT_CALL(grant_table_op),
+    HVM_CALL(grant_table_op),
     COMPAT_CALL(vcpu_op),
     COMPAT_CALL(physdev_op),
     COMPAT_CALL(xen_version),
@@ -160,8 +148,6 @@ static const hypercall_table_t hvm_hypercall_table[] = {
 
 #undef do_physdev_op
 #undef compat_physdev_op
-#undef do_grant_table_op
-#undef compat_grant_table_op
 #undef do_arch_1
 
 #undef HYPERCALL
-- 
2.1.4


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

[PATCH 5/5] x86/hvm: Improve physdev_op hypercall dispatching

[Andrew Cooper]

hvm_physdev_op() and hvm_physdev_op_compat32() are almost identical, but there
is no need to have two functions instantiated at the end of different function
pointers.

Combine the two into a single hvm_physdev_op() and dispatch to
{do,compat}_physdev_op() based on the hcall_64bit setting.

This also fixes an inconsistency where 64bit PVH hardware domains were
permitted access to extra physdev ops, but 32bit domains weren't.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hvm/hypercall.c | 33 +++++++++------------------------
 1 file changed, 9 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 140676d..46aad4e 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -73,10 +73,12 @@ static long hvm_grant_table_op(
 
 static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
+    struct vcpu *curr = current;
+
     switch ( cmd )
     {
     default:
-        if ( !is_pvh_vcpu(current) || !is_hardware_domain(current->domain) )
+        if ( !is_pvh_vcpu(curr) || !is_hardware_domain(curr->domain) )
             return -ENOSYS;
         /* fall through */
     case PHYSDEVOP_map_pirq:
@@ -84,26 +86,13 @@ static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case PHYSDEVOP_eoi:
     case PHYSDEVOP_irq_status_query:
     case PHYSDEVOP_get_free_pirq:
-        return do_physdev_op(cmd, arg);
-    }
-}
-
-static long hvm_physdev_op_compat32(
-    int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
-    switch ( cmd )
-    {
-        case PHYSDEVOP_map_pirq:
-        case PHYSDEVOP_unmap_pirq:
-        case PHYSDEVOP_eoi:
-        case PHYSDEVOP_irq_status_query:
-        case PHYSDEVOP_get_free_pirq:
-            return compat_physdev_op(cmd, arg);
-        break;
-    default:
-            return -ENOSYS;
         break;
     }
+
+    if ( curr->arch.hvm_vcpu.hcall_64bit )
+        return do_physdev_op(cmd, arg);
+    else
+        return compat_physdev_op(cmd, arg);
 }
 
 #define HYPERCALL(x)                                         \
@@ -118,15 +107,13 @@ static long hvm_physdev_op_compat32(
     [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x,  \
                                (hypercall_fn_t *) compat_ ## x }
 
-#define do_physdev_op         hvm_physdev_op
-#define compat_physdev_op     hvm_physdev_op_compat32
 #define do_arch_1             paging_domctl_continuation
 
 static const hypercall_table_t hvm_hypercall_table[] = {
     HVM_CALL(memory_op),
     HVM_CALL(grant_table_op),
     COMPAT_CALL(vcpu_op),
-    COMPAT_CALL(physdev_op),
+    HVM_CALL(physdev_op),
     COMPAT_CALL(xen_version),
     HYPERCALL(console_io),
     HYPERCALL(event_channel_op),
@@ -146,8 +133,6 @@ static const hypercall_table_t hvm_hypercall_table[] = {
     HYPERCALL(arch_1)
 };
 
-#undef do_physdev_op
-#undef compat_physdev_op
 #undef do_arch_1
 
 #undef HYPERCALL
-- 
2.1.4


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

Re: [PATCH 1/5] x86/hvm: Rework HVM_HCALL_invalidate handling

[Boris Ostrovsky]

On 02/13/2017 08:03 AM, Andrew Cooper wrote:
> Sending an invalidation to the device model is an internal detail of
> completing the hypercall; callers should not need to be responsible for it.
> Drop HVM_HCALL_invalidate entirely and call send_invalidate_req() when
> appropriate.
>
> This makes the function boolean in nature, although the existing
> HVM_HCALL_{completed,preempted} to aid code clarity.  While updating the
> return type, drop _do from the name, as it is redundant.
>
> 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
https://lists.xen.org/xen-devel

Re: [PATCH 0/5] Improvements to HVM Hypercall dispatching

[Wei Liu]

On Mon, Feb 13, 2017 at 01:03:43PM +0000, Andrew Cooper wrote:
> Andrew Cooper (5):
>   x86/hvm: Rework HVM_HCALL_invalidate handling
>   x86/hvm: Split the hypercall dispatching infrastructure out of hvm.c
>   x86/hvm: Improve memory_op hypercall dispatching
>   x86/hvm: Improve grant_table_op hypercall dispatching
>   x86/hvm: Improve physdev_op hypercall dispatching

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

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

Re: [PATCH 1/5] x86/hvm: Rework HVM_HCALL_invalidate handling

[Jan Beulich]

>>> On 13.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
> Sending an invalidation to the device model is an internal detail of
> completing the hypercall; callers should not need to be responsible for it.
> Drop HVM_HCALL_invalidate entirely and call send_invalidate_req() when
> appropriate.
> 
> This makes the function boolean in nature, although the existing
> HVM_HCALL_{completed,preempted} to aid code clarity.

"are being retained" missing somewhere here?

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3874,7 +3874,7 @@ static const hypercall_table_t hvm_hypercall_table[] = 
> {
>  #undef HYPERCALL
>  #undef COMPAT_CALL
>  
> -int hvm_do_hypercall(struct cpu_user_regs *regs)
> +bool hvm_hypercall(struct cpu_user_regs *regs)

I don't think bool is a particularly good choice when the callers can't
sensibly use the result without comparing with HVM_HCALL_*

> @@ -4011,9 +4011,8 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>          return HVM_HCALL_preempted;
>  
>      if ( unlikely(currd->arch.hvm_domain.qemu_mapcache_invalidate) &&
> -         test_and_clear_bool(currd->arch.hvm_domain.
> -                             qemu_mapcache_invalidate) )
> -        return HVM_HCALL_invalidate;
> +         test_and_clear_bool(currd->arch.hvm_domain.qemu_mapcache_invalidate) )
> +        send_invalidate_req();

I wonder why things were done the old way in the first place. Did
something else change, making that old model no longer required?
I'm somewhat afraid we overlook something here, and hence an
attempt to understand why this more immediate model wasn't
used (and perhaps usable) back then might help... That aside, the
patch looks fine.

Jan


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

Re: [PATCH 1/5] x86/hvm: Rework HVM_HCALL_invalidate handling

[Andrew Cooper]

On 13/02/17 16:49, Jan Beulich wrote:
>>>> On 13.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
>> Sending an invalidation to the device model is an internal detail of
>> completing the hypercall; callers should not need to be responsible for it.
>> Drop HVM_HCALL_invalidate entirely and call send_invalidate_req() when
>> appropriate.
>>
>> This makes the function boolean in nature, although the existing
>> HVM_HCALL_{completed,preempted} to aid code clarity.
> "are being retained" missing somewhere here?

Yes.  I already noticed and fixed that up.  It now reads

"This makes the function boolean in nature, although the existing
HVM_HCALL_{completed,preempted} constants are kept to aid code clarity."

>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3874,7 +3874,7 @@ static const hypercall_table_t hvm_hypercall_table[] = 
>> {
>>  #undef HYPERCALL
>>  #undef COMPAT_CALL
>>  
>> -int hvm_do_hypercall(struct cpu_user_regs *regs)
>> +bool hvm_hypercall(struct cpu_user_regs *regs)
> I don't think bool is a particularly good choice when the callers can't
> sensibly use the result without comparing with HVM_HCALL_*

Ok.  I will keep it as int.

>
>> @@ -4011,9 +4011,8 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>>          return HVM_HCALL_preempted;
>>  
>>      if ( unlikely(currd->arch.hvm_domain.qemu_mapcache_invalidate) &&
>> -         test_and_clear_bool(currd->arch.hvm_domain.
>> -                             qemu_mapcache_invalidate) )
>> -        return HVM_HCALL_invalidate;
>> +         test_and_clear_bool(currd->arch.hvm_domain.qemu_mapcache_invalidate) )
>> +        send_invalidate_req();
> I wonder why things were done the old way in the first place. Did
> something else change, making that old model no longer required?
> I'm somewhat afraid we overlook something here, and hence an
> attempt to understand why this more immediate model wasn't
> used (and perhaps usable) back then might help... That aside, the
> patch looks fine.

Looks like it has been the same since it was first introduced in aeb2e1298b7

While that change does indicate it was replacing various I/O port hacks,
I can't see any reason why HVM_HCALL_invalidate was exposed outside of
hvm_do_hypercall().

~Andrew

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

Re: [PATCH 1/5] x86/hvm: Rework HVM_HCALL_invalidate handling

[Jan Beulich]

>>> On 13.02.17 at 18:01, <andrew.cooper3@citrix.com> wrote:
> On 13/02/17 16:49, Jan Beulich wrote:
>>>>> On 13.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
>>> Sending an invalidation to the device model is an internal detail of
>>> completing the hypercall; callers should not need to be responsible for it.
>>> Drop HVM_HCALL_invalidate entirely and call send_invalidate_req() when
>>> appropriate.
>>>
>>> This makes the function boolean in nature, although the existing
>>> HVM_HCALL_{completed,preempted} to aid code clarity.
>> "are being retained" missing somewhere here?
> 
> Yes.  I already noticed and fixed that up.  It now reads
> 
> "This makes the function boolean in nature, although the existing
> HVM_HCALL_{completed,preempted} constants are kept to aid code clarity."
> 
>>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3874,7 +3874,7 @@ static const hypercall_table_t hvm_hypercall_table[] = 
>>> {
>>>  #undef HYPERCALL
>>>  #undef COMPAT_CALL
>>>  
>>> -int hvm_do_hypercall(struct cpu_user_regs *regs)
>>> +bool hvm_hypercall(struct cpu_user_regs *regs)
>> I don't think bool is a particularly good choice when the callers can't
>> sensibly use the result without comparing with HVM_HCALL_*
> 
> Ok.  I will keep it as int.

With that
Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 1/5] x86/hvm: Rework HVM_HCALL_invalidate handling

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Monday, February 13, 2017 9:04 PM
> 
> Sending an invalidation to the device model is an internal detail of
> completing the hypercall; callers should not need to be responsible for it.
> Drop HVM_HCALL_invalidate entirely and call send_invalidate_req() when
> appropriate.
> 
> This makes the function boolean in nature, although the existing
> HVM_HCALL_{completed,preempted} to aid code clarity.  While updating the
> return type, drop _do from the name, as it is redundant.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [PATCH 2/5] x86/hvm: Split the hypercall dispatching infrastructure out of hvm.c

[Jan Beulich]

>>> On 13.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
> --- /dev/null
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -0,0 +1,332 @@
> +/******************************************************************************
> + * arch/hvm/hypercall.c
> + *
> + * HVM hypercall dispatching routines
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (c) 2017 Citrix Systems Ltd.
> + */
> +#include <xen/lib.h>
> +#include <xen/hypercall.h>
> +
> +#include <asm/hvm/support.h>
> +
> +static int grant_table_op_is_allowed(unsigned int cmd)
> +{
> +    switch (cmd) {

Pure code motion or not, I think it would be nice to fix coding style
(here and elsewhere, albeit the only other issue I can spot are a
few missing blanks lines between non-fall-through case statements).

With that
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 2/5] x86/hvm: Split the hypercall dispatching infrastructure out of hvm.c

[Andrew Cooper]

On 14/02/17 10:33, Jan Beulich wrote:
>>>> On 13.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -0,0 +1,332 @@
>> +/******************************************************************************
>> + * arch/hvm/hypercall.c
>> + *
>> + * HVM hypercall dispatching routines
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
>> + *
>> + * Copyright (c) 2017 Citrix Systems Ltd.
>> + */
>> +#include <xen/lib.h>
>> +#include <xen/hypercall.h>
>> +
>> +#include <asm/hvm/support.h>
>> +
>> +static int grant_table_op_is_allowed(unsigned int cmd)
>> +{
>> +    switch (cmd) {
> Pure code motion or not, I think it would be nice to fix coding style
> (here and elsewhere, albeit the only other issue I can spot are a
> few missing blanks lines between non-fall-through case statements).
>
> With that
> Acked-by: Jan Beulich <jbeulich@suse.com>

I believe all of those issues are addressed in the following patches. 
If not, I can certainly fix them up here.

~Andrew

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

Re: [PATCH 3/5] x86/hvm: Improve memory_op hypercall dispatching

[Jan Beulich]

>>> On 13.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -23,6 +23,29 @@
>  
>  #include <asm/hvm/support.h>
>  
> +static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    struct vcpu *curr = current;

const? (yes, you modify *curr->domain below, but *curr isn't being
altered)

Other than that
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 4/5] x86/hvm: Improve grant_table_op hypercall dispatching

[Jan Beulich]

>>> On 13.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
> hvm_grant_table_op() and hvm_grant_table_op_compat32() are almost identical,
> but there is no need to have two functions instantiated at the end of
> different function pointers.
> 
> Combine the two into a single hvm_grant_table_op() (folding
> grant_table_op_is_allowed() into is now-single caller) and dispatch to
> {do,compat}_grant_table_op() based on the hcall_64bit setting.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 5/5] x86/hvm: Improve physdev_op hypercall dispatching

[Jan Beulich]

>>> On 13.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -73,10 +73,12 @@ static long hvm_grant_table_op(
>  
>  static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> +    struct vcpu *curr = current;

With this constified
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 2/5] x86/hvm: Split the hypercall dispatching infrastructure out of hvm.c

[Jan Beulich]

>>> On 14.02.17 at 11:33, <andrew.cooper3@citrix.com> wrote:
> On 14/02/17 10:33, Jan Beulich wrote:
>>>>> On 13.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>> @@ -0,0 +1,332 @@
>>> 
> +/***************************************************************************
> ***
>>> + * arch/hvm/hypercall.c
>>> + *
>>> + * HVM hypercall dispatching routines
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>> + *
>>> + * Copyright (c) 2017 Citrix Systems Ltd.
>>> + */
>>> +#include <xen/lib.h>
>>> +#include <xen/hypercall.h>
>>> +
>>> +#include <asm/hvm/support.h>
>>> +
>>> +static int grant_table_op_is_allowed(unsigned int cmd)
>>> +{
>>> +    switch (cmd) {
>> Pure code motion or not, I think it would be nice to fix coding style
>> (here and elsewhere, albeit the only other issue I can spot are a
>> few missing blanks lines between non-fall-through case statements).
>>
>> With that
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> I believe all of those issues are addressed in the following patches. 
> If not, I can certainly fix them up here.

Right, I've just seen that while going through the later ones.
Either way is fine with me.

Jan


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