Re: [PATCH v2 2/2] KVM: x86: Do not gate MCE logging based on IA32_MCi_CTL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch incorrectly overwrites the MCE banks when a double/triple fault occurs (MCIP flag already set).

Pre-existing issues:
- [High] The patch fails to apply the MCE logging fix to the global control register IA32_MCG_CTL, leaving MCE emulation architecturally broken.
--

commit 146247f45f3a8d3f982b364bba19a51f76828ce0
Author: Carlos López <clopez@suse.de>

KVM: x86: Do not gate MCE logging based on IA32_MCi_CTL

This patch hoists the MCE logging logic above the IA32_MCi_CTL check.
This ensures that KVM logs uncorrectable machine check errors even if
reporting is disabled for that specific bank, conforming to the Intel SDM.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5486,24 +5486,31 @@ static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu,
>  	if ((mce->status & MCI_STATUS_UC) && (mcg_cap & MCG_CTL_P) &&
>  	    vcpu->arch.mcg_ctl != ~(u64)0)
>  		return 0;

[Severity: High]
This isn't a bug introduced by this patch, but does this global control
register check also need to be adjusted?

Just like IA32_MCi_CTL, the Intel SDM states that error logging happens
regardless of the setting of the IA32_MCG_CTL bits. By returning early
here, KVM skips logging the uncorrectable error to the bank if the global
reporting is disabled.

Additionally, KVM currently suppresses generation and logging for all
banks if any bit in IA32_MCG_CTL is 0, rather than checking the specific
bit for the bank. Should the logging logic be hoisted above this check
as well?

> -	/*
> -	 * if IA32_MCi_CTL is not all 1s, the uncorrected error
> -	 * reporting is disabled for the bank
> -	 */
> -	if ((mce->status & MCI_STATUS_UC) && banks[0] != ~(u64)0)
> -		return 0;
> +
>  	if (mce->status & MCI_STATUS_UC) {
> +		/*
> +		 * Per the SDM, error logging always happens independently of
> +		 * IA32_MCi_CTL
> +		 */
> +		if (banks[1] & MCI_STATUS_VAL)
> +			mce->status |= MCI_STATUS_OVER;
> +		banks[2] = mce->addr;
> +		banks[3] = mce->misc;
> +		banks[1] = mce->status;
> +
> +		/*
> +		 * if IA32_MCi_CTL is not all 1s, the uncorrected error
> +		 * reporting is disabled for the bank
> +		 */
> +		if (banks[0] != ~(u64)0)
> +			return 0;
> +
>  		if ((vcpu->arch.mcg_status & MCG_STATUS_MCIP) ||
>  		    !kvm_is_cr4_bit_set(vcpu, X86_CR4_MCE)) {
>  			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
>  			return 0;
>  		}

[Severity: High]
By hoisting the error logging block above the IA32_MCi_CTL check, does
this now incorrectly overwrite the MCE banks when a double or triple fault
occurs?

According to the SDM, if the processor encounters another machine-check
error while the MCIP flag is already set, it causes a triple fault and
does not update the status registers for the second error.

Since the logging block in kvm_vcpu_ioctl_x86_set_mce() now executes
before checking for MCG_STATUS_MCIP, it looks like it will overwrite the
original addr, misc, and status of the first error before requesting the
triple fault.

> -		if (banks[1] & MCI_STATUS_VAL)
> -			mce->status |= MCI_STATUS_OVER;
> -		banks[2] = mce->addr;
> -		banks[3] = mce->misc;
>  		vcpu->arch.mcg_status = mce->mcg_status;
> -		banks[1] = mce->status;
>  		kvm_queue_exception(vcpu, MC_VECTOR);
>  	} else if (!(banks[1] & MCI_STATUS_VAL)
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609105525.2545638-2-clopez@suse.de?part=2