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@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.
> 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
>
