Received-SPF: pass (google.com: domain of linux-kernel+bounces-186518-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <e58a144a-c4f6-4d20-9ef9-56ee225c31db@amd.com>
Date: Wed, 22 May 2024 11:50:40 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v2 4/8] ACPI: APEI: EINJ: Enable the discovery of
 EINJv2 capabilities
To: Zaid Alali <zaidal@os.amperecomputing.com>, rafael@kernel.org,
 lenb@kernel.org, james.morse@arm.com, tony.luck@intel.com, bp@alien8.de,
 robert.moore@intel.com, Jonathan.Cameron@huawei.com,
 dan.j.williams@intel.com, arnd@arndb.de, Avadhut.Naik@amd.com,
 u.kleine-koenig@pengutronix.de, john.allen@amd.com,
 linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
 acpica-devel@lists.linux.dev
References: <20240521211036.227674-1-zaidal@os.amperecomputing.com>
 <20240521211036.227674-5-zaidal@os.amperecomputing.com>
Content-Language: en-US
From: Ben Cheatham <benjamin.cheatham@amd.com>
In-Reply-To: <20240521211036.227674-5-zaidal@os.amperecomputing.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?c2RiQVlSbHRuc0ZhYVVUdXZYaXNYSWJkSFF0YnhKaVloRVptMUV4bkJkdm03?=
 =?utf-8?B?QVczRkg5Ny9EK3dpbWZ3TDR3NlloOTZTYmJUK1BQVG9BUm0vTTNYYjJEU0dB?=
 =?utf-8?B?YXNRUEljYUg4QUc2T2NiQlZXV3UxS3U1ZUhhZEs0ZkJJOGNqK0Y1NkJDRmts?=
 =?utf-8?B?QXJ2S3lXSFBGRHFwNEljRVNiSTdMeDRFZEVlQnA3b2JLeEVsRlZOSTk4Qmg2?=
 =?utf-8?B?cmxadTg4N1NyWXlxL0dsR3ZuRzB5VmF6TWY5VWFySVIvTlR4dG14TXFEMmVI?=
 =?utf-8?B?L2NobWJQTjJrZEI0MGx1cUlWaC9QVEUwNjJkdVZNNWV6cDE2UG1DNG1kZFhY?=
 =?utf-8?B?aGFiQWdraUJUbktyV3JFSTluMWdxU2g3cHJxR3NNQ01zVk9LWnRCUHBRRTM5?=
 =?utf-8?B?KzFDdERSNEpUTWcweWdZNHRKQUFOSStWem9CZFBONWdLTGZINWkzUFZwYnhG?=
 =?utf-8?B?NlRkNjVPZ3QwTlhFK3hnWXRjZzgycUxhdnhlenk2OFJrVHYxN1NVMTFFUCtH?=
 =?utf-8?B?NVlaZ3NyZjRLTmdSSHVaYjhhL0hzQ0hZa1Ezd1VMcVNBZXFnSlJqWHl0MTl6?=
 =?utf-8?B?YW4xM21GbmpUbDAvMmIvUytHTVNVQzZ4WUs4aENPWEdhcno2a1NNUkxHQmk1?=
 =?utf-8?B?S1QyTmdjODZHMEJxZEFDWTNIOE1SWWZFUWdXWmcxOVdzWHQ3NFQ1UmRKb2NF?=
 =?utf-8?B?ZnpNaVRVd0QyUm90WlpRR1UxZUhCSFo0OU5KWk8vWktxUmVUVkM3emZQclRL?=
 =?utf-8?B?V2htd0NEamFZYzBtWERlbXFhNDFQSlp0bjdYUWVGd1hSMXp5SHdvZm1EdFd3?=
 =?utf-8?B?Y1cwTnppbkVtY210djVhZGRCa1BHdHhXZFovNDRUc05la3ZUN3pKT1dVUEIz?=
 =?utf-8?B?cE9FOW1XT25tejNxeldpYmMwbDltc2JnT2x3NmdmSzVwazg4STU0SXQ1aXVX?=
 =?utf-8?B?U3dpK2ptcjVWclBIWEZtUUZmc1dkc1pmL1l1dldyQW1SQXVNZHBuSEVTT3hF?=
 =?utf-8?B?TzFIV0ljRHp2RjMxMlQrNFc1MDdtUU9IR2lOTU9uVjVNeFF3UHFkZ3Z5NXBX?=
 =?utf-8?B?dE5zMkYxaGNpaXVKakJpTVlMdHU0cjJqQWR6QnZ6Q0IrVWQ5ZFUzdmZXRUU3?=
 =?utf-8?B?Rzd3UStmYjFrWXpnK3VHRjNnZTZUYmdxUnRMYlJ1eWszWUJsTHFRN3NJazgx?=
 =?utf-8?B?RFZnU1ZKVTZ2eEFiQmFBSi9pZ1k3VDNLNm83M252MEM1c1ZTOVRJN1A3aG9K?=
 =?utf-8?B?ZGFTRUR1bDhxUHcxUzc0TDNGVkt0RzlmZitjTUluc052VUZ5aC9CeVM5N0ti?=
 =?utf-8?B?RE11VVNMTDVtamhWTHZvaGFGTkthZWN5c2dEUGo3K3dPQXZaT2dKdUdWMVVX?=
 =?utf-8?B?NUp6OGRNSi9HRnpoc1FaTnlDS2JxdktxMy9CNnphVEZJVmtRWnZId09qMktB?=
 =?utf-8?B?aDhHcEM2TndsN014djM4Z3pHOVNDc3EyczhPcXU5SVJIRnJkaHZCdUloSWMv?=
 =?utf-8?B?UXVmelVsYmhTSEJKNUVTQmQ3clMxZktpRGptbUsyNmovY05tRXFVTW1GNWN1?=
 =?utf-8?B?b21pRnFYcWplRDlrRzFtUFVVMDc3V29WeEowamsrLy92MTFMblpsRlR4Unlq?=
 =?utf-8?B?bVcwa1NUZ3VjazZmNW5wQ3RyOFN2bmsvSjlRWFN0SlRjQjczbjhDNDBYdU4z?=
 =?utf-8?B?UjVxckRUYlNWQ3dOdlBlUHZPSERFVFA4VTA1L0I3UlIxTTBNZWUzMm5MdTFt?=
 =?utf-8?B?Vk5QUUh2TVc0aGNFOW5mMThiTUpuS0RWZnNzNzFINElYZ0R2MUhHQXh2bU9X?=
 =?utf-8?B?ZTRiMzdtRWo4dzcrTFIySWR3eGpPVnE3d1N2VVRLNVJ4NVdESzdEaFdVSlFn?=
 =?utf-8?B?Q2l5a0EyclY0QndHcGV3VTRqbUU3WmllYXE2T3drZGdzQzNOZjIvVTgvUVBz?=
 =?utf-8?B?d2NqMzdoRytqS0RqeHlQM05OOUVEMVhRZ2IvWURzZkhoUVV5VTlWajg3TDEr?=
 =?utf-8?B?RE9vWS9YSVNDelFwM1N5c3cxUmRCTlRheGtUcE0yRmFwRldYRVIveVJ5T2JI?=
 =?utf-8?B?SXdUZTZUR1AzZm1JQmFacXRhNkc3Y2NUOTQ2U3h1NEhLTjZRNW5oeXNaWGdT?=
 =?utf-8?Q?Cca7n7ta6aRw8gUIVPfvH5iCt?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 492b8d3b-f355-4e2f-517d-08dc7a7f5347
