[PATCH v3 0/3] Correct for ACPI 5.1->6.0 spec changes in MADT GICC entries

[PATCH v3 0/3] Correct for ACPI 5.1->6.0 spec changes in MADT GICC entries

[Al Stone]

In the ACPI 5.1 version of the spec, the struct for the GICC subtable
(struct acpi_madt_generic_interrupt) of the MADT is 76 bytes long; in
ACPI 6.0, the struct is 80 bytes long.  But, there is only one definition
in ACPICA for this struct -- and that is the 6.0 version.  Hence, when
BAD_MADT_ENTRY() compares the struct size to the length in the GICC
subtable, it fails if 5.1 structs are in use, and there are systems in
the wild that have them.

Note that this was found in linux-next and these patches apply against
that tree and the arm64 kernel tree; 4.1-rc8 does not appear to have this
problem since it still has the 5.1 struct definition.

Even though there is precendent in ia64 code for ignoring the changes in
size, this patch set instead tries to verify correctness.  The first patch
in the set adds macros for easily using the ACPI spec version.  The second
patch adds the BAD_MADT_GICC_ENTRY() macro that uses the version macros to
check the GICC subtable only, accounting for the difference in specification
versions that are possible.  The final patch replaces BAD_MADT_ENTRY usage
with the BAD_MADT_GICC_ENTRY macro in arm64 code, which is currently the
only architecture affected.  The BAD_MADT_ENTRY() will continue to work as
is for all other MADT subtables.

I have tested these patches on an APM Mustang with version 1.15 firmware,
where the problem was found, and they fix the problem.

Changes for v3:
  -- Modified the macros for using spec version numbers in order
     to make them clearer (Rafael, Hanjun)
  -- Moved the definition of the BAD_MADT_GICC_ENTRY macro to an
     arm64-specific header file since only this architecture uses
     the GICC subtable (Rafael)
  -- Added Reviewed-by (Hanjun) and Acked-by (Will) tags to 3/3, the
     only unchanged patch; other tags could be applied but the patches
     have changed.
  -- Added Fixes: tag to patches

Changes for v2:
  -- Replace magic constants with proper defines (Lorenzo)
  -- Minor syntax clean-up noted by checkpatch
  -- Send out CCs properly this time
  -- Minor clean-up of the paragraphs in this cover letter

Al Stone (3):
  ACPI : introduce macros for using the ACPI specification version
  ACPI / ARM64: add BAD_MADT_GICC_ENTRY() macro
  ACPI / ARM64 : use the new BAD_MADT_GICC_ENTRY macro

 arch/arm64/include/asm/acpi.h | 11 +++++++++++
 arch/arm64/kernel/smp.c       |  2 +-
 drivers/irqchip/irq-gic.c     |  2 +-
 include/linux/acpi.h          | 10 ++++++++++
 4 files changed, 23 insertions(+), 2 deletions(-)

-- 
2.4.3

[PATCH v3 1/3] ACPI : introduce macros for using the ACPI specification version

[Al Stone]

Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version
number from a major and minor revision number.  Add also macros that use
the helper to construct the current version from the values in the FADT
(i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.

These macros are added in order to simplify retrieving and comparing ACPI
specification version numbers, since this is becoming a more frequent need.
In particular, there are some architectures that require at least a certain
version of the spec, and there are differences in some structure sizes that
have changed with recent versions but can only be tracked by spec version
number.

Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
Signed-off-by: Al Stone <al.stone@linaro.org>
---
 include/linux/acpi.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index c471dfc..0e525e8 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -48,6 +48,16 @@
 #include <acpi/acpi_io.h>
 #include <asm/acpi.h>
 
+#define __ACPI_FADT_SPEC_VERSION(major, minor) \
+	((unsigned int)major << 8 | (unsigned int)minor)
+
+#define ACPI_FADT_SPEC_VERSION	\
+	__ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
+				 acpi_gbl_FADT.minor_revision)
+
+#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1)
+#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
+
 static inline acpi_handle acpi_device_handle(struct acpi_device *adev)
 {
 	return adev ? adev->handle : NULL;
-- 
2.4.3

[PATCH v3 2/3] ACPI / ARM64: add BAD_MADT_GICC_ENTRY() macro

[Al Stone]

The BAD_MADT_ENTRY() macro is designed to work for all of the subtables
of the MADT.  In the ACPI 5.1 version of the spec, the struct for the
GICC subtable (struct acpi_madt_generic_interrupt) is 76 bytes long; in
ACPI 6.0, the struct is 80 bytes long.  But, there is only one definition
in ACPICA for this struct -- and that is the 6.0 version.  Hence, when
BAD_MADT_ENTRY() compares the struct size to the length in the GICC
subtable, it fails if 5.1 structs are in use, and there are systems in
the wild that have them.

This patch adds the BAD_MADT_GICC_ENTRY() that checks the GICC subtable
only, accounting for the difference in specification versions that are
possible.  The BAD_MADT_ENTRY() will continue to work as is for all other
MADT subtables.

This code is being added to an arm64 header file since that is currently
the only architecture using the GICC subtable of the MADT.  As a GIC is
specific to ARM, it is also unlikely the subtable will be used elsewhere.

Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
Signed-off-by: Al Stone <al.stone@linaro.org>
---
 arch/arm64/include/asm/acpi.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 39248d3..a3c26a4 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -19,6 +19,17 @@
 #include <asm/psci.h>
 #include <asm/smp_plat.h>
 
+/* Macros for consistency checks of the GICC subtable of MADT */
+#define ACPI_MADT_GICC_51_LENGTH       76
+#define ACPI_MADT_GICC_60_LENGTH       80
+
+#define BAD_MADT_GICC_ENTRY(entry, end) (                                   \
+		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
+		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) &&   \
+		 (entry->header.length != ACPI_MADT_GICC_51_LENGTH))    ||  \
+		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) &&   \
+		 (entry->header.length != ACPI_MADT_GICC_60_LENGTH)))
+
 /* Basic configuration for ACPI */
 #ifdef	CONFIG_ACPI
 /* ACPI table mapping after acpi_gbl_permanent_mmap is set */
