On Tue, Apr 12, 2022 at 7:22 AM Jue Wang <juew@xxxxxxxxxx> wrote:
>
> Thanks a lot for the review feedback, Sean!
>
> I will break the current patch into a several patch series and
> incorporate these feedbacks in the V2 to be sent out.
>
> Thanks,
> -Jue
>
> On Mon, Apr 11, 2022 at 12:08 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> >
> > On Wed, Mar 23, 2022, Jue Wang wrote:
> > > CMCI
> >
> > Please write Corrected Machine Check Interrupt at least once.
> >
> > > is supported since Nehalem.
> >
> > While possibly interesting, this info is defitely not the most interesting tidbit
> > in the changelong, i.e. shouldn't be the opening line.
> >
> > > UCNA (uncorrectable no action required) errors signaled via CMCI allows a
> > > guest to be notified as soon as uncorrectable memory errors get detected by
> > > some background threads, e.g., threads that migrate guest memory across
> > > hosts.
> > >
> > > Upon receiving UCNAs, guest kernel isolates the poisoned pages from
> > > future accesses much earlier than a potential fatal Machine Check
> > > Exception due to accesses from a guest thread.
> > >
> > > Add CMCI signaling based on the per vCPU opt-in of MCG_CMCI_P.
> >
> > This changelog needs much longer explanation of what exactly is being added, e.g. I
> > had to read the code to find out that this introduces new userspace functionality
> > to allow injecting UNCA #MCs and thus CMCI IRQs.
> >
> > That's also a symptom of this patch needing to be split into a proper series, e.g.
> > exposing the UNCA injection point to userspace needs to be a separate patch.
> >
> > Looking through this, 5 or 6 patches is probably appropriate:
> >
> > 1. Replace existing magic numbers with #defines
> > 2. Clean up the existing LVT mess
> > 3. Add in-kernel LVTCMCI support (unreachable until #, but easier to review)
> > 4. Add support for MSR_IA32_MCx_CTL2 MSRs
> > 5. Add CMCI support
> > 6. Add UNCA injection support
> >
> > I can't tell if #4 is necessary as a separate patch, it might belong with #3.
> >
> > > Signed-off-by: Jue Wang <juew@xxxxxxxxxx>
> > > ---
> > > arch/x86/include/asm/kvm_host.h | 11 +++
> > > arch/x86/kvm/lapic.c | 65 ++++++++++++++----
> > > arch/x86/kvm/lapic.h | 2 +-
> > > arch/x86/kvm/vmx/vmx.c | 1 +
> > > arch/x86/kvm/x86.c | 115 +++++++++++++++++++++++++++++---
> > > 5 files changed, 171 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > index ec9830d2aabf..d57f3d1284a3 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -613,6 +613,8 @@ struct kvm_vcpu_xen {
> > > unsigned long evtchn_pending_sel;
> > > };
> > >
> > > +#define KVM_MCA_REG_PER_BANK 5
> > > +
> > > struct kvm_vcpu_arch {
> > > /*
> > > * rip and regs accesses must go through
> > > @@ -799,6 +801,15 @@ struct kvm_vcpu_arch {
> > > u64 mcg_status;
> > > u64 mcg_ctl;
> > > u64 mcg_ext_ctl;
> > > + /*
> > > + * 5 registers per bank for up to KVM_MAX_MCE_BANKS.
> > > + * Register order within each bank:
> > > + * mce_banks[5 * bank] - IA32_MCi_CTL
> > > + * mce_banks[5 * bank + 1] - IA32_MCi_STATUS
> > > + * mce_banks[5 * bank + 2] - IA32_MCi_ADDR
> > > + * mce_banks[5 * bank + 3] - IA32_MCi_MISC
> > > + * mce_banks[5 * bank + 4] - IA32_MCi_CTL2
> > > + */
> > > u64 *mce_banks;
> >
> > Why shove CTL2 into mce_banks? AFAICT, it just makes everything harder. Adding
> > a new "u64 *mce_ctl2_banks" or whatever would simplify everything except the
> > memory allocation.
> >
> > > /* Cache MMIO info */
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 9322e6340a74..b388eb82308a 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -27,6 +27,7 @@
> > > #include <linux/math64.h>
> > > #include <linux/slab.h>
> > > #include <asm/processor.h>
> > > +#include <asm/mce.h>
> > > #include <asm/msr.h>
> > > #include <asm/page.h>
> > > #include <asm/current.h>
> > > @@ -53,8 +54,6 @@
> > > #define PRIu64 "u"
> > > #define PRIo64 "o"
> > >
> > > -/* 14 is the version for Xeon and Pentium 8.4.8*/
> > > -#define APIC_VERSION (0x14UL | ((KVM_APIC_LVT_NUM - 1) << 16))
> >
> > Eh, probably worth keeping the APIC_VERSION #define and just move out the LVT crud.
> >
> > > #define LAPIC_MMIO_LENGTH (1 << 12)
> > > /* followed define is not in apicdef.h */
> > > #define MAX_APIC_VECTOR 256
> > > @@ -367,7 +366,10 @@ static inline int apic_lvt_nmi_mode(u32 lvt_val)
> > > void kvm_apic_set_version(struct kvm_vcpu *vcpu)
> > > {
> > > struct kvm_lapic *apic = vcpu->arch.apic;
> > > - u32 v = APIC_VERSION;
> > > + int lvt_num = vcpu->arch.mcg_cap & MCG_CMCI_P ? KVM_APIC_LVT_NUM :
> > > + KVM_APIC_LVT_NUM - 1;
> >
> > Retrieving the number of LVT entries belongs in a helper, and this is rather gross
> > absuse of KVM_APIC_LVT_NUM as there's zero indication that it's pseudo-dynamic.
> > The code that handles accesses to APIC_LVTCMCI is even worse:
> >
> > val &= apic_lvt_mask[KVM_APIC_LVT_NUM - 1];
> >
> > The easiest and best way to handle this is to define an enum, especially since
> > the LVT entries aren't exactly intuitive (e.g. LVT_LINT0 isn't entry 0)
> >
> > enum lapic_lvt_entry {
> > LVT_TIMER,
> > LVT_THERMAL_MONITOR,
> > LVT_PERFORMANCE_COUNTER,
> > LVT_LINT0,
> > LVT_LINT1,
> > LVT_ERROR,
> > LVT_CMCI,
> >
> > KVM_APIC_MAX_NR_LVT_ENTRIES,
> > }
> >
> > And use those for the initialization of apic_lvt_mask[] and drop the comments:
> >
> > static const unsigned int apic_lvt_mask[KVM_APIC_LVT_NUM] = {
> > [LVT_TIMER] = LVT_MASK , /* timer mode mask added at runtime */
> > [LVT_TERMAL_MONITOR] = LVT_MASK | APIC_MODE_MASK,
> >
> > and so on and so forth
> > };
> >
> > Then there's no need for the ugly KVM_APIC_LVT_NUM - 1 shenanigans to access the
> > CMCI entry, and the only place that needs to be aware at all is the helper to
> > query the number of LVT entries. Heh, and if we wanted to be clever/supid...
> >
> > static inline kvm_apic_get_nr_lvt_entries(struct kvm_vcpu *vcpu)
> > {
> > return KVM_APIC_MAX_NR_LVT_ENTRIES - !kvm_is_cmci_supported(vcpu);
> > }
> >
> > > + /* 14 is the version for Xeon and Pentium 8.4.8*/
> > > + u32 v = 0x14UL | ((lvt_num - 1) << 16);
> > >
> > > if (!lapic_in_kernel(vcpu))
> > > return;
> > > @@ -390,7 +392,8 @@ static const unsigned int apic_lvt_mask[KVM_APIC_LVT_NUM] = {
> > > LVT_MASK | APIC_MODE_MASK, /* LVTTHMR */
> > > LVT_MASK | APIC_MODE_MASK, /* LVTPC */
> > > LINT_MASK, LINT_MASK, /* LVT0-1 */
> > > - LVT_MASK /* LVTERR */
> > > + LVT_MASK, /* LVTERR */
> > > + LVT_MASK | APIC_MODE_MASK /* LVTCMCI */
> > > };
> > >
> > > static int find_highest_vector(void *bitmap)
> > > @@ -1405,6 +1408,9 @@ int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
> > > APIC_REG_MASK(APIC_TMCCT) |
> > > APIC_REG_MASK(APIC_TDCR);
> > >
> > > + if (apic->vcpu->arch.mcg_cap & MCG_CMCI_P)
> >
> > As alluded to above, this belongs in a helper too.
> >
> > > + valid_reg_mask |= APIC_REG_MASK(APIC_LVTCMCI);
> > > +
> > > /* ARBPRI is not valid on x2APIC */
> > > if (!apic_x2apic_mode(apic))
> > > valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI);
> > > @@ -1993,6 +1999,18 @@ static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val)
> > > }
> > > }
> > >
> > > +static int kvm_lvt_reg_by_index(int i)
> > > +{
> > > + if (i < 0 || i >= KVM_APIC_LVT_NUM) {
> > > + pr_warn("lvt register index out of bound: %i\n", i);
> >
> > This sanity check is unnecessary, @i is fully KVM controlled. And a pr_warn() in
> > that case is nowhere near strong enough, e.g. at minimum this should be WARN_ON,
> > though again, I don't think that's necessary.
> >
> > Actually, if we really wanted to sanity check @i, we could make this __always_inline
> > and turn it into a BUILD_BUG_ON(), though I bet there's a config option that will
> > result in the compiler not unrolling the callers and ruining that idea.
> >
> > > + return 0;
> > > + }
> > > +
> > > + if (i < KVM_APIC_LVT_NUM - 1)
> >
> > Far better is
> >
> > if (i == LVT_CMCI)
> > return APIC_LVTCMCI;
> >
> > return return APIC_LVTT + 0x10 * i;
> >
> > Though given the nature of the usage, it might actually be better to bury this in
> > a macro (or a helper function masquerading as a macro by having a wierd name).
> >
> > #define APIC_LVTx(x) \
> > ({ \
> > int __apic_reg; \
> > \
> > if ((x) != LVT_CMCI) \
> > __apic_reg = APIC_LVTCMCI; \
> > else \
> > __apic_reg = APIC_LVTT + 0x10 * (x); \
> > __apic_reg; \
> > }
> >
> >
> > Then the usage stays quite readable and doesn't need temp variables, e.g.
> >
> > for (i = 0; i < KVM_APIC_MAX_NR_LVT_ENTRIES; i++) {
> > lvt_val = kvm_lapic_get_reg(apic, APIC_LVTx(i));
> > kvm_lapic_set_reg(apic, APIC_LVTx(i),
> > lvt_val | APIC_LVT_MASKED);
> > }
> >
> > > + return APIC_LVTT + 0x10 * i;
> > > + return APIC_LVTCMCI;
> > > +}
> > > +
> >
> > ...
> >
> > > @@ -2341,8 +2376,14 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
> > > kvm_apic_set_xapic_id(apic, vcpu->vcpu_id);
> > > kvm_apic_set_version(apic->vcpu);
> > >
> > > - for (i = 0; i < KVM_APIC_LVT_NUM; i++)
> > > - kvm_lapic_set_reg(apic, APIC_LVTT + 0x10 * i, APIC_LVT_MASKED);
> > > + lvt_num = vcpu->arch.mcg_cap & MCG_CMCI_P ? KVM_APIC_LVT_NUM :
> > > + KVM_APIC_LVT_NUM - 1;
> > > + for (i = 0; i < lvt_num; i++) {
> > > + int lvt_reg = kvm_lvt_reg_by_index(i);
> > > +
> > > + if (lvt_reg)
> > > + kvm_lapic_set_reg(apic, lvt_reg, APIC_LVT_MASKED);
> > > + }
> > > apic_update_lvtt(apic);
> > > if (kvm_vcpu_is_reset_bsp(vcpu) &&
> > > kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_LINT0_REENABLED))
> > > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> > > index 2b44e533fc8d..e2ae097613ca 100644
> > > --- a/arch/x86/kvm/lapic.h
> > > +++ b/arch/x86/kvm/lapic.h
> > > @@ -10,7 +10,7 @@
> > >
> > > #define KVM_APIC_INIT 0
> > > #define KVM_APIC_SIPI 1
> > > -#define KVM_APIC_LVT_NUM 6
> > > +#define KVM_APIC_LVT_NUM 7
> > >
> > > #define APIC_SHORT_MASK 0xc0000
> > > #define APIC_DEST_NOSHORT 0x0
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index b730d799c26e..63aa2b3d30ca 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -8035,6 +8035,7 @@ static __init int hardware_setup(void)
> > > }
> > >
> > > kvm_mce_cap_supported |= MCG_LMCE_P;
> > > + kvm_mce_cap_supported |= MCG_CMCI_P;
> > >
> > > if (pt_mode != PT_MODE_SYSTEM && pt_mode != PT_MODE_HOST_GUEST)
> > > return -EINVAL;
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index eb4029660bd9..6626723bf51b 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -3180,6 +3180,25 @@ 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:
> > > + {
> >
> > Kernel style when curly braces is needed for a case statement is to put the opening
> > braces with the case and not indent. Though my vote is to hoist "offset" to be
> > declared at the function level so that each statement doesn't need curly braces
> > just to define a fairly common varaible.
> >
> > > + u32 offset;
> > > + /* BIT[30] - CMCI_ENABLE */
> > > + /* BIT[0:14] - CMCI_THRESHOLD */
> > > + u64 mask = (1 << 30) | 0x7fff;
> >
> > Add proper #defines, not comments.
> >
> > > +
> > > + if (!(mcg_cap & MCG_CMCI_P) &&
> > > + (data || !msr_info->host_initiated))
> >
> > This looks wrong, userspace should either be able to write the MSR or not, '0'
> > isn't special. Unless there's a danger to KVM, which I don't think there is,
> > userspace should be allowed to ignore architectural restrictions, i.e. bypass
> > the MCG_CMCI_P check, so that KVM doesn't create an unnecessary dependency between
> > ioctls. I.e. this should be:
>
> I think the idea was writing unsupported values into an MSR should
> cause #GP. The code is consistent (copied) from the code that handles
> MSR_IA32_MCG_CTL:
>
> https://elixir.bootlin.com/linux/v5.17-rc8/source/arch/x86/kvm/x86.c#L3177
>
> Can you elaborate what unnecessary dependency between ioctls this may cause?
>
> >
> > if (!(mcg_cap & MCG_CMCI_P) && !msr_info->host_initiated)
> > return 1;
> >
> > > + return 1;
> > > + /* An attempt to write a 1 to a reserved bit raises #GP*/
> > > + if (data & ~mask)
> > > + return 1;
> > > + offset = array_index_nospec(
> > > + msr - MSR_IA32_MC0_CTL2,
> > > + MSR_IA32_MCx_CTL2(bank_num) - MSR_IA32_MC0_CTL2);
> >
> > The existing code is gross, don't copy it :-) I'd rather this run over the 80 char
> > soft limit.
> >
> > > + vcpu->arch.mce_banks[offset * KVM_MCA_REG_PER_BANK + 4] = (data & mask);
> >
> > The AND with the mask is pointless, @data has already been verified.
> >
> > With all of the above, this becomes:
> >
> > case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1:
> > if (!(mcg_cap & MCG_CMCI_P) && !msr_info->host_initiated)
> > return 1;
> >
> > if (data & ~(MCx_CTL2_CMCI_ENABLE | MCx_CTL2_CMCI_THRESHOLD))
> > return 1;
> >
> > offset = array_index_nospec(msr - MSR_IA32_MC0_CTL2,
> > MSR_IA32_MCx_CTL2(bank_num) - MSR_IA32_MC0_CTL2);
> > vcpu->arch.mce_ctl2_banks = [offset] = data;
> > break;
> >
> >
> > > + }
> > > + break;
> > > default:
> > > if (msr >= MSR_IA32_MC0_CTL &&
> > > msr < MSR_IA32_MCx_CTL(bank_num)) {
> > > @@ -3203,7 +3222,14 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > > return -1;
> > > }
> > >
> > > - vcpu->arch.mce_banks[offset] = data;
> > > + /* MSR_IA32_MCi_CTL addresses are incremented by 4 bytes
> > > + * per bank.
> > > + * kvm_vcpu_arch.mce_banks has 5 registers per bank, see
> > > + * register layout details in kvm_host.h.
> > > + * MSR_IA32_MCi_CTL is the first register in each bank
> > > + * within kvm_vcpu_arch.mce_banks.
> > > + */
> > > + vcpu->arch.mce_banks[offset * KVM_MCA_REG_PER_BANK / 4] = data;
> >
> > This mess goes away if CTL2 gets a separate array.
> >
> > > break;
> > > }
> > > return 1;
> > > @@ -3489,7 +3515,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 +3673,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:
> > > @@ -3767,6 +3795,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:
> > > + {
> > > + u32 offset;
> > > +
> > > + 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.mce_banks[offset * KVM_MCA_REG_PER_BANK + 4];
> >
> > Same comments as the write path.
> >
> > > + }
> > > + break;
> > > default:
> > > if (msr >= MSR_IA32_MC0_CTL &&
> > > msr < MSR_IA32_MCx_CTL(bank_num)) {
> > > @@ -3774,7 +3814,7 @@ static int get_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
> > > msr - MSR_IA32_MC0_CTL,
> > > MSR_IA32_MCx_CTL(bank_num) - MSR_IA32_MC0_CTL);
> > >
> > > - data = vcpu->arch.mce_banks[offset];
> > > + data = vcpu->arch.mce_banks[offset * KVM_MCA_REG_PER_BANK / 4];
> > > break;
> > > }
> > > return 1;
> >
> > ...
> >
> > > +static bool is_ucna(u64 mcg_status, u64 mci_status)
> > > +{
> > > + return !mcg_status &&
> > > + !(mci_status & (MCI_STATUS_PCC|MCI_STATUS_S|MCI_STATUS_AR));
> >
> > Spaces around the '|'.
> >
> > > +}
> > > +
> > > +static int kvm_vcpu_x86_set_ucna(struct kvm_vcpu *vcpu,
> > > + struct kvm_x86_mce *mce)
> > > +{
> > > + u64 mcg_cap = vcpu->arch.mcg_cap;
> > > + unsigned int bank_num = mcg_cap & 0xff;
> > > + u64 *banks = vcpu->arch.mce_banks;
> > > +
> > > + /* Check for legal bank number in guest */
> > > + if (mce->bank >= bank_num)
> > > + return -EINVAL;
> > > +
> > > + /* Disallow bits that are used for machine check signalling */
> >
> > This needs a more verbose comment/explanation. I can kinda sorta piece things
> > together, but the intent is unclear.
> >
> > > + if (mce->mcg_status ||
> > > + (mce->status & (MCI_STATUS_PCC|MCI_STATUS_S|MCI_STATUS_AR)))
> > > + return -EINVAL;
> > > +
> > > + /* UCNA must have VAL and UC bits set */
> > > + if ((mce->status & (MCI_STATUS_VAL|MCI_STATUS_UC)) !=
> > > + (MCI_STATUS_VAL|MCI_STATUS_UC))
> >
> > Spaces again, though my personal preference would be:
> >
> > if (!(mce->status & MCI_STATUS_VAL) || !(mce->status & MCI_STATUC_UC))
> >
> > > + return -EINVAL;
> > > +
> > > + banks += KVM_MCA_REG_PER_BANK * mce->bank;
> > > + banks[1] = mce->status;
> > > + banks[2] = mce->addr;
> > > + banks[3] = mce->misc;
> > > + vcpu->arch.mcg_status = mce->mcg_status;
> > > +
> > > + /*
> > > + * if MCG_CMCI_P is 0 or BIT[30] of IA32_MCi_CTL2 is 0, CMCI signaling
> > > + * is disabled for the bank
> > > + */
> > > + if (!(mcg_cap & MCG_CMCI_P) || !(banks[4] & (1 << 30)))
> >
> > #defines, not comments for the BIT 32 thing.
> >
> > > + return 0;
> > > +
> > > + if (lapic_in_kernel(vcpu))
> >
> > Any reason to support UNCA injection without an in-kernel APIC?
Even without a viable path for CMCI signaling, the UCNA errors should
still be logged in registers so it gives consistent semantics to
hardware for e.g., MCE overflow checks on an MCE signaled later.
> >
> > > + kvm_apic_local_deliver(vcpu->arch.apic, APIC_LVTCMCI);
> > > +
> > > + return 0;
> > > +}
