Hi Ben, Ben Cheatham wrote: > From: Yazen Ghannam <yazen.ghannam@xxxxxxx> > > V2 Changes: > - Added Link tags for links > - removed stray unused variable > > This patch is a follow up to the discussion at [1], and builds on Tony's > CXL error patch at [2]. > > The new CXL error types will use the Memory Address field in the > SET_ERROR_TYPE_WITH_ADDRESS structure in order to target a CXL 1.1 > compliant memory-mapped Downstream port. The value of the Memory Address > will be in the port's MMIO range, and it will not represent physical > (normal or persistent) memory. > > Allow error injection for CXL 1.1 systems by skipping memory range > validation for CXL error injection types. This just feels a bit too loose especially when the kernel has the cxl_acpi driver to perform the enumeration of CXL root ports. I know that Terry and Robert are teaching the PCI AER core how to coordinate with RCRB information [1] (I still need to go dig in deeper on that set). I would expect ACPI EINJ could benefit from similar coordination and validate these addresses. Now, is it any address in the downstream-port RCRB range that is valid, or only the base? Another minor comment below... [1]: http://lore.kernel.org/r/20230523232214.55282-1-terry.bowman@xxxxxxx > > Output trying to inject CXL.mem error without patch: > > -bash: echo: write error: Invalid argument > > [1]: > Link: https://lore.kernel.org/linux-acpi/20221206205234.606073-1-Benjamin.Cheatham@xxxxxxx/ > [2]: > Link: https://lore.kernel.org/linux-cxl/CAJZ5v0hNQUfWViqxbJ5B4JCGJUuHpWWSpqpCFWPNpGuagoFbsQ@xxxxxxxxxxxxxx/T/#t > > Signed-off-by: Yazen Ghannam <yazen.ghannam@xxxxxxx> > Signed-off-by: Ben Cheatham <benjamin.cheatham@xxxxxxx> > --- > drivers/acpi/apei/einj.c | 12 +++++++++++- > include/acpi/actbl1.h | 6 ++++++ > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c > index 013eb621dc92..68a20326ed7c 100644 > --- a/drivers/acpi/apei/einj.c > +++ b/drivers/acpi/apei/einj.c > @@ -37,6 +37,13 @@ > ACPI_EINJ_MEMORY_UNCORRECTABLE | \ > ACPI_EINJ_MEMORY_FATAL) > > +#define CXL_ERROR_MASK (ACPI_EINJ_CXL_CACHE_CORRECTABLE | \ > + ACPI_EINJ_CXL_CACHE_UNCORRECTABLE | \ > + ACPI_EINJ_CXL_CACHE_FATAL | \ > + ACPI_EINJ_CXL_MEM_CORRECTABLE | \ > + ACPI_EINJ_CXL_MEM_UNCORRECTABLE | \ > + ACPI_EINJ_CXL_MEM_FATAL) > + > /* > * ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action. > */ > @@ -537,8 +544,11 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, > if (type & ACPI5_VENDOR_BIT) { > if (vendor_flags != SETWA_FLAGS_MEM) > goto inject; > - } else if (!(type & MEM_ERROR_MASK) && !(flags & SETWA_FLAGS_MEM)) > + } else if (!(type & MEM_ERROR_MASK) && !(flags & SETWA_FLAGS_MEM)) { > + goto inject; > + } else if (type & CXL_ERROR_MASK) { > goto inject; > + } > > /* > * Disallow crazy address masks that give BIOS leeway to pick > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h > index 81b9e794424d..c39837266414 100644 > --- a/include/acpi/actbl1.h > +++ b/include/acpi/actbl1.h > @@ -1044,6 +1044,12 @@ enum acpi_einj_command_status { > #define ACPI_EINJ_PLATFORM_CORRECTABLE (1<<9) > #define ACPI_EINJ_PLATFORM_UNCORRECTABLE (1<<10) > #define ACPI_EINJ_PLATFORM_FATAL (1<<11) > +#define ACPI_EINJ_CXL_CACHE_CORRECTABLE BIT(12) > +#define ACPI_EINJ_CXL_CACHE_UNCORRECTABLE BIT(13) > +#define ACPI_EINJ_CXL_CACHE_FATAL BIT(14) > +#define ACPI_EINJ_CXL_MEM_CORRECTABLE BIT(15) > +#define ACPI_EINJ_CXL_MEM_UNCORRECTABLE BIT(16) > +#define ACPI_EINJ_CXL_MEM_FATAL BIT(17) I expect these to come from the next ACPICA update just like the other definitions. The change in style from (x<<N) to BIT(N) was a tip-off. The process is to submit a pull request to the ACPICA project, for example: https://github.com/acpica/acpica/commit/e948142526c0