-- 
2.4.3

[PATCH v3 3/3] ACPI / ARM64 : use the new BAD_MADT_GICC_ENTRY macro

[Al Stone]

For those parts of the arm64 ACPI code that need to check GICC subtables
in the MADT, use the new BAD_MADT_GICC_ENTRY macro instead of the previous
BAD_MADT_ENTRY.  The new macro takes into account differences in the size
of the GICC subtable that the old macro did not; this caused failures even
though the subtable entries are valid.

Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
Signed-off-by: Al Stone <al.stone@linaro.org>
Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
Acked-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/smp.c   | 2 +-
 drivers/irqchip/irq-gic.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index a1883bf..25fc88c 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -438,7 +438,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
 	struct acpi_madt_generic_interrupt *processor;
 
 	processor = (struct acpi_madt_generic_interrupt *)header;
-	if (BAD_MADT_ENTRY(processor, end))
+	if (BAD_MADT_GICC_ENTRY(processor, end))
 		return -EINVAL;
 
 	acpi_table_print_madt_entry(header);
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 8d7e1c8..4dd8826 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1055,7 +1055,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 
 	processor = (struct acpi_madt_generic_interrupt *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
+	if (BAD_MADT_GICC_ENTRY(processor, end))
 		return -EINVAL;
 
 	/*
-- 
2.4.3

Re: [PATCH v3 1/3] ACPI : introduce macros for using the ACPI specification version

[Rafael J. Wysocki]

On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
> Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version
> number from a major and minor revision number.  Add also macros that use
> the helper to construct the current version from the values in the FADT
> (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
> 
> These macros are added in order to simplify retrieving and comparing ACPI
> specification version numbers, since this is becoming a more frequent need.
> In particular, there are some architectures that require at least a certain
> version of the spec, and there are differences in some structure sizes that
> have changed with recent versions but can only be tracked by spec version
> number.
> 
> Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  include/linux/acpi.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index c471dfc..0e525e8 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -48,6 +48,16 @@
>  #include <acpi/acpi_io.h>
>  #include <asm/acpi.h>
>  
> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
> +	((unsigned int)major << 8 | (unsigned int)minor)
> +
> +#define ACPI_FADT_SPEC_VERSION	\
> +	__ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
> +				 acpi_gbl_FADT.minor_revision)
> +
> +#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1)
> +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)

I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0

And what if there is 5.2 or even 5.3?

> +
>  static inline acpi_handle acpi_device_handle(struct acpi_device *adev)
>  {
>  	return adev ? adev->handle : NULL;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 1/3] ACPI : introduce macros for using the ACPI specification version

[Hanjun Guo]

Hi Al,

On 2015/7/3 7:48, Al Stone wrote:
> Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version
> number from a major and minor revision number.  Add also macros that use
> the helper to construct the current version from the values in the FADT
> (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
>
> These macros are added in order to simplify retrieving and comparing ACPI
> specification version numbers, since this is becoming a more frequent need.
> In particular, there are some architectures that require at least a certain
> version of the spec, and there are differences in some structure sizes that
> have changed with recent versions but can only be tracked by spec version
> number.
>
> Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)

If I understand correctly, will it be (and for next two patches as well):

0cff8dc0099f (ACPICA: ACPI 6.0: Add changes for MADT table.)?

FADT table update was not introduce any regressions for ARM64
I think.

Thanks
Hanjun

Re: [PATCH v3 1/3] ACPI : introduce macros for using the ACPI specification version

[Hanjun Guo]

Hi Rafael,

On 2015/7/3 8:21, Rafael J. Wysocki wrote:
> On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
>> Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version
>> number from a major and minor revision number.  Add also macros that use
>> the helper to construct the current version from the values in the FADT
>> (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
>>
>> These macros are added in order to simplify retrieving and comparing ACPI
>> specification version numbers, since this is becoming a more frequent need.
>> In particular, there are some architectures that require at least a certain
>> version of the spec, and there are differences in some structure sizes that
>> have changed with recent versions but can only be tracked by spec version
>> number.
>>
>> Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>  include/linux/acpi.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index c471dfc..0e525e8 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -48,6 +48,16 @@
>>  #include <acpi/acpi_io.h>
>>  #include <asm/acpi.h>
>>  
>> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
>> +	((unsigned int)major << 8 | (unsigned int)minor)
>> +
>> +#define ACPI_FADT_SPEC_VERSION	\
>> +	__ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
>> +				 acpi_gbl_FADT.minor_revision)
>> +
>> +#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1)
>> +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
> I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0

Agreed.

>
> And what if there is 5.2 or even 5.3?

Hmm, do you mean in the future or just now? for both two cases,
only 5.1 and 5.1 errata (still has the same ACPI version with 5.1)
will be available, then jump to 6.0 and going forward if new versions
in the future.

I'm not sure if I understand your question correctly, if
not, please correct me :)

Thanks
Hanjun

Re: [PATCH v3 2/3] ACPI / ARM64: add BAD_MADT_GICC_ENTRY() macro

[Catalin Marinas]

On Thu, Jul 02, 2015 at 05:48:35PM -0600, Al Stone wrote:
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 39248d3..a3c26a4 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -19,6 +19,17 @@
>  #include <asm/psci.h>
>  #include <asm/smp_plat.h>
>  
> +/* Macros for consistency checks of the GICC subtable of MADT */
> +#define ACPI_MADT_GICC_51_LENGTH       76
> +#define ACPI_MADT_GICC_60_LENGTH       80
> +
> +#define BAD_MADT_GICC_ENTRY(entry, end) (                                   \
> +		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) &&   \
> +		 (entry->header.length != ACPI_MADT_GICC_51_LENGTH))    ||  \
> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) &&   \
> +		 (entry->header.length != ACPI_MADT_GICC_60_LENGTH)))

