| Message ID | 20190325171649.7311-2-bp@alien8.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/kvm: Enable MCE injection in the guest | expand |
On Mon, Mar 25, 2019 at 10:17 AM Borislav Petkov <bp@alien8.de> wrote: > > From: Borislav Petkov <bp@suse.de> > > This is an AMD-specific MSR. Put it where it belongs. > > Signed-off-by: Borislav Petkov <bp@suse.de> > Tested-by: Yazen Ghannam <yazen.ghannam@amd.com> > --- > arch/x86/kvm/svm.c | 14 ++++++++++++++ > arch/x86/kvm/x86.c | 12 ------------ > 2 files changed, 14 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index b5b128a0a051..00eb44a2a377 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -4201,6 +4201,9 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > case MSR_F10H_DECFG: > msr_info->data = svm->msr_decfg; > break; > + case MSR_K7_HWCR: > + msr_info->data = 0; > + break; If the guest has written a non-zero value to this MSR, shouldn't it be able to read that value back?
On Mon, Mar 25, 2019 at 11:05:48AM -0700, Jim Mattson wrote: > If the guest has written a non-zero value to this MSR, shouldn't it be > able to read that value back? See the second patch.
On Mon, Mar 25, 2019 at 06:16:48PM +0100, Borislav Petkov wrote: > From: Borislav Petkov <bp@suse.de> > > This is an AMD-specific MSR. Put it where it belongs. > > Signed-off-by: Borislav Petkov <bp@suse.de> > Tested-by: Yazen Ghannam <yazen.ghannam@amd.com> > --- > arch/x86/kvm/svm.c | 14 ++++++++++++++ > arch/x86/kvm/x86.c | 12 ------------ > 2 files changed, 14 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index b5b128a0a051..00eb44a2a377 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -4201,6 +4201,9 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > case MSR_F10H_DECFG: > msr_info->data = svm->msr_decfg; > break; > + case MSR_K7_HWCR: > + msr_info->data = 0; > + break; > default: > return kvm_get_msr_common(vcpu, msr_info); > } > @@ -4405,6 +4408,17 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) > svm->msr_decfg = data; > break; > } > + case MSR_K7_HWCR: > + data &= ~(u64)0x40; /* ignore flush filter disable */ > + data &= ~(u64)0x100; /* ignore ignne emulation enable */ > + data &= ~(u64)0x8; /* ignore TLB cache disable */ > + data &= ~(u64)0x40000; /* ignore Mc status write enable */ > + if (data != 0) { > + vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n", > + data); > + return 1; > + } > + break; > case MSR_IA32_APICBASE: > if (kvm_vcpu_apicv_active(vcpu)) > avic_update_vapic_bar(to_svm(vcpu), data); > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 65e4559eef2f..e53d13cfceba 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -2445,17 +2445,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > break; > case MSR_EFER: > return set_efer(vcpu, data); > - case MSR_K7_HWCR: > - data &= ~(u64)0x40; /* ignore flush filter disable */ > - data &= ~(u64)0x100; /* ignore ignne emulation enable */ > - data &= ~(u64)0x8; /* ignore TLB cache disable */ > - data &= ~(u64)0x40000; /* ignore Mc status write enable */ > - if (data != 0) { > - vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n", > - data); > - return 1; > - } > - break; > case MSR_FAM10H_MMIO_CONF_BASE: > if (data != 0) { > vcpu_unimpl(vcpu, "unimplemented MMIO_CONF_BASE wrmsr: " > @@ -2724,7 +2713,6 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > case MSR_K8_SYSCFG: > case MSR_K8_TSEG_ADDR: > case MSR_K8_TSEG_MASK: > - case MSR_K7_HWCR: Won't this prevent emulating an AMD guest on Intel hardware, e.g. due to injecting #GPs during boot? Keeping support in kvm_{get,set}_msr_common doesn't preclude svm_{get,set}_msr() from having SVM-specific handling for the MSR. > case MSR_VM_HSAVE_PA: > case MSR_K8_INT_PENDING_MSG: > case MSR_AMD64_NB_CFG: > -- > 2.21.0 >
On Mon, Mar 25, 2019 at 11:21:33AM -0700, Sean Christopherson wrote: > Won't this prevent emulating an AMD guest on Intel hardware, e.g. due to > injecting #GPs during boot? I guess, but... > Keeping support in kvm_{get,set}_msr_common > doesn't preclude svm_{get,set}_msr() from having SVM-specific handling for > the MSR. ... is kvm_{get,set}_msr_common() supposed to cover for all those overlapping MSRs between AMD and Intel? svm_{get,set}_msr() have a lot more AMD-specific MSRs just like vmx_{get,set}_msr() respectively for Intel. Which would mean that if you really want to support those cross-vendor emulations, you don't need the svm* and vmx* MSR accessors... or am I missing something?
On Mon, Mar 25, 2019 at 07:39:09PM +0100, Borislav Petkov wrote: > On Mon, Mar 25, 2019 at 11:21:33AM -0700, Sean Christopherson wrote: > > Won't this prevent emulating an AMD guest on Intel hardware, e.g. due to > > injecting #GPs during boot? > > I guess, but... > > > Keeping support in kvm_{get,set}_msr_common > > doesn't preclude svm_{get,set}_msr() from having SVM-specific handling for > > the MSR. > > ... is kvm_{get,set}_msr_common() supposed to cover for all those > overlapping MSRs between AMD and Intel? svm_{get,set}_msr() have a lot > more AMD-specific MSRs just like vmx_{get,set}_msr() respectively for > Intel. > > Which would mean that if you really want to support those cross-vendor > emulations, you don't need the svm* and vmx* MSR accessors... or am I > missing something? Generally speaking, the goal is to support cross-vendor VMs without having to modify the guest kernel, i.e. exact emulation is out of scope. This means "emulating" cross-vendor MSRs that the guest expects to exist to the point where the guest won't explode, e.g. in the case of MSR_K7_HWCR, Linux expects the MSR to exist on all AMD platforms and AFAICT will die during boot if it doesn't. The rule of thumb for "what MSRs can a guest reasonably expect to exist" is fluid. The most clear cut cases are when support is explicitly enumerated via some feature bit and KVM reports support for said feature to userspace, e.g. if userspace advertises a feature (to the guest) that KVM doesn't support, then it's a userspace bug. But for MSRs like MSR_K7_HWCR and MSR_F10H_DECFG where their existence is implicit, whoever came first often wins. For example, MSR_K7_HWCR existed long before KVM and guest kernels expect it to exist on all AMD CPUs, so KVM emulates it unconditionally. Whereas MSR_F10H_DECFG was recently added and obviously not emulated on existing hypervisors, so the kernel has to assume the MSR might not exist when running under a hypervisor, which means KVM doesn't need to pretend the MSR uncondtionally exists.
On Mon, Mar 25, 2019 at 12:21:11PM -0700, Sean Christopherson wrote: > Generally speaking, the goal is to support cross-vendor VMs without having > to modify the guest kernel, i.e. exact emulation is out of scope. This > means "emulating" cross-vendor MSRs that the guest expects to exist to the > point where the guest won't explode, e.g. in the case of MSR_K7_HWCR, Linux > expects the MSR to exist on all AMD platforms and AFAICT will die during > boot if it doesn't. > > The rule of thumb for "what MSRs can a guest reasonably expect to exist" > is fluid. Ok, I'll keep it in the common MSR accessors in the next version. Thx for confirming what I was suspecting.
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index b5b128a0a051..00eb44a2a377 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -4201,6 +4201,9 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) case MSR_F10H_DECFG: msr_info->data = svm->msr_decfg; break; + case MSR_K7_HWCR: + msr_info->data = 0; + break; default: return kvm_get_msr_common(vcpu, msr_info); } @@ -4405,6 +4408,17 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) svm->msr_decfg = data; break; } + case MSR_K7_HWCR: + data &= ~(u64)0x40; /* ignore flush filter disable */ + data &= ~(u64)0x100; /* ignore ignne emulation enable */ + data &= ~(u64)0x8; /* ignore TLB cache disable */ + data &= ~(u64)0x40000; /* ignore Mc status write enable */ + if (data != 0) { + vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n", + data); + return 1; + } + break; case MSR_IA32_APICBASE: if (kvm_vcpu_apicv_active(vcpu)) avic_update_vapic_bar(to_svm(vcpu), data); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 65e4559eef2f..e53d13cfceba 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -2445,17 +2445,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) break; case MSR_EFER: return set_efer(vcpu, data); - case MSR_K7_HWCR: - data &= ~(u64)0x40; /* ignore flush filter disable */ - data &= ~(u64)0x100; /* ignore ignne emulation enable */ - data &= ~(u64)0x8; /* ignore TLB cache disable */ - data &= ~(u64)0x40000; /* ignore Mc status write enable */ - if (data != 0) { - vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n", - data); - return 1; - } - break; case MSR_FAM10H_MMIO_CONF_BASE: if (data != 0) { vcpu_unimpl(vcpu, "unimplemented MMIO_CONF_BASE wrmsr: " @@ -2724,7 +2713,6 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) case MSR_K8_SYSCFG: case MSR_K8_TSEG_ADDR: case MSR_K8_TSEG_MASK: - case MSR_K7_HWCR: case MSR_VM_HSAVE_PA: case MSR_K8_INT_PENDING_MSG: case MSR_AMD64_NB_CFG: