Re: [PATCH v4 7/8] KVM: x86: Enable CMCI capability by default and handle injected UCNA errors

[David Matlack <dmatlack@google.com>]

On Fri, May 20, 2022 at 10:37 AM Jue Wang <juew@google.com> wrote:
>
> Make KVM support the CMCI capability by default by adding MCG_CMCI_P to
> kvm_mce_cap_supported. A vCPU can request for this capability via
> KVM_X86_SETUP_MCE. Uncorrectable Error No Action required (UCNA) injection
> reuses the MCE injection ioctl KVM_X86_SET_MCE.
>
> Neither of the CMCI and UCNA emulations depends on hardware.
>
> Signed-off-by: Jue Wang <juew@google.com>
> ---
>  arch/x86/kvm/vmx/vmx.c |  1 +
>  arch/x86/kvm/x86.c     | 50 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 610355b9ccce..1aed964ee4ee 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -8037,6 +8037,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 f8ab592f519b..d0b1bb6e5e4a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4826,6 +4826,52 @@ static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
>         return r;
>  }
>
> +/*
> + * Validate this is an UCNA error by checking the MCG_STATUS and MCi_STATUS
> + * registers that none of the bits for Machine Check Exceptions are set and
> + * both the VAL (valid) and UC (uncorrectable) bits are set.
> + * UCNA - UnCorrectable No Action required
> + * SRAR - Software Recoverable Action Required
> + * MCI_STATUS_PCC - Processor Context Corrupted
> + * MCI_STATUS_S - Signaled as a Machine Check Exception
> + * MCI_STATUS_AR - This MCE is "software recoverable action required"
> + */
> +static bool is_ucna(struct kvm_x86_mce *mce)
> +{
> +       return  !mce->mcg_status &&
> +               !(mce->status & (MCI_STATUS_PCC | MCI_STATUS_S | MCI_STATUS_AR)) &&
> +               (mce->status & MCI_STATUS_VAL) &&
> +               (mce->status & MCI_STATUS_UC);
> +}
> +
> +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;
> +
> +       if (mce->bank >= bank_num)
> +               return -EINVAL;

Drop this check. The caller already checks it.

> +
> +       if (!is_ucna(mce))
> +               return -EINVAL;

Drop this check. The only caller of this function already checks is_ucna().

> +
> +       banks += 4 * mce->bank;

The caller also computes banks. Perhaps just pass that in rather that
re-calculating it here?

Also, calculating banks should probably use array_index_nospec() since
the index is untrusted (coming from userspace).

> +       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))
> +               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)
>  {
> @@ -4835,6 +4881,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))
> +               return kvm_vcpu_x86_set_ucna(vcpu, mce);
> +
>         /*
>          * if IA32_MCG_CTL is not all 1s, the uncorrected error
>          * reporting is disabled
> --
> 2.36.1.124.g0e6072fb45-goog
>