Re: [PATCH v2 4/4] KVM: x86: Add support for MCG_CMCI_P and handling of injected UCNAs.

[Sean Christopherson <seanjc@google.com>]

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.

> 
> Signed-off-by: Jue Wang <juew@google.com>
> ---
>  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.

>  	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'?

> +{
> +	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 ;-)

	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.

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.

> +	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))

> +		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;


> +		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
>