Message ID | 1530710466-88309-1-git-send-email-jingqi.liu@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 04/07/2018 15:21, Jingqi Liu wrote: > The MSR (33H) controls support for #AC exception > for split locked accesses. When bit 29 of the MSR (33H) > is set, the processor causes an #AC exception to > be issued instead of suppressing LOCK on bus > (during split lock access). > > Signed-off-by: Jingqi Liu <jingqi.liu@intel.com> > --- > target/i386/cpu.h | 2 ++ > target/i386/kvm.c | 13 +++++++++++++ > target/i386/machine.c | 20 ++++++++++++++++++++ > 3 files changed, 35 insertions(+) > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 8eaefee..9728552 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -348,6 +348,7 @@ typedef enum X86Seg { > #define MSR_IA32_APICBASE_ENABLE (1<<11) > #define MSR_IA32_APICBASE_EXTD (1 << 10) > #define MSR_IA32_APICBASE_BASE (0xfffffU<<12) > +#define MSR_SPLIT_LOCK_CTRL 0x00000033 > #define MSR_IA32_FEATURE_CONTROL 0x0000003a > #define MSR_TSC_ADJUST 0x0000003b > #define MSR_IA32_SPEC_CTRL 0x48 > @@ -1209,6 +1210,7 @@ typedef struct CPUX86State { > uint32_t pkru; > > uint64_t spec_ctrl; > + uint64_t split_lock_ctrl; Please call everything MSR_TEST_CTL or test_ctl. Yes, it's a horrible name, but if that's what the manual calls it, we should do the same. Thanks, Paolo > uint64_t virt_ssbd; > > /* End of state preserved by INIT (dummy marker). */ > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index 032f0ad..043ca9b 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -92,6 +92,7 @@ static bool has_msr_hv_frequencies; > static bool has_msr_hv_reenlightenment; > static bool has_msr_xss; > static bool has_msr_spec_ctrl; > +static bool has_msr_split_lock_ctrl; > static bool has_msr_virt_ssbd; > static bool has_msr_smi_count; > > @@ -1272,6 +1273,9 @@ static int kvm_get_supported_msrs(KVMState *s) > case MSR_IA32_SPEC_CTRL: > has_msr_spec_ctrl = true; > break; > + case MSR_SPLIT_LOCK_CTRL: > + has_msr_split_lock_ctrl = true; > + break; > case MSR_VIRT_SSBD: > has_msr_virt_ssbd = true; > break; > @@ -1786,6 +1790,9 @@ static int kvm_put_msrs(X86CPU *cpu, int level) > if (has_msr_spec_ctrl) { > kvm_msr_entry_add(cpu, MSR_IA32_SPEC_CTRL, env->spec_ctrl); > } > + if (has_msr_split_lock_ctrl) { > + kvm_msr_entry_add(cpu, MSR_SPLIT_LOCK_CTRL, env->split_lock_ctrl); > + } > if (has_msr_virt_ssbd) { > kvm_msr_entry_add(cpu, MSR_VIRT_SSBD, env->virt_ssbd); > } > @@ -2169,6 +2176,9 @@ static int kvm_get_msrs(X86CPU *cpu) > if (has_msr_spec_ctrl) { > kvm_msr_entry_add(cpu, MSR_IA32_SPEC_CTRL, 0); > } > + if (has_msr_split_lock_ctrl) { > + kvm_msr_entry_add(cpu, MSR_SPLIT_LOCK_CTRL, 0); > + } > if (has_msr_virt_ssbd) { > kvm_msr_entry_add(cpu, MSR_VIRT_SSBD, 0); > } > @@ -2551,6 +2561,9 @@ static int kvm_get_msrs(X86CPU *cpu) > case MSR_IA32_SPEC_CTRL: > env->spec_ctrl = msrs[i].data; > break; > + case MSR_SPLIT_LOCK_CTRL: > + env->split_lock_ctrl = msrs[i].data; > + break; > case MSR_VIRT_SSBD: > env->virt_ssbd = msrs[i].data; > break; > diff --git a/target/i386/machine.c b/target/i386/machine.c > index 4d98d36..c82dc0d 100644 > --- a/target/i386/machine.c > +++ b/target/i386/machine.c > @@ -935,6 +935,25 @@ static const VMStateDescription vmstate_msr_virt_ssbd = { > } > }; > > +static bool split_lock_ctrl_needed(void *opaque) > +{ > + X86CPU *cpu = opaque; > + CPUX86State *env = &cpu->env; > + > + return env->split_lock_ctrl != 0; > +} > + > +static const VMStateDescription vmstate_split_lock_ctrl = { > + .name = "cpu/split_lock_ctrl", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = split_lock_ctrl_needed, > + .fields = (VMStateField[]){ > + VMSTATE_UINT64(env.split_lock_ctrl, X86CPU), > + VMSTATE_END_OF_LIST() > + } > +}; > + > VMStateDescription vmstate_x86_cpu = { > .name = "cpu", > .version_id = 12, > @@ -1059,6 +1078,7 @@ VMStateDescription vmstate_x86_cpu = { > &vmstate_mcg_ext_ctl, > &vmstate_msr_intel_pt, > &vmstate_msr_virt_ssbd, > + &vmstate_split_lock_ctrl, > NULL > } > }; >
On Wed, Jul 04, 2018 at 09:21:06PM +0800, Jingqi Liu wrote: > The MSR (33H) controls support for #AC exception > for split locked accesses. When bit 29 of the MSR (33H) > is set, the processor causes an #AC exception to > be issued instead of suppressing LOCK on bus > (during split lock access). > > Signed-off-by: Jingqi Liu <jingqi.liu@intel.com> [...] > diff --git a/target/i386/machine.c b/target/i386/machine.c > index 4d98d36..c82dc0d 100644 > --- a/target/i386/machine.c > +++ b/target/i386/machine.c > @@ -935,6 +935,25 @@ static const VMStateDescription vmstate_msr_virt_ssbd = { > } > }; > > +static bool split_lock_ctrl_needed(void *opaque) > +{ > + X86CPU *cpu = opaque; > + CPUX86State *env = &cpu->env; > + > + return env->split_lock_ctrl != 0; > +} Based on the Linux patch at [1], guests may try to detect the feature by writing to the MSR unconditionally. If this happens, KVM needs to provide a mechanism to enable/disable the MSR emulation. Otherwise users will end up with VMs that can't be migrated to older hosts even if they are using older machine-types. [1] https://lkml.org/lkml/2018/6/29/408
On 04/07/2018 21:37, Paolo Bonzini wrote: > On 04/07/2018 15:21, Jingqi Liu wrote: > > The MSR (33H) controls support for #AC exception for split locked > > accesses. When bit 29 of the MSR (33H) is set, the processor causes an > > #AC exception to be issued instead of suppressing LOCK on bus (during > > split lock access). > > > > Signed-off-by: Jingqi Liu <jingqi.liu@intel.com> > > --- > > target/i386/cpu.h | 2 ++ > > target/i386/kvm.c | 13 +++++++++++++ > > target/i386/machine.c | 20 ++++++++++++++++++++ > > 3 files changed, 35 insertions(+) > > > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h index > > 8eaefee..9728552 100644 > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -348,6 +348,7 @@ typedef enum X86Seg { > > #define MSR_IA32_APICBASE_ENABLE (1<<11) > > #define MSR_IA32_APICBASE_EXTD (1 << 10) > > #define MSR_IA32_APICBASE_BASE (0xfffffU<<12) > > +#define MSR_SPLIT_LOCK_CTRL 0x00000033 > > #define MSR_IA32_FEATURE_CONTROL 0x0000003a > > #define MSR_TSC_ADJUST 0x0000003b > > #define MSR_IA32_SPEC_CTRL 0x48 > > @@ -1209,6 +1210,7 @@ typedef struct CPUX86State { > > uint32_t pkru; > > > > uint64_t spec_ctrl; > > + uint64_t split_lock_ctrl; > > Please call everything MSR_TEST_CTL or test_ctl. Yes, it's a horrible name, but if > that's what the manual calls it, we should do the same. > Hi Paolo, Sure. I will unify the name in next version. Thanks, Jingqi > Thanks, > > Paolo >
On 05/07/2018 03:43, Eduardo Habkost wrote: > On Wed, Jul 04, 2018 at 09:21:06PM +0800, Jingqi Liu wrote: > > The MSR (33H) controls support for #AC exception for split locked > > accesses. When bit 29 of the MSR (33H) is set, the processor causes an > > #AC exception to be issued instead of suppressing LOCK on bus (during > > split lock access). > > > > Signed-off-by: Jingqi Liu <jingqi.liu@intel.com> > [...] > > diff --git a/target/i386/machine.c b/target/i386/machine.c index > > 4d98d36..c82dc0d 100644 > > --- a/target/i386/machine.c > > +++ b/target/i386/machine.c > > @@ -935,6 +935,25 @@ static const VMStateDescription > vmstate_msr_virt_ssbd = { > > } > > }; > > > > +static bool split_lock_ctrl_needed(void *opaque) { > > + X86CPU *cpu = opaque; > > + CPUX86State *env = &cpu->env; > > + > > + return env->split_lock_ctrl != 0; } > > Based on the Linux patch at [1], guests may try to detect the feature by writing > to the MSR unconditionally. > > If this happens, KVM needs to provide a mechanism to enable/disable the MSR > emulation. Otherwise users will end up with VMs that can't be migrated to older > hosts even if they are using older machine-types. > Hi Eduardo, Thanks for your review, will provide the mechanism in next version. Jingqi > [1] https://lkml.org/lkml/2018/6/29/408 > > -- > Eduardo
On Wed, Jul 04, 2018 at 04:43:27PM -0300, Eduardo Habkost wrote: > On Wed, Jul 04, 2018 at 09:21:06PM +0800, Jingqi Liu wrote: > > The MSR (33H) controls support for #AC exception > > for split locked accesses. When bit 29 of the MSR (33H) > > is set, the processor causes an #AC exception to > > be issued instead of suppressing LOCK on bus > > (during split lock access). > > > > Signed-off-by: Jingqi Liu <jingqi.liu@intel.com> > [...] > > diff --git a/target/i386/machine.c b/target/i386/machine.c > > index 4d98d36..c82dc0d 100644 > > --- a/target/i386/machine.c > > +++ b/target/i386/machine.c > > @@ -935,6 +935,25 @@ static const VMStateDescription vmstate_msr_virt_ssbd = { > > } > > }; > > > > +static bool split_lock_ctrl_needed(void *opaque) > > +{ > > + X86CPU *cpu = opaque; > > + CPUX86State *env = &cpu->env; > > + > > + return env->split_lock_ctrl != 0; > > +} > > Based on the Linux patch at [1], guests may try to detect the > feature by writing to the MSR unconditionally. > > If this happens, KVM needs to provide a mechanism to > enable/disable the MSR emulation. Otherwise users will end up > with VMs that can't be migrated to older hosts even if they are > using older machine-types. Is there really no CPUID flag that can be used to detect the feature ? Unconditionally probing for existance of arbitrary MSRs seems to be just re-inventing CPUID feature detection, but worse because as you say we need to now invent a way to control existance of individual MSRs too :-( Regards, Daniel
On 06/07/2018 10:43, Daniel P. Berrangé wrote: >> Based on the Linux patch at [1], guests may try to detect the >> feature by writing to the MSR unconditionally. >> >> If this happens, KVM needs to provide a mechanism to >> enable/disable the MSR emulation. Otherwise users will end up >> with VMs that can't be migrated to older hosts even if they are >> using older machine-types. > Is there really no CPUID flag that can be used to detect the feature ? > Unconditionally probing for existance of arbitrary MSRs seems to be > just re-inventing CPUID feature detection, but worse because as you > say we need to now invent a way to control existance of individual > MSRs too :-( Now that I know that no silicon exists for this feature, the solution is simple. The feature will be rejected until a CPUID bit exists. Paolo
On Fri, Jul 06, 2018 at 11:31:38AM +0200, Paolo Bonzini wrote: > On 06/07/2018 10:43, Daniel P. Berrangé wrote: > >> Based on the Linux patch at [1], guests may try to detect the > >> feature by writing to the MSR unconditionally. > >> > >> If this happens, KVM needs to provide a mechanism to > >> enable/disable the MSR emulation. Otherwise users will end up > >> with VMs that can't be migrated to older hosts even if they are > >> using older machine-types. > > Is there really no CPUID flag that can be used to detect the feature ? > > Unconditionally probing for existance of arbitrary MSRs seems to be > > just re-inventing CPUID feature detection, but worse because as you > > say we need to now invent a way to control existance of individual > > MSRs too :-( > > Now that I know that no silicon exists for this feature, the solution is > simple. The feature will be rejected until a CPUID bit exists. This sounds even better than requiring a new mechanism to enable/disable MSRs in KVM.
diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 8eaefee..9728552 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -348,6 +348,7 @@ typedef enum X86Seg { #define MSR_IA32_APICBASE_ENABLE (1<<11) #define MSR_IA32_APICBASE_EXTD (1 << 10) #define MSR_IA32_APICBASE_BASE (0xfffffU<<12) +#define MSR_SPLIT_LOCK_CTRL 0x00000033 #define MSR_IA32_FEATURE_CONTROL 0x0000003a #define MSR_TSC_ADJUST 0x0000003b #define MSR_IA32_SPEC_CTRL 0x48 @@ -1209,6 +1210,7 @@ typedef struct CPUX86State { uint32_t pkru; uint64_t spec_ctrl; + uint64_t split_lock_ctrl; uint64_t virt_ssbd; /* End of state preserved by INIT (dummy marker). */ diff --git a/target/i386/kvm.c b/target/i386/kvm.c index 032f0ad..043ca9b 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -92,6 +92,7 @@ static bool has_msr_hv_frequencies; static bool has_msr_hv_reenlightenment; static bool has_msr_xss; static bool has_msr_spec_ctrl; +static bool has_msr_split_lock_ctrl; static bool has_msr_virt_ssbd; static bool has_msr_smi_count; @@ -1272,6 +1273,9 @@ static int kvm_get_supported_msrs(KVMState *s) case MSR_IA32_SPEC_CTRL: has_msr_spec_ctrl = true; break; + case MSR_SPLIT_LOCK_CTRL: + has_msr_split_lock_ctrl = true; + break; case MSR_VIRT_SSBD: has_msr_virt_ssbd = true; break; @@ -1786,6 +1790,9 @@ static int kvm_put_msrs(X86CPU *cpu, int level) if (has_msr_spec_ctrl) { kvm_msr_entry_add(cpu, MSR_IA32_SPEC_CTRL, env->spec_ctrl); } + if (has_msr_split_lock_ctrl) { + kvm_msr_entry_add(cpu, MSR_SPLIT_LOCK_CTRL, env->split_lock_ctrl); + } if (has_msr_virt_ssbd) { kvm_msr_entry_add(cpu, MSR_VIRT_SSBD, env->virt_ssbd); } @@ -2169,6 +2176,9 @@ static int kvm_get_msrs(X86CPU *cpu) if (has_msr_spec_ctrl) { kvm_msr_entry_add(cpu, MSR_IA32_SPEC_CTRL, 0); } + if (has_msr_split_lock_ctrl) { + kvm_msr_entry_add(cpu, MSR_SPLIT_LOCK_CTRL, 0); + } if (has_msr_virt_ssbd) { kvm_msr_entry_add(cpu, MSR_VIRT_SSBD, 0); } @@ -2551,6 +2561,9 @@ static int kvm_get_msrs(X86CPU *cpu) case MSR_IA32_SPEC_CTRL: env->spec_ctrl = msrs[i].data; break; + case MSR_SPLIT_LOCK_CTRL: + env->split_lock_ctrl = msrs[i].data; + break; case MSR_VIRT_SSBD: env->virt_ssbd = msrs[i].data; break; diff --git a/target/i386/machine.c b/target/i386/machine.c index 4d98d36..c82dc0d 100644 --- a/target/i386/machine.c +++ b/target/i386/machine.c @@ -935,6 +935,25 @@ static const VMStateDescription vmstate_msr_virt_ssbd = { } }; +static bool split_lock_ctrl_needed(void *opaque) +{ + X86CPU *cpu = opaque; + CPUX86State *env = &cpu->env; + + return env->split_lock_ctrl != 0; +} + +static const VMStateDescription vmstate_split_lock_ctrl = { + .name = "cpu/split_lock_ctrl", + .version_id = 1, + .minimum_version_id = 1, + .needed = split_lock_ctrl_needed, + .fields = (VMStateField[]){ + VMSTATE_UINT64(env.split_lock_ctrl, X86CPU), + VMSTATE_END_OF_LIST() + } +}; + VMStateDescription vmstate_x86_cpu = { .name = "cpu", .version_id = 12, @@ -1059,6 +1078,7 @@ VMStateDescription vmstate_x86_cpu = { &vmstate_mcg_ext_ctl, &vmstate_msr_intel_pt, &vmstate_msr_virt_ssbd, + &vmstate_split_lock_ctrl, NULL } };
The MSR (33H) controls support for #AC exception for split locked accesses. When bit 29 of the MSR (33H) is set, the processor causes an #AC exception to be issued instead of suppressing LOCK on bus (during split lock access). Signed-off-by: Jingqi Liu <jingqi.liu@intel.com> --- target/i386/cpu.h | 2 ++ target/i386/kvm.c | 13 +++++++++++++ target/i386/machine.c | 20 ++++++++++++++++++++ 3 files changed, 35 insertions(+)