Re: [PATCH v4 1/2] x86/mce: Check for writes ignored in MCA_STATUS register

[Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>]

On 4/6/2022 9:08 AM, Borislav Petkov wrote:
> On Mon, Feb 14, 2022 at 05:36:39PM -0600, Smita Koralahalli wrote:
>> diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
>> index 5fbd7ffb3233..43ba63b7dc73 100644
>> --- a/arch/x86/kernel/cpu/mce/inject.c
>> +++ b/arch/x86/kernel/cpu/mce/inject.c
>> @@ -470,11 +470,36 @@ static void toggle_nb_mca_mst_cpu(u16 nid)
>>   		       __func__, PCI_FUNC(F3->devfn), NBCFG);
>>   }
>>   
>> +struct mce_err_handler {
>> +	struct mce *mce;
>> +	int err;
>> +};
> Well, we already have a struct mce i_mce which serves as a sort-of
> global container for injection data which gets dumped into the hardware
> upon injection time.
>
> Now you're adding another struct which contains additional info and are
> adding a pointer to that i_mce.
>
> But this is not a clean design - when you do stuff like that you need to
> unify all those global-info-containing structs and make the code dealing
> with them straight-forward . I.e., if you don't look at the big picture
> of a design, it soon grows into an unwieldy mess which some poor sod
> would need to clean up afterwards and that poor sod is in most cases the
> maintainer.
>
> So I went and did that ontop of your patch, this is one possible way to
> do it but it.
>
> Here it is, a combined diff ontop of tip:ras/core. Please split it into
> patches: first patch converts to the new descriptor and then a second
> one adds the MCA_STATUS checking bits.
>
> There are a couple of other changes in there, if they're not clear, feel
> free to ask.
Okay, will make changes as mentioned. Thanks.
I understood most of it, just have one doubt below..
>
> Thx.
>
> ---
> diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
> index 43ba63b7dc73..0fd1eea2f754 100644
> --- a/arch/x86/kernel/cpu/mce/inject.c
> +++ b/arch/x86/kernel/cpu/mce/inject.c
> @@ -33,10 +33,14 @@
>   
>   #include "internal.h"
>   
> -/*
> - * Collect all the MCi_XXX settings
> - */
> -static struct mce i_mce;
> +static bool hw_injection_possible = true;
> +
> +/* Collect all the MCi_XXX settings */
> +static struct inject_desc {
> +	struct mce m;
> +	int err;
> +} inj_desc;
> +
>   static struct dentry *dfs_inj;
>   
>   #define MAX_FLAG_OPT_SIZE	4
> @@ -110,9 +114,11 @@ static int inj_ipid_set(void *data, u64 val)
>   
>   DEFINE_SIMPLE_ATTRIBUTE(ipid_fops, inj_ipid_get, inj_ipid_set, "%llx\n");
>   
> -static void setup_inj_struct(struct mce *m)
> +static void setup_inj_struct(void)
>   {
> -	memset(m, 0, sizeof(struct mce));
> +	struct mce *m = &inj_desc.m;
> +
> +	memset(&inj_desc, 0, sizeof(struct inject_desc));
>   
>   	m->cpuvendor = boot_cpu_data.x86_vendor;
>   	m->time	     = ktime_get_real_seconds();
> @@ -470,84 +476,78 @@ static void toggle_nb_mca_mst_cpu(u16 nid)
>   		       __func__, PCI_FUNC(F3->devfn), NBCFG);
>   }
>   
> -struct mce_err_handler {
> -	struct mce *mce;
> -	int err;
> -};
> -
> -static struct mce_err_handler mce_err;
> -
> -static bool prepare_mca_status(struct mce *m)
> +static bool prepare_mca_status(void)
>   {
> -	u32 status_reg = mca_msr_reg(m->bank, MCA_STATUS);
> -	u64 status_val = m->status;
> +	u32 status_reg = mca_msr_reg(inj_desc.m.bank, MCA_STATUS);
> +	u64 status_val = inj_desc.m.status;
>   
>   	wrmsrl(status_reg, status_val);
>   	rdmsrl(status_reg, status_val);
>   
> -	return status_val;
> +	return status_val == inj_desc.m.status;
>   }
>   
> -static void prepare_msrs(void *info)
> +static void prepare_msrs(void *unused)
>   {
> -	struct mce_err_handler *i_mce_err = ((struct mce_err_handler *)info);
> -	struct mce m = *i_mce_err->mce;
> -	u8 b = m.bank;
> +	struct mce *m = &inj_desc.m;
> +	u8 b = inj_desc.m.bank;
>   
> -	if (!prepare_mca_status(&m)) {
> +	if (!prepare_mca_status()) {
>   		pr_err("Platform does not allow error injection, try using APEI EINJ instead.\n");
> -		i_mce_err->err = -EINVAL;
> +		inj_desc.err = -EINVAL;
> +		hw_injection_possible = false;
>   		return;
>   	}
>   
> -	wrmsrl(MSR_IA32_MCG_STATUS, m.mcgstatus);
> +	wrmsrl(MSR_IA32_MCG_STATUS, m->mcgstatus);
>   
>   	if (boot_cpu_has(X86_FEATURE_SMCA)) {
> -		if (m.inject_flags == DFR_INT_INJ) {
> -			wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m.status);
> -			wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m.addr);
> +		if (m->inject_flags == DFR_INT_INJ) {
> +			wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m->status);
> +			wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m->addr);
>   		} else {
> -			wrmsrl(MSR_AMD64_SMCA_MCx_STATUS(b), m.status);
> -			wrmsrl(MSR_AMD64_SMCA_MCx_ADDR(b), m.addr);
> +			wrmsrl(MSR_AMD64_SMCA_MCx_STATUS(b), m->status);
> +			wrmsrl(MSR_AMD64_SMCA_MCx_ADDR(b), m->addr);
>   		}
>   
> -		wrmsrl(MSR_AMD64_SMCA_MCx_MISC(b), m.misc);
> -		wrmsrl(MSR_AMD64_SMCA_MCx_SYND(b), m.synd);
> +		wrmsrl(MSR_AMD64_SMCA_MCx_MISC(b), m->misc);
> +		wrmsrl(MSR_AMD64_SMCA_MCx_SYND(b), m->synd);
>   	} else {
> -		wrmsrl(MSR_IA32_MCx_STATUS(b), m.status);
> -		wrmsrl(MSR_IA32_MCx_ADDR(b), m.addr);
> -		wrmsrl(MSR_IA32_MCx_MISC(b), m.misc);
> +		wrmsrl(MSR_IA32_MCx_STATUS(b), m->status);
> +		wrmsrl(MSR_IA32_MCx_ADDR(b), m->addr);
> +		wrmsrl(MSR_IA32_MCx_MISC(b), m->misc);
>   	}
>   }
>   
> -static void do_inject(void)
> +static int do_inject(void)
>   {
> +	struct mce *m = &inj_desc.m;
> +	unsigned int cpu = m->extcpu;
>   	u64 mcg_status = 0;
> -	unsigned int cpu = i_mce.extcpu;
> -	u8 b = i_mce.bank;
> +	u8 b = m->bank;
>   
> -	mce_err.mce = &i_mce;
> -	mce_err.err = 0;
> +	m->tsc = rdtsc_ordered();
>   
> -	i_mce.tsc = rdtsc_ordered();
> +	m->status |= MCI_STATUS_VAL;
>   
> -	i_mce.status |= MCI_STATUS_VAL;
> +	if (m->misc)
> +		m->status |= MCI_STATUS_MISCV;
>   
> -	if (i_mce.misc)
> -		i_mce.status |= MCI_STATUS_MISCV;
> -
> -	if (i_mce.synd)
> -		i_mce.status |= MCI_STATUS_SYNDV;
> +	if (m->synd)
> +		m->status |= MCI_STATUS_SYNDV;
>   
>   	if (inj_type == SW_INJ) {
> -		mce_log(&i_mce);
> -		return;
> +		mce_log(m);
> +		return 0;
>   	}
>   
> +	if (!hw_injection_possible)
> +		return -EINVAL;
> +
>   	/* prep MCE global settings for the injection */
>   	mcg_status = MCG_STATUS_MCIP | MCG_STATUS_EIPV;
>   
> -	if (!(i_mce.status & MCI_STATUS_PCC))
> +	if (!(m->status & MCI_STATUS_PCC))
>   		mcg_status |= MCG_STATUS_RIPV;
>   
>   	/*
> @@ -556,8 +556,8 @@ static void do_inject(void)
>   	 * - MCx_STATUS[UC] cleared: deferred errors are _not_ UC
>   	 */
>   	if (inj_type == DFR_INT_INJ) {
> -		i_mce.status |= MCI_STATUS_DEFERRED;
> -		i_mce.status &= ~MCI_STATUS_UC;
> +		m->status |= MCI_STATUS_DEFERRED;
> +		m->status &= ~MCI_STATUS_UC;
>   	}
>   
>   	/*
> @@ -578,13 +578,13 @@ static void do_inject(void)
>   
>   	toggle_hw_mce_inject(cpu, true);
>   
> -	i_mce.mcgstatus = mcg_status;
> -	i_mce.inject_flags = inj_type;
> -	smp_call_function_single(cpu, prepare_msrs, &mce_err, 0);
> +	m->mcgstatus = mcg_status;
> +	m->inject_flags = inj_type;
> +	smp_call_function_single(cpu, prepare_msrs, NULL, 0);
>   
>   	toggle_hw_mce_inject(cpu, false);
>   
> -	if (mce_err.err)
> +	if (inj_desc.err)
>   		goto err;
>   
>   	switch (inj_type) {
> @@ -601,6 +601,7 @@ static void do_inject(void)
>   err:
>   	cpus_read_unlock();
>   
> +	return inj_desc.err;
>   }
>   
>   /*
> @@ -611,6 +612,7 @@ static int inj_bank_set(void *data, u64 val)
>   {
>   	struct mce *m = (struct mce *)data;
>   	u8 n_banks;
> +	int err;
>   	u64 cap;
>   
>   	/* Get bank count on target CPU so we can handle non-uniform values. */
> @@ -650,12 +652,12 @@ static int inj_bank_set(void *data, u64 val)
>   	}
>   
>   inject:
> -	do_inject();
> +	err = do_inject();
>   
>   	/* Reset injection struct */
> -	setup_inj_struct(&i_mce);
> +	setup_inj_struct();
>   
> -	return mce_err.err;
> +	return err;
>   }
>   
>   MCE_INJECT_GET(bank);
> @@ -745,7 +747,7 @@ static void __init debugfs_init(void)
>   
>   	for (i = 0; i < ARRAY_SIZE(dfs_fls); i++)
>   		debugfs_create_file(dfs_fls[i].name, dfs_fls[i].perm, dfs_inj,
> -				    &i_mce, dfs_fls[i].fops);
> +				    &inj_desc.m, dfs_fls[i].fops);
>   }
>   
>   static int __init inject_init(void)
> @@ -758,7 +760,7 @@ static int __init inject_init(void)
>   	register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, "mce_notify");
>   	mce_register_injector_chain(&inject_nb);
>   
> -	setup_inj_struct(&i_mce);
> +	setup_inj_struct();
>   
>   	pr_info("Machine check injector initialized\n");
>   
> diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
> index 4ae0e603f7fa..7e03f5b7f6bd 100644
> --- a/arch/x86/kernel/cpu/mce/internal.h
> +++ b/arch/x86/kernel/cpu/mce/internal.h
> @@ -211,7 +211,7 @@ noinstr u64 mce_rdmsrl(u32 msr);
>   
>   static __always_inline u32 mca_msr_reg(int bank, enum mca_msr reg)
>   {
> -	if (mce_flags.smca) {
> +	if (cpu_feature_enabled(X86_FEATURE_SMCA)) {
Should this change be part of this patch series? Also, why mce_flags.smca
check won't work?

Thanks,
Smita
>   		switch (reg) {
>   		case MCA_CTL:	 return MSR_AMD64_SMCA_MCx_CTL(bank);
>   		case MCA_ADDR:	 return MSR_AMD64_SMCA_MCx_ADDR(bank);
>