Thanks a lot, Sean.
On Wed, May 11, 2022 at 12:00 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Tue, Apr 12, 2022, Jue Wang wrote:
> > Note the support of CMCI (MCG_CMCI_P) is not enabled in
> > kvm_mce_cap_supported yet.
>
> You can probably guess what I would say here :-) A reader should not have to go
> wade through Intel's SDM to understand the relationship between CTL2 and CMCI.
Included details in the change log for V3 about CTL2 and CMCI_P
register / bits. Also rewrote the change log so they contain full text
form of UCNA and CMCI, and a complete context of this patch series.
>
> > Signed-off-by: Jue Wang <juew@xxxxxxxxxx>
> > ---
> > arch/x86/include/asm/kvm_host.h | 1 +
> > arch/x86/kvm/x86.c | 50 +++++++++++++++++++++++++--------
> > 2 files changed, 40 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index ec9830d2aabf..639ef92d01d1 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -800,6 +800,7 @@ struct kvm_vcpu_arch {
> > u64 mcg_ctl;
> > u64 mcg_ext_ctl;
> > u64 *mce_banks;
> > + u64 *mci_ctl2_banks;
> >
> > /* Cache MMIO info */
> > u64 mmio_gva;
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index eb4029660bd9..73c64d2b9e60 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3167,6 +3167,7 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > unsigned bank_num = mcg_cap & 0xff;
> > u32 msr = msr_info->index;
> > u64 data = msr_info->data;
> > + u32 offset;
> >
> > switch (msr) {
> > case MSR_IA32_MCG_STATUS:
> > @@ -3180,10 +3181,22 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > return 1;
> > vcpu->arch.mcg_ctl = data;
> > break;
> > + case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1:
>
> This is wrong. Well, incomplete. It will allow writes to MSRs that do not exist,
> i.e. if bank_num >= offset < KVM_MAX_MCE_BANKS.
>
> I believe this will suffice and is also the simplest?
>
> if (msr >= MSR_IA32_MCx_CTL2(bank_num))
> return 1;
>
> Actually, tying in with the array_index_nopsec() comments below, if this captures
> the last MSR in a variable, then the overrun is much more reasonable (sample idea
> at the bottom).
>
> > + if (!(mcg_cap & MCG_CMCI_P) &&
> > + (data || !msr_info->host_initiated))
>
> Funky indentation. Should be:
>
> if (!(mcg_cap & MCG_CMCI_P) &&
> (data || !msr_info->host_initiated))
> return 1;
Updated, I need to further fix my VIM configs.
>
> > + return 1;
> > + /* An attempt to write a 1 to a reserved bit raises #GP */
> > + if (data & ~(MCI_CTL2_CMCI_EN | MCI_CTL2_CMCI_THRESHOLD_MASK))
> > + return 1;
> > + offset = array_index_nospec(
> > + msr - MSR_IA32_MC0_CTL2,
> > + MSR_IA32_MCx_CTL2(bank_num) - MSR_IA32_MC0_CTL2);
>
> My preference would be to let this run over (by a fair amount)
>
> offset = array_index_nospec(msr - MSR_IA32_MC0_CTL2,
> MSR_IA32_MCx_CTL2(bank_num) - MSR_IA32_MC0_CTL2);
>
> But if we use a local variable, there's no overrun:
>
> case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1:
> last_msr = MSR_IA32_MCx_CTL2(bank_num) - 1;
> if (msr > last_msr)
> return 1;
>
> if (!(mcg_cap & MCG_CMCI_P) && (data || !msr_info->host_initiated))
> return 1;
> /* An attempt to write a 1 to a reserved bit raises #GP */
> if (data & ~(MCI_CTL2_CMCI_EN | MCI_CTL2_CMCI_THRESHOLD_MASK))
> return 1;
> offset = array_index_nospec(msr - MSR_IA32_MC0_CTL2,
> last_msr + 1 - MSR_IA32_MC0_CTL2);
> vcpu->arch.mci_ctl2_banks[offset] = data;
> break;
>
Updated to use this version.
>
> And if we go that route, in a follow-up patch at the end of the series, clean up
> the "default" path to hoist the if-statement into a proper case statement (unless
> I've misread the code), e.g. with some opportunstic cleanup:
>
> case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1:
> last_msr = MSR_IA32_MCx_CTL(bank_num) - 1;
> if (msr > last_msr)
> return 1;
>
> offset = array_index_nospec(msr - MSR_IA32_MC0_CTL,
> last_msr + 1 - MSR_IA32_MC0_CTL);
>
> /*
> * Only 0 or all 1s can be written to IA32_MCi_CTL, though some
> * Linux kernels clear bit 10 in bank 4 to workaround a BIOS/GART
> * TLB issue on AMD K8s, ignore this to avoid an uncatched #GP in
> * the guest.
> */
> if ((offset & 0x3) == 0 &&
> data != 0 && (data | (1 << 10)) != ~(u64)0)
> return -1;
>
> /* MCi_STATUS */
> if (!msr_info->host_initiated && (offset & 0x3) == 1 &&
> data != 0 && !can_set_mci_status(vcpu))
> return -1;
>
> vcpu->arch.mce_banks[offset] = data;
> break;
>
> Actually, rereading that, isn't "return -1" wrong? That will cause kvm_emulate_wrmsr()
> to exit to userspace, not inject a #GP.
>
> *sigh* Yep, indeed, the -1 gets interpreted as -EPERM and kills the guest.
>
> Scratch the idea of doing the above on top, I'll send separate patches and a KUT
> testcase. I'll Cc you on the patches, it'd save Paolo a merge conflict (or you a
> rebase) if this series is based on top of that bugfix + cleanup.
I will base this series on top of the bugfix + cleanup series.
>
> > + vcpu->arch.mci_ctl2_banks[offset] = data;
> > + break;
> > default:
> > if (msr >= MSR_IA32_MC0_CTL &&
> > msr < MSR_IA32_MCx_CTL(bank_num)) {
> > - u32 offset = array_index_nospec(
> > + offset = array_index_nospec(
> > msr - MSR_IA32_MC0_CTL,
> > MSR_IA32_MCx_CTL(bank_num) - MSR_IA32_MC0_CTL);
> >
> > @@ -3489,7 +3502,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > return 1;
> > }
> > break;
> > - case 0x200 ... 0x2ff:
> > + case 0x200 ... MSR_IA32_MC0_CTL2 - 1:
> > + case MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) ... 0x2ff:
> > return kvm_mtrr_set_msr(vcpu, msr, data);
> > case MSR_IA32_APICBASE:
> > return kvm_set_apic_base(vcpu, msr_info);
> > @@ -3646,6 +3660,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > 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:
> > return set_msr_mce(vcpu, msr_info);
> >
> > case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3:
> > @@ -3750,6 +3765,7 @@ static int get_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
> > u64 data;
> > u64 mcg_cap = vcpu->arch.mcg_cap;
> > unsigned bank_num = mcg_cap & 0xff;
> > + u32 offset;
> >
> > switch (msr) {
> > case MSR_IA32_P5_MC_ADDR:
> > @@ -3767,10 +3783,18 @@ static int get_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
> > case MSR_IA32_MCG_STATUS:
> > data = vcpu->arch.mcg_status;
> > break;
> > + case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1:
>
> Same comments as the WRMSR path. I'll also handle the "default" case in my cleanup.
Updated similarly as the WRMSR path. Thanks again, I will base V3 on
the cleanup + fix series.
>
> > + if (!(mcg_cap & MCG_CMCI_P) && !host)
> > + return 1;
> > + offset = array_index_nospec(
> > + msr - MSR_IA32_MC0_CTL2,
> > + MSR_IA32_MCx_CTL2(bank_num) - MSR_IA32_MC0_CTL2);
> > + data = vcpu->arch.mci_ctl2_banks[offset];
> > + break;
> > default:
> > if (msr >= MSR_IA32_MC0_CTL &&
> > msr < MSR_IA32_MCx_CTL(bank_num)) {
> > - u32 offset = array_index_nospec(
> > + offset = array_index_nospec(
> > msr - MSR_IA32_MC0_CTL,
> > MSR_IA32_MCx_CTL(bank_num) - MSR_IA32_MC0_CTL);
> >
>
> ...
>
> > @@ -11126,9 +11152,11 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> > goto fail_free_lapic;
> > vcpu->arch.pio_data = page_address(page);
> >
> > - vcpu->arch.mce_banks = kzalloc(KVM_MAX_MCE_BANKS * sizeof(u64) * 4,
> > + vcpu->arch.mce_banks = kcalloc(KVM_MAX_MCE_BANKS * 4, sizeof(u64),
> > + GFP_KERNEL_ACCOUNT);
>
> Switching to kcalloc() should be a separate patch.
Done.
>
> > + vcpu->arch.mci_ctl2_banks = kcalloc(KVM_MAX_MCE_BANKS, sizeof(u64),
> > GFP_KERNEL_ACCOUNT);
> > - if (!vcpu->arch.mce_banks)
> > + if (!vcpu->arch.mce_banks | !vcpu->arch.mci_ctl2_banks)
>
> This wants to be a logical-OR, not a bitwise-OR.
>
> Oh, and vcpu->arch.mci_ctl2_banks needs to be freed if a later step fails.
Thanks for the catch, fixed in V3.
>
> > goto fail_free_pio_data;
> > vcpu->arch.mcg_cap = KVM_MAX_MCE_BANKS;
> >
> > --
> > 2.35.1.1178.g4f1659d476-goog
> >