X-MS-Exchange-CrossTenant-AuthSource: CH3PR12MB8535.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2024 16:50:45.7799
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /ZDeOJIacpvtnn7gqkKKdlsVM2zXjFAKKOiWZLdP6DfseDwFnfbqbrA0aHYYV2KQIagO3Lo9uDGouWpcgsqCag==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR12MB6570

Hi Zaid,

I've got comments inline with a couple (mostly little) concerns, but this looks really good so far!

On 5/21/24 4:10 PM, Zaid Alali wrote:
> Enable the driver to show all supported error injections for EINJ
> and EINJv2 at the same time. EINJv2 capabilities can be discovered
> by checking the return value of get_error_type, where bit 30 set
> indicates EINJv2 support.
> 
> Signed-off-by: Zaid Alali <zaidal@os.amperecomputing.com>
> ---
>  drivers/acpi/apei/apei-internal.h |  2 +-
>  drivers/acpi/apei/einj-core.c     | 35 ++++++++++++++++++++++++-------
>  drivers/acpi/apei/einj-cxl.c      |  2 +-
>  3 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
> index cd2766c69d78..9a3dbaeed39a 100644
> --- a/drivers/acpi/apei/apei-internal.h
> +++ b/drivers/acpi/apei/apei-internal.h
> @@ -131,7 +131,7 @@ static inline u32 cper_estatus_len(struct acpi_hest_generic_status *estatus)
>  
>  int apei_osc_setup(void);
>  
> -int einj_get_available_error_type(u32 *type);
> +int einj_get_available_error_type(u32 *type, int version);
>  int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
>  		      u64 param4);
>  int einj_cxl_rch_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
> diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
> index b1bbbee9c664..cc5ad1f45ea4 100644
> --- a/drivers/acpi/apei/einj-core.c
> +++ b/drivers/acpi/apei/einj-core.c
> @@ -33,6 +33,7 @@
>  #define SLEEP_UNIT_MAX		5000			/* 5ms */
>  /* Firmware should respond within 1 seconds */
>  #define FIRMWARE_TIMEOUT	(1 * USEC_PER_SEC)
> +#define ACPI65_EINJV2_SUPP	BIT(30)
>  #define ACPI5_VENDOR_BIT	BIT(31)
>  #define MEM_ERROR_MASK		(ACPI_EINJ_MEMORY_CORRECTABLE | \
>  				ACPI_EINJ_MEMORY_UNCORRECTABLE | \
> @@ -84,6 +85,7 @@ static struct debugfs_blob_wrapper vendor_errors;
>  static char vendor_dev[64];
>  
>  static u32 available_error_type;
> +static u32 available_error_type_v2;
>  
>  /*
>   * Some BIOSes allow parameters to the SET_ERROR_TYPE entries in the
> @@ -159,13 +161,13 @@ static void einj_exec_ctx_init(struct apei_exec_context *ctx)
>  			   EINJ_TAB_ENTRY(einj_tab), einj_tab->entries);
>  }
>  
> -static int __einj_get_available_error_type(u32 *type)
> +static int __einj_get_available_error_type(u32 *type, int version)
>  {
>  	struct apei_exec_context ctx;
>  	int rc;
>  
>  	einj_exec_ctx_init(&ctx);
> -	rc = apei_exec_run(&ctx, ACPI_EINJ_GET_ERROR_TYPE);
> +	rc = apei_exec_run(&ctx, version);
>  	if (rc)
>  		return rc;
>  	*type = apei_exec_ctx_get_output(&ctx);
> @@ -174,12 +176,12 @@ static int __einj_get_available_error_type(u32 *type)
>  }
>  
>  /* Get error injection capabilities of the platform */
> -int einj_get_available_error_type(u32 *type)
> +int einj_get_available_error_type(u32 *type, int version)
>  {
>  	int rc;
>  
>  	mutex_lock(&einj_mutex);
> -	rc = __einj_get_available_error_type(type);
> +	rc = __einj_get_available_error_type(type, version);
>  	mutex_unlock(&einj_mutex);
>  
>  	return rc;
> @@ -647,15 +649,27 @@ static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>  	{ BIT(11), "Platform Uncorrectable fatal"},
>  	{ BIT(31), "Vendor Defined Error Types" },
>  };
> +static struct { u32 mask; const char *str; } const einjv2_error_type_string[] = {
> +	{ BIT(0), "EINJV2 Processor Error" },
> +	{ BIT(1), "EINJV2 Memory Error" },
> +	{ BIT(2), "EINJV2 PCI Express Error" },
> +};
>  
>  static int available_error_type_show(struct seq_file *m, void *v)
>  {
>  
> +	seq_printf(m, "EINJ error types:\n");
>  	for (int pos = 0; pos < ARRAY_SIZE(einj_error_type_string); pos++)
>  		if (available_error_type & einj_error_type_string[pos].mask)
>  			seq_printf(m, "0x%08x\t%s\n", einj_error_type_string[pos].mask,
> -				   einj_error_type_string[pos].str);
> -
> +					   einj_error_type_string[pos].str);
> +	if (available_error_type & ACPI65_EINJV2_SUPP) {
> +		seq_printf(m, "EINJv2 error types:\n");

I think this print and the added one above are not needed since the EINJv2 error type
strings have EINJV2 in them already.

> +		for (int pos = 0; pos < ARRAY_SIZE(einjv2_error_type_string); pos++)
> +			if (available_error_type_v2 & einjv2_error_type_string[pos].mask)
> +				seq_printf(m, "0x%08x\t%s\n", einjv2_error_type_string[pos].mask,
> +						   einjv2_error_type_string[pos].str);
> +	}
>  	return 0;
>  }
>  
> @@ -692,7 +706,7 @@ int einj_validate_error_type(u64 type)
>  	if (tval & (tval - 1))
>  		return -EINVAL;
>  	if (!vendor)
> -		if (!(type & available_error_type))
> +		if (!(type & (available_error_type | available_error_type_v2)))
>  			return -EINVAL;

