[PATCH v2 0/3] Fixes regarding PV MSRs

[PATCH v2 0/3] Fixes regarding PV MSRs

[Andrew Cooper]

These are all bugfixes for hypercalls relating to migration, which were
discovered during the migration v2 work, but logically distinct and provided
separately to help reduce the length of the migration v2 series.

[PATCH v2 1/3] x86/domctl: Implement XEN_DOMCTL_{get, set}_vcpu_msrs

[Andrew Cooper]

Despite my 'Reviewed-by' tag on c/s 65e3554908 "x86/PV: support data
breakpoint extension registers", I have re-evaluated my position as far as the
hypercall interface is concerned.

Previously, for the sake of not modifying the migration code in libxc,
XEN_DOMCTL_get_ext_vcpucontext would jump though hoops to return -ENOBUFS if
and only if MSRs were in use and no buffer was present.

This is fragile, and awkward from a toolstack point-of-view when actually
sending MSR content in the migration stream.  It also complicates fixing a
further race condition, between querying the number of MSRs for a vcpu, and
the vcpu touching a new one.

As this code is still only in unstable, take this opportunity to redesign the
interface.  This patch introduces the brand new XEN_DOMCTL_{get,set}_vcpu_msrs
subops.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

--
v2:
* Permit partial writes with undersized buffers.
* Leave debugging hint when trying to load a bad MSR.
---
 xen/arch/x86/domctl.c       |  127 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/public/domctl.h |   38 +++++++++++++
 2 files changed, 165 insertions(+)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 87af350..8f1b2af 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1346,6 +1346,133 @@ long arch_do_domctl(
     }
     break;
 
+    case XEN_DOMCTL_get_vcpu_msrs:
+    case XEN_DOMCTL_set_vcpu_msrs:
+    {
+        struct xen_domctl_vcpu_msrs *vmsrs = &domctl->u.vcpu_msrs;
+        struct xen_domctl_vcpu_msr msr;
+        struct vcpu *v;
+        uint32_t nr_msrs = 0;
+
+        ret = -ESRCH;
+        if ( (vmsrs->vcpu >= d->max_vcpus) ||
+             ((v = d->vcpu[vmsrs->vcpu]) == NULL) )
+            break;
+
+        ret = -EINVAL;
+        if ( (v == current) || /* no vcpu_pause() */
+             !is_pv_domain(d) )
+            break;
+
+        /* Count maximum number of optional msrs. */
+        if ( boot_cpu_has(X86_FEATURE_DBEXT) )
+            nr_msrs += 4;
+
+        if ( domctl->cmd == XEN_DOMCTL_get_vcpu_msrs )
+        {
+            ret = 0; copyback = 1;
+
+            /* NULL guest handle is a request for max size. */
+            if ( guest_handle_is_null(vmsrs->msrs) )
+                vmsrs->msr_count = nr_msrs;
+            else
+            {
+                i = 0;
+
+                vcpu_pause(v);
+
+                if ( boot_cpu_has(X86_FEATURE_DBEXT) )
+                {
+                    unsigned int j;
+
+                    if ( v->arch.pv_vcpu.dr_mask[0] )
+                    {
+                        if ( i < vmsrs->msr_count && !ret )
+                        {
+                            msr.index = MSR_AMD64_DR0_ADDRESS_MASK;
+                            msr.reserved = 0;
+                            msr.value = v->arch.pv_vcpu.dr_mask[0];
+                            if ( copy_to_guest_offset(vmsrs->msrs, i, &msr, 1) )
+                                ret = -EFAULT;
+                        }
+                        ++i;
+                    }
+
+                    for ( j = 0; j < 3; ++j )
+                    {
+                        if ( !v->arch.pv_vcpu.dr_mask[1 + j] )
+                            continue;
+                        if ( i < vmsrs->msr_count && !ret )
+                        {
+                            msr.index = MSR_AMD64_DR1_ADDRESS_MASK + j;
+                            msr.reserved = 0;
+                            msr.value = v->arch.pv_vcpu.dr_mask[1 + j];
+                            if ( copy_to_guest_offset(vmsrs->msrs, i, &msr, 1) )
+                                ret = -EFAULT;
+                        }
+                        ++i;
+                    }
+                }
+
+                vcpu_unpause(v);
+
+                if ( i > vmsrs->msr_count && !ret )
+                    ret = -ENOBUFS;
+                vmsrs->msr_count = i;
+            }
+        }
+        else
+        {
+            ret = -EINVAL;
+            if ( vmsrs->msr_count > nr_msrs )
+                break;
+
+            vcpu_pause(v);
+
+            for ( i = 0; i < vmsrs->msr_count; ++i )
+            {
+                ret = -EFAULT;
+                if ( copy_from_guest_offset(&msr, vmsrs->msrs, i, 1) )
+                    break;
+
+                ret = -EINVAL;
+                if ( msr.reserved )
+                    break;
+
+                switch ( msr.index )
+                {
+                case MSR_AMD64_DR0_ADDRESS_MASK:
+                    if ( !boot_cpu_has(X86_FEATURE_DBEXT) ||
+                         (msr.value >> 32) )
+                        break;
+                    v->arch.pv_vcpu.dr_mask[0] = msr.value;
+                    continue;
+
+                case MSR_AMD64_DR1_ADDRESS_MASK ...
+                    MSR_AMD64_DR3_ADDRESS_MASK:
+                    if ( !boot_cpu_has(X86_FEATURE_DBEXT) ||
+                         (msr.value >> 32) )
+                        break;
+                    msr.index -= MSR_AMD64_DR1_ADDRESS_MASK - 1;
+                    v->arch.pv_vcpu.dr_mask[msr.index] = msr.value;
+                    continue;
+                }
+                break;
+            }
+
+            vcpu_unpause(v);
+
+            if ( i == vmsrs->msr_count )
+                ret = 0;
+            else
+            {
+                vmsrs->msr_count = i;
+                copyback = 1;
+            }
+        }
+    }
+    break;
+
     default:
         ret = iommu_do_domctl(domctl, d, u_domctl);
         break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 385b053..0ddffaa 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -895,6 +895,41 @@ struct xen_domctl_cacheflush {
 typedef struct xen_domctl_cacheflush xen_domctl_cacheflush_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_cacheflush_t);
 
