When userspace issues KVM_X86_SET_MCE, kvm_vcpu_ioctl_x86_set_mce() decides whether to log an uncorrectable MCE by looking at the corresponding IA32_MCi_CTL MSR. This is not the behavior specified in the Intel SDM (17.3.2.1 IA32_MCi_CTL MSRs): Setting an EEj flag enables signaling #MC of the associated error and clearing it disables signaling of the error. Error logging happens regardless of the setting of these bits. The processor drops writes to bits that are not implemented. Perform the logging before checking MCi_CTL, unless there is already a valid UC error logged for the bank, in which case the SDM (17.3.2.2 "IA32_MCi_STATUS MSRS") specifies that error information should not be overwritten. To avoid even more complex control flow, hoist the logging logic into a separate function, which then enables the removal of the non-UC branch in kvm_vcpu_ioctl_x86_set_mce(), which only existed to perform logging. Note that the SDM is ambiguous regarding the effects of IA32_MCG_CTL on logging, so preserve the existing logic (i.e. do not log the error if MCG_CTL is disabled). Fixes: 890ca9aefa78 ("KVM: Add MCE support") Signed-off-by: Carlos López --- arch/x86/kvm/x86.c | 62 +++++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 77a780177c4e..af3662aa3ce3 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -5462,6 +5462,28 @@ static int kvm_vcpu_x86_set_ucna(struct kvm_vcpu *vcpu, struct kvm_x86_mce *mce, return 0; } +/* + * Record @mce into @banks per the SDM logging rules: + * - Per 17.3.2.1, logging happens regardless of IA32_MCi_CTL. + * - Per 17.3.2.2.1 and 17.3.2.2, a UC error does not overwrite a + * previous valid UC log in the same bank; only OVER is set. + * Any other prior state (invalid, or valid non-UC) is replaced, + * with OVER set if a prior valid log was overwritten. + */ +static void kvm_log_mce(u64 *banks, struct kvm_x86_mce *mce) +{ + bool overflow = banks[1] & MCI_STATUS_VAL; + + if (!overflow || !(banks[1] & MCI_STATUS_UC)) { + banks[2] = mce->addr; + banks[3] = mce->misc; + banks[1] = mce->status; + } + + if (overflow) + banks[1] |= MCI_STATUS_OVER; +} + static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu, struct kvm_x86_mce *mce) { @@ -5485,34 +5507,28 @@ 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; + + kvm_log_mce(banks, mce); + + if (!(mce->status & MCI_STATUS_UC)) + return 0; + /* * 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) + if (banks[0] != ~(u64)0) return 0; - if (mce->status & MCI_STATUS_UC) { - 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; - } - 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) - || !(banks[1] & MCI_STATUS_UC)) { - if (banks[1] & MCI_STATUS_VAL) - mce->status |= MCI_STATUS_OVER; - banks[2] = mce->addr; - banks[3] = mce->misc; - banks[1] = mce->status; - } else - banks[1] |= MCI_STATUS_OVER; + + 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; + } + + vcpu->arch.mcg_status = mce->mcg_status; + kvm_queue_exception(vcpu, MC_VECTOR); + return 0; } -- 2.51.0