Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver

From: Mark Rutland
Date: Wed Nov 24 2021 - 13:51:41 EST

Next message: Mathieu Poirier: "Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device"
Previous message: Sean Christopherson: "Re: [PATCH v2] KVM: selftests: Make sure kvm_create_max_vcpus test won't hit RLIMIT_NOFILE"
In reply to: Darren Hart: "Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver"
Next in thread: Tyler Baicar: "[PATCH 2/2] trace, ras: add ARM RAS extension trace event"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

I haven't looked at this in great detail, but I spotted a few issues
from an initial scan.

On Wed, Nov 24, 2021 at 12:07:07PM -0500, Tyler Baicar wrote:
> Add support for parsing the ARM Error Source Table and basic handling of
> errors reported through both memory mapped and system register interfaces.
>
> Assume system register interfaces are only registered with private
> peripheral interrupts (PPIs); otherwise there is no guarantee the
> core handling the error is the core which took the error and has the
> syndrome info in its system registers.

Can we actually assume that? What does the specification mandate?

> Add logging for all detected errors and trigger a kernel panic if there is
> any uncorrected error present.

Has this been tested on any hardware or software platform?

[...]

> +#define ERRDEVARCH_REV_SHIFT 0x16

IIUC This should be 16, not 0x16 (i.e. 22).

> +#define ERRDEVARCH_REV_MASK 0xf
> +
> +#define RAS_REV_v1_1 0x1
> +
> +struct ras_ext_regs {
> + u64 err_fr;
> + u64 err_ctlr;
> + u64 err_status;
> + u64 err_addr;
> + u64 err_misc0;
> + u64 err_misc1;
> + u64 err_misc2;
> + u64 err_misc3;
> +};

These last four might be better an an array.

[...]

> +static bool ras_extn_v1p1(void)
> +{
> + unsigned long fld, reg = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
> +
> + fld = cpuid_feature_extract_unsigned_field(reg, ID_AA64PFR0_RAS_SHIFT);
> +
> + return fld >= ID_AA64PFR0_RAS_V1P1;
> +}

I suspect it'd be better to pass this value around directly as
`version`, rather than dropping this into a `misc23_present` temporary
variable, as that would be a little clearer, and future-proof if/when
more registers get added.

[...]