I don't think this will work? Take the following scenario:

available_error_type = 0x2
available_error_type_v2 = 0x1

If I specify an error type of 0x1 and then inject a EINJv1 error I will have
injected an invalid error type, but still have passed the validation check.
I think you can just get rid of the check for the EINJv2 type here since you also
check it before the actual injection in patch 7/8.

>  
>  	return 0;
> @@ -769,9 +783,14 @@ static int __init einj_probe(struct platform_device *pdev)
>  		goto err_put_table;
>  	}
>  
> -	rc = einj_get_available_error_type(&available_error_type);
> +	rc = einj_get_available_error_type(&available_error_type, ACPI_EINJ_GET_ERROR_TYPE);
>  	if (rc)
>  		return rc;
> +	if (available_error_type & ACPI65_EINJV2_SUPP) {
> +		rc = einj_get_available_error_type(&available_error_type_v2, ACPI_EINJV2_GET_ERROR_TYPE);
> +		if (rc)
> +			return rc;
> +	}
>  
>  	rc = -ENOMEM;
>  	einj_debug_dir = debugfs_create_dir("einj", apei_get_debugfs_dir());
> diff --git a/drivers/acpi/apei/einj-cxl.c b/drivers/acpi/apei/einj-cxl.c
> index 8b8be0c90709..25adc9b03d18 100644
> --- a/drivers/acpi/apei/einj-cxl.c
> +++ b/drivers/acpi/apei/einj-cxl.c
> @@ -30,7 +30,7 @@ int einj_cxl_available_error_type_show(struct seq_file *m, void *v)
>  	int cxl_err, rc;
>  	u32 available_error_type = 0;
>  
> -	rc = einj_get_available_error_type(&available_error_type);
> +	rc = einj_get_available_error_type(&available_error_type, ACPI_EINJ_GET_ERROR_TYPE);
>  	if (rc)
>  		return rc;
>