| Message ID | 1519433546-33879-1-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Feb 24, 2018 at 01:52:26AM +0100, Paolo Bonzini wrote: > Use the new MSR feature framework to expose the ARCH_CAPABILITIES MSR to > userspace. This way, userspace can access the capabilities even if it > does not have the permissions to read MSRs. ... That is good but could you expand a bit of why it would want this? I am 99% sure it is due to the lovely spectre_v2 mitigation but could you include that in the commit message so that in say a year folks would know what this is? Also what branch is this based on? I am not seeing this vmx_get_msr_feature in kvm/master or kvm/linux-next ? > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/kvm/vmx.c | 12 +++++++++++- > arch/x86/kvm/x86.c | 1 + > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 130fca0ea1bf..99689061e11e 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -3228,7 +3228,17 @@ static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu, > > static int vmx_get_msr_feature(struct kvm_msr_entry *msr) > { > - return 1; > + switch (msr->index) { > + case MSR_IA32_ARCH_CAPABILITIES: > + if (!boot_cpu_has(X86_FEATURE_ARCH_CAPABILITIES)) > + return 1; > + rdmsrl(msr->index, msr->data); > + break; > + default: > + return 1; > + } > + > + return 0; > } > > /* > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 54b4ed55945b..e9a8cc9e3b2b 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1054,6 +1054,7 @@ bool kvm_rdpmc(struct kvm_vcpu *vcpu) > * can be used by a hypervisor to validate requested CPU features. > */ > static u32 msr_based_features[] = { > + MSR_IA32_ARCH_CAPABILITIES, > MSR_F10H_DECFG, > }; > > -- > 1.8.3.1 >
On Mon, Feb 26, 2018 at 05:13:00PM -0500, Konrad Rzeszutek Wilk wrote: > On Sat, Feb 24, 2018 at 01:52:26AM +0100, Paolo Bonzini wrote: > > Use the new MSR feature framework to expose the ARCH_CAPABILITIES MSR to > > userspace. This way, userspace can access the capabilities even if it > > does not have the permissions to read MSRs. > > ... That is good but could you expand a bit of why it would want this? > > I am 99% sure it is due to the lovely spectre_v2 mitigation but > could you include that in the commit message so that in say a year > folks would know what this is? > > Also what branch is this based on? I am not seeing this vmx_get_msr_feature > in kvm/master or kvm/linux-next ? Ah I see you posted them! Sorry for that particular noise. > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > arch/x86/kvm/vmx.c | 12 +++++++++++- > > arch/x86/kvm/x86.c | 1 + > > 2 files changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > > index 130fca0ea1bf..99689061e11e 100644 > > --- a/arch/x86/kvm/vmx.c > > +++ b/arch/x86/kvm/vmx.c > > @@ -3228,7 +3228,17 @@ static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu, > > > > static int vmx_get_msr_feature(struct kvm_msr_entry *msr) > > { > > - return 1; > > + switch (msr->index) { > > + case MSR_IA32_ARCH_CAPABILITIES: > > + if (!boot_cpu_has(X86_FEATURE_ARCH_CAPABILITIES)) > > + return 1; > > + rdmsrl(msr->index, msr->data); > > + break; > > + default: > > + return 1; > > + } > > + > > + return 0; > > } > > > > /* > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 54b4ed55945b..e9a8cc9e3b2b 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -1054,6 +1054,7 @@ bool kvm_rdpmc(struct kvm_vcpu *vcpu) > > * can be used by a hypervisor to validate requested CPU features. > > */ > > static u32 msr_based_features[] = { > > + MSR_IA32_ARCH_CAPABILITIES, > > MSR_F10H_DECFG, > > }; > > > > -- > > 1.8.3.1 > >
[Resent after removing g@char.us.oracle.com.] 2018-02-26 17:13-0500, Konrad Rzeszutek Wilk: > On Sat, Feb 24, 2018 at 01:52:26AM +0100, Paolo Bonzini wrote: > > Use the new MSR feature framework to expose the ARCH_CAPABILITIES MSR to > > userspace. This way, userspace can access the capabilities even if it > > does not have the permissions to read MSRs. > > ... That is good but could you expand a bit of why it would want this? > > I am 99% sure it is due to the lovely spectre_v2 mitigation but > could you include that in the commit message so that in say a year > folks would know what this is? Userspace can currently get the MSR by creating a VCPU and reading its MSR_IA32_ARCH_CAPABILITIES, because it is set from the hardware MSR. I thought that "permissions to read MSRs" talked about hardware MSRs, so the purpose of this patch would be a better interface, but I don't see how if we keep the auto-setting on VCPU creation. Is this aimed towards userspaces that want nothing else from KVM than the MSR value? Thanks.
On 01/03/2018 22:39, Radim Krčmář wrote: > [Resent after removing g@char.us.oracle.com.] > > 2018-02-26 17:13-0500, Konrad Rzeszutek Wilk: >> On Sat, Feb 24, 2018 at 01:52:26AM +0100, Paolo Bonzini wrote: >>> Use the new MSR feature framework to expose the ARCH_CAPABILITIES MSR to >>> userspace. This way, userspace can access the capabilities even if it >>> does not have the permissions to read MSRs. >> >> ... That is good but could you expand a bit of why it would want this? >> >> I am 99% sure it is due to the lovely spectre_v2 mitigation but >> could you include that in the commit message so that in say a year >> folks would know what this is? > > Userspace can currently get the MSR by creating a VCPU and reading its > MSR_IA32_ARCH_CAPABILITIES, because it is set from the hardware MSR. > > I thought that "permissions to read MSRs" talked about hardware MSRs, so > the purpose of this patch would be a better interface, but I don't see > how if we keep the auto-setting on VCPU creation. Yeah, it's mostly about a better interface and being able to do checks before creating the VCPU. The commit message was written before I noticed the auto-setting on VCPU creation, and I failed to update it. Thanks, Paolo > Is this aimed towards userspaces that want nothing else from KVM than > the MSR value? > > Thanks. >
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 130fca0ea1bf..99689061e11e 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -3228,7 +3228,17 @@ static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu, static int vmx_get_msr_feature(struct kvm_msr_entry *msr) { - return 1; + switch (msr->index) { + case MSR_IA32_ARCH_CAPABILITIES: + if (!boot_cpu_has(X86_FEATURE_ARCH_CAPABILITIES)) + return 1; + rdmsrl(msr->index, msr->data); + break; + default: + return 1; + } + + return 0; } /* diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 54b4ed55945b..e9a8cc9e3b2b 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1054,6 +1054,7 @@ bool kvm_rdpmc(struct kvm_vcpu *vcpu) * can be used by a hypervisor to validate requested CPU features. */ static u32 msr_based_features[] = { + MSR_IA32_ARCH_CAPABILITIES, MSR_F10H_DECFG, };
Use the new MSR feature framework to expose the ARCH_CAPABILITIES MSR to userspace. This way, userspace can access the capabilities even if it does not have the permissions to read MSRs. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/vmx.c | 12 +++++++++++- arch/x86/kvm/x86.c | 1 + 2 files changed, 12 insertions(+), 1 deletion(-)