Received-SPF: pass (google.com: domain of linux-kernel+bounces-188013-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Subject: Re: [PATCH 2/4] acpi/ghes, efi/cper: Recognize and process CXL
 Protocol Errors.
To: Dave Jiang <dave.jiang@intel.com>, linux-efi@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-cxl@vger.kernel.org
Cc: Ard Biesheuvel <ardb@kernel.org>,
 Alison Schofield <alison.schofield@intel.com>,
 Vishal Verma <vishal.l.verma@intel.com>, Ira Weiny <ira.weiny@intel.com>,
 Dan Williams <dan.j.williams@intel.com>,
 Jonathan Cameron <Jonathan.Cameron@huawei.com>,
 Yazen Ghannam <yazen.ghannam@amd.com>, Bowman Terry <terry.bowman@amd.com>
References: <20240522150839.27578-1-Smita.KoralahalliChannabasappa@amd.com>
 <20240522150839.27578-3-Smita.KoralahalliChannabasappa@amd.com>
 <8470dcc5-fbd4-4977-b8e8-5f7bfbf4b220@intel.com>
From: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Message-ID: <08dc0027-371c-5783-fd65-ad6f8b228fee@amd.com>
Date: Thu, 23 May 2024 14:19:51 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
In-Reply-To: <8470dcc5-fbd4-4977-b8e8-5f7bfbf4b220@intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
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?MzYvdVFIYmE3dzNqZU5qNkFmZ2pYTG54dHQ0N2ZhTXlyWjJveEFpQmZ6MXVo?=
 =?utf-8?B?dWcvcTdWSGdGVmtsSE1BYjI4K0JCaTUvZk5tQ0lKcThVT3A2ZmpEakgxbmhp?=
 =?utf-8?B?dUFtWDdyLytBbkYzWXVWZDJLUldsL0VxSTA1eTErdVVRc09FNlQvZ3lmd3d6?=
 =?utf-8?B?MjNzN3RKMTBaSlNzSGMxWm5BdHRTTUNUMUZFczc5NVVwOTZ3YnFzT1BZT2c2?=
 =?utf-8?B?YXE0MXFSRFNyTkhaUzBRMG0rTkFpblVrcGUxajhnOE5idVM5NTNvZWVzeXNC?=
 =?utf-8?B?dVBBQjJmUU9LQS9saTdPYTdFU3JVc0VUVnp0K0ZnR3o5V2QybW9TUDVIdXlI?=
 =?utf-8?B?bmRQN3ZCM0ZIL1JMQU1hR1ZHY3dhTlczZFlRempPMmJwVU44ZnlmWTZ6dWFT?=
 =?utf-8?B?ZlhCK2RUcHRYUWRTTUplMno3RlYzMWVOVkQxZGdCc0pDOFZMMUMreHhNZWdY?=
 =?utf-8?B?NVFzUDd2NVYvQ3V1Q2ZJZkExTm5yZkI3OUY5Q2poOUlZNXVKYk40MzB0cmkv?=
 =?utf-8?B?bHhhUXRZS2d0VDFDMEQ5RG5scyt1QzIweEVIamdKUUFiSVY3WHlKdkpmTGFM?=
 =?utf-8?B?cGY3WGdpSmZRaGd3Vjh6NzB1Q3BzU2RFTGh1bDZ1WGh5b0tkRkhWZGFSNE9P?=
 =?utf-8?B?NjhNNVp5czdWeTZNVlFxOW9lRXA5QmJOZlc4eHZHdnpFK0NhMS9qV3lIWU5W?=
 =?utf-8?B?MHFVbGhhbnZKcVJ3Q01wODhYSUFmQ3FHK0ZGYi9yMmhadHVJK3NwMU9wdXFl?=
 =?utf-8?B?YjNXRkpXOVZvN3JmZVVlbTdOdzRkVzJoT0RaVkRONnJyVDYwdTRyMkxHV1FR?=
 =?utf-8?B?Y2ZNUzNxdmhwQVV1TGtYdjlVYTFHVG5GNmVrVGcxeWVGU2k3MjBnWDNIRUVJ?=
 =?utf-8?B?Y0RIc3NZWFV3aUJDeHlJM3ZaSEd4cmNQZC8zU2djeHBiSDBaTGVyVEhRWnYv?=
 =?utf-8?B?NDJyTzhTT3NkMGY1OC90OXhEaTlRNks1blgxNGZqWG0wOWNKSFlHMkxnWDRk?=
 =?utf-8?B?cDZWeUxsc0o5Uk5aLzU3cElEZWJlQ3NnemhQR21kYXpOdXU3UjhxbVV1Mzkr?=
 =?utf-8?B?SFp5Y3dvM09ObFY1N2RNRWNpWEQxT2hOcGZpa0Jia3NjRVBIUTc5NW5iYzg1?=
 =?utf-8?B?Y2hnbDdNZ0pQWWc5dFJVV0ZkTUVCWTF4RkdpdkcxUHI5cDVZT211VnRRMTgz?=
 =?utf-8?B?RklpaGN3MTBIZS9UNHlqNHBIcnNjVkpvdG1VWDdSQ3Y4THN6aHZOZmJEN2Fv?=
 =?utf-8?B?dU1uVlMvL0NuOUZnOG9JSHlRU01NRTlsMjNkb2crOEgyTTMzeXV0dHNHTWE2?=
 =?utf-8?B?YTNoZkxNQXErdTdyY3VoS05WOTB4NGNkNURUdnZZOGNLRnFibGdWdEphVTZG?=
 =?utf-8?B?NXRRNTJvRE05ODEwSWx3NytOUVh2SU85OE1WS2lnZ3RJR2hPdTArd2hwN2h3?=
 =?utf-8?B?aENqUTdzcURScmRKSUVXdi9tNW05WEtWNk9hN2FSN0gxc1E5S25ZMFBjWklT?=
 =?utf-8?B?UWtxUW5Cb1VDaVc5dDFzWkhMYndxM1hCMGoxZFphTDdVUWh4NHo5UXRCcXYr?=
 =?utf-8?B?aXk2czIvV0trVTJ3U2cyOG9ybWtObzU2OU9BaGJrVHczRjVvWm9Sd3RoZWRS?=
 =?utf-8?B?MlBWeVJIVTBJcnBlSG1BVlp6Zk5kZ2ljMlBvdFNCd1JmZStuZ2RXU1JFRjZj?=
 =?utf-8?B?cTRLK3JvWlVwZmhSVTg2VXBzYUR3anlvOGl1VURQcFV2TEFsTzRqV1BTWGZn?=
 =?utf-8?B?eDBacTNndkNCcEJhWDNFMnpIUTRNSUlVeW1lUTd3TG5lUGlQVFJtMnpDY2po?=
 =?utf-8?B?TUQ1REhjc2I5bHQrUFY0aExIUjdZcVladDg4cllPam9OSWNISWJyZWFHS2R5?=
 =?utf-8?B?a2J6QkM0R2QvNXE0MEJXR2E5QTlQNHRRT3h4dk12WDFnbkVPM0lsNTZ3Sko3?=
 =?utf-8?B?eTIwdUhyZ25ScHNOY1J0TWdDUi9nc2I2dlJQVmhEOW9xdE80cFlUa1VTaTFK?=
 =?utf-8?B?TURJUi9zYk9tWmYzMFoyTkQvSGh2bHlvTldINS9aQWdITCsydmorT2owaHl4?=
 =?utf-8?B?NXdUd3JZVVhKYUt5UWlNbDNrZDc0M1dTc2tLSXRPMjdsYy9rem1XMlJRYjl0?=
 =?utf-8?Q?bed1snRplZvJu1eHLo1weqyz+?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3caf55aa-0541-4813-3c9f-08dc7b6e163e
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB2869.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 May 2024 21:19:53.0624
 (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: qui553IDAvSr7ooo4FKUvmk7dLWTupXfAygNr0l9o+Mp11zNvNEZiuZYHv4pYMRQv1LHe+Uf8ibSabyxwxtCDw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB7329

Hi Dave,

On 5/22/2024 10:59 AM, Dave Jiang wrote:
> 
> 
> On 5/22/24 8:08 AM, Smita Koralahalli wrote:
>> UEFI v2.10 section N.2.13 defines a CPER record for CXL Protocol errors.
>>
>> Add GHES support to detect CXL CPER Protocol Error Record and Cache Error
>> Severity, Device ID, Device Serial number and CXL RAS capability struct in
>> struct cxl_cper_prot_err. Include this struct as a member of struct
>> cxl_cper_work_data.
>>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
>> ---
>>   drivers/acpi/apei/ghes.c        | 10 +++++
>>   drivers/firmware/efi/cper_cxl.c | 66 +++++++++++++++++++++++++++++++++
>>   include/linux/cxl-event.h       | 26 +++++++++++++
>>   3 files changed, 102 insertions(+)
>>

[snip]


>> +	 * The device ID or agent address is required for CXL RCD, CXL
>> +	 * SLD, CXL LD, CXL Fabric Manager Managed LD, CXL Root Port,
>> +	 * CXL Downstream Switch Port and CXL Upstream Switch Port.
>> +	 */
>> +	if (prot_err->agent_type <= 0x7 && prot_err->agent_type != RCH_DP) {
> 
> Perhaps define an enum CXL_AGENT_TYPE_MAX instead of 0x7 magic number? Otherwise if a new type is introduced, it would break this code.

Agreed. I will define a boolean array indexed by agent type as suggested 
by Alison. That would avoid all these comparisons and not worry about 
breaking code in future.

>   
>> +		p_err->segment = prot_err->agent_addr.segment;
>> +		p_err->bus = prot_err->agent_addr.bus;
>> +		p_err->device = prot_err->agent_addr.device;
>> +		p_err->function = prot_err->agent_addr.function;
>> +	} else {
>> +		pr_err(FW_WARN "Invalid agent type\n");
>> +		return -EINVAL;
>> +	}
> 
> Up to you if you want to do this or not, but maybe:
> 
> 	if (prot_err->agent_type >= CXL_AGENT_TYPE_MAX || prot_err->agent_type == RCH_DP) {
> 		pr_warn(...);
> 		return -EINVAL;
> 	}
> 
> 	p_err->segment = ...;
> 	p_err->bus = ...;

Noted.

> 	...
> 
> Although perhaps a helper function cxl_cper_valid_agent_type() that checks invalid agent type by checking the valid_bits, the agent_type boundary, and if agent_type != RCH_DP?

Okay.

>> +
>> +	if (!(prot_err->valid_bits & PROT_ERR_VALID_ERROR_LOG)) {
>> +		pr_err(FW_WARN "Invalid Protocol Error log\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dvsec_start = (u8 *)(prot_err + 1);
>> +	cap_start = dvsec_start + prot_err->dvsec_len;
>> +	p_err->cxl_ras = *(struct cxl_ras_capability_regs *)cap_start;
>> +
>> +	/*
>> +	 * Set device serial number unconditionally.
>> +	 *
>> +	 * Print a warning message if it is not valid. The device serial
>> +	 * number is required for CXL RCD, CXL SLD, CXL LD and CXL Fabric
>> +	 * Manager Managed LD.
>> +	 */
>> +	if (!(prot_err->valid_bits & PROT_ERR_VALID_SERIAL_NUMBER) ||
>> +	      prot_err->agent_type > 0x4 || prot_err->agent_type == RCH_DP)
> 
> prot_err->agent_type > FM_LD? Although maybe it would be a clearer read if a helper function is defined to identify the agent types such as cxl_cper_prot_err_serial_needed() or cxl_cper_prot_agent_type_device() and with it a switch statement to explicitly identify all the agent types that require serial number. If a future device is defined, the > 0x4 logic may break.

Probably helper function is not required if boolean array is defined? 
What do you think?

Thanks,
Smita

[snip]