This looks ugly but, well, we could live with this.

However, I'd like to avoid having to extend this macro every time we get
a new spec released, like 6.1 defining another 80 or 84 etc. So, how
about we only update this when there is an actual change in the length?
Something like:

#define ACPI_MADT_GICC_LENGTH	({					\
	u8 length;							\
	if (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0)	\
		length = 76;						\
	else								\
		length = 80;						\
	length;								\
})

or just:

#define ACPI_MADT_GICC_LENGTH	\
	(ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0 ? 76 : 80)

(the latter is simpler but may not look nice if we change it again in
6.1; though we could re-write this macro when needed, not a problem)

-- 
Catalin

Re: [PATCH v3 1/3] ACPI : introduce macros for using the ACPI specification version

[Al Stone]

On 07/02/2015 11:23 PM, Hanjun Guo wrote:
> Hi Rafael,
> 
> On 2015/7/3 8:21, Rafael J. Wysocki wrote:
>> On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
>>> Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version
>>> number from a major and minor revision number.  Add also macros that use
>>> the helper to construct the current version from the values in the FADT
>>> (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
>>>
>>> These macros are added in order to simplify retrieving and comparing ACPI
>>> specification version numbers, since this is becoming a more frequent need.
>>> In particular, there are some architectures that require at least a certain
>>> version of the spec, and there are differences in some structure sizes that
>>> have changed with recent versions but can only be tracked by spec version
>>> number.
>>>
>>> Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
>>> Signed-off-by: Al Stone <al.stone@linaro.org>
>>> ---
>>>  include/linux/acpi.h | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>>> index c471dfc..0e525e8 100644
>>> --- a/include/linux/acpi.h
>>> +++ b/include/linux/acpi.h
>>> @@ -48,6 +48,16 @@
>>>  #include <acpi/acpi_io.h>
>>>  #include <asm/acpi.h>
>>>  
>>> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
>>> +	((unsigned int)major << 8 | (unsigned int)minor)
>>> +
>>> +#define ACPI_FADT_SPEC_VERSION	\
>>> +	__ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
>>> +				 acpi_gbl_FADT.minor_revision)
>>> +
>>> +#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1)
>>> +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
>> I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0
> 
> Agreed.

Will do.  This was a flip of the coin, on my part.

>> And what if there is 5.2 or even 5.3?
> 
> Hmm, do you mean in the future or just now? for both two cases,
> only 5.1 and 5.1 errata (still has the same ACPI version with 5.1)
> will be available, then jump to 6.0 and going forward if new versions
> in the future.
> 
> I'm not sure if I understand your question correctly, if
> not, please correct me :)
> 
> Thanks
> Hanjun

I'm not sure I understand the question, either.  Traditionally, the
spec versioning has been exclusively linear -- i.e., now that 6.0
has replaced 5.1, there will be no more 5.x.  There may be errata
published (e.g., there was a 5.1A, and a 6.0A is forthcoming) but
the errata are not encoded in tables anywhere since they are meant
only as corrections to the base version.  This is unlikely to change,
but not impossible, of course :).

The only reason for putting in macros for 5.1 and 6.0 is that those
are the versions that I'm concerned with for this particular fix and
I know I will use them.  If others are needed, I'd have those that
need them add them.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH v3 1/3] ACPI : introduce macros for using the ACPI specification version

