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 <[email protected]>
Reviewed-by: Yazen Ghannam <[email protected]>
---
Changes in v2:
1. Since is_ghes_assist_struct() returns if any of the conditions is hit
if-else-if chain is redundant. Replace it with just if statements.
2. Fix formatting errors.
3. Add Reviewed-by: Yazen Ghannam <[email protected]>
---
drivers/acpi/apei/hest.c | 51 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 6aef1ee5e1bd..99db7621adb7 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,54 @@ 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
+ * 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)
+ return true;
+ if (mces.mc && mces.mc->flags & ACPI_HEST_GHES_ASSIST &&
+ related_source_id == mces.mc->header.source_id)
+ return true;
+ if (mces.dmc && mces.dmc->flags & ACPI_HEST_GHES_ASSIST &&
+ related_source_id == mces.dmc->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)
@@ -114,6 +160,11 @@ static int apei_hest_parse(apei_hest_func_t func, void *data)
return -EINVAL;
}
+ if (is_ghes_assist_struct(hest_hdr)) {
+ hest_hdr = (void *)hest_hdr + len;
+ continue;
+ }
+
rc = func(hest_hdr, data);
if (rc)
return rc;
--
2.34.1
> 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".
What combination of CONFIG options and BIOS table support results in this?
I don't see much under "/sys/bus/platform/drivers/GHES" on my lab machine
(Dual socket * 36 cores * HT = 144 logical CPUs).
-Tony
Hi,
On 11/6/2023 14:42, Luck, Tony 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".
>
> What combination of CONFIG options and BIOS table support results in this?
>
From the kernel side, CONFIG_ACPI_APEI_GHES will have to be set, at the minimum.
From the BIOS perspective, it depends on HEST Table implementation and whether
or not it supports GHES_ASSIST feature.
The actual number of GHES_ASSIST structures will depend on number of logical CPUs
in the SOC, number of MCA banks per logical CPU and the number of enabled MCA error
sources.
> I don't see much under "/sys/bus/platform/drivers/GHES" on my lab machine
> (Dual socket * 36 cores * HT = 144 logical CPUs).
>
Does the BIOS on your machine support GHES_ASSIST? Can you confirm the number
of entries you actually see?
Typically, bit 2 of the Flags field of MCA Error sources in HEST indicates
GHES_ASSIST support. Below are the results from one of my systems without and
with the patch respectively.
This system has 256 logical CPUs with 28 MCA banks (I think) per logical CPU and 2
enabled (Machine Check and Corrected Machine Check) error sources. That's a total
of (256*2*28) 14336 GHES_ASSIST structures for MCA in HEST.
[root avadnaik]# ls /sys/bus/platform/drivers/GHES/ | grep -i "ghes.*" | wc -l
14349
[root avadnaik]# ls /sys/bus/platform/drivers/GHES/ | grep -i "ghes.*" | wc -l
13
--
Thanks,
Avadhut Naik
> -Tony
Hi,
Any further feedback on this patch?
On 11/6/2023 14:13, 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 <[email protected]>
> Reviewed-by: Yazen Ghannam <[email protected]>
> ---
> Changes in v2:
> 1. Since is_ghes_assist_struct() returns if any of the conditions is hit
> if-else-if chain is redundant. Replace it with just if statements.
> 2. Fix formatting errors.
> 3. Add Reviewed-by: Yazen Ghannam <[email protected]>
> ---
> drivers/acpi/apei/hest.c | 51 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
> index 6aef1ee5e1bd..99db7621adb7 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,54 @@ 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
> + * 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)
> + return true;
> + if (mces.mc && mces.mc->flags & ACPI_HEST_GHES_ASSIST &&
> + related_source_id == mces.mc->header.source_id)
> + return true;
> + if (mces.dmc && mces.dmc->flags & ACPI_HEST_GHES_ASSIST &&
> + related_source_id == mces.dmc->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)
> @@ -114,6 +160,11 @@ static int apei_hest_parse(apei_hest_func_t func, void *data)
> return -EINVAL;
> }
>
> + if (is_ghes_assist_struct(hest_hdr)) {
> + hest_hdr = (void *)hest_hdr + len;
> + continue;
> + }
> +
> rc = func(hest_hdr, data);
> if (rc)
> return rc;
--
Thanks,
Avadhut Naik
On Wed, Nov 29, 2023 at 12:29:16PM -0600, Avadhut Naik wrote:
> Hi,
>
> Any further feedback on this patch?
Yes. See below.
>
> On 11/6/2023 14:13, 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 <[email protected]>
> > Reviewed-by: Yazen Ghannam <[email protected]>
> > ---
> > Changes in v2:
> > 1. Since is_ghes_assist_struct() returns if any of the conditions is hit
> > if-else-if chain is redundant. Replace it with just if statements.
> > 2. Fix formatting errors.
> > 3. Add Reviewed-by: Yazen Ghannam <[email protected]>
> > ---
> > drivers/acpi/apei/hest.c | 51 ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
> > index 6aef1ee5e1bd..99db7621adb7 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;
You are using this static structure to save values while computing
the length of the HEST structure in hest_esrc_len() to be used later
when is_ghes_assist_struct() checks to see if it should be skipped.
But you don't clear it between iterations in apei_hest_parse(). So if
the assist structure was early in the array of HEST structures, your
is_ghes_assist_struct() will keep looking at stale mces.{cmc,mc,dmc}
values.
It may not break because the related_source_id in the subsequent
structures won't match the one you saved. But this seems wrong.
On the other hand, if this caching of values from some structures
to be compared against values in later structures is intended. Then
you need a comment on this structure to say that's what you are
doing.
> > +
> > 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,54 @@ 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
> > + * 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)
> > + return true;
> > + if (mces.mc && mces.mc->flags & ACPI_HEST_GHES_ASSIST &&
> > + related_source_id == mces.mc->header.source_id)
> > + return true;
> > + if (mces.dmc && mces.dmc->flags & ACPI_HEST_GHES_ASSIST &&
> > + related_source_id == mces.dmc->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)
> > @@ -114,6 +160,11 @@ static int apei_hest_parse(apei_hest_func_t func, void *data)
> > return -EINVAL;
> > }
> >
> > + if (is_ghes_assist_struct(hest_hdr)) {
> > + hest_hdr = (void *)hest_hdr + len;
> > + continue;
> > + }
> > +
> > rc = func(hest_hdr, data);
> > if (rc)
> > return rc;
>
> --
> Thanks,
> Avadhut Naik
-Tony
Hi,
On 11/29/2023 12:52, Tony Luck wrote:
> On Wed, Nov 29, 2023 at 12:29:16PM -0600, Avadhut Naik wrote:
>> Hi,
>>
>> Any further feedback on this patch?
>
> Yes. See below.
>>
>> On 11/6/2023 14:13, 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 <[email protected]>
>>> Reviewed-by: Yazen Ghannam <[email protected]>
>>> ---
>>> Changes in v2:
>>> 1. Since is_ghes_assist_struct() returns if any of the conditions is hit
>>> if-else-if chain is redundant. Replace it with just if statements.
>>> 2. Fix formatting errors.
>>> 3. Add Reviewed-by: Yazen Ghannam <[email protected]>
>>> ---
>>> drivers/acpi/apei/hest.c | 51 ++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 51 insertions(+)
>>>
>>> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
>>> index 6aef1ee5e1bd..99db7621adb7 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;
>
> You are using this static structure to save values while computing
> the length of the HEST structure in hest_esrc_len() to be used later
> when is_ghes_assist_struct() checks to see if it should be skipped.
>
> But you don't clear it between iterations in apei_hest_parse(). So if
> the assist structure was early in the array of HEST structures, your
> is_ghes_assist_struct() will keep looking at stale mces.{cmc,mc,dmc}
> values.
>
> It may not break because the related_source_id in the subsequent
> structures won't match the one you saved. But this seems wrong.
>
> On the other hand, if this caching of values from some structures
> to be compared against values in later structures is intended. Then
> you need a comment on this structure to say that's what you are
> doing.
>
Yes, the static structure is being used to cache detected MCA error sources
in the HEST. Per ACPI spec 6.5, each MCA error source i.e. machine check,
correctable machine check and deferred machine check will only have one entry
in the HEST and the pointers in the static structure will be initialized to
point to those entries when they are detected.
Later, if and when GHES structres are encountered in the HEST, is_ghes_assist_struct()
will check if the cached error sources have GHES_ASSIST flag set. If they do, the
related_source_id field of the GHES structure will be compared against the source_id
field of the error sources. If there is a match, initialization of that GHES
structure will be skipped (since it is a GHES_ASSIST structure for MCA) and we move on
to the next entry in the HEST.
Thus, clearing the cached error sources between iterations in apei_hest_parse() might
defeat the purpose of the patch. We need the error sources, specifically thier flags
and source_id fields to determine if the initialization of a GHES structure should be
skipped.
> the assist structure was early in the array of HEST structures, your
> is_ghes_assist_struct() will keep looking at stale mces.{cmc,mc,dmc}
> values.
Didnt really understand what you meant by this. Can you please elaborate more?
IIUC, GHES_ASSIST structure will not occur before its corresponding error source
in the HEST.
Actually, caching of values from an earlier structure to be compared against values
in a later structure is what we intend to do here i.e. comparing cached source_id
from an earlier structure with related_source_id in a later structure. To hint
towards the same, I had added the below comment on top of the static structure's
declaration:
>>> + * 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.
>>> + */
Is this not good enough? Should I make it more explicit?
>>> +
>>> 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,54 @@ 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
>>> + * 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)
>>> + return true;
>>> + if (mces.mc && mces.mc->flags & ACPI_HEST_GHES_ASSIST &&
>>> + related_source_id == mces.mc->header.source_id)
>>> + return true;
>>> + if (mces.dmc && mces.dmc->flags & ACPI_HEST_GHES_ASSIST &&
>>> + related_source_id == mces.dmc->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)
>>> @@ -114,6 +160,11 @@ static int apei_hest_parse(apei_hest_func_t func, void *data)
>>> return -EINVAL;
>>> }
>>>
>>> + if (is_ghes_assist_struct(hest_hdr)) {
>>> + hest_hdr = (void *)hest_hdr + len;
>>> + continue;
>>> + }
>>> +
>>> rc = func(hest_hdr, data);
>>> if (rc)
>>> return rc;
>>
>> --
>> Thanks,
>> Avadhut Naik
>
> -Tony
--
Thanks,
Avadhut Naik
> Actually, caching of values from an earlier structure to be compared against values
> in a later structure is what we intend to do here i.e. comparing cached source_id
> from an earlier structure with related_source_id in a later structure. To hint
> towards the same, I had added the below comment on top of the static structure's
> declaration:
>
> >>> + * 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.
> >>> + */
>
> Is this not good enough? Should I make it more explicit?
I understand now, but missed that the caching and use are across
different entries in the HEST table.
Maybe something like this:
* Since GHES_ASSIST is not supported, skip initialization of GHES_ASSIST
* structures for MCA. During HEST parsing, detected MCA error sources
* are cached from early table entries so that Flags and Source Id
* fields from these cached values are then referred to in later table
* entries to determine if the encountered GHES_ASSIST structure should
* be initialized.
-Tony
Hi,
On 11/30/2023 14:03, Luck, Tony wrote:
>> Actually, caching of values from an earlier structure to be compared against values
>> in a later structure is what we intend to do here i.e. comparing cached source_id
>> from an earlier structure with related_source_id in a later structure. To hint
>> towards the same, I had added the below comment on top of the static structure's
>> declaration:
>>
>>>>> + * 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.
>>>>> + */
>>
>> Is this not good enough? Should I make it more explicit?
>
> I understand now, but missed that the caching and use are across
> different entries in the HEST table.
>
> Maybe something like this:
>
> * Since GHES_ASSIST is not supported, skip initialization of GHES_ASSIST
> * structures for MCA. During HEST parsing, detected MCA error sources
> * are cached from early table entries so that Flags and Source Id
> * fields from these cached values are then referred to in later table
> * entries to determine if the encountered GHES_ASSIST structure should
> * be initialized.
>
Sounds good! Thank you for this. Will update accordingly and resubmit.
> -Tony
--
Thanks,
Avadhut Naik