* [PATCH v4 0/2] x86/AMD support data breakpoint extension registers
@ 2014-04-16 14:23 Jan Beulich
2014-04-16 14:33 ` [PATCH v4 1/2] SVM: " Jan Beulich
` (3 more replies)
0 siblings, 4 replies; 21+ messages in thread
From: Jan Beulich @ 2014-04-16 14:23 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, Ian Jackson, Ian Campbell,
Aravind Gopalakrishnan, suravee.suthikulpanit
1: SVM: support data breakpoint extension registers
2: PV: support data breakpoint extension registers
The tools side patch isn't intended for committing at this point - it
seems more reasonable to handle the migration aspect of this once
Andrew's re-write is in place. Hence the patch is included here just
for reference, and the series is titled to consist of only 2 patches.
3: libxc/PV: save/restore data breakpoint extension registers
v4: Avoid DR7 write in activate_debugregs() when called from set_debugreg()
(in patch 2; both other patches unchanged).
Aravind - it wasn't clear from your most recent response whether the
Reviewed-by applied to only the feature mask changes (previously in
a single series with the changes here) or also to these ones, so I left
it off. Please clarify.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 1/2] SVM: support data breakpoint extension registers
2014-04-16 14:23 [PATCH v4 0/2] x86/AMD support data breakpoint extension registers Jan Beulich
@ 2014-04-16 14:33 ` Jan Beulich
2014-04-16 14:47 ` Aravind Gopalakrishnan
` (2 more replies)
2014-04-16 14:34 ` [PATCH v4 2/2] x86/PV: " Jan Beulich
` (2 subsequent siblings)
3 siblings, 3 replies; 21+ messages in thread
From: Jan Beulich @ 2014-04-16 14:33 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, Ian Jackson, Ian Campbell,
Aravind Gopalakrishnan, suravee.suthikulpanit
[-- Attachment #1: Type: text/plain, Size: 11542 bytes --]
Leveraging the generic MSR save/restore logic introduced a little while
ago.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Aravind Gopalakrishnan<aravind.gopalakrishnan@amd.com>
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
--- a/tools/libxc/xc_cpufeature.h
+++ b/tools/libxc/xc_cpufeature.h
@@ -125,6 +125,7 @@
#define X86_FEATURE_NODEID_MSR 19 /* NodeId MSR */
#define X86_FEATURE_TBM 21 /* trailing bit manipulations */
#define X86_FEATURE_TOPOEXT 22 /* topology extensions CPUID leafs */
+#define X86_FEATURE_DBEXT 26 /* data breakpoint extension */
/* Intel-defined CPU features, CPUID level 0x00000007:0 (ebx) */
#define X86_FEATURE_FSGSBASE 0 /* {RD,WR}{FS,GS}BASE instructions */
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -110,9 +110,10 @@ static void amd_xc_cpuid_policy(
bitmaskof(X86_FEATURE_3DNOWPREFETCH) |
bitmaskof(X86_FEATURE_OSVW) |
bitmaskof(X86_FEATURE_XOP) |
+ bitmaskof(X86_FEATURE_LWP) |
bitmaskof(X86_FEATURE_FMA4) |
bitmaskof(X86_FEATURE_TBM) |
- bitmaskof(X86_FEATURE_LWP));
+ bitmaskof(X86_FEATURE_DBEXT));
regs[3] &= (0x0183f3ff | /* features shared with 0x00000001:EDX */
(is_pae ? bitmaskof(X86_FEATURE_NX) : 0) |
(is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) |
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3080,6 +3080,9 @@ void hvm_cpuid(unsigned int input, unsig
/* Only provide PSE36 when guest runs in 32bit PAE or in long mode */
if ( !(hvm_pae_enabled(v) || hvm_long_mode_enabled(v)) )
*edx &= ~cpufeat_mask(X86_FEATURE_PSE36);
+ /* Hide data breakpoint extensions if the hardware has not support. */
+ if ( !boot_cpu_has(X86_FEATURE_DBEXT) )
+ *ecx &= ~cpufeat_mask(X86_FEATURE_DBEXT);
break;
case 0x80000008:
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -160,14 +160,28 @@ void svm_intercept_msr(struct vcpu *v, u
static void svm_save_dr(struct vcpu *v)
{
struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+ unsigned int flag_dr_dirty = v->arch.hvm_vcpu.flag_dr_dirty;
- if ( !v->arch.hvm_vcpu.flag_dr_dirty )
+ if ( !flag_dr_dirty )
return;
/* Clear the DR dirty flag and re-enable intercepts for DR accesses. */
v->arch.hvm_vcpu.flag_dr_dirty = 0;
vmcb_set_dr_intercepts(vmcb, ~0u);
+ if ( flag_dr_dirty & 2 )
+ {
+ svm_intercept_msr(v, MSR_AMD64_DR0_ADDRESS_MASK, MSR_INTERCEPT_RW);
+ svm_intercept_msr(v, MSR_AMD64_DR1_ADDRESS_MASK, MSR_INTERCEPT_RW);
+ svm_intercept_msr(v, MSR_AMD64_DR2_ADDRESS_MASK, MSR_INTERCEPT_RW);
+ svm_intercept_msr(v, MSR_AMD64_DR3_ADDRESS_MASK, MSR_INTERCEPT_RW);
+
+ rdmsrl(MSR_AMD64_DR0_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[0]);
+ rdmsrl(MSR_AMD64_DR1_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[1]);
+ rdmsrl(MSR_AMD64_DR2_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[2]);
+ rdmsrl(MSR_AMD64_DR3_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[3]);
+ }
+
v->arch.debugreg[0] = read_debugreg(0);
v->arch.debugreg[1] = read_debugreg(1);
v->arch.debugreg[2] = read_debugreg(2);
@@ -178,12 +192,32 @@ static void svm_save_dr(struct vcpu *v)
static void __restore_debug_registers(struct vmcb_struct *vmcb, struct vcpu *v)
{
+ unsigned int ecx;
+
if ( v->arch.hvm_vcpu.flag_dr_dirty )
return;
v->arch.hvm_vcpu.flag_dr_dirty = 1;
vmcb_set_dr_intercepts(vmcb, 0);
+ ASSERT(v == current);
+ hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
+ if ( test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
+ {
+ svm_intercept_msr(v, MSR_AMD64_DR0_ADDRESS_MASK, MSR_INTERCEPT_NONE);
+ svm_intercept_msr(v, MSR_AMD64_DR1_ADDRESS_MASK, MSR_INTERCEPT_NONE);
+ svm_intercept_msr(v, MSR_AMD64_DR2_ADDRESS_MASK, MSR_INTERCEPT_NONE);
+ svm_intercept_msr(v, MSR_AMD64_DR3_ADDRESS_MASK, MSR_INTERCEPT_NONE);
+
+ wrmsrl(MSR_AMD64_DR0_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[0]);
+ wrmsrl(MSR_AMD64_DR1_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[1]);
+ wrmsrl(MSR_AMD64_DR2_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[2]);
+ wrmsrl(MSR_AMD64_DR3_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[3]);
+
+ /* Can't use hvm_cpuid() in svm_save_dr(): v != current. */
+ v->arch.hvm_vcpu.flag_dr_dirty |= 2;
+ }
+
write_debugreg(0, v->arch.debugreg[0]);
write_debugreg(1, v->arch.debugreg[1]);
write_debugreg(2, v->arch.debugreg[2]);
@@ -355,6 +389,72 @@ static int svm_load_vmcb_ctxt(struct vcp
return 0;
}
+static unsigned int __init svm_init_msr(void)
+{
+ return boot_cpu_has(X86_FEATURE_DBEXT) ? 4 : 0;
+}
+
+static void svm_save_msr(struct vcpu *v, struct hvm_msr *ctxt)
+{
+ if ( boot_cpu_has(X86_FEATURE_DBEXT) )
+ {
+ ctxt->msr[ctxt->count].val = v->arch.hvm_svm.dr_mask[0];
+ if ( ctxt->msr[ctxt->count].val )
+ ctxt->msr[ctxt->count++].index = MSR_AMD64_DR0_ADDRESS_MASK;
+
+ ctxt->msr[ctxt->count].val = v->arch.hvm_svm.dr_mask[1];
+ if ( ctxt->msr[ctxt->count].val )
+ ctxt->msr[ctxt->count++].index = MSR_AMD64_DR1_ADDRESS_MASK;
+
+ ctxt->msr[ctxt->count].val = v->arch.hvm_svm.dr_mask[2];
+ if ( ctxt->msr[ctxt->count].val )
+ ctxt->msr[ctxt->count++].index = MSR_AMD64_DR2_ADDRESS_MASK;
+
+ ctxt->msr[ctxt->count].val = v->arch.hvm_svm.dr_mask[3];
+ if ( ctxt->msr[ctxt->count].val )
+ ctxt->msr[ctxt->count++].index = MSR_AMD64_DR3_ADDRESS_MASK;
+ }
+}
+
+static int svm_load_msr(struct vcpu *v, struct hvm_msr *ctxt)
+{
+ unsigned int i, idx;
+ int err = 0;
+
+ for ( i = 0; i < ctxt->count; ++i )
+ {
+ switch ( idx = ctxt->msr[i].index )
+ {
+ case MSR_AMD64_DR0_ADDRESS_MASK:
+ if ( !boot_cpu_has(X86_FEATURE_DBEXT) )
+ err = -ENXIO;
+ else if ( ctxt->msr[i].val >> 32 )
+ err = -EDOM;
+ else
+ v->arch.hvm_svm.dr_mask[0] = ctxt->msr[i].val;
+ break;
+
+ case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
+ if ( !boot_cpu_has(X86_FEATURE_DBEXT) )
+ err = -ENXIO;
+ else if ( ctxt->msr[i].val >> 32 )
+ err = -EDOM;
+ else
+ v->arch.hvm_svm.dr_mask[idx - MSR_AMD64_DR1_ADDRESS_MASK + 1] =
+ ctxt->msr[i].val;
+ break;
+
+ default:
+ continue;
+ }
+ if ( err )
+ break;
+ ctxt->msr[i]._rsvd = 1;
+ }
+
+ return err;
+}
+
static void svm_fpu_enter(struct vcpu *v)
{
struct vmcb_struct *n1vmcb = vcpu_nestedhvm(v).nv_n1vmcx;
@@ -1451,6 +1551,8 @@ static int svm_msr_read_intercept(unsign
switch ( msr )
{
+ unsigned int ecx;
+
case MSR_IA32_SYSENTER_CS:
*msr_content = v->arch.hvm_svm.guest_sysenter_cs;
break;
@@ -1526,6 +1628,21 @@ static int svm_msr_read_intercept(unsign
vpmu_do_rdmsr(msr, msr_content);
break;
+ case MSR_AMD64_DR0_ADDRESS_MASK:
+ hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
+ if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
+ goto gpf;
+ *msr_content = v->arch.hvm_svm.dr_mask[0];
+ break;
+
+ case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
+ hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
+ if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
+ goto gpf;
+ *msr_content =
+ v->arch.hvm_svm.dr_mask[msr - MSR_AMD64_DR1_ADDRESS_MASK + 1];
+ break;
+
case MSR_AMD_OSVW_ID_LENGTH:
case MSR_AMD_OSVW_STATUS:
ret = svm_handle_osvw(v, msr, msr_content, 1);
@@ -1594,6 +1711,8 @@ static int svm_msr_write_intercept(unsig
switch ( msr )
{
+ unsigned int ecx;
+
case MSR_IA32_SYSENTER_CS:
vmcb->sysenter_cs = v->arch.hvm_svm.guest_sysenter_cs = msr_content;
break;
@@ -1669,6 +1788,21 @@ static int svm_msr_write_intercept(unsig
*/
break;
+ case MSR_AMD64_DR0_ADDRESS_MASK:
+ hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
+ if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) || (msr_content >> 32) )
+ goto gpf;
+ v->arch.hvm_svm.dr_mask[0] = msr_content;
+ break;
+
+ case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
+ hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
+ if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) || (msr_content >> 32) )
+ goto gpf;
+ v->arch.hvm_svm.dr_mask[msr - MSR_AMD64_DR1_ADDRESS_MASK + 1] =
+ msr_content;
+ break;
+
case MSR_AMD_OSVW_ID_LENGTH:
case MSR_AMD_OSVW_STATUS:
ret = svm_handle_osvw(v, msr, &msr_content, 0);
@@ -2022,6 +2156,9 @@ static struct hvm_function_table __initd
.vcpu_destroy = svm_vcpu_destroy,
.save_cpu_ctxt = svm_save_vmcb_ctxt,
.load_cpu_ctxt = svm_load_vmcb_ctxt,
+ .init_msr = svm_init_msr,
+ .save_msr = svm_save_msr,
+ .load_msr = svm_load_msr,
.get_interrupt_shadow = svm_get_interrupt_shadow,
.set_interrupt_shadow = svm_set_interrupt_shadow,
.guest_x86_mode = svm_guest_x86_mode,
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -134,6 +134,7 @@
#define X86_FEATURE_NODEID_MSR (6*32+19) /* NodeId MSR */
#define X86_FEATURE_TBM (6*32+21) /* trailing bit manipulations */
#define X86_FEATURE_TOPOEXT (6*32+22) /* topology extensions CPUID leafs */
+#define X86_FEATURE_DBEXT (6*32+26) /* data breakpoint extension */
/* Intel-defined CPU features, CPUID level 0x00000007:0 (ebx), word 7 */
#define X86_FEATURE_FSGSBASE (7*32+ 0) /* {RD,WR}{FS,GS}BASE instructions */
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -515,6 +515,9 @@ struct arch_svm_struct {
uint64_t guest_lwp_cfg; /* guest version */
uint64_t cpu_lwp_cfg; /* CPU version */
+ /* data breakpoint extension MSRs */
+ uint32_t dr_mask[4];
+
/* OSVW MSRs */
struct {
u64 length;
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -140,7 +140,7 @@ struct hvm_vcpu {
int xen_port;
- bool_t flag_dr_dirty;
+ u8 flag_dr_dirty;
bool_t debug_state_latch;
bool_t single_step;
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -208,6 +208,11 @@
#define MSR_AMD64_DC_CFG 0xc0011022
#define AMD64_NB_CFG_CF8_EXT_ENABLE_BIT 46
+#define MSR_AMD64_DR0_ADDRESS_MASK 0xc0011027
+#define MSR_AMD64_DR1_ADDRESS_MASK 0xc0011019
+#define MSR_AMD64_DR2_ADDRESS_MASK 0xc001101a
+#define MSR_AMD64_DR3_ADDRESS_MASK 0xc001101b
+
/* AMD Family10h machine check MSRs */
#define MSR_F10_MC4_MISC1 0xc0000408
#define MSR_F10_MC4_MISC2 0xc0000409
[-- Attachment #2: SVM-debug-address-mask-MSRs.patch --]
[-- Type: text/plain, Size: 11594 bytes --]
x86/SVM: support data breakpoint extension registers
Leveraging the generic MSR save/restore logic introduced a little while
ago.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Aravind Gopalakrishnan<aravind.gopalakrishnan@amd.com>
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
--- a/tools/libxc/xc_cpufeature.h
+++ b/tools/libxc/xc_cpufeature.h
@@ -125,6 +125,7 @@
#define X86_FEATURE_NODEID_MSR 19 /* NodeId MSR */
#define X86_FEATURE_TBM 21 /* trailing bit manipulations */
#define X86_FEATURE_TOPOEXT 22 /* topology extensions CPUID leafs */
+#define X86_FEATURE_DBEXT 26 /* data breakpoint extension */
/* Intel-defined CPU features, CPUID level 0x00000007:0 (ebx) */
#define X86_FEATURE_FSGSBASE 0 /* {RD,WR}{FS,GS}BASE instructions */
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -110,9 +110,10 @@ static void amd_xc_cpuid_policy(
bitmaskof(X86_FEATURE_3DNOWPREFETCH) |
bitmaskof(X86_FEATURE_OSVW) |
bitmaskof(X86_FEATURE_XOP) |
+ bitmaskof(X86_FEATURE_LWP) |
bitmaskof(X86_FEATURE_FMA4) |
bitmaskof(X86_FEATURE_TBM) |
- bitmaskof(X86_FEATURE_LWP));
+ bitmaskof(X86_FEATURE_DBEXT));
regs[3] &= (0x0183f3ff | /* features shared with 0x00000001:EDX */
(is_pae ? bitmaskof(X86_FEATURE_NX) : 0) |
(is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) |
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3080,6 +3080,9 @@ void hvm_cpuid(unsigned int input, unsig
/* Only provide PSE36 when guest runs in 32bit PAE or in long mode */
if ( !(hvm_pae_enabled(v) || hvm_long_mode_enabled(v)) )
*edx &= ~cpufeat_mask(X86_FEATURE_PSE36);
+ /* Hide data breakpoint extensions if the hardware has not support. */
+ if ( !boot_cpu_has(X86_FEATURE_DBEXT) )
+ *ecx &= ~cpufeat_mask(X86_FEATURE_DBEXT);
break;
case 0x80000008:
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -160,14 +160,28 @@ void svm_intercept_msr(struct vcpu *v, u
static void svm_save_dr(struct vcpu *v)
{
struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+ unsigned int flag_dr_dirty = v->arch.hvm_vcpu.flag_dr_dirty;
- if ( !v->arch.hvm_vcpu.flag_dr_dirty )
+ if ( !flag_dr_dirty )
return;
/* Clear the DR dirty flag and re-enable intercepts for DR accesses. */
v->arch.hvm_vcpu.flag_dr_dirty = 0;
vmcb_set_dr_intercepts(vmcb, ~0u);
+ if ( flag_dr_dirty & 2 )
+ {
+ svm_intercept_msr(v, MSR_AMD64_DR0_ADDRESS_MASK, MSR_INTERCEPT_RW);
+ svm_intercept_msr(v, MSR_AMD64_DR1_ADDRESS_MASK, MSR_INTERCEPT_RW);
+ svm_intercept_msr(v, MSR_AMD64_DR2_ADDRESS_MASK, MSR_INTERCEPT_RW);
+ svm_intercept_msr(v, MSR_AMD64_DR3_ADDRESS_MASK, MSR_INTERCEPT_RW);
+
+ rdmsrl(MSR_AMD64_DR0_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[0]);
+ rdmsrl(MSR_AMD64_DR1_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[1]);
+ rdmsrl(MSR_AMD64_DR2_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[2]);
+ rdmsrl(MSR_AMD64_DR3_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[3]);
+ }
+
v->arch.debugreg[0] = read_debugreg(0);
v->arch.debugreg[1] = read_debugreg(1);
v->arch.debugreg[2] = read_debugreg(2);
@@ -178,12 +192,32 @@ static void svm_save_dr(struct vcpu *v)
static void __restore_debug_registers(struct vmcb_struct *vmcb, struct vcpu *v)
{
+ unsigned int ecx;
+
if ( v->arch.hvm_vcpu.flag_dr_dirty )
return;
v->arch.hvm_vcpu.flag_dr_dirty = 1;
vmcb_set_dr_intercepts(vmcb, 0);
+ ASSERT(v == current);
+ hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
+ if ( test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
+ {
+ svm_intercept_msr(v, MSR_AMD64_DR0_ADDRESS_MASK, MSR_INTERCEPT_NONE);
+ svm_intercept_msr(v, MSR_AMD64_DR1_ADDRESS_MASK, MSR_INTERCEPT_NONE);
+ svm_intercept_msr(v, MSR_AMD64_DR2_ADDRESS_MASK, MSR_INTERCEPT_NONE);
+ svm_intercept_msr(v, MSR_AMD64_DR3_ADDRESS_MASK, MSR_INTERCEPT_NONE);
+
+ wrmsrl(MSR_AMD64_DR0_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[0]);
+ wrmsrl(MSR_AMD64_DR1_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[1]);
+ wrmsrl(MSR_AMD64_DR2_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[2]);
+ wrmsrl(MSR_AMD64_DR3_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[3]);
+
+ /* Can't use hvm_cpuid() in svm_save_dr(): v != current. */
+ v->arch.hvm_vcpu.flag_dr_dirty |= 2;
+ }
+
write_debugreg(0, v->arch.debugreg[0]);
write_debugreg(1, v->arch.debugreg[1]);
write_debugreg(2, v->arch.debugreg[2]);
@@ -355,6 +389,72 @@ static int svm_load_vmcb_ctxt(struct vcp
return 0;
}
+static unsigned int __init svm_init_msr(void)
+{
+ return boot_cpu_has(X86_FEATURE_DBEXT) ? 4 : 0;
+}
+
+static void svm_save_msr(struct vcpu *v, struct hvm_msr *ctxt)
+{
+ if ( boot_cpu_has(X86_FEATURE_DBEXT) )
+ {
+ ctxt->msr[ctxt->count].val = v->arch.hvm_svm.dr_mask[0];
+ if ( ctxt->msr[ctxt->count].val )
+ ctxt->msr[ctxt->count++].index = MSR_AMD64_DR0_ADDRESS_MASK;
+
+ ctxt->msr[ctxt->count].val = v->arch.hvm_svm.dr_mask[1];
+ if ( ctxt->msr[ctxt->count].val )
+ ctxt->msr[ctxt->count++].index = MSR_AMD64_DR1_ADDRESS_MASK;
+
+ ctxt->msr[ctxt->count].val = v->arch.hvm_svm.dr_mask[2];
+ if ( ctxt->msr[ctxt->count].val )
+ ctxt->msr[ctxt->count++].index = MSR_AMD64_DR2_ADDRESS_MASK;
+
+ ctxt->msr[ctxt->count].val = v->arch.hvm_svm.dr_mask[3];
+ if ( ctxt->msr[ctxt->count].val )
+ ctxt->msr[ctxt->count++].index = MSR_AMD64_DR3_ADDRESS_MASK;
+ }
+}
+
+static int svm_load_msr(struct vcpu *v, struct hvm_msr *ctxt)
+{
+ unsigned int i, idx;
+ int err = 0;
+
+ for ( i = 0; i < ctxt->count; ++i )
+ {
+ switch ( idx = ctxt->msr[i].index )
+ {
+ case MSR_AMD64_DR0_ADDRESS_MASK:
+ if ( !boot_cpu_has(X86_FEATURE_DBEXT) )
+ err = -ENXIO;
+ else if ( ctxt->msr[i].val >> 32 )
+ err = -EDOM;
+ else
+ v->arch.hvm_svm.dr_mask[0] = ctxt->msr[i].val;
+ break;
+
+ case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
+ if ( !boot_cpu_has(X86_FEATURE_DBEXT) )
+ err = -ENXIO;
+ else if ( ctxt->msr[i].val >> 32 )
+ err = -EDOM;
+ else
+ v->arch.hvm_svm.dr_mask[idx - MSR_AMD64_DR1_ADDRESS_MASK + 1] =
+ ctxt->msr[i].val;
+ break;
+
+ default:
+ continue;
+ }
+ if ( err )
+ break;
+ ctxt->msr[i]._rsvd = 1;
+ }
+
+ return err;
+}
+
static void svm_fpu_enter(struct vcpu *v)
{
struct vmcb_struct *n1vmcb = vcpu_nestedhvm(v).nv_n1vmcx;
@@ -1451,6 +1551,8 @@ static int svm_msr_read_intercept(unsign
switch ( msr )
{
+ unsigned int ecx;
+
case MSR_IA32_SYSENTER_CS:
*msr_content = v->arch.hvm_svm.guest_sysenter_cs;
break;
@@ -1526,6 +1628,21 @@ static int svm_msr_read_intercept(unsign
vpmu_do_rdmsr(msr, msr_content);
break;
+ case MSR_AMD64_DR0_ADDRESS_MASK:
+ hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
+ if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
+ goto gpf;
+ *msr_content = v->arch.hvm_svm.dr_mask[0];
+ break;
+
+ case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
+ hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
+ if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
+ goto gpf;
+ *msr_content =
+ v->arch.hvm_svm.dr_mask[msr - MSR_AMD64_DR1_ADDRESS_MASK + 1];
+ break;
+
case MSR_AMD_OSVW_ID_LENGTH:
case MSR_AMD_OSVW_STATUS:
ret = svm_handle_osvw(v, msr, msr_content, 1);
@@ -1594,6 +1711,8 @@ static int svm_msr_write_intercept(unsig
switch ( msr )
{
+ unsigned int ecx;
+
case MSR_IA32_SYSENTER_CS:
vmcb->sysenter_cs = v->arch.hvm_svm.guest_sysenter_cs = msr_content;
break;
@@ -1669,6 +1788,21 @@ static int svm_msr_write_intercept(unsig
*/
break;
+ case MSR_AMD64_DR0_ADDRESS_MASK:
+ hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
+ if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) || (msr_content >> 32) )
+ goto gpf;
+ v->arch.hvm_svm.dr_mask[0] = msr_content;
+ break;
+
+ case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
+ hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
+ if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) || (msr_content >> 32) )
+ goto gpf;
+ v->arch.hvm_svm.dr_mask[msr - MSR_AMD64_DR1_ADDRESS_MASK + 1] =
+ msr_content;
+ break;
+
case MSR_AMD_OSVW_ID_LENGTH:
case MSR_AMD_OSVW_STATUS:
ret = svm_handle_osvw(v, msr, &msr_content, 0);
@@ -2022,6 +2156,9 @@ static struct hvm_function_table __initd
.vcpu_destroy = svm_vcpu_destroy,
.save_cpu_ctxt = svm_save_vmcb_ctxt,
.load_cpu_ctxt = svm_load_vmcb_ctxt,
+ .init_msr = svm_init_msr,
+ .save_msr = svm_save_msr,
+ .load_msr = svm_load_msr,
.get_interrupt_shadow = svm_get_interrupt_shadow,
.set_interrupt_shadow = svm_set_interrupt_shadow,
.guest_x86_mode = svm_guest_x86_mode,
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -134,6 +134,7 @@
#define X86_FEATURE_NODEID_MSR (6*32+19) /* NodeId MSR */
#define X86_FEATURE_TBM (6*32+21) /* trailing bit manipulations */
#define X86_FEATURE_TOPOEXT (6*32+22) /* topology extensions CPUID leafs */
+#define X86_FEATURE_DBEXT (6*32+26) /* data breakpoint extension */
/* Intel-defined CPU features, CPUID level 0x00000007:0 (ebx), word 7 */
#define X86_FEATURE_FSGSBASE (7*32+ 0) /* {RD,WR}{FS,GS}BASE instructions */
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -515,6 +515,9 @@ struct arch_svm_struct {
uint64_t guest_lwp_cfg; /* guest version */
uint64_t cpu_lwp_cfg; /* CPU version */
+ /* data breakpoint extension MSRs */
+ uint32_t dr_mask[4];
+
/* OSVW MSRs */
struct {
u64 length;
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -140,7 +140,7 @@ struct hvm_vcpu {
int xen_port;
- bool_t flag_dr_dirty;
+ u8 flag_dr_dirty;
bool_t debug_state_latch;
bool_t single_step;
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -208,6 +208,11 @@
#define MSR_AMD64_DC_CFG 0xc0011022
#define AMD64_NB_CFG_CF8_EXT_ENABLE_BIT 46
+#define MSR_AMD64_DR0_ADDRESS_MASK 0xc0011027
+#define MSR_AMD64_DR1_ADDRESS_MASK 0xc0011019
+#define MSR_AMD64_DR2_ADDRESS_MASK 0xc001101a
+#define MSR_AMD64_DR3_ADDRESS_MASK 0xc001101b
+
/* AMD Family10h machine check MSRs */
#define MSR_F10_MC4_MISC1 0xc0000408
#define MSR_F10_MC4_MISC2 0xc0000409
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 2/2] x86/PV: support data breakpoint extension registers
2014-04-16 14:23 [PATCH v4 0/2] x86/AMD support data breakpoint extension registers Jan Beulich
2014-04-16 14:33 ` [PATCH v4 1/2] SVM: " Jan Beulich
@ 2014-04-16 14:34 ` Jan Beulich
2014-04-16 14:48 ` Aravind Gopalakrishnan
2014-04-23 10:23 ` Ian Campbell
2014-04-16 14:35 ` [PATCH v4 3/2] libxc/PV: save/restore " Jan Beulich
2014-04-22 12:37 ` Ping: [PATCH v4 0/2] x86/AMD support " Jan Beulich
3 siblings, 2 replies; 21+ messages in thread
From: Jan Beulich @ 2014-04-16 14:34 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, Ian Jackson, Ian Campbell,
Aravind Gopalakrishnan, suravee.suthikulpanit
Introducing an extension to XEN_DOMCTL_[gs]et_ext_vcpucontext similar
to the generic MSR save/restore logic recently added for HVM.
This also moves some debug register related declarations/definition to
the header intended for these.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Aravind Gopalakrishnan<aravind.gopalakrishnan@amd.com>
---
v4: Avoid DR7 write in activate_debugregs() when called from set_debugreg().
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -9,6 +9,7 @@
#include <xen/smp.h>
#include <asm/processor.h>
#include <asm/msr.h>
+#include <asm/debugreg.h>
#include <asm/flushtlb.h>
#include <asm/hvm/hvm.h>
#include <asm/hvm/support.h>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1341,14 +1341,7 @@ static void paravirt_ctxt_switch_to(stru
write_cr4(cr4);
if ( unlikely(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
- {
- write_debugreg(0, v->arch.debugreg[0]);
- write_debugreg(1, v->arch.debugreg[1]);
- write_debugreg(2, v->arch.debugreg[2]);
- write_debugreg(3, v->arch.debugreg[3]);
- write_debugreg(6, v->arch.debugreg[6]);
- write_debugreg(7, v->arch.debugreg[7]);
- }
+ activate_debugregs(v);
if ( (v->domain->arch.tsc_mode == TSC_MODE_PVRDTSCP) &&
boot_cpu_has(X86_FEATURE_RDTSCP) )
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -52,6 +52,7 @@ long arch_do_domctl(
{
long ret = 0;
bool_t copyback = 0;
+ unsigned long i;
switch ( domctl->cmd )
{
@@ -321,7 +322,6 @@ long arch_do_domctl(
case XEN_DOMCTL_getmemlist:
{
- int i;
unsigned long max_pfns = domctl->u.getmemlist.max_pfns;
uint64_t mfn;
struct page_info *page;
@@ -647,7 +647,6 @@ long arch_do_domctl(
unsigned long mfn = domctl->u.memory_mapping.first_mfn;
unsigned long nr_mfns = domctl->u.memory_mapping.nr_mfns;
int add = domctl->u.memory_mapping.add_mapping;
- unsigned long i;
ret = -EINVAL;
if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
@@ -820,6 +819,7 @@ 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;
@@ -865,7 +865,42 @@ 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;
- ret = 0;
+ 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;
+
vcpu_unpause(v);
copyback = 1;
}
@@ -920,9 +955,49 @@ 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);
}
}
@@ -932,7 +1007,6 @@ long arch_do_domctl(
{
xen_domctl_cpuid_t *ctl = &domctl->u.cpuid;
cpuid_input_t *cpuid = NULL;
- int i;
for ( i = 0; i < MAX_CPUID_INPUT; i++ )
{
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2499,6 +2499,23 @@ static int emulate_privileged_op(struct
if ( wrmsr_safe(regs->ecx, msr_content) != 0 )
goto fail;
break;
+
+ case MSR_AMD64_DR0_ADDRESS_MASK:
+ if ( !boot_cpu_has(X86_FEATURE_DBEXT) || (msr_content >> 32) )
+ goto fail;
+ v->arch.pv_vcpu.dr_mask[0] = msr_content;
+ if ( v->arch.debugreg[7] & DR7_ACTIVE_MASK )
+ wrmsrl(MSR_AMD64_DR0_ADDRESS_MASK, msr_content);
+ break;
+ case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
+ if ( !boot_cpu_has(X86_FEATURE_DBEXT) || (msr_content >> 32) )
+ goto fail;
+ v->arch.pv_vcpu.dr_mask
+ [regs->_ecx - MSR_AMD64_DR1_ADDRESS_MASK + 1] = msr_content;
+ if ( v->arch.debugreg[7] & DR7_ACTIVE_MASK )
+ wrmsrl(regs->_ecx, msr_content);
+ break;
+
default:
if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) == 1 )
break;
@@ -2586,6 +2603,21 @@ static int emulate_privileged_op(struct
regs->eax = (uint32_t)msr_content;
regs->edx = (uint32_t)(msr_content >> 32);
break;
+
+ case MSR_AMD64_DR0_ADDRESS_MASK:
+ if ( !boot_cpu_has(X86_FEATURE_DBEXT) )
+ goto fail;
+ regs->eax = v->arch.pv_vcpu.dr_mask[0];
+ regs->edx = 0;
+ break;
+ case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
+ if ( !boot_cpu_has(X86_FEATURE_DBEXT) )
+ goto fail;
+ regs->eax = v->arch.pv_vcpu.dr_mask
+ [regs->_ecx - MSR_AMD64_DR1_ADDRESS_MASK + 1];
+ regs->edx = 0;
+ break;
+
default:
if ( rdmsr_hypervisor_regs(regs->ecx, &val) )
{
@@ -3629,7 +3661,34 @@ long do_set_trap_table(XEN_GUEST_HANDLE_
return rc;
}
-long set_debugreg(struct vcpu *v, int reg, unsigned long value)
+void activate_debugregs(const struct vcpu *curr)
+{
+ ASSERT(curr == current);
+
+ write_debugreg(0, curr->arch.debugreg[0]);
+ write_debugreg(1, curr->arch.debugreg[1]);
+ write_debugreg(2, curr->arch.debugreg[2]);
+ write_debugreg(3, curr->arch.debugreg[3]);
+ write_debugreg(6, curr->arch.debugreg[6]);
+
+ /*
+ * Avoid writing the subsequently getting replaced value when getting
+ * called from set_debugreg() below. Eventual future callers will need
+ * to take this into account.
+ */
+ if ( curr->arch.debugreg[7] & DR7_ACTIVE_MASK )
+ write_debugreg(7, curr->arch.debugreg[7]);
+
+ if ( boot_cpu_has(X86_FEATURE_DBEXT) )
+ {
+ wrmsrl(MSR_AMD64_DR0_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[0]);
+ wrmsrl(MSR_AMD64_DR1_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[1]);
+ wrmsrl(MSR_AMD64_DR2_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[2]);
+ wrmsrl(MSR_AMD64_DR3_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[3]);
+ }
+}
+
+long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
{
int i;
struct vcpu *curr = current;
@@ -3710,11 +3769,8 @@ long set_debugreg(struct vcpu *v, int re
if ( (v == curr) &&
!(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
{
- write_debugreg(0, v->arch.debugreg[0]);
- write_debugreg(1, v->arch.debugreg[1]);
- write_debugreg(2, v->arch.debugreg[2]);
- write_debugreg(3, v->arch.debugreg[3]);
- write_debugreg(6, v->arch.debugreg[6]);
+ activate_debugregs(v);
+ break;
}
}
if ( v == curr )
--- a/xen/include/asm-x86/debugreg.h
+++ b/xen/include/asm-x86/debugreg.h
@@ -64,4 +64,16 @@
#define DR_GLOBAL_EXACT_ENABLE (0x00000200ul) /* Global exact enable */
#define DR_GENERAL_DETECT (0x00002000ul) /* General detect enable */
+#define write_debugreg(reg, val) do { \
+ unsigned long __val = val; \
+ asm volatile ( "mov %0,%%db" #reg : : "r" (__val) ); \
+} while (0)
+#define read_debugreg(reg) ({ \
+ unsigned long __val; \
+ asm volatile ( "mov %%db" #reg ",%0" : "=r" (__val) ); \
+ __val; \
+})
+long set_debugreg(struct vcpu *, unsigned int reg, unsigned long value);
+void activate_debugregs(const struct vcpu *);
+
#endif /* _X86_DEBUGREG_H */
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -374,6 +374,9 @@ struct pv_vcpu
unsigned long shadow_ldt_mapcnt;
spinlock_t shadow_ldt_lock;
+ /* data breakpoint extension MSRs */
+ uint32_t dr_mask[4];
+
/* Deferred VA-based update state. */
bool_t need_update_runstate_area;
struct vcpu_time_info pending_system_time;
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -462,17 +462,6 @@ long set_gdt(struct vcpu *d,
unsigned long *frames,
unsigned int entries);
-#define write_debugreg(reg, val) do { \
- unsigned long __val = val; \
- asm volatile ( "mov %0,%%db" #reg : : "r" (__val) ); \
-} while (0)
-#define read_debugreg(reg) ({ \
- unsigned long __val; \
- asm volatile ( "mov %%db" #reg ",%0" : "=r" (__val) ); \
- __val; \
-})
-long set_debugreg(struct vcpu *p, int reg, unsigned long value);
-
/* REP NOP (PAUSE) is a good thing to insert into busy-wait loops. */
static always_inline void rep_nop(void)
{
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -36,7 +36,7 @@
#include "grant_table.h"
#include "hvm/save.h"
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000009
+#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000a
/*
* NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -564,6 +564,16 @@ typedef struct xen_domctl_pin_mem_cachea
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 {
@@ -583,6 +593,7 @@ struct xen_domctl_ext_vcpucontext {
uint16_t sysenter_callback_cs;
uint8_t syscall32_disables_events;
uint8_t sysenter_disables_events;
+ uint16_t msr_count;
#if defined(__GNUC__)
union {
uint64_aligned_t mcg_cap;
@@ -591,6 +602,7 @@ 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;
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 3/2] libxc/PV: save/restore data breakpoint extension registers
2014-04-16 14:23 [PATCH v4 0/2] x86/AMD support data breakpoint extension registers Jan Beulich
2014-04-16 14:33 ` [PATCH v4 1/2] SVM: " Jan Beulich
2014-04-16 14:34 ` [PATCH v4 2/2] x86/PV: " Jan Beulich
@ 2014-04-16 14:35 ` Jan Beulich
2014-04-22 12:37 ` Ping: [PATCH v4 0/2] x86/AMD support " Jan Beulich
3 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2014-04-16 14:35 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, Ian Jackson, Ian Campbell,
Aravind Gopalakrishnan, suravee.suthikulpanit
[-- Attachment #1: Type: text/plain, Size: 7085 bytes --]
Requiring the handling of the generic MSR extension to
XEN_DOMCTL_[gs]et_ext_vcpucontext.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -38,6 +38,7 @@
#include <stdlib.h>
#include <unistd.h>
+#include <assert.h>
#include "xg_private.h"
#include "xg_save_restore.h"
@@ -590,8 +591,13 @@ static int buffer_tail_pv(xc_interface *
uint32_t vcpuextstate_size)
{
unsigned int i;
- size_t pfnlen, vcpulen;
+ size_t pfnlen, vcpulen, total;
+ int alloc = 0;
struct domain_info_context *dinfo = &ctx->dinfo;
+ union {
+ const unsigned char *raw;
+ xen_domctl_ext_vcpucontext_t *evc;
+ } ptr;
/* TODO: handle changing pfntab and vcpu counts */
/* PFN tab */
@@ -634,11 +640,42 @@ static int buffer_tail_pv(xc_interface *
ERROR("Error allocating VCPU ctxt tail buffer");
goto free_pfntab;
}
+ alloc = 1;
}
// DPRINTF("Reading VCPUS: %d bytes\n", vcpulen);
- if ( RDEXACT(fd, buf->vcpubuf, vcpulen) ) {
- PERROR("Error when reading ctxt");
- goto free_vcpus;
+ for ( total = i = 0, ptr.raw = buf->vcpubuf; ext_vcpucontext; ) {
+ if ( RDEXACT(fd, buf->vcpubuf + total, vcpulen) ) {
+ PERROR("Error when reading ctxt");
+ goto free_vcpus;
+ }
+ total += vcpulen;
+ for ( vcpulen = 0; i < buf->vcpucount; ++i ) {
+ size_t msrlen;
+
+ if ( (const unsigned char *)(ptr.evc + 1) > buf->vcpubuf + total )
+ break;
+ if ( ptr.evc->size <
+ (offsetof(xen_domctl_ext_vcpucontext_t, msrs) +
+ sizeof(ptr.evc->msrs)) )
+ ptr.evc->msr_count = 0;
+ msrlen = ptr.evc->msr_count * sizeof(xen_domctl_ext_vcpu_msr_t);
+ vcpulen += msrlen;
+ ptr.raw += 128 + msrlen + vcpuextstate_size;
+ }
+ if ( !vcpulen ) {
+ assert(i == buf->vcpucount);
+ break;
+ }
+ if ( alloc ) {
+ void *nbuf = realloc(buf->vcpubuf, total + vcpulen);
+
+ if ( !nbuf ) {
+ ERROR("Error growing VCPU ctxt tail buffer");
+ goto free_vcpus;
+ }
+ ptr.raw = nbuf + (ptr.raw - buf->vcpubuf);
+ buf->vcpubuf = nbuf;
+ }
}
/* load shared_info_page */
@@ -2130,9 +2167,28 @@ int xc_domain_restore(xc_interface *xch,
goto vcpu_ext_state_restore;
memcpy(&domctl.u.ext_vcpucontext, vcpup, 128);
vcpup += 128;
- domctl.cmd = XEN_DOMCTL_set_ext_vcpucontext;
- domctl.domain = dom;
- frc = xc_domctl(xch, &domctl);
+ if ( domctl.u.ext_vcpucontext.msr_count )
+ {
+ xen_domctl_ext_vcpu_msr_t *msrs = (void *)vcpup;
+ size_t sz = domctl.u.ext_vcpucontext.msr_count * sizeof(*msrs);
+ DECLARE_HYPERCALL_BOUNCE(msrs, sz, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+ if ( xc_hypercall_bounce_pre(xch, msrs) )
+ {
+ PERROR("Can't bounce in vcpu%d MSRs", i);
+ goto out;
+ }
+ set_xen_guest_handle(domctl.u.ext_vcpucontext.msrs, msrs);
+ domctl.cmd = XEN_DOMCTL_set_ext_vcpucontext;
+ domctl.domain = dom;
+ frc = xc_domctl(xch, &domctl);
+ xc_hypercall_bounce_post(xch, msrs);
+ vcpup += sz;
+ } else {
+ domctl.cmd = XEN_DOMCTL_set_ext_vcpucontext;
+ domctl.domain = dom;
+ frc = xc_domctl(xch, &domctl);
+ }
if ( frc != 0 )
{
PERROR("Couldn't set extended vcpu%d info", i);
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -836,6 +836,9 @@ int xc_domain_save(xc_interface *xch, in
/* base of the region in which domain memory is mapped */
unsigned char *region_base = NULL;
+ /* MSR extensions to xen_domctl_ext_vcpucontext_t */
+ DECLARE_HYPERCALL_BUFFER(xen_domctl_ext_vcpu_msr_t, msrs);
+
/* A copy of the CPU eXtended States of the guest. */
DECLARE_HYPERCALL_BUFFER(void, buffer);
@@ -1960,16 +1963,42 @@ int xc_domain_save(xc_interface *xch, in
domctl.domain = dom;
memset(&domctl.u, 0, sizeof(domctl.u));
domctl.u.ext_vcpucontext.vcpu = i;
- if ( xc_domctl(xch, &domctl) < 0 )
+ frc = xc_domctl(xch, &domctl);
+ if ( frc < 0 && errno == ENOBUFS &&
+ domctl.u.ext_vcpucontext.size >=
+ (offsetof(xen_domctl_ext_vcpucontext_t, msrs) +
+ sizeof(domctl.u.ext_vcpucontext.msrs)) &&
+ domctl.u.ext_vcpucontext.msr_count )
+ {
+ msrs = xc_hypercall_buffer_alloc(xch, msrs,
+ domctl.u.ext_vcpucontext.msr_count *
+ sizeof(*msrs));
+ set_xen_guest_handle(domctl.u.ext_vcpucontext.msrs, msrs);
+ frc = msrs ? xc_domctl(xch, &domctl) : -1;
+ /* Don't save the actual pointer. */
+ set_xen_guest_handle_raw(domctl.u.ext_vcpucontext.msrs, NULL);
+ }
+ if ( frc < 0 )
{
PERROR("No extended context for VCPU%d", i);
goto out;
}
if ( wrexact(io_fd, &domctl.u.ext_vcpucontext, 128) )
{
- PERROR("Error when writing to state file (2)");
+ PERROR("Error when writing to state file (ext ctxt)");
goto out;
}
+ if ( msrs )
+ {
+ if ( wrexact(io_fd, msrs,
+ domctl.u.ext_vcpucontext.msr_count * sizeof(*msrs)) )
+ {
+ PERROR("Error when writing to state file (MSRs)");
+ goto out;
+ }
+ xc_hypercall_buffer_free(xch, msrs);
+ msrs = NULL;
+ }
/* Start to fetch CPU eXtended States */
/* Get buffer size first */
@@ -2134,6 +2163,7 @@ int xc_domain_save(xc_interface *xch, in
xc_hypercall_buffer_free_pages(xch, to_send, NRPAGES(bitmap_size(dinfo->p2m_size)));
xc_hypercall_buffer_free_pages(xch, to_skip, NRPAGES(bitmap_size(dinfo->p2m_size)));
+ xc_hypercall_buffer_free(xch, msrs);
free(pfn_type);
free(pfn_batch);
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -62,6 +62,9 @@
* the block size.
* "extv" : Presence indicates use of extended VCPU context in
* tail, data size is 0.
+ * This block may be followed by an array of
+ * xen_domctl_ext_vcpu_msr_t as indicated by
+ * xen_domctl_ext_vcpucontext_t's msr_count field.
*
* p2m (PV-only):
*
[-- Attachment #2: PV-debug-address-mask-MSRs-sr.patch --]
[-- Type: text/plain, Size: 7143 bytes --]
libxc/PV: save/restore data breakpoint extension registers
Requiring the handling of the generic MSR extension to
XEN_DOMCTL_[gs]et_ext_vcpucontext.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -38,6 +38,7 @@
#include <stdlib.h>
#include <unistd.h>
+#include <assert.h>
#include "xg_private.h"
#include "xg_save_restore.h"
@@ -590,8 +591,13 @@ static int buffer_tail_pv(xc_interface *
uint32_t vcpuextstate_size)
{
unsigned int i;
- size_t pfnlen, vcpulen;
+ size_t pfnlen, vcpulen, total;
+ int alloc = 0;
struct domain_info_context *dinfo = &ctx->dinfo;
+ union {
+ const unsigned char *raw;
+ xen_domctl_ext_vcpucontext_t *evc;
+ } ptr;
/* TODO: handle changing pfntab and vcpu counts */
/* PFN tab */
@@ -634,11 +640,42 @@ static int buffer_tail_pv(xc_interface *
ERROR("Error allocating VCPU ctxt tail buffer");
goto free_pfntab;
}
+ alloc = 1;
}
// DPRINTF("Reading VCPUS: %d bytes\n", vcpulen);
- if ( RDEXACT(fd, buf->vcpubuf, vcpulen) ) {
- PERROR("Error when reading ctxt");
- goto free_vcpus;
+ for ( total = i = 0, ptr.raw = buf->vcpubuf; ext_vcpucontext; ) {
+ if ( RDEXACT(fd, buf->vcpubuf + total, vcpulen) ) {
+ PERROR("Error when reading ctxt");
+ goto free_vcpus;
+ }
+ total += vcpulen;
+ for ( vcpulen = 0; i < buf->vcpucount; ++i ) {
+ size_t msrlen;
+
+ if ( (const unsigned char *)(ptr.evc + 1) > buf->vcpubuf + total )
+ break;
+ if ( ptr.evc->size <
+ (offsetof(xen_domctl_ext_vcpucontext_t, msrs) +
+ sizeof(ptr.evc->msrs)) )
+ ptr.evc->msr_count = 0;
+ msrlen = ptr.evc->msr_count * sizeof(xen_domctl_ext_vcpu_msr_t);
+ vcpulen += msrlen;
+ ptr.raw += 128 + msrlen + vcpuextstate_size;
+ }
+ if ( !vcpulen ) {
+ assert(i == buf->vcpucount);
+ break;
+ }
+ if ( alloc ) {
+ void *nbuf = realloc(buf->vcpubuf, total + vcpulen);
+
+ if ( !nbuf ) {
+ ERROR("Error growing VCPU ctxt tail buffer");
+ goto free_vcpus;
+ }
+ ptr.raw = nbuf + (ptr.raw - buf->vcpubuf);
+ buf->vcpubuf = nbuf;
+ }
}
/* load shared_info_page */
@@ -2130,9 +2167,28 @@ int xc_domain_restore(xc_interface *xch,
goto vcpu_ext_state_restore;
memcpy(&domctl.u.ext_vcpucontext, vcpup, 128);
vcpup += 128;
- domctl.cmd = XEN_DOMCTL_set_ext_vcpucontext;
- domctl.domain = dom;
- frc = xc_domctl(xch, &domctl);
+ if ( domctl.u.ext_vcpucontext.msr_count )
+ {
+ xen_domctl_ext_vcpu_msr_t *msrs = (void *)vcpup;
+ size_t sz = domctl.u.ext_vcpucontext.msr_count * sizeof(*msrs);
+ DECLARE_HYPERCALL_BOUNCE(msrs, sz, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+ if ( xc_hypercall_bounce_pre(xch, msrs) )
+ {
+ PERROR("Can't bounce in vcpu%d MSRs", i);
+ goto out;
+ }
+ set_xen_guest_handle(domctl.u.ext_vcpucontext.msrs, msrs);
+ domctl.cmd = XEN_DOMCTL_set_ext_vcpucontext;
+ domctl.domain = dom;
+ frc = xc_domctl(xch, &domctl);
+ xc_hypercall_bounce_post(xch, msrs);
+ vcpup += sz;
+ } else {
+ domctl.cmd = XEN_DOMCTL_set_ext_vcpucontext;
+ domctl.domain = dom;
+ frc = xc_domctl(xch, &domctl);
+ }
if ( frc != 0 )
{
PERROR("Couldn't set extended vcpu%d info", i);
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -836,6 +836,9 @@ int xc_domain_save(xc_interface *xch, in
/* base of the region in which domain memory is mapped */
unsigned char *region_base = NULL;
+ /* MSR extensions to xen_domctl_ext_vcpucontext_t */
+ DECLARE_HYPERCALL_BUFFER(xen_domctl_ext_vcpu_msr_t, msrs);
+
/* A copy of the CPU eXtended States of the guest. */
DECLARE_HYPERCALL_BUFFER(void, buffer);
@@ -1960,16 +1963,42 @@ int xc_domain_save(xc_interface *xch, in
domctl.domain = dom;
memset(&domctl.u, 0, sizeof(domctl.u));
domctl.u.ext_vcpucontext.vcpu = i;
- if ( xc_domctl(xch, &domctl) < 0 )
+ frc = xc_domctl(xch, &domctl);
+ if ( frc < 0 && errno == ENOBUFS &&
+ domctl.u.ext_vcpucontext.size >=
+ (offsetof(xen_domctl_ext_vcpucontext_t, msrs) +
+ sizeof(domctl.u.ext_vcpucontext.msrs)) &&
+ domctl.u.ext_vcpucontext.msr_count )
+ {
+ msrs = xc_hypercall_buffer_alloc(xch, msrs,
+ domctl.u.ext_vcpucontext.msr_count *
+ sizeof(*msrs));
+ set_xen_guest_handle(domctl.u.ext_vcpucontext.msrs, msrs);
+ frc = msrs ? xc_domctl(xch, &domctl) : -1;
+ /* Don't save the actual pointer. */
+ set_xen_guest_handle_raw(domctl.u.ext_vcpucontext.msrs, NULL);
+ }
+ if ( frc < 0 )
{
PERROR("No extended context for VCPU%d", i);
goto out;
}
if ( wrexact(io_fd, &domctl.u.ext_vcpucontext, 128) )
{
- PERROR("Error when writing to state file (2)");
+ PERROR("Error when writing to state file (ext ctxt)");
goto out;
}
+ if ( msrs )
+ {
+ if ( wrexact(io_fd, msrs,
+ domctl.u.ext_vcpucontext.msr_count * sizeof(*msrs)) )
+ {
+ PERROR("Error when writing to state file (MSRs)");
+ goto out;
+ }
+ xc_hypercall_buffer_free(xch, msrs);
+ msrs = NULL;
+ }
/* Start to fetch CPU eXtended States */
/* Get buffer size first */
@@ -2134,6 +2163,7 @@ int xc_domain_save(xc_interface *xch, in
xc_hypercall_buffer_free_pages(xch, to_send, NRPAGES(bitmap_size(dinfo->p2m_size)));
xc_hypercall_buffer_free_pages(xch, to_skip, NRPAGES(bitmap_size(dinfo->p2m_size)));
+ xc_hypercall_buffer_free(xch, msrs);
free(pfn_type);
free(pfn_batch);
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -62,6 +62,9 @@
* the block size.
* "extv" : Presence indicates use of extended VCPU context in
* tail, data size is 0.
+ * This block may be followed by an array of
+ * xen_domctl_ext_vcpu_msr_t as indicated by
+ * xen_domctl_ext_vcpucontext_t's msr_count field.
*
* p2m (PV-only):
*
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/2] SVM: support data breakpoint extension registers
2014-04-16 14:33 ` [PATCH v4 1/2] SVM: " Jan Beulich
@ 2014-04-16 14:47 ` Aravind Gopalakrishnan
2014-04-22 14:04 ` Boris Ostrovsky
2014-04-24 8:21 ` Ian Campbell
2 siblings, 0 replies; 21+ messages in thread
From: Aravind Gopalakrishnan @ 2014-04-16 14:47 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Ian Campbell, Keir Fraser, Ian Jackson, suravee.suthikulpanit,
Tim Deegan
On 4/16/2014 9:33 AM, Jan Beulich wrote:
> Leveraging the generic MSR save/restore logic introduced a little while
> ago.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Aravind Gopalakrishnan<aravind.gopalakrishnan@amd.com>
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
>
>
Reviewed-by: Aravind Gopalakrishnan<aravind.gopalakrishnan@amd.com>
Thanks,
-Aravind.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] x86/PV: support data breakpoint extension registers
2014-04-16 14:34 ` [PATCH v4 2/2] x86/PV: " Jan Beulich
@ 2014-04-16 14:48 ` Aravind Gopalakrishnan
2014-04-23 10:23 ` Ian Campbell
1 sibling, 0 replies; 21+ messages in thread
From: Aravind Gopalakrishnan @ 2014-04-16 14:48 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Ian Campbell, Keir Fraser, Ian Jackson, suravee.suthikulpanit,
Tim Deegan
On 4/16/2014 9:34 AM, Jan Beulich wrote:
> Introducing an extension to XEN_DOMCTL_[gs]et_ext_vcpucontext similar
> to the generic MSR save/restore logic recently added for HVM.
>
> This also moves some debug register related declarations/definition to
> the header intended for these.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Aravind Gopalakrishnan<aravind.gopalakrishnan@amd.com>
> ---
>
Reviewed-by: Aravind Gopalakrishnan<aravind.gopalakrishnan@amd.com>
Thanks,
-Aravind.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Ping: [PATCH v4 0/2] x86/AMD support data breakpoint extension registers
2014-04-16 14:23 [PATCH v4 0/2] x86/AMD support data breakpoint extension registers Jan Beulich
` (2 preceding siblings ...)
2014-04-16 14:35 ` [PATCH v4 3/2] libxc/PV: save/restore " Jan Beulich
@ 2014-04-22 12:37 ` Jan Beulich
3 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2014-04-22 12:37 UTC (permalink / raw)
To: suravee.suthikulpanit, Ian Campbell, Ian Jackson,
Boris Ostrovsky, Keir Fraser, Tim Deegan
Cc: xen-devel, Aravind Gopalakrishnan
>>> On 16.04.14 at 16:23, <JBeulich@suse.com> wrote:
> 1: SVM: support data breakpoint extension registers
This would minimally need an ack by an SVM maintainer; acks for the
(trivial) tools side as well as the other x86 changes would be nice too.
> 2: PV: support data breakpoint extension registers
This needs an ack from one of THE REST maintainers on the public
interface change; an ack for the x86 changes would be desirable.
Thanks, Jan
> The tools side patch isn't intended for committing at this point - it
> seems more reasonable to handle the migration aspect of this once
> Andrew's re-write is in place. Hence the patch is included here just
> for reference, and the series is titled to consist of only 2 patches.
>
> 3: libxc/PV: save/restore data breakpoint extension registers
>
> v4: Avoid DR7 write in activate_debugregs() when called from set_debugreg()
> (in patch 2; both other patches unchanged).
>
> Aravind - it wasn't clear from your most recent response whether the
> Reviewed-by applied to only the feature mask changes (previously in
> a single series with the changes here) or also to these ones, so I left
> it off. Please clarify.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/2] SVM: support data breakpoint extension registers
2014-04-22 14:04 ` Boris Ostrovsky
@ 2014-04-22 14:04 ` Jan Beulich
2014-04-22 14:17 ` Boris Ostrovsky
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2014-04-22 14:04 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Keir Fraser, Tim Deegan, Ian Jackson, Ian Campbell,
Aravind Gopalakrishnan, suravee.suthikulpanit, xen-devel
>>> On 22.04.14 at 16:04, <boris.ostrovsky@oracle.com> wrote:
> On 04/16/2014 10:33 AM, Jan Beulich wrote:
>> @@ -1526,6 +1628,21 @@ static int svm_msr_read_intercept(unsign
>> vpmu_do_rdmsr(msr, msr_content);
>> break;
>>
>> + case MSR_AMD64_DR0_ADDRESS_MASK:
>> + hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
>> + if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
>> + goto gpf;
>> + *msr_content = v->arch.hvm_svm.dr_mask[0];
>> + break;
>> +
>> + case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
>> + hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
>> + if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
>> + goto gpf;
>> + *msr_content =
>> + v->arch.hvm_svm.dr_mask[msr - MSR_AMD64_DR1_ADDRESS_MASK + 1];
>> + break;
>> +
>
> Can you merge these two cases (here and in write_intercept)? They are
> doing logically the same thing, the only difference is index calculation
> (and you can have a macro or something similar for that).
Do you really think this would be better - since the MSR numbers
aren't contiguous (and since one might want to consider adding
support for Fam15 supporting just the DR0 extension), I think they
would better be kept separate.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/2] SVM: support data breakpoint extension registers
2014-04-16 14:33 ` [PATCH v4 1/2] SVM: " Jan Beulich
2014-04-16 14:47 ` Aravind Gopalakrishnan
@ 2014-04-22 14:04 ` Boris Ostrovsky
2014-04-22 14:04 ` Jan Beulich
2014-04-24 8:21 ` Ian Campbell
2 siblings, 1 reply; 21+ messages in thread
From: Boris Ostrovsky @ 2014-04-22 14:04 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, Tim Deegan, Ian Jackson, Ian Campbell,
Aravind Gopalakrishnan, suravee.suthikulpanit, xen-devel
(Sorry for being late).
On 04/16/2014 10:33 AM, Jan Beulich wrote:
> @@ -1451,6 +1551,8 @@ static int svm_msr_read_intercept(unsign
>
> switch ( msr )
> {
> + unsigned int ecx;
> +
> case MSR_IA32_SYSENTER_CS:
> *msr_content = v->arch.hvm_svm.guest_sysenter_cs;
> break;
> @@ -1526,6 +1628,21 @@ static int svm_msr_read_intercept(unsign
> vpmu_do_rdmsr(msr, msr_content);
> break;
>
> + case MSR_AMD64_DR0_ADDRESS_MASK:
> + hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
> + if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
> + goto gpf;
> + *msr_content = v->arch.hvm_svm.dr_mask[0];
> + break;
> +
> + case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
> + hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
> + if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
> + goto gpf;
> + *msr_content =
> + v->arch.hvm_svm.dr_mask[msr - MSR_AMD64_DR1_ADDRESS_MASK + 1];
> + break;
> +
Can you merge these two cases (here and in write_intercept)? They are
doing logically the same thing, the only difference is index calculation
(and you can have a macro or something similar for that).
-boris
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/2] SVM: support data breakpoint extension registers
2014-04-22 14:04 ` Jan Beulich
@ 2014-04-22 14:17 ` Boris Ostrovsky
2014-04-22 14:32 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: Boris Ostrovsky @ 2014-04-22 14:17 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, Tim Deegan, Ian Jackson, Ian Campbell,
Aravind Gopalakrishnan, suravee.suthikulpanit, xen-devel
On 04/22/2014 10:04 AM, Jan Beulich wrote:
>>>> On 22.04.14 at 16:04, <boris.ostrovsky@oracle.com> wrote:
>> On 04/16/2014 10:33 AM, Jan Beulich wrote:
>>> @@ -1526,6 +1628,21 @@ static int svm_msr_read_intercept(unsign
>>> vpmu_do_rdmsr(msr, msr_content);
>>> break;
>>>
>>> + case MSR_AMD64_DR0_ADDRESS_MASK:
>>> + hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
>>> + if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
>>> + goto gpf;
>>> + *msr_content = v->arch.hvm_svm.dr_mask[0];
>>> + break;
>>> +
>>> + case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
>>> + hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
>>> + if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
>>> + goto gpf;
>>> + *msr_content =
>>> + v->arch.hvm_svm.dr_mask[msr - MSR_AMD64_DR1_ADDRESS_MASK + 1];
>>> + break;
>>> +
>> Can you merge these two cases (here and in write_intercept)? They are
>> doing logically the same thing, the only difference is index calculation
>> (and you can have a macro or something similar for that).
> Do you really think this would be better - since the MSR numbers
> aren't contiguous (and since one might want to consider adding
> support for Fam15 supporting just the DR0 extension), I think they
> would better be kept separate.
These MSR are tied to CPUID bit so if they are going to change then it
will require a new bit anyway.
But it's a nit really so
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
-boris
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/2] SVM: support data breakpoint extension registers
2014-04-22 14:17 ` Boris Ostrovsky
@ 2014-04-22 14:32 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2014-04-22 14:32 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Keir Fraser, Tim Deegan, Ian Jackson, Ian Campbell,
Aravind Gopalakrishnan, suravee.suthikulpanit, xen-devel
>>> On 22.04.14 at 16:17, <boris.ostrovsky@oracle.com> wrote:
> On 04/22/2014 10:04 AM, Jan Beulich wrote:
>>>>> On 22.04.14 at 16:04, <boris.ostrovsky@oracle.com> wrote:
>>> On 04/16/2014 10:33 AM, Jan Beulich wrote:
>>>> @@ -1526,6 +1628,21 @@ static int svm_msr_read_intercept(unsign
>>>> vpmu_do_rdmsr(msr, msr_content);
>>>> break;
>>>>
>>>> + case MSR_AMD64_DR0_ADDRESS_MASK:
>>>> + hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
>>>> + if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
>>>> + goto gpf;
>>>> + *msr_content = v->arch.hvm_svm.dr_mask[0];
>>>> + break;
>>>> +
>>>> + case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
>>>> + hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
>>>> + if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
>>>> + goto gpf;
>>>> + *msr_content =
>>>> + v->arch.hvm_svm.dr_mask[msr - MSR_AMD64_DR1_ADDRESS_MASK + 1];
>>>> + break;
>>>> +
>>> Can you merge these two cases (here and in write_intercept)? They are
>>> doing logically the same thing, the only difference is index calculation
>>> (and you can have a macro or something similar for that).
>> Do you really think this would be better - since the MSR numbers
>> aren't contiguous (and since one might want to consider adding
>> support for Fam15 supporting just the DR0 extension), I think they
>> would better be kept separate.
>
> These MSR are tied to CPUID bit so if they are going to change then it
> will require a new bit anyway.
Not according to my reading of the doc - Fam16 introduced the CPUID
bit and supports MSRs for all four DRs, whereas Fam15 doesn't have
a CPUID bit indicating just the MSR attached to DR0 being supported.
As that's therefore coupled to family, a guest may expect the MSR to
be available if seeing a Fam15 CPU.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] x86/PV: support data breakpoint extension registers
2014-04-16 14:34 ` [PATCH v4 2/2] x86/PV: " Jan Beulich
2014-04-16 14:48 ` Aravind Gopalakrishnan
@ 2014-04-23 10:23 ` Ian Campbell
2014-04-23 11:52 ` Jan Beulich
1 sibling, 1 reply; 21+ messages in thread
From: Ian Campbell @ 2014-04-23 10:23 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, Tim Deegan, Ian Jackson, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
On Wed, 2014-04-16 at 15:34 +0100, Jan Beulich wrote:
> @@ -583,6 +593,7 @@ struct xen_domctl_ext_vcpucontext {
> uint16_t sysenter_callback_cs;
> uint8_t syscall32_disables_events;
> uint8_t sysenter_disables_events;
> + uint16_t msr_count;
> #if defined(__GNUC__)
> union {
> uint64_aligned_t mcg_cap;
> @@ -591,6 +602,7 @@ struct xen_domctl_ext_vcpucontext {
> #else
> struct hvm_vmce_vcpu vmce;
> #endif
> + XEN_GUEST_HANDLE_64(xen_domctl_ext_vcpu_msr_t) msrs;
I must be missing something because I can't see where the tools are
initialising msrs, nor does the hypervisor appear to check it is valid
before trying to save stuff to it (although that would be caught by the
copy_to_user I expect).
Also how does one go about determining the correct msr_count before
retrieving this state?
(I have a feeling I asked this before, but I can't find any references.
Perhaps it was in the context of the similar HVM patch...)
Ian.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] x86/PV: support data breakpoint extension registers
2014-04-23 10:23 ` Ian Campbell
@ 2014-04-23 11:52 ` Jan Beulich
2014-04-23 12:23 ` Ian Campbell
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2014-04-23 11:52 UTC (permalink / raw)
To: Ian Campbell
Cc: Keir Fraser, Tim Deegan, IanJackson, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
>>> On 23.04.14 at 12:23, <Ian.Campbell@eu.citrix.com> wrote:
> On Wed, 2014-04-16 at 15:34 +0100, Jan Beulich wrote:
>> @@ -583,6 +593,7 @@ struct xen_domctl_ext_vcpucontext {
>> uint16_t sysenter_callback_cs;
>> uint8_t syscall32_disables_events;
>> uint8_t sysenter_disables_events;
>> + uint16_t msr_count;
>> #if defined(__GNUC__)
>> union {
>> uint64_aligned_t mcg_cap;
>> @@ -591,6 +602,7 @@ struct xen_domctl_ext_vcpucontext {
>> #else
>> struct hvm_vmce_vcpu vmce;
>> #endif
>> + XEN_GUEST_HANDLE_64(xen_domctl_ext_vcpu_msr_t) msrs;
>
> I must be missing something because I can't see where the tools are
> initialising msrs, nor does the hypervisor appear to check it is valid
> before trying to save stuff to it (although that would be caught by the
> copy_to_user I expect).
>
> Also how does one go about determining the correct msr_count before
> retrieving this state?
When msr_count is zero and MSRs are there that need storing, the
call will return -ENOBUFS and set msr_count to the required (minimum)
value. Furthermore the field is only being looked at if the size stored
inside the structure covers the entire msrs field. And yes, if
msr_count is non-zero but msrs doesn't point to a valid memory block,
copy_to_guest() will catch this (as usual).
So as is the tools are fine not explicitly setting msr_count (it's being
implicitly set to zero) - state save will fail in that case. It's the 3rd
(unfinished and now delayed until after Andrew's re-write) patch
that would be dealing with that error, re-issuing the call after
allocating a suitable array.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] x86/PV: support data breakpoint extension registers
2014-04-23 11:52 ` Jan Beulich
@ 2014-04-23 12:23 ` Ian Campbell
2014-04-23 12:35 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: Ian Campbell @ 2014-04-23 12:23 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, Tim Deegan, IanJackson, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
On Wed, 2014-04-23 at 12:52 +0100, Jan Beulich wrote:
> >>> On 23.04.14 at 12:23, <Ian.Campbell@eu.citrix.com> wrote:
> > On Wed, 2014-04-16 at 15:34 +0100, Jan Beulich wrote:
> >> @@ -583,6 +593,7 @@ struct xen_domctl_ext_vcpucontext {
> >> uint16_t sysenter_callback_cs;
> >> uint8_t syscall32_disables_events;
> >> uint8_t sysenter_disables_events;
> >> + uint16_t msr_count;
> >> #if defined(__GNUC__)
> >> union {
> >> uint64_aligned_t mcg_cap;
> >> @@ -591,6 +602,7 @@ struct xen_domctl_ext_vcpucontext {
> >> #else
> >> struct hvm_vmce_vcpu vmce;
> >> #endif
> >> + XEN_GUEST_HANDLE_64(xen_domctl_ext_vcpu_msr_t) msrs;
> >
> > I must be missing something because I can't see where the tools are
> > initialising msrs, nor does the hypervisor appear to check it is valid
> > before trying to save stuff to it (although that would be caught by the
> > copy_to_user I expect).
> >
> > Also how does one go about determining the correct msr_count before
> > retrieving this state?
>
> When msr_count is zero and MSRs are there that need storing, the
> call will return -ENOBUFS and set msr_count to the required (minimum)
> value. Furthermore the field is only being looked at if the size stored
> inside the structure covers the entire msrs field. And yes, if
> msr_count is non-zero but msrs doesn't point to a valid memory block,
> copy_to_guest() will catch this (as usual).
All makes sense. Worth a comment though?
> So as is the tools are fine not explicitly setting msr_count (it's being
> implicitly set to zero) - state save will fail in that case. It's the 3rd
> (unfinished and now delayed until after Andrew's re-write) patch
> that would be dealing with that error, re-issuing the call after
> allocating a suitable array.
That would be a bisection hazard wouldn't it? The tools side needs to be
part of this patch I think. In any case I suppose this series should
wait until the 3rd patch is available?
Ian.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] x86/PV: support data breakpoint extension registers
2014-04-23 12:23 ` Ian Campbell
@ 2014-04-23 12:35 ` Jan Beulich
2014-04-23 12:39 ` Ian Campbell
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2014-04-23 12:35 UTC (permalink / raw)
To: Ian Campbell
Cc: Keir Fraser, TimDeegan, IanJackson, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
>>> On 23.04.14 at 14:23, <Ian.Campbell@eu.citrix.com> wrote:
> On Wed, 2014-04-23 at 12:52 +0100, Jan Beulich wrote:
>> >>> On 23.04.14 at 12:23, <Ian.Campbell@eu.citrix.com> wrote:
>> > On Wed, 2014-04-16 at 15:34 +0100, Jan Beulich wrote:
>> >> @@ -583,6 +593,7 @@ struct xen_domctl_ext_vcpucontext {
>> >> uint16_t sysenter_callback_cs;
>> >> uint8_t syscall32_disables_events;
>> >> uint8_t sysenter_disables_events;
>> >> + uint16_t msr_count;
>> >> #if defined(__GNUC__)
>> >> union {
>> >> uint64_aligned_t mcg_cap;
>> >> @@ -591,6 +602,7 @@ struct xen_domctl_ext_vcpucontext {
>> >> #else
>> >> struct hvm_vmce_vcpu vmce;
>> >> #endif
>> >> + XEN_GUEST_HANDLE_64(xen_domctl_ext_vcpu_msr_t) msrs;
>> >
>> > I must be missing something because I can't see where the tools are
>> > initialising msrs, nor does the hypervisor appear to check it is valid
>> > before trying to save stuff to it (although that would be caught by the
>> > copy_to_user I expect).
>> >
>> > Also how does one go about determining the correct msr_count before
>> > retrieving this state?
>>
>> When msr_count is zero and MSRs are there that need storing, the
>> call will return -ENOBUFS and set msr_count to the required (minimum)
>> value. Furthermore the field is only being looked at if the size stored
>> inside the structure covers the entire msrs field. And yes, if
>> msr_count is non-zero but msrs doesn't point to a valid memory block,
>> copy_to_guest() will catch this (as usual).
>
> All makes sense. Worth a comment though?
Not sure - it's no more subtle than other code in the handling of that
specific sub-hypercall. But yes, by my own argumentation elsewhere
maybe I shouldn't be extending badness - if only I saw ways of
describing things like this without just converting C to human language
(which doesn't seem all that helpful)...
>> So as is the tools are fine not explicitly setting msr_count (it's being
>> implicitly set to zero) - state save will fail in that case. It's the 3rd
>> (unfinished and now delayed until after Andrew's re-write) patch
>> that would be dealing with that error, re-issuing the call after
>> allocating a suitable array.
>
> That would be a bisection hazard wouldn't it?
Only with guests using the extension and wanting to be migrated.
> The tools side needs to be
> part of this patch I think. In any case I suppose this series should
> wait until the 3rd patch is available?
That might mean I'd have to carry this for months. I'd rather want
to get this in as is, and live with save/migration not working for
guests using the extension. Even more so that I'm hoping to get
someone more fluent in libxc speak to take care of that.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] x86/PV: support data breakpoint extension registers
2014-04-23 12:35 ` Jan Beulich
@ 2014-04-23 12:39 ` Ian Campbell
2014-04-23 12:50 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: Ian Campbell @ 2014-04-23 12:39 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, TimDeegan, IanJackson, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
On Wed, 2014-04-23 at 13:35 +0100, Jan Beulich wrote:
> >>> On 23.04.14 at 14:23, <Ian.Campbell@eu.citrix.com> wrote:
> > On Wed, 2014-04-23 at 12:52 +0100, Jan Beulich wrote:
> >> >>> On 23.04.14 at 12:23, <Ian.Campbell@eu.citrix.com> wrote:
> >> > On Wed, 2014-04-16 at 15:34 +0100, Jan Beulich wrote:
> >> >> @@ -583,6 +593,7 @@ struct xen_domctl_ext_vcpucontext {
> >> >> uint16_t sysenter_callback_cs;
> >> >> uint8_t syscall32_disables_events;
> >> >> uint8_t sysenter_disables_events;
> >> >> + uint16_t msr_count;
> >> >> #if defined(__GNUC__)
> >> >> union {
> >> >> uint64_aligned_t mcg_cap;
> >> >> @@ -591,6 +602,7 @@ struct xen_domctl_ext_vcpucontext {
> >> >> #else
> >> >> struct hvm_vmce_vcpu vmce;
> >> >> #endif
> >> >> + XEN_GUEST_HANDLE_64(xen_domctl_ext_vcpu_msr_t) msrs;
> >> >
> >> > I must be missing something because I can't see where the tools are
> >> > initialising msrs, nor does the hypervisor appear to check it is valid
> >> > before trying to save stuff to it (although that would be caught by the
> >> > copy_to_user I expect).
> >> >
> >> > Also how does one go about determining the correct msr_count before
> >> > retrieving this state?
> >>
> >> When msr_count is zero and MSRs are there that need storing, the
> >> call will return -ENOBUFS and set msr_count to the required (minimum)
> >> value. Furthermore the field is only being looked at if the size stored
> >> inside the structure covers the entire msrs field. And yes, if
> >> msr_count is non-zero but msrs doesn't point to a valid memory block,
> >> copy_to_guest() will catch this (as usual).
> >
> > All makes sense. Worth a comment though?
>
> Not sure - it's no more subtle than other code in the handling of that
> specific sub-hypercall. But yes, by my own argumentation elsewhere
> maybe I shouldn't be extending badness - if only I saw ways of
> describing things like this without just converting C to human language
> (which doesn't seem all that helpful)...
Nothing the behaviour of msr_count you describe doesn't sound like just
converting the C to human readable to me, unless you expect readers of
this interface header to go digging into the implementation to figure
this out, which shouldn't be needed (that's the point of docs after all!
)
> >> So as is the tools are fine not explicitly setting msr_count (it's being
> >> implicitly set to zero) - state save will fail in that case. It's the 3rd
> >> (unfinished and now delayed until after Andrew's re-write) patch
> >> that would be dealing with that error, re-issuing the call after
> >> allocating a suitable array.
> >
> > That would be a bisection hazard wouldn't it?
>
> Only with guests using the extension and wanting to be migrated.
I hadn't realised there was this condition as well (since hte feature
looks more generic at the level I'm looking at it).
So any such guests would fail to migrate today anyway, right?
> > The tools side needs to be
> > part of this patch I think. In any case I suppose this series should
> > wait until the 3rd patch is available?
>
> That might mean I'd have to carry this for months. I'd rather want
> to get this in as is, and live with save/migration not working for
> guests using the extension. Even more so that I'm hoping to get
> someone more fluent in libxc speak to take care of that.
Yes, so long as it isn't a regression I think that is fine.
Ian.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] x86/PV: support data breakpoint extension registers
2014-04-23 12:39 ` Ian Campbell
@ 2014-04-23 12:50 ` Jan Beulich
2014-04-23 13:03 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2014-04-23 12:50 UTC (permalink / raw)
To: Ian Campbell
Cc: Keir Fraser, TimDeegan, IanJackson, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
>>> On 23.04.14 at 14:39, <Ian.Campbell@eu.citrix.com> wrote:
> On Wed, 2014-04-23 at 13:35 +0100, Jan Beulich wrote:
>> >>> On 23.04.14 at 14:23, <Ian.Campbell@eu.citrix.com> wrote:
>> > All makes sense. Worth a comment though?
>>
>> Not sure - it's no more subtle than other code in the handling of that
>> specific sub-hypercall. But yes, by my own argumentation elsewhere
>> maybe I shouldn't be extending badness - if only I saw ways of
>> describing things like this without just converting C to human language
>> (which doesn't seem all that helpful)...
>
> Nothing the behaviour of msr_count you describe doesn't sound like just
> converting the C to human readable to me, unless you expect readers of
> this interface header to go digging into the implementation to figure
> this out, which shouldn't be needed (that's the point of docs after all!)
Oh, right, I didn't realize your comment was in the context of the
public header changes - I blindly assumed them to be on the code
implementing the interface extension. Will see to put something like
the above in there.
>> >> So as is the tools are fine not explicitly setting msr_count (it's being
>> >> implicitly set to zero) - state save will fail in that case. It's the 3rd
>> >> (unfinished and now delayed until after Andrew's re-write) patch
>> >> that would be dealing with that error, re-issuing the call after
>> >> allocating a suitable array.
>> >
>> > That would be a bisection hazard wouldn't it?
>>
>> Only with guests using the extension and wanting to be migrated.
>
> I hadn't realised there was this condition as well (since hte feature
> looks more generic at the level I'm looking at it).
>
> So any such guests would fail to migrate today anyway, right?
I think such a guest wouldn't make it to migration, but would rather be
killed (by sending it a #GP it may not expect) for an invalid MSR write.
I.e. the improvement of this patch is to make it work at all, but fail on
attempts to be migrated.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] x86/PV: support data breakpoint extension registers
2014-04-23 12:50 ` Jan Beulich
@ 2014-04-23 13:03 ` Jan Beulich
2014-04-23 13:35 ` Ian Campbell
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2014-04-23 13:03 UTC (permalink / raw)
To: Ian Campbell
Cc: Keir Fraser, TimDeegan, IanJackson, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
>>> On 23.04.14 at 14:50, <JBeulich@suse.com> wrote:
>>>> On 23.04.14 at 14:39, <Ian.Campbell@eu.citrix.com> wrote:
>> On Wed, 2014-04-23 at 13:35 +0100, Jan Beulich wrote:
>>> >>> On 23.04.14 at 14:23, <Ian.Campbell@eu.citrix.com> wrote:
>>> > All makes sense. Worth a comment though?
>>>
>>> Not sure - it's no more subtle than other code in the handling of that
>>> specific sub-hypercall. But yes, by my own argumentation elsewhere
>>> maybe I shouldn't be extending badness - if only I saw ways of
>>> describing things like this without just converting C to human language
>>> (which doesn't seem all that helpful)...
>>
>> Nothing the behaviour of msr_count you describe doesn't sound like just
>> converting the C to human readable to me, unless you expect readers of
>> this interface header to go digging into the implementation to figure
>> this out, which shouldn't be needed (that's the point of docs after all!)
>
> Oh, right, I didn't realize your comment was in the context of the
> public header changes - I blindly assumed them to be on the code
> implementing the interface extension. Will see to put something like
> the above in there.
/*
* 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.
*/
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] x86/PV: support data breakpoint extension registers
2014-04-23 13:03 ` Jan Beulich
@ 2014-04-23 13:35 ` Ian Campbell
0 siblings, 0 replies; 21+ messages in thread
From: Ian Campbell @ 2014-04-23 13:35 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, TimDeegan, IanJackson, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
On Wed, 2014-04-23 at 14:03 +0100, Jan Beulich wrote:
> >>> On 23.04.14 at 14:50, <JBeulich@suse.com> wrote:
> >>>> On 23.04.14 at 14:39, <Ian.Campbell@eu.citrix.com> wrote:
> >> On Wed, 2014-04-23 at 13:35 +0100, Jan Beulich wrote:
> >>> >>> On 23.04.14 at 14:23, <Ian.Campbell@eu.citrix.com> wrote:
> >>> > All makes sense. Worth a comment though?
> >>>
> >>> Not sure - it's no more subtle than other code in the handling of that
> >>> specific sub-hypercall. But yes, by my own argumentation elsewhere
> >>> maybe I shouldn't be extending badness - if only I saw ways of
> >>> describing things like this without just converting C to human language
> >>> (which doesn't seem all that helpful)...
> >>
> >> Nothing the behaviour of msr_count you describe doesn't sound like just
> >> converting the C to human readable to me, unless you expect readers of
> >> this interface header to go digging into the implementation to figure
> >> this out, which shouldn't be needed (that's the point of docs after all!)
> >
> > Oh, right, I didn't realize your comment was in the context of the
> > public header changes - I blindly assumed them to be on the code
> > implementing the interface extension. Will see to put something like
> > the above in there.
>
> /*
> * 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.
> */
>From the ARM (complete lack of impact) and generic headers pov with this
comment in the appropriate place:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/2] SVM: support data breakpoint extension registers
2014-04-16 14:33 ` [PATCH v4 1/2] SVM: " Jan Beulich
2014-04-16 14:47 ` Aravind Gopalakrishnan
2014-04-22 14:04 ` Boris Ostrovsky
@ 2014-04-24 8:21 ` Ian Campbell
2014-04-24 8:30 ` Jan Beulich
2 siblings, 1 reply; 21+ messages in thread
From: Ian Campbell @ 2014-04-24 8:21 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, Tim Deegan, Ian Jackson, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
On Wed, 2014-04-16 at 15:33 +0100, Jan Beulich wrote:
> Leveraging the generic MSR save/restore logic introduced a little while
> ago.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Aravind Gopalakrishnan<aravind.gopalakrishnan@amd.com>
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
>
> --- a/tools/libxc/xc_cpufeature.h
> +++ b/tools/libxc/xc_cpufeature.h
> @@ -125,6 +125,7 @@
> #define X86_FEATURE_NODEID_MSR 19 /* NodeId MSR */
> #define X86_FEATURE_TBM 21 /* trailing bit manipulations */
> #define X86_FEATURE_TOPOEXT 22 /* topology extensions CPUID leafs */
> +#define X86_FEATURE_DBEXT 26 /* data breakpoint extension */
>
> /* Intel-defined CPU features, CPUID level 0x00000007:0 (ebx) */
> #define X86_FEATURE_FSGSBASE 0 /* {RD,WR}{FS,GS}BASE instructions */
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -110,9 +110,10 @@ static void amd_xc_cpuid_policy(
> bitmaskof(X86_FEATURE_3DNOWPREFETCH) |
> bitmaskof(X86_FEATURE_OSVW) |
> bitmaskof(X86_FEATURE_XOP) |
> + bitmaskof(X86_FEATURE_LWP) |
> bitmaskof(X86_FEATURE_FMA4) |
> bitmaskof(X86_FEATURE_TBM) |
> - bitmaskof(X86_FEATURE_LWP));
> + bitmaskof(X86_FEATURE_DBEXT));
> regs[3] &= (0x0183f3ff | /* features shared with 0x00000001:EDX */
> (is_pae ? bitmaskof(X86_FEATURE_NX) : 0) |
> (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) |
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3080,6 +3080,9 @@ void hvm_cpuid(unsigned int input, unsig
> /* Only provide PSE36 when guest runs in 32bit PAE or in long mode */
> if ( !(hvm_pae_enabled(v) || hvm_long_mode_enabled(v)) )
> *edx &= ~cpufeat_mask(X86_FEATURE_PSE36);
> + /* Hide data breakpoint extensions if the hardware has not support. */
s/not/no/ is more grammatical.
Other than that for the tools bit (and I realise I overshot with that
comment): Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/2] SVM: support data breakpoint extension registers
2014-04-24 8:21 ` Ian Campbell
@ 2014-04-24 8:30 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2014-04-24 8:30 UTC (permalink / raw)
To: Ian Campbell
Cc: Keir Fraser, Tim Deegan, IanJackson, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
>>> On 24.04.14 at 10:21, <Ian.Campbell@eu.citrix.com> wrote:
> On Wed, 2014-04-16 at 15:33 +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3080,6 +3080,9 @@ void hvm_cpuid(unsigned int input, unsig
>> /* Only provide PSE36 when guest runs in 32bit PAE or in long mode
> */
>> if ( !(hvm_pae_enabled(v) || hvm_long_mode_enabled(v)) )
>> *edx &= ~cpufeat_mask(X86_FEATURE_PSE36);
>> + /* Hide data breakpoint extensions if the hardware has not support.
> */
>
> s/not/no/ is more grammatical.
>
> Other than that for the tools bit (and I realise I overshot with that
> comment
In no way - I have no idea why I wrote "not" (fingers on autopilot?)...
>): Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks, Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2014-04-24 8:30 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-16 14:23 [PATCH v4 0/2] x86/AMD support data breakpoint extension registers Jan Beulich
2014-04-16 14:33 ` [PATCH v4 1/2] SVM: " Jan Beulich
2014-04-16 14:47 ` Aravind Gopalakrishnan
2014-04-22 14:04 ` Boris Ostrovsky
2014-04-22 14:04 ` Jan Beulich
2014-04-22 14:17 ` Boris Ostrovsky
2014-04-22 14:32 ` Jan Beulich
2014-04-24 8:21 ` Ian Campbell
2014-04-24 8:30 ` Jan Beulich
2014-04-16 14:34 ` [PATCH v4 2/2] x86/PV: " Jan Beulich
2014-04-16 14:48 ` Aravind Gopalakrishnan
2014-04-23 10:23 ` Ian Campbell
2014-04-23 11:52 ` Jan Beulich
2014-04-23 12:23 ` Ian Campbell
2014-04-23 12:35 ` Jan Beulich
2014-04-23 12:39 ` Ian Campbell
2014-04-23 12:50 ` Jan Beulich
2014-04-23 13:03 ` Jan Beulich
2014-04-23 13:35 ` Ian Campbell
2014-04-16 14:35 ` [PATCH v4 3/2] libxc/PV: save/restore " Jan Beulich
2014-04-22 12:37 ` Ping: [PATCH v4 0/2] x86/AMD support " Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.