[Al Stone]

On 07/02/2015 11:16 PM, Hanjun Guo wrote:
> Hi Al,
> 
> On 2015/7/3 7:48, Al Stone wrote:
>> Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version
>> number from a major and minor revision number.  Add also macros that use
>> the helper to construct the current version from the values in the FADT
>> (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
>>
>> These macros are added in order to simplify retrieving and comparing ACPI
>> specification version numbers, since this is becoming a more frequent need.
>> In particular, there are some architectures that require at least a certain
>> version of the spec, and there are differences in some structure sizes that
>> have changed with recent versions but can only be tracked by spec version
>> number.
>>
>> Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
> 
> If I understand correctly, will it be (and for next two patches as well):
> 
> 0cff8dc0099f (ACPICA: ACPI 6.0: Add changes for MADT table.)?
> 
> FADT table update was not introduce any regressions for ARM64
> I think.
> 
> Thanks
> Hanjun
> 

Right.  As I noted in the discussion over v2 of these patches, the FADT
change is correctly implementing a change for ACPI 6.0.  However, it is this
change which adds eight bytes to the GICC subtable so that the struct length
and the length field in the GICC subtable no longer match, which is what
causes BAD_MADT_ENTRY to fail.

So, while this commit is correct, the patches in this set are needed to work
around the ambiguity this causes in the spec and the ACPICA implementation of
it.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH v3 2/3] ACPI / ARM64: add BAD_MADT_GICC_ENTRY() macro

[Al Stone]

On 07/03/2015 08:06 AM, Catalin Marinas wrote:
> On Thu, Jul 02, 2015 at 05:48:35PM -0600, Al Stone wrote:
>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
>> index 39248d3..a3c26a4 100644
>> --- a/arch/arm64/include/asm/acpi.h
>> +++ b/arch/arm64/include/asm/acpi.h
>> @@ -19,6 +19,17 @@
>>  #include <asm/psci.h>
>>  #include <asm/smp_plat.h>
>>  
>> +/* Macros for consistency checks of the GICC subtable of MADT */
>> +#define ACPI_MADT_GICC_51_LENGTH       76
>> +#define ACPI_MADT_GICC_60_LENGTH       80
>> +
>> +#define BAD_MADT_GICC_ENTRY(entry, end) (                                   \
>> +		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
>> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) &&   \
>> +		 (entry->header.length != ACPI_MADT_GICC_51_LENGTH))    ||  \
>> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) &&   \
>> +		 (entry->header.length != ACPI_MADT_GICC_60_LENGTH)))
> 
> This looks ugly but, well, we could live with this.

Nod.  It's right at the hairy edge of becoming a function, I think.

> However, I'd like to avoid having to extend this macro every time we get
> a new spec released, like 6.1 defining another 80 or 84 etc. So, how
> about we only update this when there is an actual change in the length?
> Something like:
> 
> #define ACPI_MADT_GICC_LENGTH	({					\
> 	u8 length;							\
> 	if (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0)	\
> 		length = 76;						\
> 	else								\
> 		length = 80;						\
> 	length;								\
> })
> 
> or just:
> 
> #define ACPI_MADT_GICC_LENGTH	\
> 	(ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0 ? 76 : 80)
> 
> (the latter is simpler but may not look nice if we change it again in
> 6.1; though we could re-write this macro when needed, not a problem)
> 

Perhaps the sanity checking for the MADT subtables needs to be revisited
and a more general solution provided -- this is not the only MADT subtable
with this problem and it may occur again.

Even the versions above are not technically compliant with the spec.  If
we implement what the spec currently says, it might look something like
this:

#define ACPI_MADT_GICC_LENGTH ({					\
	u8 length;							\
	switch (ACPI_FADT_SPEC_VERSION) {				\
	case ACPI_FADT_SPEC_VERSION_5_0:				\
		length = 40;						\
		break;							\
	case ACPI_FADT_SPEC_VERSION_5_1:				\
		length = 76;						\
		break;							\
	default:	/* use 6.0 size */				\
		length = 80;						\
	}								\
	length;								\
})

So it's just messy and there will be a need for change.  Let me think about
making this a function instead of a macro; it may make sense to really fix
BAD_MADT_ENTRY in general instead of just dealing with the GICC subtable,
but it could also be overkill.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH v3 1/3] ACPI : introduce macros for using the ACPI specification version

[Rafael J. Wysocki]

