Message-ID: <20240214113630.GIZcylvp6-m-FNNE7H@fat_crate.local>
Date: Wed, 14 Feb 2024 12:36:30 +0100
From: Borislav Petkov <bp@...en8.de>
To: Yazen Ghannam <yazen.ghannam@....com>
Cc: tony.luck@...el.com, linux-edac@...r.kernel.org,
	linux-kernel@...r.kernel.org, avadhut.naik@....com,
	john.allen@....com, muralidhara.mk@....com,
	naveenkrishna.chatradhi@....com, sathyapriya.k@....com
Subject: Re: [PATCH 2/2] RAS: Introduce the FRU Memory Poison Manager

On Tue, Feb 13, 2024 at 09:35:16PM -0600, Yazen Ghannam wrote:
> +static void update_fru_record(struct fru_rec *rec, struct mce *m)
> +{
> +	struct cper_sec_fru_mem_poison *fmp = get_fmp(rec);
> +	struct cper_fru_poison_desc fpd;
> +	u32 entry = 0;
> +
> +	mutex_lock(&fmpm_update_mutex);
> +
> +	init_fpd(&fpd, m);
> +
> +	/* This is the first entry, so just save it. */
> +	if (!has_valid_entries(fmp->validation_bits))
> +		goto save_fpd;

Not needed - if it is the first entry, it'll get saved there.

> +	/* Ignore already recorded errors. */
> +	if (is_dup_fpd(rec, &fpd))
> +		goto out_unlock;
> +
> +	if (fmp->nr_entries >= max_nr_entries) {
> +		pr_warn("Exceeded number of entries for FRU 0x%016llx", fmp->fru_id);
> +		goto out_unlock;
> +	}
> +
> +	entry = fmp->nr_entries;

..

> +static void retire_dram_row(u64 addr, u64 id, u32 cpu)
> +{
> +	struct atl_err a_err;

Yap, exactly, this should use atl_err and not struct mce.

> +
> +	memset(&a_err, 0, sizeof(struct atl_err));
> +
> +	a_err.addr = addr;
> +	a_err.ipid = id;
> +	a_err.cpu  = cpu;
> +
> +	amd_retire_dram_row(&a_err);
> +}
> +
> +static int fru_mem_poison_handler(struct notifier_block *nb, unsigned long val, void *data)
> +{
> +	struct mce *m = (struct mce *)data;
> +	struct fru_rec *rec;
> +
> +	if (!mce_is_memory_error(m))
> +		return NOTIFY_DONE;
> +
> +	retire_dram_row(m->addr, m->ipid, m->extcpu);
> +
> +	/*
> +	 * This should not happen on real errors. But it could happen from

What exactly is "This" here?

> +	 * software error injection, etc.
> +	 */
> +	rec = get_fru_record(m->ppin);
> +	if (!rec)
> +		return NOTIFY_DONE;
> +
> +	update_fru_record(rec, m);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block fru_mem_poison_nb = {
> +	.notifier_call  = fru_mem_poison_handler,
> +	.priority	= MCE_PRIO_LOWEST,
> +};
> +
> +static u32 get_cpu_from_fru_id(u64 fru_id)

Fold into the single callsite.

> +{
> +	unsigned int cpu = 0;
> +
> +	/* Should there be more robust error handling if none found? */
> +	for_each_online_cpu(cpu) {
> +		if (topology_ppin(cpu) == fru_id)
> +			break;
> +	}
> +
> +	return cpu;
> +}
> +
> +static void retire_mem_fmp(struct fru_rec *rec, u32 nr_entries, u32 cpu)
> +{
> +	struct cper_fru_poison_desc *fpd;
> +	unsigned int i;
> +
> +	for (i = 0; i < nr_entries; i++) {
> +		fpd = get_fpd(rec, i);
> +
> +		if (fpd->hw_id_type != FPD_HW_ID_TYPE_MCA_IPID)
> +			continue;
> +
> +		if (fpd->addr_type != FPD_ADDR_TYPE_MCA_ADDR)
> +			continue;
> +
> +		retire_dram_row(fpd->addr, fpd->hw_id, cpu);
> +	}
> +}
> +
> +static void retire_mem_records(void)
> +{
> +	struct cper_sec_fru_mem_poison *fmp;
> +	struct fru_rec *rec;
> +	unsigned int i;
> +	u32 cpu;
> +
> +	for_each_fru(i, rec) {
> +		fmp = get_fmp(rec);
> +
> +		if (!has_valid_entries(fmp->validation_bits))
> +			continue;
> +
> +		cpu = get_cpu_from_fru_id(fmp->fru_id);

Pass in that fmp thing into retire_dram_row() so that you can delay
that get_cpu_from_fru_id() call until the moment you actually need it.

> +static int save_new_records(void)
> +{
> +	struct fru_rec *rec;
> +	unsigned int i;
> +	int ret = 0;
> +
> +	for_each_fru(i, rec) {
> +		/* Skip restored records. Should these be fixed up? */

I don't understand that question.

> +		if (rec->hdr.record_length)
> +			continue;
> +
> +		set_rec_fields(rec);
> +
> +		ret = update_record_on_storage(rec);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
> +static bool is_valid_fmp(struct fru_rec *rec)

fmp_is_valid()

> +{
> +	struct cper_sec_fru_mem_poison *fmp = get_fmp(rec);
> +	u32 len = get_fmp_len(rec);
> +
> +	if (!fmp)
> +		return false;
> +
> +	if (!len)
> +		return false;
> +
> +	/* Checksum must sum to zero for the entire section. */
> +	if (do_fmp_checksum(fmp, len))
> +		return false;
> +
> +	if (!(fmp->validation_bits & FMP_VALID_ARCH_TYPE))
> +		return false;
> +
> +	if (fmp->fru_arch_type != FMP_ARCH_TYPE_X86_CPUID_1_EAX)
> +		return false;
> +
> +	if (!(fmp->validation_bits & FMP_VALID_ARCH))
> +		return false;
> +
> +	if (fmp->fru_arch != cpuid_eax(1))
> +		return false;
> +
> +	if (!(fmp->validation_bits & FMP_VALID_ID_TYPE))
> +		return false;
> +
> +	if (fmp->fru_id_type != FMP_ID_TYPE_X86_PPIN)
> +		return false;
> +
> +	if (!(fmp->validation_bits & FMP_VALID_ID))
> +		return false;
> +
> +	return true;
> +}
> +
> +static void restore_record(struct fru_rec *new, struct fru_rec *old)
> +{
> +	/* Records larger than max_rec_len were skipped earlier. */
> +	size_t len = min(max_rec_len, old->hdr.record_length);
> +
> +	memcpy(new, old, len);
> +}

Fold into the single call site.

> +
> +static bool valid_record(struct fru_rec *old)
> +{
> +	struct fru_rec *new;
> +
> +	if (!is_valid_fmp(old)) {
> +		pr_debug("Ignoring invalid record");
> +		return false;
> +	}
> +
> +	new = get_fru_record(old->fmp.fru_id);
> +	if (!new) {
> +		pr_debug("Ignoring record for absent FRU");
> +		return false;
> +	}
> +
> +	/* What if ERST has duplicate FRU entries? */
> +	restore_record(new, old);
> +
> +	return true;
> +}
> +
> +/*
> + * Fetch saved records from persistent storage.
> + *
> + * For each found record:
> + * - If it was not created by this module, then ignore it.
> + * - If it is valid, then copy its data to the local cache.
> + * - If it is not valid, then erase it.
> + */
> +static int get_saved_records(void)
> +{
> +	struct fru_rec *old;
> +	u64 record_id;
> +	int ret, pos;
> +	ssize_t len;
> +
> +	/*
> +	 * Assume saved records match current max size.
> +	 *
> +	 * However, this may not be true depending on module parameters.

This must work with module parameters, though. Or, as said and
preferrably, there should not be any module parameters at all.

> +	 */
> +	old = kmalloc(max_rec_len, GFP_KERNEL);
> +	if (!old) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = erst_get_record_id_begin(&pos);
> +	if (ret < 0)
> +		goto out_end;
> +
> +	while (!erst_get_record_id_next(&pos, &record_id)) {
> +		/*
> +		 * Make sure to clear temporary buffer between reads to avoid
> +		 * leftover data from records of various sizes.
> +		 */
> +		memset(old, 0, max_rec_len);
> +
> +		len = erst_read_record(record_id, &old->hdr, max_rec_len,
> +				       sizeof(struct fru_rec), &CPER_CREATOR_FMP);
> +
> +		/* Should this be retried if the temporary buffer is too small? */

Only when it turns out that it is necessary.

> +		if (len < 0)
> +			continue;
> +
> +		if (!valid_record(old))
> +			erst_clear(record_id);

Where is the check which ignores the record not created by this module?

Because this clears all records it deems not valid and that thing needs
to be really careful here and be sure what exactly it clears...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Powered by blists - more mailing lists