| Message ID | 20221020031615.890400-1-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Fix the initial value of mcg_cap | expand |
On Thu, Oct 20, 2022, Xiaoyao Li wrote: > vcpu->arch.mcg_cap represents the value of MSR_IA32_MCG_CAP. It's > set via ioctl(KVM_X86_SETUP_MCE) from userspace when exposing and > configuring MCE to guest. > > It's wrong to leave the default value as KVM_MAX_MCE_BANKS. Why? I agree it's an odd default, but the whole MCE API is odd. Functionally, I don't see anything that's broken by allowing the guest to access the MCx_CTL MSRs by default.
On 10/20/2022 10:27 PM, Sean Christopherson wrote: > On Thu, Oct 20, 2022, Xiaoyao Li wrote: >> vcpu->arch.mcg_cap represents the value of MSR_IA32_MCG_CAP. It's >> set via ioctl(KVM_X86_SETUP_MCE) from userspace when exposing and >> configuring MCE to guest. >> >> It's wrong to leave the default value as KVM_MAX_MCE_BANKS. > > Why? I agree it's an odd default, but the whole MCE API is odd. Functionally, > I don't see anything that's broken by allowing the guest to access the MCx_CTL MSRs > by default. Yes. Allowing the access doesn't cause any issue for a VM. However, for the perspective of virtualization. It virtualizes a magic hardware that even CPUID.MCA/MCE is not advertised and MCE is not set up by userspace, guest is told there are 32 banks and all the banks can be accessed. The patch doesn't fix any issue but try to make the code more reasonable.
On Thu, Oct 20, 2022, Xiaoyao Li wrote: > On 10/20/2022 10:27 PM, Sean Christopherson wrote: > > On Thu, Oct 20, 2022, Xiaoyao Li wrote: > > > vcpu->arch.mcg_cap represents the value of MSR_IA32_MCG_CAP. It's > > > set via ioctl(KVM_X86_SETUP_MCE) from userspace when exposing and > > > configuring MCE to guest. > > > > > > It's wrong to leave the default value as KVM_MAX_MCE_BANKS. > > > > Why? I agree it's an odd default, but the whole MCE API is odd. Functionally, > > I don't see anything that's broken by allowing the guest to access the MCx_CTL MSRs > > by default. > > Yes. Allowing the access doesn't cause any issue for a VM. > > However, for the perspective of virtualization. It virtualizes a magic > hardware that even CPUID.MCA/MCE is not advertised and MCE is not set up by > userspace, guest is told there are 32 banks and all the banks can be > accessed. '0' isn't necessarily better though, e.g. if userspace parrots back KVM's "supported" CPUID without invoking KVM_X86_SETUP_MCE, then it's equally odd that the guest will see no supported MCE MSRS. Older versions of the SDM also state (or at least very strongly imply) that banks 0-3 are always available on P6. Bank 0 is an especially weird case, as several of the MSRs are aliased to other MSRs that predate the machine check architecture. Anyways, if this were newly introduced code I'd be all for defaulting to '0', but KVM has defaulted to KVM_MAX_MCE_BANKS since KVM_X86_SETUP_MCE was added way back in 2009. Unless there's a bug that's fixed by this, I'm inclined to keep the current behavior even though it's weird, as hiding all MCE MSRs by default could theoretically cause a regression, e.g. by triggering #GP on MSRs that an older guest expects to always exist. If we really want to clean up this code, I think the correct approach would be to inject #GP on all relevant MSRs if CPUID.MCA==0, e.g. diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 4bd5f8a751de..97fafd851d8d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3260,6 +3260,9 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info) u64 data = msr_info->data; u32 offset, last_msr; + if (!msr_info->host_initiated && !guest_cpuid_has(X86_FEATURE_MCA)) + return 1; + switch (msr) { case MSR_IA32_MCG_STATUS: vcpu->arch.mcg_status = data; @@ -3891,6 +3894,14 @@ static int get_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host) unsigned bank_num = mcg_cap & 0xff; u32 offset, last_msr; + if (msr == MSR_IA32_P5_MC_ADDR || msr == MSR_IA32_P5_MC_TYPE) { + *pdata = 0; + return 0; + } + + if (!host && !guest_cpuid_has(X86_FEATURE_MCA)) + return 1; + switch (msr) { case MSR_IA32_P5_MC_ADDR: case MSR_IA32_P5_MC_TYPE: Or alternatively, this should work too: diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 4bd5f8a751de..e4a44d7af0a6 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3774,6 +3774,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) case MSR_IA32_MCG_STATUS: case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1: case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1: + if (!msr_info->host_initiated && + !guest_cpuid_has(X86_FEATURE_MCA)) + return 1; return set_msr_mce(vcpu, msr_info); case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3: @@ -4142,13 +4145,17 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) msr_info->data = vcpu->arch.msr_kvm_poll_control; break; - case MSR_IA32_P5_MC_ADDR: - case MSR_IA32_P5_MC_TYPE: case MSR_IA32_MCG_CAP: case MSR_IA32_MCG_CTL: case MSR_IA32_MCG_STATUS: case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1: case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1: + if (!msr_info->host_initiated && + !guest_cpuid_has(X86_FEATURE_MCA)) + return 1; + fallthrough; + case MSR_IA32_P5_MC_ADDR: + case MSR_IA32_P5_MC_TYPE: return get_msr_mce(vcpu, msr_info->index, &msr_info->data, msr_info->host_initiated); case MSR_IA32_XSS:
On 10/21/2022 12:32 AM, Sean Christopherson wrote: > On Thu, Oct 20, 2022, Xiaoyao Li wrote: >> On 10/20/2022 10:27 PM, Sean Christopherson wrote: >>> On Thu, Oct 20, 2022, Xiaoyao Li wrote: >>>> vcpu->arch.mcg_cap represents the value of MSR_IA32_MCG_CAP. It's >>>> set via ioctl(KVM_X86_SETUP_MCE) from userspace when exposing and >>>> configuring MCE to guest. >>>> >>>> It's wrong to leave the default value as KVM_MAX_MCE_BANKS. >>> >>> Why? I agree it's an odd default, but the whole MCE API is odd. Functionally, >>> I don't see anything that's broken by allowing the guest to access the MCx_CTL MSRs >>> by default. >> >> Yes. Allowing the access doesn't cause any issue for a VM. >> >> However, for the perspective of virtualization. It virtualizes a magic >> hardware that even CPUID.MCA/MCE is not advertised and MCE is not set up by >> userspace, guest is told there are 32 banks and all the banks can be >> accessed. > > '0' isn't necessarily better though, e.g. if userspace parrots back KVM's "supported" > CPUID without invoking KVM_X86_SETUP_MCE, then it's equally odd that the guest will > see no supported MCE MSRS. > > Older versions of the SDM also state (or at least very strongly imply) that banks > 0-3 are always available on P6. > > Bank 0 is an especially weird case, as several of the MSRs are aliased to other > MSRs that predate the machine check architecture. > > Anyways, if this were newly introduced code I'd be all for defaulting to '0', but > KVM has defaulted to KVM_MAX_MCE_BANKS since KVM_X86_SETUP_MCE was added way back > in 2009. Unless there's a bug that's fixed by this, I'm inclined to keep the > current behavior even though it's weird, as hiding all MCE MSRs by default could > theoretically cause a regression, e.g. by triggering #GP on MSRs that an older > guest expects to always exist. fair enough. > If we really want to clean up this code, I think the correct approach would be to > inject #GP on all relevant MSRs if CPUID.MCA==0, e.g. It's what I thought of as well. But I didn't find any statement in SDM of "Accessing Machine Check MSRs gets #GP if no CPUID.MCA" > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 4bd5f8a751de..97fafd851d8d 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3260,6 +3260,9 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > u64 data = msr_info->data; > u32 offset, last_msr; > > + if (!msr_info->host_initiated && !guest_cpuid_has(X86_FEATURE_MCA)) > + return 1; > + > switch (msr) { > case MSR_IA32_MCG_STATUS: > vcpu->arch.mcg_status = data; > @@ -3891,6 +3894,14 @@ static int get_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host) > unsigned bank_num = mcg_cap & 0xff; > u32 offset, last_msr; > > + if (msr == MSR_IA32_P5_MC_ADDR || msr == MSR_IA32_P5_MC_TYPE) { > + *pdata = 0; > + return 0; > + } > + > + if (!host && !guest_cpuid_has(X86_FEATURE_MCA)) > + return 1; > + > switch (msr) { > case MSR_IA32_P5_MC_ADDR: > case MSR_IA32_P5_MC_TYPE: > > Or alternatively, this should work too: > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 4bd5f8a751de..e4a44d7af0a6 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3774,6 +3774,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > case MSR_IA32_MCG_STATUS: > case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1: > case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1: > + if (!msr_info->host_initiated && > + !guest_cpuid_has(X86_FEATURE_MCA)) > + return 1; > return set_msr_mce(vcpu, msr_info); > > case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3: > @@ -4142,13 +4145,17 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > msr_info->data = vcpu->arch.msr_kvm_poll_control; > break; > - case MSR_IA32_P5_MC_ADDR: > - case MSR_IA32_P5_MC_TYPE: > case MSR_IA32_MCG_CAP: > case MSR_IA32_MCG_CTL: > case MSR_IA32_MCG_STATUS: > case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1: > case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1: > + if (!msr_info->host_initiated && > + !guest_cpuid_has(X86_FEATURE_MCA)) > + return 1; > + fallthrough; > + case MSR_IA32_P5_MC_ADDR: > + case MSR_IA32_P5_MC_TYPE: > return get_msr_mce(vcpu, msr_info->index, &msr_info->data, > msr_info->host_initiated); > case MSR_IA32_XSS: >
On Fri, Oct 21, 2022, Xiaoyao Li wrote: > On 10/21/2022 12:32 AM, Sean Christopherson wrote: > > If we really want to clean up this code, I think the correct approach would be to > > inject #GP on all relevant MSRs if CPUID.MCA==0, e.g. > > It's what I thought of as well. But I didn't find any statement in SDM of > "Accessing Machine Check MSRs gets #GP if no CPUID.MCA" Ugh, stupid SDM. Really old SDMs, e.g. circa 1997, explicity state in the CPUID.MCA entry that: Processor supports the MCG_CAP MSR. But, when Intel introduced the "Architectural MSRs" section (2001 or so), the wording was changed to be less explicit: The Machine Check Architecture, which provides a compatible mechanism for error reporting in P6 family, Pentium 4, and Intel Xeon processors, and future processors, is supported. The MCG_CAP MSR contains feature bits describing how many banks of error reporting MSRs are supported. and the entry in the MSR index just lists P6 as the dependency: IA32_MCG_CAP (MCG_CAP) Global Machine Check Capability (R/O) 06_01H So I think it's technically true that MCG_CAP is supposed to exist iff CPUID.MCA=1, but we'd probably need an SDM change to really be able to enforce that :-(
On 10/22/2022 2:35 AM, Sean Christopherson wrote: > On Fri, Oct 21, 2022, Xiaoyao Li wrote: >> On 10/21/2022 12:32 AM, Sean Christopherson wrote: >>> If we really want to clean up this code, I think the correct approach would be to >>> inject #GP on all relevant MSRs if CPUID.MCA==0, e.g. >> >> It's what I thought of as well. But I didn't find any statement in SDM of >> "Accessing Machine Check MSRs gets #GP if no CPUID.MCA" > > Ugh, stupid SDM. Really old SDMs, e.g. circa 1997, explicity state in the > CPUID.MCA entry that: > > Processor supports the MCG_CAP MSR. > > But, when Intel introduced the "Architectural MSRs" section (2001 or so), the > wording was changed to be less explicit: > > The Machine Check Architecture, which provides a compatible mechanism for error > reporting in P6 family, Pentium 4, and Intel Xeon processors, and future processors, > is supported. The MCG_CAP MSR contains feature bits describing how many banks of > error reporting MSRs are supported. > > and the entry in the MSR index just lists P6 as the dependency: > > IA32_MCG_CAP (MCG_CAP) Global Machine Check Capability (R/O) 06_01H > > So I think it's technically true that MCG_CAP is supposed to exist iff CPUID.MCA=1, > but we'd probably need an SDM change to really be able to enforce that :-( I'll talk to Intel architects for this. :)
On Mon, Oct 24, 2022 at 09:37:59AM +0800, Xiaoyao Li wrote: > On 10/22/2022 2:35 AM, Sean Christopherson wrote: > > On Fri, Oct 21, 2022, Xiaoyao Li wrote: > > > On 10/21/2022 12:32 AM, Sean Christopherson wrote: > > > > If we really want to clean up this code, I think the correct approach would be to > > > > inject #GP on all relevant MSRs if CPUID.MCA==0, e.g. > > > > > > It's what I thought of as well. But I didn't find any statement in SDM of > > > "Accessing Machine Check MSRs gets #GP if no CPUID.MCA" > > > > Ugh, stupid SDM. Really old SDMs, e.g. circa 1997, explicity state in the > > CPUID.MCA entry that: > > > > Processor supports the MCG_CAP MSR. > > > > But, when Intel introduced the "Architectural MSRs" section (2001 or so), the > > wording was changed to be less explicit: > > > > The Machine Check Architecture, which provides a compatible mechanism for error > > reporting in P6 family, Pentium 4, and Intel Xeon processors, and future processors, > > is supported. The MCG_CAP MSR contains feature bits describing how many banks of > > error reporting MSRs are supported. > > > > and the entry in the MSR index just lists P6 as the dependency: > > > > IA32_MCG_CAP (MCG_CAP) Global Machine Check Capability (R/O) 06_01H > > > > So I think it's technically true that MCG_CAP is supposed to exist iff CPUID.MCA=1, > > but we'd probably need an SDM change to really be able to enforce that :-( > > I'll talk to Intel architects for this. :) [I'm not a h/w architect ... but I do write/support the Linux machine check code] Current edition of the SDM describes the MCA bit in CPUID(EAX=1).EDX in volume 2, Table 3-11: Machine Check Architecture. A value of 1 indicates the Machine Check Architecture of reporting machine errors is supported. The MCG_CAP MSR contains feature bits describing how many banks of error reporting MSRs are supported So a value of 0 would mean Machine check architecture is NOT supported. The only rationale meaning for "Machine check architecture is supported" is you get everything in Vol3B chapter 15 if MCA is supported, and you don't get it if it isn't. The unsupported behaviour is not explicitly defined ... so if you want the do something other than #GP, you could do so ... but that sounds like s silly choice. Ditto for accessing a machine check bank with number greater than that specified in IA32_MCG_CAP.count. SDM doesn't say that this must #GP, but #GP would be a sane and reasonble response. You could also read as all zero and drop writes. -Tony
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 4bd5f8a751de..ca8f4a3e698d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -11801,7 +11801,6 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) GFP_KERNEL_ACCOUNT); if (!vcpu->arch.mce_banks || !vcpu->arch.mci_ctl2_banks) goto fail_free_mce_banks; - vcpu->arch.mcg_cap = KVM_MAX_MCE_BANKS; if (!zalloc_cpumask_var(&vcpu->arch.wbinvd_dirty_mask, GFP_KERNEL_ACCOUNT))
vcpu->arch.mcg_cap represents the value of MSR_IA32_MCG_CAP. It's set via ioctl(KVM_X86_SETUP_MCE) from userspace when exposing and configuring MCE to guest. It's wrong to leave the default value as KVM_MAX_MCE_BANKS. Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- arch/x86/kvm/x86.c | 1 - 1 file changed, 1 deletion(-)