* [PATCH v2 1/3] x86/domctl: Implement XEN_DOMCTL_{get, set}_vcpu_msrs
2014-06-06 17:18 [PATCH v2 0/3] Fixes regarding PV MSRs Andrew Cooper
@ 2014-06-06 17:18 ` Andrew Cooper
2014-06-06 17:18 ` [PATCH v2 2/3] tools/libxc: Use an explicit check for PV MSRs in xc_domain_save() Andrew Cooper
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2014-06-06 17:18 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 2/3] tools/libxc: Use an explicit check for PV MSRs in xc_domain_save()
2014-06-06 17:18 [PATCH v2 0/3] Fixes regarding PV MSRs Andrew Cooper
2014-06-06 17:18 ` [PATCH v2 1/3] x86/domctl: Implement XEN_DOMCTL_{get, set}_vcpu_msrs Andrew Cooper
@ 2014-06-06 17:18 ` Andrew Cooper
2014-06-10 8:51 ` Ian Campbell
2014-06-06 17:18 ` [PATCH v2 3/3] x86/domctl: Remove PV MSR parts of XEN_DOMCTL_[gs]et_ext_vcpucontext Andrew Cooper
2014-06-10 8:39 ` [PATCH v2 0/3] Fixes regarding PV MSRs Jan Beulich
3 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2014-06-06 17:18 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Jan Beulich
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/3] tools/libxc: Use an explicit check for PV MSRs in xc_domain_save()
2014-06-06 17:18 ` [PATCH v2 2/3] tools/libxc: Use an explicit check for PV MSRs in xc_domain_save() Andrew Cooper
@ 2014-06-10 8:51 ` Ian Campbell
0 siblings, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2014-06-10 8:51 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Ian Jackson, Jan Beulich, Xen-devel
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] x86/domctl: Remove PV MSR parts of XEN_DOMCTL_[gs]et_ext_vcpucontext
2014-06-06 17:18 [PATCH v2 0/3] Fixes regarding PV MSRs Andrew Cooper
2014-06-06 17:18 ` [PATCH v2 1/3] x86/domctl: Implement XEN_DOMCTL_{get, set}_vcpu_msrs Andrew Cooper
2014-06-06 17:18 ` [PATCH v2 2/3] tools/libxc: Use an explicit check for PV MSRs in xc_domain_save() Andrew Cooper
@ 2014-06-06 17:18 ` Andrew Cooper
2014-06-10 8:39 ` [PATCH v2 0/3] Fixes regarding PV MSRs Jan Beulich
3 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2014-06-06 17:18 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 0/3] Fixes regarding PV MSRs
2014-06-06 17:18 [PATCH v2 0/3] Fixes regarding PV MSRs Andrew Cooper
` (2 preceding siblings ...)
2014-06-06 17:18 ` [PATCH v2 3/3] x86/domctl: Remove PV MSR parts of XEN_DOMCTL_[gs]et_ext_vcpucontext Andrew Cooper
@ 2014-06-10 8:39 ` Jan Beulich
3 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2014-06-10 8:39 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread