RE: [PATCH v2] ACPI, APEI, EINJ: Remove memory range validation for CXL error types

[Dan Williams <dan.j.williams@intel.com>]

Hi Ben,

Ben Cheatham wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> 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@amd.com

> 
> 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@amd.com/
> [2]:
> Link: https://lore.kernel.org/linux-cxl/CAJZ5v0hNQUfWViqxbJ5B4JCGJUuHpWWSpqpCFWPNpGuagoFbsQ@mail.gmail.com/T/#t
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Signed-off-by: Ben Cheatham <benjamin.cheatham@amd.com>
> ---
>  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