On Wed, May 11, 2022 at 12:20 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Tue, Apr 12, 2022, Jue Wang wrote:
> > Note prior to this patch, the UCNA type of signaling can already be
> > processed by kvm_vcpu_ioctl_x86_set_mce and does not result in correct
> > CMCI signaling semantic.
>
> Same as before...
>
> UCNA should be spelled out at least once.
Rewrote the change log to include full context of this change and full
text definition of UCNA and CMCI.
>
> >
> > Signed-off-by: Jue Wang <juew@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/vmx/vmx.c | 1 +
> > arch/x86/kvm/x86.c | 48 ++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 49 insertions(+)
> >
> > 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;
>
> Is there really no hardware dependency on CMCI? Honest question If not, that
> should be explicitly called out in the changelog.
CMCI emulation does not depend on hardware, it only depends on vcpu's
lapic being available.
Updated the change log.
>
> > 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 73c64d2b9e60..eb6058ca1e70 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4775,6 +4775,50 @@ static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
> > return r;
> > }
> >
> > +static bool is_ucna(u64 mcg_status, u64 mci_status)
>
> Any reason not to take 'struct kvm_x86_mce *mce'?
Updated.
>
> > +{
> > + return !mcg_status &&
> > + !(mci_status & (MCI_STATUS_PCC | MCI_STATUS_S | MCI_STATUS_AR));
>
> As someone who knows nothing about MCI encoding, can you add a function comment
> explaing, in detail, what's going on here?
>
> Also, my preference is to align the indentation on multi-line returns. Paolo scoffs
> at this nit of mine, but he's obviously wrong ;-)
Added function comments in V3 and updated alignment.
>
> return !mcg_status &&
> !(mci_status & (MCI_STATUS_PCC | MCI_STATUS_S | MCI_STATUS_AR));
>
> > +}
> > +
> > +static int kvm_vcpu_x86_set_ucna(struct kvm_vcpu *vcpu,
> > + struct kvm_x86_mce *mce)
>
> Please align the params. Actually, just let it run over, it's a single char.
Done.
>
> 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 */
>
> Eh, don't think this warrants a comment.
Removed.
>
> > + if (mce->bank >= bank_num)
> > + return -EINVAL;
> > +
> > + /*
> > + * UCNA signals should not set bits that are only used for machine check
> > + * exceptions.
> > + */
> > + if (mce->mcg_status ||
> > + (mce->status & (MCI_STATUS_PCC | MCI_STATUS_S | MCI_STATUS_AR)))
>
> Unless mine eyes deceive me, this is the same as:
>
> if (!is_ucna(mce->mcg_status, mce->status))
>
Good catch, also folded the MCI_STATUS_VAL and MCI_STATUS_UC checks
into is_ucna.
> > + return -EINVAL;
> > +
> > + /* UCNA must have VAL and UC bits set */
> > + if (!(mce->status & MCI_STATUS_VAL) || !(mce->status & MCI_STATUS_UC))
> > + return -EINVAL;
> > +
> > + banks += 4 * mce->bank;
> > + banks[1] = mce->status;
> > + banks[2] = mce->addr;
> > + banks[3] = mce->misc;
> > + vcpu->arch.mcg_status = mce->mcg_status;
> > +
> > + if (!(mcg_cap & MCG_CMCI_P) || !(vcpu->arch.mci_ctl2_banks[mce->bank] & MCI_CTL2_CMCI_EN))
>
> This one's worth wrapping, that's quite a long line, and there's a natural split point:
>
> if (!(mcg_cap & MCG_CMCI_P) ||
> !(vcpu->arch.mci_ctl2_banks[mce->bank] & MCI_CTL2_CMCI_EN))
> return 0;
>
>
Done.
> > + return 0;
> > +
> > + if (lapic_in_kernel(vcpu))
> > + kvm_apic_local_deliver(vcpu->arch.apic, APIC_LVTCMCI);
> > +
> > + return 0;
> > +}
> > +
> > static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu,
> > struct kvm_x86_mce *mce)
> > {
> > @@ -4784,6 +4828,10 @@ static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu,
> >
> > if (mce->bank >= bank_num || !(mce->status & MCI_STATUS_VAL))
> > return -EINVAL;
> > +
> > + if (is_ucna(mce->mcg_status, mce->status))
> > + return kvm_vcpu_x86_set_ucna(vcpu, mce);
> > +
> > /*
> > * if IA32_MCG_CTL is not all 1s, the uncorrected error
> > * reporting is disabled
> > --
> > 2.35.1.1178.g4f1659d476-goog
> >