+#if defined(__i386__) || defined(__x86_64__)
+struct xen_domctl_vcpu_msr {
+    uint32_t         index;
+    uint32_t         reserved;
+    uint64_aligned_t value;
+};
+typedef struct xen_domctl_vcpu_msr xen_domctl_vcpu_msr_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_vcpu_msr_t);
+
+/*
+ * XEN_DOMCTL_set_vcpu_msrs / XEN_DOMCTL_get_vcpu_msrs.
+ *
+ * Input:
+ * - A NULL 'msrs' guest handle is a request for the maximum 'msr_count'.
+ * - Otherwise, 'msr_count' is the number of entries in 'msrs'.
+ *
+ * Output for get:
+ * - If 'msr_count' is less than the number Xen needs to write, -ENOBUFS shall
+ *   be returned and 'msr_count' updated to reflect the intended number.
+ * - On success, 'msr_count' shall indicate the number of MSRs written, which
+ *   may be less than the maximum if some are not currently used by the vcpu.
+ *
+ * Output for set:
+ * - If Xen encounters an error with a specific MSR, -EINVAL shall be returned
+ *   and 'msr_count' shall be set to the offending index, to aid debugging.
+ */
+struct xen_domctl_vcpu_msrs {
+    uint32_t vcpu;                                   /* IN     */
+    uint32_t msr_count;                              /* IN/OUT */
+    XEN_GUEST_HANDLE_64(xen_domctl_vcpu_msr_t) msrs; /* IN/OUT */
+};
+typedef struct xen_domctl_vcpu_msrs xen_domctl_vcpu_msrs_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_vcpu_msrs_t);
+#endif
+
 struct xen_domctl {
     uint32_t cmd;
 #define XEN_DOMCTL_createdomain                   1
@@ -965,6 +1000,8 @@ struct xen_domctl {
 #define XEN_DOMCTL_getnodeaffinity               69
 #define XEN_DOMCTL_set_max_evtchn                70
 #define XEN_DOMCTL_cacheflush                    71
+#define XEN_DOMCTL_get_vcpu_msrs                 72
+#define XEN_DOMCTL_set_vcpu_msrs                 73
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1014,6 +1051,7 @@ struct xen_domctl {
 #if defined(__i386__) || defined(__x86_64__)
         struct xen_domctl_cpuid             cpuid;
         struct xen_domctl_vcpuextstate      vcpuextstate;
+        struct xen_domctl_vcpu_msrs         vcpu_msrs;
 #endif
         struct xen_domctl_set_access_required access_required;
         struct xen_domctl_audit_p2m         audit_p2m;
-- 
1.7.10.4

[PATCH v2 2/3] tools/libxc: Use an explicit check for PV MSRs in xc_domain_save()

[Andrew Cooper]

Migrating PV domains using MSRs is not supported.  This uses the new
XEN_DOMCTL_get_vcpu_msrs and will fail the migration with an explicit error.

This is an improvement upon the current failure of
  "No extended context for VCPUxx (ENOBUFS)"

Support for migrating PV domains which are using MSRs will be included in the
migration v2 work.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

--
v2: Reduce code based on altered Xen implementation
---
 tools/libxc/xc_domain_save.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index acf3685..778cbde 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -1995,6 +1995,26 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
             goto out;
         }
 
+        /* Check there are no PV MSRs in use. */
+        domctl.cmd = XEN_DOMCTL_get_vcpu_msrs;
+        domctl.domain = dom;
+        memset(&domctl.u, 0, sizeof(domctl.u));
+        domctl.u.vcpu_msrs.vcpu = i;
+        domctl.u.vcpu_msrs.msr_count = 0;
+        set_xen_guest_handle_raw(domctl.u.vcpu_msrs.msrs, (void*)1);
+
+        if ( xc_domctl(xch, &domctl) < 0 )
+        {
+            if ( errno == ENOBUFS )
+            {
+                errno = EOPNOTSUPP;
+                PERROR("Unable to migrate PV guest using MSRs (yet)");
+            }
+            else
+                PERROR("Error querying maximum number of MSRs for VCPU%d", i);
+            goto out;
+        }
+
         /* Start to fetch CPU eXtended States */
         /* Get buffer size first */
         domctl.cmd = XEN_DOMCTL_getvcpuextstate;
-- 
1.7.10.4

[PATCH v2 3/3] x86/domctl: Remove PV MSR parts of XEN_DOMCTL_[gs]et_ext_vcpucontext

[Andrew Cooper]

The PV MSR functionality is now implemented as a separate set of domctls.

This is a revert of parts of c/s65e3554908
  "x86/PV: support data breakpoint extension registers"

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/domctl.c       |   78 +------------------------------------------
 xen/include/public/domctl.h |   19 -----------
 2 files changed, 1 insertion(+), 96 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 8f1b2af..af34d0b 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -818,7 +818,6 @@ long arch_do_domctl(
     {
         struct xen_domctl_ext_vcpucontext *evc;
         struct vcpu *v;
-        struct xen_domctl_ext_vcpu_msr msr;
 
         evc = &domctl->u.ext_vcpucontext;
 
@@ -864,42 +863,7 @@ long arch_do_domctl(
             evc->vmce.mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2;
             evc->vmce.mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2;
 
-            i = ret = 0;
-            if ( boot_cpu_has(X86_FEATURE_DBEXT) )
-            {
-                unsigned int j;
-
-                if ( v->arch.pv_vcpu.dr_mask[0] )
-                {
-                    if ( i < evc->msr_count && !ret )
-                    {
-                        msr.index = MSR_AMD64_DR0_ADDRESS_MASK;
-                        msr.reserved = 0;
-                        msr.value = v->arch.pv_vcpu.dr_mask[0];
-                        if ( copy_to_guest_offset(evc->msrs, i, &msr, 1) )
-                            ret = -EFAULT;
-                    }
-                    ++i;
-                }
-                for ( j = 0; j < 3; ++j )
-                {
-                    if ( !v->arch.pv_vcpu.dr_mask[1 + j] )
-                        continue;
-                    if ( i < evc->msr_count && !ret )
-                    {
-                        msr.index = MSR_AMD64_DR1_ADDRESS_MASK + j;
-                        msr.reserved = 0;
-                        msr.value = v->arch.pv_vcpu.dr_mask[1 + j];
-                        if ( copy_to_guest_offset(evc->msrs, i, &msr, 1) )
-                            ret = -EFAULT;
-                    }
-                    ++i;
-                }
-            }
-            if ( i > evc->msr_count && !ret )
-                ret = -ENOBUFS;
-            evc->msr_count = i;
-
+            ret = 0;
             vcpu_unpause(v);
             copyback = 1;
         }
@@ -954,49 +918,9 @@ long arch_do_domctl(
 
                 ret = vmce_restore_vcpu(v, &vmce);
             }
-            else if ( evc->size > offsetof(typeof(*evc), vmce) )
-                ret = -EINVAL;
             else
                 ret = 0;
 
-            if ( ret || evc->size <= offsetof(typeof(*evc), msrs) )
-                /* nothing */;
-            else if ( evc->size < offsetof(typeof(*evc), msrs) +
-                                  sizeof(evc->msrs) )
-                ret = -EINVAL;
-            else
-            {
-                for ( i = 0; i < evc->msr_count; ++i )
-                {
-                    ret = -EFAULT;
-                    if ( copy_from_guest_offset(&msr, evc->msrs, i, 1) )
-                        break;
-                    ret = -EINVAL;
-                    if ( msr.reserved )
-                        break;
-                    switch ( msr.index )
-                    {
-                    case MSR_AMD64_DR0_ADDRESS_MASK:
-                        if ( !boot_cpu_has(X86_FEATURE_DBEXT) ||
-                             (msr.value >> 32) )
-                            break;
-                        v->arch.pv_vcpu.dr_mask[0] = msr.value;
-                        continue;
-                    case MSR_AMD64_DR1_ADDRESS_MASK ...
-                         MSR_AMD64_DR3_ADDRESS_MASK:
-                        if ( !boot_cpu_has(X86_FEATURE_DBEXT) ||
-                             (msr.value >> 32) )
-                            break;
-                        msr.index -= MSR_AMD64_DR1_ADDRESS_MASK - 1;
-                        v->arch.pv_vcpu.dr_mask[msr.index] = msr.value;
-                        continue;
-                    }
-                    break;
-                }
-                if ( i == evc->msr_count )
-                    ret = 0;
-            }
-
             domain_unpause(d);
         }
     }
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 0ddffaa..2967133 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -543,16 +543,6 @@ typedef struct xen_domctl_pin_mem_cacheattr xen_domctl_pin_mem_cacheattr_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_pin_mem_cacheattr_t);
 
 
-#if defined(__i386__) || defined(__x86_64__)
-struct xen_domctl_ext_vcpu_msr {
-    uint32_t         index;
-    uint32_t         reserved;
-    uint64_aligned_t value;
-};
-typedef struct xen_domctl_ext_vcpu_msr xen_domctl_ext_vcpu_msr_t;
-DEFINE_XEN_GUEST_HANDLE(xen_domctl_ext_vcpu_msr_t);
-#endif
-
 /* XEN_DOMCTL_set_ext_vcpucontext */
 /* XEN_DOMCTL_get_ext_vcpucontext */
 struct xen_domctl_ext_vcpucontext {
@@ -572,14 +562,6 @@ struct xen_domctl_ext_vcpucontext {
     uint16_t         sysenter_callback_cs;
     uint8_t          syscall32_disables_events;
     uint8_t          sysenter_disables_events;
-    /*
-     * When, for the "get" version, msr_count is too small to cover all MSRs
-     * the hypervisor needs to be saved, the call will return -ENOBUFS and
-     * set msr_count to the required (minimum) value. Furthermore, for both
-     * "get" and "set", that field as well as the msrs one only get looked at
-     * if the size field above covers the structure up to the entire msrs one.
-     */
-    uint16_t         msr_count;
 #if defined(__GNUC__)
     union {
         uint64_aligned_t mcg_cap;
@@ -588,7 +570,6 @@ struct xen_domctl_ext_vcpucontext {
 #else
     struct hvm_vmce_vcpu vmce;
 #endif
-    XEN_GUEST_HANDLE_64(xen_domctl_ext_vcpu_msr_t) msrs;
 #endif
 };
 typedef struct xen_domctl_ext_vcpucontext xen_domctl_ext_vcpucontext_t;
-- 
1.7.10.4

Re: [PATCH v2 0/3] Fixes regarding PV MSRs

[Jan Beulich]

>>> On 06.06.14 at 19:18, <andrew.cooper3@citrix.com> wrote:
> These are all bugfixes for hypercalls relating to migration, which were
> discovered during the migration v2 work, but logically distinct and provided
> separately to help reduce the length of the migration v2 series.

Patches 1 and 3
Reviewed-by: Jan Beulich <jbeulich@suse.com>

I'd prefer to apply the series in one go though, so will wait with patch
1 until patch 2 got a tools maintainer's ack.

Jan

Re: [PATCH v2 2/3] tools/libxc: Use an explicit check for PV MSRs in xc_domain_save()

[Ian Campbell]

On Fri, 2014-06-06 at 18:18 +0100, Andrew Cooper wrote:
> Migrating PV domains using MSRs is not supported.  This uses the new
> XEN_DOMCTL_get_vcpu_msrs and will fail the migration with an explicit error.
> 
> This is an improvement upon the current failure of
>   "No extended context for VCPUxx (ENOBUFS)"
> 
> Support for migrating PV domains which are using MSRs will be included in the
> migration v2 work.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>