On Friday, July 03, 2015 01:22:13 PM Al Stone wrote:
> On 07/02/2015 11:23 PM, Hanjun Guo wrote:
> > Hi Rafael,
> > 
> > On 2015/7/3 8:21, Rafael J. Wysocki wrote:
> >> On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
> >>> Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version
> >>> number from a major and minor revision number.  Add also macros that use
> >>> the helper to construct the current version from the values in the FADT
> >>> (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
> >>>
> >>> These macros are added in order to simplify retrieving and comparing ACPI
> >>> specification version numbers, since this is becoming a more frequent need.
> >>> In particular, there are some architectures that require at least a certain
> >>> version of the spec, and there are differences in some structure sizes that
> >>> have changed with recent versions but can only be tracked by spec version
> >>> number.
> >>>
> >>> Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
> >>> Signed-off-by: Al Stone <al.stone@linaro.org>
> >>> ---
> >>>  include/linux/acpi.h | 10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> >>> index c471dfc..0e525e8 100644
> >>> --- a/include/linux/acpi.h
> >>> +++ b/include/linux/acpi.h
> >>> @@ -48,6 +48,16 @@
> >>>  #include <acpi/acpi_io.h>
> >>>  #include <asm/acpi.h>
> >>>  
> >>> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
> >>> +	((unsigned int)major << 8 | (unsigned int)minor)
> >>> +
> >>> +#define ACPI_FADT_SPEC_VERSION	\
> >>> +	__ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
> >>> +				 acpi_gbl_FADT.minor_revision)
> >>> +
> >>> +#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1)
> >>> +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
> >> I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0
> > 
> > Agreed.
> 
> Will do.  This was a flip of the coin, on my part.
> 
> >> And what if there is 5.2 or even 5.3?
> > 
> > Hmm, do you mean in the future or just now? for both two cases,
> > only 5.1 and 5.1 errata (still has the same ACPI version with 5.1)
> > will be available, then jump to 6.0 and going forward if new versions
> > in the future.
> > 
> > I'm not sure if I understand your question correctly, if
> > not, please correct me :)
> > 
> > Thanks
> > Hanjun
> 
> I'm not sure I understand the question, either.  Traditionally, the
> spec versioning has been exclusively linear -- i.e., now that 6.0
> has replaced 5.1, there will be no more 5.x.  There may be errata
> published (e.g., there was a 5.1A, and a 6.0A is forthcoming) but
> the errata are not encoded in tables anywhere since they are meant
> only as corrections to the base version.  This is unlikely to change,
> but not impossible, of course :).
> 
> The only reason for putting in macros for 5.1 and 6.0 is that those
> are the versions that I'm concerned with for this particular fix and
> I know I will use them.  If others are needed, I'd have those that
> need them add them.

It seems to me that you only need to compare acpi_gbl_FADT.header.revision
with 6 for this fix, though (if less than 6, use the old way, or use the
new way otherwise).

Isn't that the case?

Rafael

Re: [PATCH v3 2/3] ACPI / ARM64: add BAD_MADT_GICC_ENTRY() macro

[Rafael J. Wysocki]

On Friday, July 03, 2015 01:51:36 PM Al Stone wrote:
> On 07/03/2015 08:06 AM, Catalin Marinas wrote:
> > On Thu, Jul 02, 2015 at 05:48:35PM -0600, Al Stone wrote:
> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> >> index 39248d3..a3c26a4 100644
> >> --- a/arch/arm64/include/asm/acpi.h
> >> +++ b/arch/arm64/include/asm/acpi.h
> >> @@ -19,6 +19,17 @@
> >>  #include <asm/psci.h>
> >>  #include <asm/smp_plat.h>
> >>  
> >> +/* Macros for consistency checks of the GICC subtable of MADT */
> >> +#define ACPI_MADT_GICC_51_LENGTH       76
> >> +#define ACPI_MADT_GICC_60_LENGTH       80
> >> +
> >> +#define BAD_MADT_GICC_ENTRY(entry, end) (                                   \
> >> +		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
> >> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) &&   \
> >> +		 (entry->header.length != ACPI_MADT_GICC_51_LENGTH))    ||  \
> >> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) &&   \
> >> +		 (entry->header.length != ACPI_MADT_GICC_60_LENGTH)))
> > 
> > This looks ugly but, well, we could live with this.
> 
> Nod.  It's right at the hairy edge of becoming a function, I think.
> 
> > However, I'd like to avoid having to extend this macro every time we get
> > a new spec released, like 6.1 defining another 80 or 84 etc. So, how
> > about we only update this when there is an actual change in the length?
> > Something like:
> > 
> > #define ACPI_MADT_GICC_LENGTH	({					\
> > 	u8 length;							\
> > 	if (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0)	\
> > 		length = 76;						\
> > 	else								\
> > 		length = 80;						\
> > 	length;								\
> > })
> > 
> > or just:
> > 
> > #define ACPI_MADT_GICC_LENGTH	\
> > 	(ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0 ? 76 : 80)
> > 
> > (the latter is simpler but may not look nice if we change it again in
> > 6.1; though we could re-write this macro when needed, not a problem)
> > 
> 
> Perhaps the sanity checking for the MADT subtables needs to be revisited
> and a more general solution provided -- this is not the only MADT subtable
> with this problem and it may occur again.
> 
> Even the versions above are not technically compliant with the spec.  If
> we implement what the spec currently says, it might look something like
> this:
> 
> #define ACPI_MADT_GICC_LENGTH ({					\
> 	u8 length;							\
> 	switch (ACPI_FADT_SPEC_VERSION) {				\
> 	case ACPI_FADT_SPEC_VERSION_5_0:				\
> 		length = 40;						\
> 		break;							\
> 	case ACPI_FADT_SPEC_VERSION_5_1:				\
> 		length = 76;						\
> 		break;							\
> 	default:	/* use 6.0 size */				\
> 		length = 80;						\
> 	}								\
> 	length;								\
> })
> 
> So it's just messy and there will be a need for change.  Let me think about
> making this a function instead of a macro; it may make sense to really fix
> BAD_MADT_ENTRY in general instead of just dealing with the GICC subtable,
> but it could also be overkill.

So here's my suggestion.

First, make ARM64 boot with 4.2+ in the simplest way possible.

Second, set out to fix BAD_MADT_ENTRY() etc.  Start with fixing ACPICA to
distinguish between the different formats depending on the spec version and
follow up from there.

Thanks,
Rafael

Re: [PATCH v3 2/3] ACPI / ARM64: add BAD_MADT_GICC_ENTRY() macro

[Catalin Marinas]

On Sat, Jul 04, 2015 at 01:54:15AM +0200, Rafael J. Wysocki wrote:
> On Friday, July 03, 2015 01:51:36 PM Al Stone wrote:
> > Perhaps the sanity checking for the MADT subtables needs to be revisited
> > and a more general solution provided -- this is not the only MADT subtable
> > with this problem and it may occur again.
> > 
> > Even the versions above are not technically compliant with the spec.  If
> > we implement what the spec currently says, it might look something like
> > this:
> > 
> > #define ACPI_MADT_GICC_LENGTH ({					\
> > 	u8 length;							\
> > 	switch (ACPI_FADT_SPEC_VERSION) {				\
> > 	case ACPI_FADT_SPEC_VERSION_5_0:				\
> > 		length = 40;						\
> > 		break;							\
> > 	case ACPI_FADT_SPEC_VERSION_5_1:				\
> > 		length = 76;						\
> > 		break;							\
> > 	default:	/* use 6.0 size */				\
> > 		length = 80;						\
> > 	}								\
> > 	length;								\
> > })
> > 
> > So it's just messy and there will be a need for change.  Let me think about
> > making this a function instead of a macro; it may make sense to really fix
> > BAD_MADT_ENTRY in general instead of just dealing with the GICC subtable,
> > but it could also be overkill.
> 
> So here's my suggestion.
> 
> First, make ARM64 boot with 4.2+ in the simplest way possible.
> 
> Second, set out to fix BAD_MADT_ENTRY() etc.  Start with fixing ACPICA to
> distinguish between the different formats depending on the spec version and
> follow up from there.

That's fine by me (as long as there is a plan to fix it properly,
ideally in 4.3).

-- 
Catalin

Re: [PATCH v3 2/3] ACPI / ARM64: add BAD_MADT_GICC_ENTRY() macro

[Al Stone]

On 07/03/2015 05:54 PM, Rafael J. Wysocki wrote:
> On Friday, July 03, 2015 01:51:36 PM Al Stone wrote:
>> On 07/03/2015 08:06 AM, Catalin Marinas wrote:
>>> On Thu, Jul 02, 2015 at 05:48:35PM -0600, Al Stone wrote:
>>>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
>>>> index 39248d3..a3c26a4 100644
>>>> --- a/arch/arm64/include/asm/acpi.h
>>>> +++ b/arch/arm64/include/asm/acpi.h
>>>> @@ -19,6 +19,17 @@
>>>>  #include <asm/psci.h>
>>>>  #include <asm/smp_plat.h>
>>>>  
>>>> +/* Macros for consistency checks of the GICC subtable of MADT */
>>>> +#define ACPI_MADT_GICC_51_LENGTH       76
>>>> +#define ACPI_MADT_GICC_60_LENGTH       80
>>>> +
>>>> +#define BAD_MADT_GICC_ENTRY(entry, end) (                                   \
>>>> +		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
>>>> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) &&   \
>>>> +		 (entry->header.length != ACPI_MADT_GICC_51_LENGTH))    ||  \
>>>> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) &&   \
>>>> +		 (entry->header.length != ACPI_MADT_GICC_60_LENGTH)))
>>>
>>> This looks ugly but, well, we could live with this.
>>
>> Nod.  It's right at the hairy edge of becoming a function, I think.
>>
>>> However, I'd like to avoid having to extend this macro every time we get
>>> a new spec released, like 6.1 defining another 80 or 84 etc. So, how
>>> about we only update this when there is an actual change in the length?
>>> Something like:
>>>
>>> #define ACPI_MADT_GICC_LENGTH	({					\
>>> 	u8 length;							\
>>> 	if (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0)	\
>>> 		length = 76;						\
>>> 	else								\
>>> 		length = 80;						\
>>> 	length;								\
>>> })
>>>
>>> or just:
>>>
>>> #define ACPI_MADT_GICC_LENGTH	\
>>> 	(ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0 ? 76 : 80)
>>>
>>> (the latter is simpler but may not look nice if we change it again in
>>> 6.1; though we could re-write this macro when needed, not a problem)
>>>
>>
>> Perhaps the sanity checking for the MADT subtables needs to be revisited
>> and a more general solution provided -- this is not the only MADT subtable
>> with this problem and it may occur again.
>>
>> Even the versions above are not technically compliant with the spec.  If
>> we implement what the spec currently says, it might look something like
>> this:
>>
>> #define ACPI_MADT_GICC_LENGTH ({					\
>> 	u8 length;							\
>> 	switch (ACPI_FADT_SPEC_VERSION) {				\
>> 	case ACPI_FADT_SPEC_VERSION_5_0:				\
>> 		length = 40;						\
>> 		break;							\
>> 	case ACPI_FADT_SPEC_VERSION_5_1:				\
>> 		length = 76;						\
>> 		break;							\
>> 	default:	/* use 6.0 size */				\
>> 		length = 80;						\
>> 	}								\
>> 	length;								\
>> })
>>
>> So it's just messy and there will be a need for change.  Let me think about
>> making this a function instead of a macro; it may make sense to really fix
>> BAD_MADT_ENTRY in general instead of just dealing with the GICC subtable,
>> but it could also be overkill.
> 
> So here's my suggestion.
> 
> First, make ARM64 boot with 4.2+ in the simplest way possible.