> +void arch_arm_ras_report_error(u64 implemented, bool clear_misc)
> +{
> + struct ras_ext_regs regs = {0};
> + unsigned int i, cpu_num;
> + bool misc23_present;
> + bool fatal = false;
> + u64 num_records;
> +
> + if (!this_cpu_has_cap(ARM64_HAS_RAS_EXTN))
> + return;
> +
> + cpu_num = get_cpu();

Why get_cpu() here? Do you just need smp_processor_id()?

The commit message explained that this would be PE-local (e.g. in a PPI
handler), and we've already checked this_cpu_has_cap() which assumes
we're not preemptible.

So I don't see why we should use get_cpu() here -- any time it would
have a difference implies something has already gone wrong.

> + num_records = read_sysreg_s(SYS_ERRIDR_EL1) & ERRIDR_NUM_MASK;
> +
> + for (i = 0; i < num_records; i++) {
> + if (!(implemented & BIT(i)))
> + continue;
> +
> + write_sysreg_s(i, SYS_ERRSELR_EL1);
> + isb();
> + regs.err_status = read_sysreg_s(SYS_ERXSTATUS_EL1);
> +
> + if (!(regs.err_status & ERR_STATUS_V))
> + continue;
> +
> + pr_err("error from processor 0x%x\n", cpu_num);

Why in hex? We normally print 'cpu%d' or 'CPU%d', since this is a
logical ID anyway.

> +
> + if (regs.err_status & ERR_STATUS_AV)
> + regs.err_addr = read_sysreg_s(SYS_ERXADDR_EL1);
> +
> + misc23_present = ras_extn_v1p1();

As above, I reckon it's better to have this as 'version' or
'ras_version', and have the checks below be:

if (version >= ID_AA64PFR0_RAS_V1P1) {
// poke SYS_ERXMISC2_EL1
// poke SYS_ERXMISC3_EL1
}

> +
> + if (regs.err_status & ERR_STATUS_MV) {
> + regs.err_misc0 = read_sysreg_s(SYS_ERXMISC0_EL1);
> + regs.err_misc1 = read_sysreg_s(SYS_ERXMISC1_EL1);
> +
> + if (misc23_present) {
> + regs.err_misc2 = read_sysreg_s(SYS_ERXMISC2_EL1);
> + regs.err_misc3 = read_sysreg_s(SYS_ERXMISC3_EL1);
> + }
> + }
> +
> + arch_arm_ras_print_error(&regs, i, misc23_present);
> +
> + /*
> + * In the future, we will treat UER conditions as potentially
> + * recoverable.
> + */
> + if (regs.err_status & ERR_STATUS_UE)
> + fatal = true;
> +
> + regs.err_status = arch_arm_ras_get_status_clear_value(regs.err_status);
> + write_sysreg_s(regs.err_status, SYS_ERXSTATUS_EL1);
> +
> + if (clear_misc) {
> + write_sysreg_s(0x0, SYS_ERXMISC0_EL1);
> + write_sysreg_s(0x0, SYS_ERXMISC1_EL1);
> +
> + if (misc23_present) {
> + write_sysreg_s(0x0, SYS_ERXMISC2_EL1);
> + write_sysreg_s(0x0, SYS_ERXMISC3_EL1);
> + }
> + }

Any reason not to clear when we read, above? e.g.

#define READ_CLEAR_MISC(nr, clear) \
({ \
unsigned long __val = read_sysreg_s(SYS_ERXMISC##nr##_EL1); \
if (clear); \
write_sysreg_s(0, SYS_ERXMISC##nr##_EL1); \
__val; \
})

if (regs.err_status & ERR_STATUS_MV) {
regs.err_misc0 = READ_CLEAR_MISC(0, clear_misc);
regs.err_misc1 = READ_CLEAR_MISC(1, clear_misc);

if (version >= ID_AA64PFR0_RAS_V1P1) {
regs.err_misc2 = READ_CLEAR_MISC(2, clear_misc);
regs.err_misc3 = READ_CLEAR_MISC(3, clear_misc);
}

}

... why does the clearing need to be conditional?

> +
> + isb();
> + }
> +
> + if (fatal)
> + panic("ARM RAS: uncorrectable error encountered");
> +
> + put_cpu();
> +}
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index e3ec1a44f94d..dc15e9896db4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1573,6 +1573,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> { SYS_DESC(SYS_ERXADDR_EL1), trap_raz_wi },
> { SYS_DESC(SYS_ERXMISC0_EL1), trap_raz_wi },
> { SYS_DESC(SYS_ERXMISC1_EL1), trap_raz_wi },
> + { SYS_DESC(SYS_ERXMISC2_EL1), trap_raz_wi },
> + { SYS_DESC(SYS_ERXMISC3_EL1), trap_raz_wi },

This should be a preparatory patch; this is preumably a latent bug in
KVM.

[...]

> +static struct aest_node_data __percpu **ppi_data;
> +static int ppi_irqs[AEST_MAX_PPI];
> +static u8 num_ppi;
> +static u8 ppi_idx;

As above, do we have any guarantee these are actually PPIs?

> +static bool aest_mmio_ras_misc23_present(u64 base_addr)
> +{
> + u32 val;
> +
> + val = readl((void *) (base_addr + ERRDEVARCH_OFFSET));
> + val <<= ERRDEVARCH_REV_SHIFT;
> + val &= ERRDEVARCH_REV_MASK;
> +
> + return val >= RAS_REV_v1_1;
> +}

Is the shift the wrong way around?

Above we have:

#define ERRDEVARCH_REV_SHIFT 0x16
#define ERRDEVARCH_REV_MASK 0xf

#define RAS_REV_v1_1 0x1

.. so this is:

val <<= 0x16;
val &= 0xf; // val[0x15:0] == 0, so this is 0

return val >= 0x1; // false

It'd be nicer to use FIELD_GET() here.

As above, I also think it would be better to retrun the value of the
field, and check that explciitly, for future proofing.

[...]

> +static void aest_proc(struct aest_node_data *data)
> +{
> + struct ras_ext_regs *regs_p, regs = {0};
> + bool misc23_present;
> + bool fatal = false;
> + u64 errgsr = 0;
> + int i;
> +
> + /*
> + * Currently SR based handling is done through the architected
> + * discovery exposed through SRs. That may change in the future
> + * if there is supplemental information in the AEST that is
> + * needed.
> + */
> + if (data->interface.type == ACPI_AEST_NODE_SYSTEM_REGISTER) {
> + arch_arm_ras_report_error(data->interface.implemented,
> + data->interface.flags & AEST_INTERFACE_CLEAR_MISC);
> + return;
> + }
> +
> + regs_p = data->interface.regs;
> + errgsr = readq((void *) (((u64) regs_p) + ERRGSR_OFFSET));
> +
> + for (i = data->interface.start; i < data->interface.end; i++) {
> + if (!(data->interface.implemented & BIT(i)))
> + continue;
> +
> + if (!(data->interface.status_reporting & BIT(i)) && !(errgsr & BIT(i)))
> + continue;
> +
> + regs.err_status = readq(&regs_p[i].err_status);
> + if (!(regs.err_status & ERR_STATUS_V))
> + continue;
> +
> + if (regs.err_status & ERR_STATUS_AV)
> + regs.err_addr = readq(&regs_p[i].err_addr);
> +
> + regs.err_fr = readq(&regs_p[i].err_fr);
> + regs.err_ctlr = readq(&regs_p[i].err_ctlr);
> +
> + if (regs.err_status & ERR_STATUS_MV) {
> + misc23_present = aest_mmio_ras_misc23_present((u64) regs_p);
> + regs.err_misc0 = readq(&regs_p[i].err_misc0);
> + regs.err_misc1 = readq(&regs_p[i].err_misc1);
> +
> + if (misc23_present) {
> + regs.err_misc2 = readq(&regs_p[i].err_misc2);
> + regs.err_misc3 = readq(&regs_p[i].err_misc3);
> + }
> + }
> +
> + aest_print(data, regs, i, misc23_present);
> +
> + if (regs.err_status & ERR_STATUS_UE)
> + fatal = true;
> +
> + regs.err_status = arch_arm_ras_get_status_clear_value(regs.err_status);
> + writeq(regs.err_status, &regs_p[i].err_status);
> +
> + if (data->interface.flags & AEST_INTERFACE_CLEAR_MISC) {
> + writeq(0x0, &regs_p[i].err_misc0);
> + writeq(0x0, &regs_p[i].err_misc1);
> +
> + if (misc23_present) {
> + writeq(0x0, &regs_p[i].err_misc2);
> + writeq(0x0, &regs_p[i].err_misc3);
> + }
> + }
> + }
> +
> + if (fatal)
> + panic("AEST: uncorrectable error encountered");

Why don't we call panic() as soon as we realise an error is fatal?

Thanks,
Mark.

Next message: Mathieu Poirier: "Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device"
Previous message: Sean Christopherson: "Re: [PATCH v2] KVM: selftests: Make sure kvm_create_max_vcpus test won't hit RLIMIT_NOFILE"
In reply to: Darren Hart: "Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver"
Next in thread: Tyler Baicar: "[PATCH 2/2] trace, ras: add ARM RAS extension trace event"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]