Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <b88e1f4f-1e85-46ca-b317-d8e10fe894a3@amd.com>
Date:   Fri, 29 Sep 2023 10:40:19 -0400
User-Agent: Mozilla Thunderbird
Cc:     yazen.ghannam@amd.com, linux-kernel@vger.kernel.org,
        avadnaik@amd.com
Subject: Re: [PATCH] ACPI: APEI: Skip initialization of GHES_ASSIST structures
 for Machine Check Architecture
To:     Avadhut Naik <avadhut.naik@amd.com>, rafael@kernel.org,
        lenb@kernel.org, linux-acpi@vger.kernel.org
References: <20230928184204.1511111-1-avadhut.naik@amd.com>
Content-Language: en-US
From:   Yazen Ghannam <yazen.ghannam@amd.com>
In-Reply-To: <20230928184204.1511111-1-avadhut.naik@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eUVheHc1QUM2LzRTUW1VMHhoZ0xQWmcrRkptQmU2MkJ5czhYRUl5MGJicDNh?=
 =?utf-8?B?VDBDNHVPcEZkbFFCOWE5NzZSSDNZNHE3bjRFUFBDSkhjcHZ4QStmb2lRNCty?=
 =?utf-8?B?SjlBRm5rSDEwU3d5emRIVWtvRUNoUVRqTG1wMzBKM2JZQTl3Mjg1WWhwTjR5?=
 =?utf-8?B?c2tGM0kzNWY2aXBmR0VSOUYvaWpiRTZETGtqektLTVc4YlZMcVBlM1VXbVVY?=
 =?utf-8?B?ajloaWlVVlc5OXYwNDRTNkRxQkdqS0FZenF4Mms2TWVxVlhuTmowVXYrQjFi?=
 =?utf-8?B?RHBPRWZNY1ByRkFOOE14RGZSNjNMRThoNmorcDZLYXBBSzQ4V1lnRG4wV0No?=
 =?utf-8?B?bFNvMnFHM0NOb0xNTmJxUlkycjhYcFY5MnU4Q0R2UnJsL05EWWpUWDYrQVEx?=
 =?utf-8?B?QmVKRm0vQWFqa0lXWjNEcnpDUFRrZmNXZzN3d2ZLSnBGaUtYckcyWmt6VEZy?=
 =?utf-8?B?VldBaWh6LzJSbWpqTnRUTmNGV0JZRDJFVUlGTDhGTmtiNk9raHg5T0NKOTh5?=
 =?utf-8?B?YVFIMkZWSmVjUGJFREpockdIckxkQ0FEUGkxdjZjSXMwdmlBKzZQZlRhbU1W?=
 =?utf-8?B?UHhHbE14RFplSzQzU0VSNjB3dmU5cXZ2Zlh3a3R0SElYN2c4U0NjM0xUejBm?=
 =?utf-8?B?VVFvbFRsczBvaElrblJMNWJXUUx1dGpodWFCaTRoSWMzUVduUEYzYklqZ0lS?=
 =?utf-8?B?LzArSU90U1A5dGFKbDRaNUdiSnFvVWZyK3VtUC9lanJGMnlMK3FVbFF1QmZ0?=
 =?utf-8?B?UWExczMvQ3JjdmR2SkFOMEtoOXozeVF3d1NSL2Jpa0lhTzY5VTVGZGtzeGRF?=
 =?utf-8?B?WnR6c1I0ZEhHMTFGS0c3bnpFTVFwMHZxcTY0amhZVm5qV214ODdjNkxQc0x1?=
 =?utf-8?B?WTduS3RJeVpWQ0xaYUE0bWRrTmkxMlZFdHg0LzdYYWdrWFl3VnQrYTM0c0pC?=
 =?utf-8?B?VHBnb0R3L0d4V3dsNFNLY01aYVV3Ykk4dUdjeG9sdmpDdER0VGVxcmtkTjJN?=
 =?utf-8?B?biszU3RSY0srUy9DV3RHTGFpU3FaWlE1UVV0UG5WYkNiTERGTDYxcTRZMEpZ?=
 =?utf-8?B?OWIxVWsxL202WVQ5aVpPblVGbmpxWXVKaXMvY1l2c0IzY0JnQTRmVHRoTytI?=
 =?utf-8?B?MDZpVmt3UXM2UnlxUkxyU1ZUSDY3eDNJbklEVm0wQ0tsaFNhbUcySTdPVUk0?=
 =?utf-8?B?eWc3MU9lUEtFNURWRmV6UXY2Ukt5NXg2b0dyWVB1eXhIeG1CTWZYeDVjcU9M?=
 =?utf-8?B?SE5XMkt5VDl3bGhvM0FHcGxYUWIwUXdqRXJyc1BEUnc0ZzJKRzQ3VStuUHlG?=
 =?utf-8?B?Q0ZNTmhmNW5wVVdZQjBXTGZRWUdyY25IYUFHbGtrQVR3TXBVWkViNHV1WmJF?=
 =?utf-8?B?Z2ZRTUpsNTNrY21GSnRkdFlINW1CYzFIanB0TUoxeTgxSGt1STdqcXBTcXEw?=
 =?utf-8?B?UFB4TUVjUjdSd3FVYmh6QURFU1BXcStkZ2NFK2MrVVNicnoyUWpJMlhyU2lj?=
 =?utf-8?B?MktHR1dHSHJHRlpMLzh4cjJjb0dVV0VWMW5SY1lMbGJCalRNeVdTVWI0V20y?=
 =?utf-8?B?VlRYRXpVV2wzWDFRUzhMc1V4aWZHNHFESVdiTlg4MlJDN04vMktaVm0wMGJK?=
 =?utf-8?B?NEVDV2MzNmhjM3VLRFhKSjJxYlVlNjVkTFU3bFQyS1lUZlZ6dUFCVUdJT3Bl?=
 =?utf-8?B?bFNDVWxkVDZMYW51MXJ3T0VyRmRDWnl1dUFudVpzL3pvSFZmSWdHTTNlUDNO?=
 =?utf-8?B?MnM2Sm05SGJ6VGZRbmRIZlRZK3ZyZjRVenZ0YlArQUpCQ3cxRkZ1WjBjVEpN?=
 =?utf-8?B?ZDNWZWlveVhsUVVKM0RtWmxXbWV1VW1IbDRlU1A5ZTl5dy9hbm4zTWQrakt4?=
 =?utf-8?B?MFV3WEdqZFdRMjNqalR2OE1idFRkOHc5Uzlkb3lMeXZsMlVHWHRLMlI2YURo?=
 =?utf-8?B?SzR2TUxQUGowdGRRUHgzeHJ3QlEvMFhlanZaOXgvaU5ndS9LL0xpV3ZHSXVE?=
 =?utf-8?B?SFZ4Wm1EaDVFUnNGVG9qb2JxeFRLMWtjcG81a05nQ3RzRzZEMUtQVWEzR05U?=
 =?utf-8?B?blJhNG9KcGJEaVRULzVwUE41WG5wUkVKSkRmaFpQbGF6SHluSy9HZ29vV2o0?=
 =?utf-8?Q?1GCsdtWPLFG5oyzCZ5P0wTxpc?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 28c6b9bf-78ea-4422-91d7-08dbc0fa024f
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2023 14:40:21.7909
 (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: EokJdzqeJobvjkxVpJRXJ/KRmWbdLY/fJ6gOrMjqT5vZmx573qTNSsb8OTitkcCVKPK+PsEhpaUper8wrBovdA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV3PR12MB9439
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Fri, 29 Sep 2023 07:40:41 -0700 (PDT)

On 9/28/23 2:42 PM, Avadhut Naik wrote:
> To support GHES_ASSIST on Machine Check Architecture (MCA) error sources,
> a set of GHES structures is provided by the system firmware for each MCA
> error source. Each of these sets consists of a GHES structure for each MCA
> bank on each logical CPU, with all structures of a set sharing a common
> Related Source ID, equal to the Source ID of one of the MCA error source
> structures.[1] On SOCs with large core counts, this typically equates to
> tens of thousands of GHES_ASSIST structures for MCA under
> "/sys/bus/platform/drivers/GHES".
> 
> Support for GHES_ASSIST however, hasn't been implemented in the kernel. As
> such, the information provided through these structures is not consumed by
> Linux. Moreover, these GHES_ASSIST structures for MCA, which are supposed
> to provide supplemental information in context of an error reported by
> hardware, are setup as independent error sources by the kernel during HEST
> initialization.
> 
> Additionally, if the Type field of the Notification structure, associated
> with these GHES_ASSIST structures for MCA, is set to Polled, the kernel
> sets up a timer for each individual structure. The duration of the timer
> is derived from the Poll Interval field of the Notification structure. On
> SOCs with high core counts, this will result in tens of thousands of
> timers expiring periodically causing unnecessary preemptions and wastage
> of CPU cycles. The problem will particularly intensify if Poll Interval
> duration is not sufficiently high.
> 
> Since GHES_ASSIST support is not present in kernel, skip initialization
> of GHES_ASSIST structures for MCA to eliminate their performance impact.
> 
> [1] ACPI specification 6.5, section 18.7
> 
> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
> ---
>  drivers/acpi/apei/hest.c | 49 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
> index 6aef1ee5e1bd..03cb0ece4235 100644
> --- a/drivers/acpi/apei/hest.c
> +++ b/drivers/acpi/apei/hest.c
> @@ -37,6 +37,20 @@ EXPORT_SYMBOL_GPL(hest_disable);
>  
>  static struct acpi_table_hest *__read_mostly hest_tab;
>  
> +/*
> + * Since GHES_ASSIST is not supported, skip initialization
> + * of GHES_ASSIST structures for MCA.
> + * During HEST parsing, detected MCA error sources are cached.
> + * Flags and Source Id fields from these cached values are
> + * then referred to determine if the encountered GHES_ASSIST
> + * structure should be initialized.
> + */
> +static struct {
> +	struct acpi_hest_ia_corrected *cmc;
> +	struct acpi_hest_ia_machine_check *mc;
> +	struct acpi_hest_ia_deferred_check *dmc;
> +} mces;
> +
>  static const int hest_esrc_len_tab[ACPI_HEST_TYPE_RESERVED] = {
>  	[ACPI_HEST_TYPE_IA32_CHECK] = -1,	/* need further calculation */
>  	[ACPI_HEST_TYPE_IA32_CORRECTED_CHECK] = -1,
> @@ -70,22 +84,53 @@ static int hest_esrc_len(struct acpi_hest_header *hest_hdr)
>  		cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
>  		len = sizeof(*cmc) + cmc->num_hardware_banks *
>  			sizeof(struct acpi_hest_ia_error_bank);
> +		mces.cmc = cmc;
>  	} else if (hest_type == ACPI_HEST_TYPE_IA32_CHECK) {
>  		struct acpi_hest_ia_machine_check *mc;
>  		mc = (struct acpi_hest_ia_machine_check *)hest_hdr;
>  		len = sizeof(*mc) + mc->num_hardware_banks *
>  			sizeof(struct acpi_hest_ia_error_bank);
> +		mces.mc = mc;
>  	} else if (hest_type == ACPI_HEST_TYPE_IA32_DEFERRED_CHECK) {
>  		struct acpi_hest_ia_deferred_check *mc;
>  		mc = (struct acpi_hest_ia_deferred_check *)hest_hdr;
>  		len = sizeof(*mc) + mc->num_hardware_banks *
>  			sizeof(struct acpi_hest_ia_error_bank);
> +		mces.dmc = mc;
>  	}
>  	BUG_ON(len == -1);
>  
>  	return len;
>  };
>  
> +/* GHES and GHESv2 structures share the same format, starting from

Please follow the general kernel multi-line comment style here.

> + * Source Id and ending in Error Status Block Length (inclusive).
> + */
> +static bool is_ghes_assist_struct(struct acpi_hest_header *hest_hdr)
> +{
> +	struct acpi_hest_generic *ghes;
> +	u16 related_source_id;
> +
> +	if (hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR &&
> +	    hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR_V2)
> +		return false;
> +
> +	ghes = (struct acpi_hest_generic *)hest_hdr;
> +	related_source_id = ghes->related_source_id;
> +
> +	if (mces.cmc && mces.cmc->flags & ACPI_HEST_GHES_ASSIST &&
> +	    related_source_id == mces.cmc->header.source_id)

IMO, there should be parentheses around statements with multiple terms
to improve clarity.

	if (mces.cmc && (mces.cmc->flags & ACPI_HEST_GHES_ASSIST) &&
	    (related_source_id == mces.cmc->header.source_id))

> +		return true;
> +	else if (mces.dmc && mces.dmc->flags & ACPI_HEST_GHES_ASSIST &&

The "else" is not needed, because of the return.

IOW, use "else if" when you want to skip a check if the previous "if"
was true. Here the check is already skipped if the previous "if" was
true and did a return.

Also, the order of the types is "cmc, mc, dmc" in the struct definition
and hest_esrc_len(). But here it is "cmc, dmc, mc". This is just a minor
nit about consistency in ordering.

> +		 related_source_id == mces.dmc->header.source_id)
> +		return true;
> +	else if (mces.mc && mces.mc->flags & ACPI_HEST_GHES_ASSIST &&
> +		 related_source_id == mces.mc->header.source_id)
> +		return true;
> +
> +	return false;
> +}
> +
>  typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data);
>  
>  static int apei_hest_parse(apei_hest_func_t func, void *data)
> @@ -113,6 +158,10 @@ static int apei_hest_parse(apei_hest_func_t func, void *data)
>  				hest_hdr->source_id);
>  			return -EINVAL;
>  		}

Newline here please.

> +		if (is_ghes_assist_struct(hest_hdr)) {
> +			hest_hdr = (void *)hest_hdr + len;
> +			continue;
> +		}
>  
>  		rc = func(hest_hdr, data);
>  		if (rc)

Besides the minor formatting comments, this looks good to me.

Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>

Thanks,
Yazen