ACK.

> Second, set out to fix BAD_MADT_ENTRY() etc.  Start with fixing ACPICA to
> distinguish between the different formats depending on the spec version and
> follow up from there.
> 
> Thanks,
> Rafael

Yup, that's what I was thinking.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH v3 1/3] ACPI : introduce macros for using the ACPI specification version

[Al Stone]

On 07/03/2015 05:50 PM, Rafael J. Wysocki wrote:
> On Friday, July 03, 2015 01:22:13 PM Al Stone wrote:
>> On 07/02/2015 11:23 PM, Hanjun Guo wrote:
>>> Hi Rafael,
>>>
>>> On 2015/7/3 8:21, Rafael J. Wysocki wrote:
>>>> On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
>>>>> Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version
>>>>> number from a major and minor revision number.  Add also macros that use
>>>>> the helper to construct the current version from the values in the FADT
>>>>> (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
>>>>>
>>>>> These macros are added in order to simplify retrieving and comparing ACPI
>>>>> specification version numbers, since this is becoming a more frequent need.
>>>>> In particular, there are some architectures that require at least a certain
>>>>> version of the spec, and there are differences in some structure sizes that
>>>>> have changed with recent versions but can only be tracked by spec version
>>>>> number.
>>>>>
>>>>> Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
>>>>> Signed-off-by: Al Stone <al.stone@linaro.org>
>>>>> ---
>>>>>  include/linux/acpi.h | 10 ++++++++++
>>>>>  1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>>>>> index c471dfc..0e525e8 100644
>>>>> --- a/include/linux/acpi.h
>>>>> +++ b/include/linux/acpi.h
>>>>> @@ -48,6 +48,16 @@
>>>>>  #include <acpi/acpi_io.h>
>>>>>  #include <asm/acpi.h>
>>>>>  
>>>>> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
>>>>> +	((unsigned int)major << 8 | (unsigned int)minor)
>>>>> +
>>>>> +#define ACPI_FADT_SPEC_VERSION	\
>>>>> +	__ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
>>>>> +				 acpi_gbl_FADT.minor_revision)
>>>>> +
>>>>> +#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1)
>>>>> +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
>>>> I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0
>>>
>>> Agreed.
>>
>> Will do.  This was a flip of the coin, on my part.
>>
>>>> And what if there is 5.2 or even 5.3?
>>>
>>> Hmm, do you mean in the future or just now? for both two cases,
>>> only 5.1 and 5.1 errata (still has the same ACPI version with 5.1)
>>> will be available, then jump to 6.0 and going forward if new versions
>>> in the future.
>>>
>>> I'm not sure if I understand your question correctly, if
>>> not, please correct me :)
>>>
>>> Thanks
>>> Hanjun
>>
>> I'm not sure I understand the question, either.  Traditionally, the
>> spec versioning has been exclusively linear -- i.e., now that 6.0
>> has replaced 5.1, there will be no more 5.x.  There may be errata
>> published (e.g., there was a 5.1A, and a 6.0A is forthcoming) but
>> the errata are not encoded in tables anywhere since they are meant
>> only as corrections to the base version.  This is unlikely to change,
>> but not impossible, of course :).
>>
>> The only reason for putting in macros for 5.1 and 6.0 is that those
>> are the versions that I'm concerned with for this particular fix and
>> I know I will use them.  If others are needed, I'd have those that
>> need them add them.
> 
> It seems to me that you only need to compare acpi_gbl_FADT.header.revision
> with 6 for this fix, though (if less than 6, use the old way, or use the
> new way otherwise).
> 
> Isn't that the case?
> 
> Rafael
> 

Ah, sorry for being a bit dense.  Yes, on arm64 we only support 5.1 or
later so for now, less than 6 is a sufficient test.  The 5.0 spec defines
yet a third size for the GICC subtable with the same MADT version number
(40 bytes) but we won't initialize ACPI if we have anything before 5.1.

So, I could take this patch out of the set and reduce patch 2/3 somewhat
by just comparing against acpi_gbl_FADT.header.revision.  I think that's
what I'll do unless you think these macros have intrinsic value for other
reasons.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH v3 1/3] ACPI : introduce macros for using the ACPI specification version

[Rafael J. Wysocki]

On Monday, July 06, 2015 03:53:14 PM Al Stone wrote:
> On 07/03/2015 05:50 PM, Rafael J. Wysocki wrote:
> > On Friday, July 03, 2015 01:22:13 PM Al Stone wrote:
> >> On 07/02/2015 11:23 PM, Hanjun Guo wrote:
> >>> Hi Rafael,
> >>>
> >>> On 2015/7/3 8:21, Rafael J. Wysocki wrote:
> >>>> On Thursday, July 02, 2015 05:48:34 PM Al Stone wrote:
> >>>>> Add the __ACPI_FADT_SPEC_VERSION() helper macro to build a proper version
> >>>>> number from a major and minor revision number.  Add also macros that use
> >>>>> the helper to construct the current version from the values in the FADT
> >>>>> (i.e., ACPI_FADT_SPEC_VERSION) and both the 5.1 and 6.0 versions.
> >>>>>
> >>>>> These macros are added in order to simplify retrieving and comparing ACPI
> >>>>> specification version numbers, since this is becoming a more frequent need.
> >>>>> In particular, there are some architectures that require at least a certain
> >>>>> version of the spec, and there are differences in some structure sizes that
> >>>>> have changed with recent versions but can only be tracked by spec version
> >>>>> number.
> >>>>>
> >>>>> Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
> >>>>> Signed-off-by: Al Stone <al.stone@linaro.org>
> >>>>> ---
> >>>>>  include/linux/acpi.h | 10 ++++++++++
> >>>>>  1 file changed, 10 insertions(+)
> >>>>>
> >>>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> >>>>> index c471dfc..0e525e8 100644
> >>>>> --- a/include/linux/acpi.h
> >>>>> +++ b/include/linux/acpi.h
> >>>>> @@ -48,6 +48,16 @@
> >>>>>  #include <acpi/acpi_io.h>
> >>>>>  #include <asm/acpi.h>
> >>>>>  
> >>>>> +#define __ACPI_FADT_SPEC_VERSION(major, minor) \
> >>>>> +	((unsigned int)major << 8 | (unsigned int)minor)
> >>>>> +
> >>>>> +#define ACPI_FADT_SPEC_VERSION	\
> >>>>> +	__ACPI_FADT_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
> >>>>> +				 acpi_gbl_FADT.minor_revision)
> >>>>> +
> >>>>> +#define ACPI_FADT_SPEC_VERSION_51 __ACPI_FADT_SPEC_VERSION(5, 1)
> >>>>> +#define ACPI_FADT_SPEC_VERSION_60 __ACPI_FADT_SPEC_VERSION(6, 0)
> >>>> I'd add underscores here, eg. ACPI_FADT_SPEC_VERSION_6_0
> >>>
> >>> Agreed.
> >>
> >> Will do.  This was a flip of the coin, on my part.
> >>
> >>>> And what if there is 5.2 or even 5.3?
> >>>
> >>> Hmm, do you mean in the future or just now? for both two cases,
> >>> only 5.1 and 5.1 errata (still has the same ACPI version with 5.1)
> >>> will be available, then jump to 6.0 and going forward if new versions
> >>> in the future.
> >>>
> >>> I'm not sure if I understand your question correctly, if
> >>> not, please correct me :)
> >>>
> >>> Thanks
> >>> Hanjun
> >>
> >> I'm not sure I understand the question, either.  Traditionally, the
> >> spec versioning has been exclusively linear -- i.e., now that 6.0
> >> has replaced 5.1, there will be no more 5.x.  There may be errata
> >> published (e.g., there was a 5.1A, and a 6.0A is forthcoming) but
> >> the errata are not encoded in tables anywhere since they are meant
> >> only as corrections to the base version.  This is unlikely to change,
> >> but not impossible, of course :).
> >>
> >> The only reason for putting in macros for 5.1 and 6.0 is that those
> >> are the versions that I'm concerned with for this particular fix and
> >> I know I will use them.  If others are needed, I'd have those that
> >> need them add them.
> > 
> > It seems to me that you only need to compare acpi_gbl_FADT.header.revision
> > with 6 for this fix, though (if less than 6, use the old way, or use the
> > new way otherwise).
> > 
> > Isn't that the case?
> > 
> > Rafael
> > 
> 
> Ah, sorry for being a bit dense.  Yes, on arm64 we only support 5.1 or
> later so for now, less than 6 is a sufficient test.  The 5.0 spec defines
> yet a third size for the GICC subtable with the same MADT version number
> (40 bytes) but we won't initialize ACPI if we have anything before 5.1.

OK

> So, I could take this patch out of the set and reduce patch 2/3 somewhat
> by just comparing against acpi_gbl_FADT.header.revision.  I think that's
> what I'll do unless you think these macros have intrinsic value for other
> reasons.

That would be fine by me, so please do it.

Thanks,
Rafael