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 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: add BAD_MADT_GICC_ENTRY() macro
ACPI / ARM64 : use the new BAD_MADT_GICC_ENTRY macro
arch/arm64/kernel/smp.c | 2 +-
drivers/irqchip/irq-gic.c | 2 +-
include/linux/acpi.h | 15 +++++++++++++++
3 files changed, 17 insertions(+), 2 deletions(-)
--
2.4.0
Add the ACPI_SPEC_VERSION() macro to build a proper version number from
a major and minor revision number. Add also the ACPI_FADT_SPEC_VERSION
that constructs a proper version number from the entries in the current
FADT.
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.
Signed-off-by: Al Stone <[email protected]>
---
include/linux/acpi.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a4acb55..33ed313 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -48,6 +48,11 @@
#include <acpi/acpi_io.h>
#include <asm/acpi.h>
+#define ACPI_SPEC_VERSION(major, minor) ((major<<8)|minor)
+#define ACPI_FADT_SPEC_VERSION \
+ ACPI_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
+ acpi_gbl_FADT.minor_revision)
+
static inline acpi_handle acpi_device_handle(struct acpi_device *adev)
{
return adev ? adev->handle : NULL;
--
2.4.0
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.
Signed-off-by: Al Stone <[email protected]>
---
include/linux/acpi.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 33ed313..d3a1758 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -127,6 +127,16 @@ static inline void acpi_initrd_override(void *data, size_t size)
(!entry) || (unsigned long)entry + sizeof(*entry) > end || \
((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
+#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_SPEC_VERSION(5, 1)) && \
+ (entry->header.length != ACPI_MADT_GICC_51_LENGTH)) || \
+ ((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(6, 0)) && \
+ (entry->header.length != ACPI_MADT_GICC_60_LENGTH)))
+
char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
void __acpi_unmap_table(char *map, unsigned long size);
int early_acpi_boot_init(void);
--
2.4.0
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.
Signed-off-by: Al Stone <[email protected]>
---
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 4b2121b..80d5984 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.0
On Thu, Jun 18, 2015 at 11:36:08PM +0100, Al Stone wrote:
> 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.
>
> Signed-off-by: Al Stone <[email protected]>
> ---
> arch/arm64/kernel/smp.c | 2 +-
> drivers/irqchip/irq-gic.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
Acked-by: Will Deacon <[email protected]>
Good to see this stuff is holding up so well...
Will
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 4b2121b..80d5984 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.0
>
On 06/19/2015 06:36 AM, Al Stone wrote:
> Add the ACPI_SPEC_VERSION() macro to build a proper version number from
> a major and minor revision number. Add also the ACPI_FADT_SPEC_VERSION
> that constructs a proper version number from the entries in the current
> FADT.
>
> 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.
>
> Signed-off-by: Al Stone <[email protected]>
> ---
> include/linux/acpi.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index a4acb55..33ed313 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -48,6 +48,11 @@
> #include <acpi/acpi_io.h>
> #include <asm/acpi.h>
>
> +#define ACPI_SPEC_VERSION(major, minor) ((major<<8)|minor)
and minor comments for code style
((major<<8)|minor) - > ((major << 8) | minor)
other than that:
Reviewed-by: Hanjun Guo <[email protected]>
Thanks
Hanjun
> +#define ACPI_FADT_SPEC_VERSION \
> + ACPI_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
> + acpi_gbl_FADT.minor_revision)
> +
> static inline acpi_handle acpi_device_handle(struct acpi_device *adev)
> {
> return adev ? adev->handle : NULL;
>
On 06/19/2015 06:36 AM, Al Stone wrote:
> 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.
>
> Signed-off-by: Al Stone <[email protected]>
> ---
> include/linux/acpi.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 33ed313..d3a1758 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -127,6 +127,16 @@ static inline void acpi_initrd_override(void *data, size_t size)
> (!entry) || (unsigned long)entry + sizeof(*entry) > end || \
> ((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
>
> +#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_SPEC_VERSION(5, 1)) && \
> + (entry->header.length != ACPI_MADT_GICC_51_LENGTH)) || \
> + ((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(6, 0)) && \
> + (entry->header.length != ACPI_MADT_GICC_60_LENGTH)))
> +
> char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
> void __acpi_unmap_table(char *map, unsigned long size);
> int early_acpi_boot_init(void);
Reviewed-by: Hanjun Guo <[email protected]>
Thanks
Hanjun
On 06/19/2015 06:36 AM, Al Stone wrote:
> 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.
>
> Signed-off-by: Al Stone <[email protected]>
> ---
> 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 4b2121b..80d5984 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;
Reviewed-by: Hanjun Guo <[email protected]>
Thanks
Hanjun
On 06/19/2015 06:36 AM, Al Stone wrote:
> 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.
I also tested on QEMU, it fixed the problem when I was using ACPICA 6.0
updates for MADT table,
Tested-by: Hanjun Guo <[email protected]>
Thanks
Hanjun
>
> 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: add BAD_MADT_GICC_ENTRY() macro
> ACPI / ARM64 : use the new BAD_MADT_GICC_ENTRY macro
>
> arch/arm64/kernel/smp.c | 2 +-
> drivers/irqchip/irq-gic.c | 2 +-
> include/linux/acpi.h | 15 +++++++++++++++
> 3 files changed, 17 insertions(+), 2 deletions(-)
>
On 06/19/2015 04:49 AM, Hanjun Guo wrote:
> On 06/19/2015 06:36 AM, Al Stone wrote:
>> Add the ACPI_SPEC_VERSION() macro to build a proper version number from
>> a major and minor revision number. Add also the ACPI_FADT_SPEC_VERSION
>> that constructs a proper version number from the entries in the current
>> FADT.
>>
>> 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.
>>
>> Signed-off-by: Al Stone <[email protected]>
>> ---
>> include/linux/acpi.h | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index a4acb55..33ed313 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -48,6 +48,11 @@
>> #include <acpi/acpi_io.h>
>> #include <asm/acpi.h>
>>
>> +#define ACPI_SPEC_VERSION(major, minor) ((major<<8)|minor)
>
> and minor comments for code style
>
> ((major<<8)|minor) - > ((major << 8) | minor)
Ah. Yeah, good idea. I can roll that into next version.
> other than that:
>
> Reviewed-by: Hanjun Guo <[email protected]>
Thanks.
> Thanks
> Hanjun
>
>> +#define ACPI_FADT_SPEC_VERSION \
>> + ACPI_SPEC_VERSION(acpi_gbl_FADT.header.revision, \
>> + acpi_gbl_FADT.minor_revision)
>> +
>> static inline acpi_handle acpi_device_handle(struct acpi_device *adev)
>> {
>> return adev ? adev->handle : NULL;
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
On 06/19/2015 04:49 AM, Hanjun Guo wrote:
> On 06/19/2015 06:36 AM, Al Stone wrote:
>> 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.
>>
>> Signed-off-by: Al Stone <[email protected]>
>> ---
>> include/linux/acpi.h | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 33ed313..d3a1758 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -127,6 +127,16 @@ static inline void acpi_initrd_override(void *data,
>> size_t size)
>> (!entry) || (unsigned long)entry + sizeof(*entry) > end || \
>> ((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
>>
>> +#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_SPEC_VERSION(5, 1)) && \
>> + (entry->header.length != ACPI_MADT_GICC_51_LENGTH)) || \
>> + ((ACPI_FADT_SPEC_VERSION == ACPI_SPEC_VERSION(6, 0)) && \
>> + (entry->header.length != ACPI_MADT_GICC_60_LENGTH)))
>> +
>> char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
>> void __acpi_unmap_table(char *map, unsigned long size);
>> int early_acpi_boot_init(void);
>
> Reviewed-by: Hanjun Guo <[email protected]>
>
> Thanks
> Hanjun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
Thanks for the review.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
On 06/19/2015 03:46 AM, Will Deacon wrote:
> On Thu, Jun 18, 2015 at 11:36:08PM +0100, Al Stone wrote:
>> 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.
>>
>> Signed-off-by: Al Stone <[email protected]>
>> ---
>> arch/arm64/kernel/smp.c | 2 +-
>> drivers/irqchip/irq-gic.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> Acked-by: Will Deacon <[email protected]>
>
> Good to see this stuff is holding up so well...
>
> Will
Thanks, Will.
>> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
>> index 4b2121b..80d5984 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.0
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
On 06/19/2015 04:54 AM, Hanjun Guo wrote:
> On 06/19/2015 06:36 AM, Al Stone wrote:
>> 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.
>
> I also tested on QEMU, it fixed the problem when I was using ACPICA 6.0
> updates for MADT table,
>
> Tested-by: Hanjun Guo <[email protected]>
>
> Thanks
> Hanjun
Cool. The patches did what they should. Looks like they work on AMD
Seattle with older firmware, also. Thanks.
>>
>> 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: add BAD_MADT_GICC_ENTRY() macro
>> ACPI / ARM64 : use the new BAD_MADT_GICC_ENTRY macro
>>
>> arch/arm64/kernel/smp.c | 2 +-
>> drivers/irqchip/irq-gic.c | 2 +-
>> include/linux/acpi.h | 15 +++++++++++++++
>> 3 files changed, 17 insertions(+), 2 deletions(-)
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
Hi Al,
On 18/06/15 23:36, Al Stone wrote:
> 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.
>
We need to get this series or a patch to remove the check(similar to
ia64) based on what Rafael prefers. Without that, platforms using ACPI
on ARM64 fails to boot with latest mainline. This blocks any testing on
ARM64/ACPI systems.
Regards,
Sudeep
On 06/30/2015 11:07 AM, Sudeep Holla wrote:
> Hi Al,
>
> On 18/06/15 23:36, Al Stone wrote:
>> 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.
>>
>
> We need to get this series or a patch to remove the check(similar to
> ia64) based on what Rafael prefers. Without that, platforms using ACPI
> on ARM64 fails to boot with latest mainline. This blocks any testing on
> ARM64/ACPI systems.
>
> Regards,
> Sudeep
I have not received any other feedback than some Reviewed-bys from
Hanjun and an ACK from Will for the arm64 patch.
And absolutely agreed: this is a blocker for arm64/ACPI, starting with
the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
Rafael? Ping? Do we need these to go through your tree or the arm64
tree? Without this series (or an ia64-like solution), we have ACPI
systems in the field that cannot boot.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
Hi Al,
On Tue, Jun 30, 2015 at 7:29 PM, Al Stone <[email protected]> wrote:
> On 06/30/2015 11:07 AM, Sudeep Holla wrote:
>> Hi Al,
>>
>> On 18/06/15 23:36, Al Stone wrote:
>>> 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.
>>>
>>
>> We need to get this series or a patch to remove the check(similar to
>> ia64) based on what Rafael prefers. Without that, platforms using ACPI
>> on ARM64 fails to boot with latest mainline. This blocks any testing on
>> ARM64/ACPI systems.
>>
>> Regards,
>> Sudeep
>
> I have not received any other feedback than some Reviewed-bys from
> Hanjun and an ACK from Will for the arm64 patch.
>
> And absolutely agreed: this is a blocker for arm64/ACPI, starting with
> the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
>
> Rafael? Ping?
I overlooked the fact that this was needed to fix a recent regression,
sorry about that.
Actually, if your patch fixes an error introduced by a specific
commit, it is good to use the Fixes: tag to indicate that. Which I
still would like to do, so which commit is fixed by this?
> Do we need these to go through your tree or the arm64
> tree? Without this series (or an ia64-like solution), we have ACPI
> systems in the field that cannot boot.
I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go
into include/linux/acpi.h. Why is it necessary in there?
Rafael
On Tue, Jun 30, 2015 at 8:25 PM, Rafael J. Wysocki <[email protected]> wrote:
> Hi Al,
>
> On Tue, Jun 30, 2015 at 7:29 PM, Al Stone <[email protected]> wrote:
>> On 06/30/2015 11:07 AM, Sudeep Holla wrote:
>>> Hi Al,
>>>
>>> On 18/06/15 23:36, Al Stone wrote:
>>>> 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.
>>>>
>>>
>>> We need to get this series or a patch to remove the check(similar to
>>> ia64) based on what Rafael prefers. Without that, platforms using ACPI
>>> on ARM64 fails to boot with latest mainline. This blocks any testing on
>>> ARM64/ACPI systems.
>>>
>>> Regards,
>>> Sudeep
>>
>> I have not received any other feedback than some Reviewed-bys from
>> Hanjun and an ACK from Will for the arm64 patch.
>>
>> And absolutely agreed: this is a blocker for arm64/ACPI, starting with
>> the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
>>
>> Rafael? Ping?
>
> I overlooked the fact that this was needed to fix a recent regression,
> sorry about that.
>
> Actually, if your patch fixes an error introduced by a specific
> commit, it is good to use the Fixes: tag to indicate that. Which I
> still would like to do, so which commit is fixed by this?
>
>> Do we need these to go through your tree or the arm64
>> tree? Without this series (or an ia64-like solution), we have ACPI
>> systems in the field that cannot boot.
>
> I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go
> into include/linux/acpi.h. Why is it necessary in there?
Like what about defining it in linux/irqchip/arm-gic-acpi.h for example?
Rafael
On 06/30/2015 12:25 PM, Rafael J. Wysocki wrote:
> Hi Al,
>
> On Tue, Jun 30, 2015 at 7:29 PM, Al Stone <[email protected]> wrote:
>> On 06/30/2015 11:07 AM, Sudeep Holla wrote:
>>> Hi Al,
>>>
>>> On 18/06/15 23:36, Al Stone wrote:
>>>> 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.
>>>>
>>>
>>> We need to get this series or a patch to remove the check(similar to
>>> ia64) based on what Rafael prefers. Without that, platforms using ACPI
>>> on ARM64 fails to boot with latest mainline. This blocks any testing on
>>> ARM64/ACPI systems.
>>>
>>> Regards,
>>> Sudeep
>>
>> I have not received any other feedback than some Reviewed-bys from
>> Hanjun and an ACK from Will for the arm64 patch.
>>
>> And absolutely agreed: this is a blocker for arm64/ACPI, starting with
>> the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
>>
>> Rafael? Ping?
>
> I overlooked the fact that this was needed to fix a recent regression,
> sorry about that.
>
> Actually, if your patch fixes an error introduced by a specific
> commit, it is good to use the Fixes: tag to indicate that. Which I
> still would like to do, so which commit is fixed by this?
>
>> Do we need these to go through your tree or the arm64
>> tree? Without this series (or an ia64-like solution), we have ACPI
>> systems in the field that cannot boot.
>
> I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go
> into include/linux/acpi.h. Why is it necessary in there?
I only placed it there since it seemed to make sense, and the issue is
generic to ACPI, not just ARM. Granted ARM is the only arch using the
GICC subtable in MADT, but this is fixing how ACPICA implemented the
spec, which in turn was ambiguous (and an errata is forthcoming to fix
that).
That being said, though, I'm definitely open to other possibilities.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
Hi Al,
On Tue, Jun 30, 2015 at 8:39 PM, Al Stone <[email protected]> wrote:
> On 06/30/2015 12:25 PM, Rafael J. Wysocki wrote:
>> Hi Al,
>>
>> On Tue, Jun 30, 2015 at 7:29 PM, Al Stone <[email protected]> wrote:
>>> On 06/30/2015 11:07 AM, Sudeep Holla wrote:
>>>> Hi Al,
>>>>
>>>> On 18/06/15 23:36, Al Stone wrote:
>>>>> 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.
>>>>>
>>>>
>>>> We need to get this series or a patch to remove the check(similar to
>>>> ia64) based on what Rafael prefers. Without that, platforms using ACPI
>>>> on ARM64 fails to boot with latest mainline. This blocks any testing on
>>>> ARM64/ACPI systems.
>>>>
>>>> Regards,
>>>> Sudeep
>>>
>>> I have not received any other feedback than some Reviewed-bys from
>>> Hanjun and an ACK from Will for the arm64 patch.
>>>
>>> And absolutely agreed: this is a blocker for arm64/ACPI, starting with
>>> the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
>>>
>>> Rafael? Ping?
>>
>> I overlooked the fact that this was needed to fix a recent regression,
>> sorry about that.
>>
>> Actually, if your patch fixes an error introduced by a specific
>> commit, it is good to use the Fixes: tag to indicate that. Which I
>> still would like to do, so which commit is fixed by this?
>>
>>> Do we need these to go through your tree or the arm64
>>> tree? Without this series (or an ia64-like solution), we have ACPI
>>> systems in the field that cannot boot.
>>
>> I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go
>> into include/linux/acpi.h. Why is it necessary in there?
>
> I only placed it there since it seemed to make sense, and the issue is
> generic to ACPI, not just ARM. Granted ARM is the only arch using the
> GICC subtable in MADT,
Precisely.
> but this is fixing how ACPICA implemented the spec,
So that should be fixed in ACPICA eventually and linux/acpi.h is not
an ACPICA file even.
It is possible to apply an ACPICA fix to Linux before it goes to
upstream ACPICA if it fixes a real problem in Linux. We've done
things like that.
> which in turn was ambiguous (and an errata is forthcoming to fix that).
>
> That being said, though, I'm definitely open to other possibilities.
So I'd prefer an ACPICA fix and if that's not viable, an ARM-specific
fix to fill the gap while ACPICA is being updated.
Thanks,
Rafael
On 06/30/2015 12:25 PM, Rafael J. Wysocki wrote:
> Hi Al,
>
> On Tue, Jun 30, 2015 at 7:29 PM, Al Stone <[email protected]> wrote:
>> On 06/30/2015 11:07 AM, Sudeep Holla wrote:
>>> Hi Al,
>>>
>>> On 18/06/15 23:36, Al Stone wrote:
>>>> 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.
>>>>
>>>
>>> We need to get this series or a patch to remove the check(similar to
>>> ia64) based on what Rafael prefers. Without that, platforms using ACPI
>>> on ARM64 fails to boot with latest mainline. This blocks any testing on
>>> ARM64/ACPI systems.
>>>
>>> Regards,
>>> Sudeep
>>
>> I have not received any other feedback than some Reviewed-bys from
>> Hanjun and an ACK from Will for the arm64 patch.
>>
>> And absolutely agreed: this is a blocker for arm64/ACPI, starting with
>> the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
>>
>> Rafael? Ping?
>
> I overlooked the fact that this was needed to fix a recent regression,
> sorry about that.
>
> Actually, if your patch fixes an error introduced by a specific
> commit, it is good to use the Fixes: tag to indicate that. Which I
> still would like to do, so which commit is fixed by this?
Ah, right. Sorry about missing the tag. On the other hand, we're not
really fixing anything so much as working around a problem in the ACPI
specification. IA64 has seen the same problem, but the choice there
was to just remove the use of BAD_MADT_ENTRY(); my preference was to keep
the safety check the macro represents, but do it properly for the MADT
subtable involved.
So, the commit that I see as the trigger is actually correct:
commit aeb823bbacc2 ("ACPICA: ACPI 6.0: Add changes for FADT table.")
That commit implements a change to the GICC subtable that is new for
ACPI 6.0, and this is the correct change. However, this commit changes
the length of the struct for the subtable. The problem is that both the
old ACPI 5.1 length field value *and* the new ACPI 6.0 length field are
now valid, but ACPICA 20150515 only has the ACPI 6.0 definition.
The right long term change is for the spec to disambiguate the different
definitions of the GICC subtable so that ACPICA knows what to implement --
and that spec change is in progress and should be noted in the next errata.
ACPICA will then pick up the errata change, I presume.
In the meantime, however, BAD_MADT_ENTRY() compares the length field of the
GICC subtable, which is now allowed to have multiple different values, with
the length of the struct holding that data, which is only the proper length
for ACPI 6.0. The macro makes no distinction between spec versions or even
MADT versions, and hence fails when it compares an ACPI 5.1 length field with
an ACPI 6.0 sized struct.
So I guess that's why the Fixes: tag did not immediately pop to mind. ACPICA
is not really broken, and the commit that triggers the problem is actually
correct. But, because of the BAD_MADT_ENTRY() macro, Linux assumes that all
MADT subtables with a length field will have that length value be the same as
the current ACPICA data structure size, which is no longer true for the GICC
subtable.
Is there a Deal-with-Spec-Weirdness tag I can use??
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
On 06/30/2015 01:05 PM, Rafael J. Wysocki wrote:
> Hi Al,
>
> On Tue, Jun 30, 2015 at 8:39 PM, Al Stone <[email protected]> wrote:
>> On 06/30/2015 12:25 PM, Rafael J. Wysocki wrote:
>>> Hi Al,
>>>
>>> On Tue, Jun 30, 2015 at 7:29 PM, Al Stone <[email protected]> wrote:
>>>> On 06/30/2015 11:07 AM, Sudeep Holla wrote:
>>>>> Hi Al,
>>>>>
>>>>> On 18/06/15 23:36, Al Stone wrote:
>>>>>> 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.
>>>>>>
>>>>>
>>>>> We need to get this series or a patch to remove the check(similar to
>>>>> ia64) based on what Rafael prefers. Without that, platforms using ACPI
>>>>> on ARM64 fails to boot with latest mainline. This blocks any testing on
>>>>> ARM64/ACPI systems.
>>>>>
>>>>> Regards,
>>>>> Sudeep
>>>>
>>>> I have not received any other feedback than some Reviewed-bys from
>>>> Hanjun and an ACK from Will for the arm64 patch.
>>>>
>>>> And absolutely agreed: this is a blocker for arm64/ACPI, starting with
>>>> the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
>>>>
>>>> Rafael? Ping?
>>>
>>> I overlooked the fact that this was needed to fix a recent regression,
>>> sorry about that.
>>>
>>> Actually, if your patch fixes an error introduced by a specific
>>> commit, it is good to use the Fixes: tag to indicate that. Which I
>>> still would like to do, so which commit is fixed by this?
>>>
>>>> Do we need these to go through your tree or the arm64
>>>> tree? Without this series (or an ia64-like solution), we have ACPI
>>>> systems in the field that cannot boot.
>>>
>>> I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go
>>> into include/linux/acpi.h. Why is it necessary in there?
>>
>> I only placed it there since it seemed to make sense, and the issue is
>> generic to ACPI, not just ARM. Granted ARM is the only arch using the
>> GICC subtable in MADT,
>
> Precisely.
>
>> but this is fixing how ACPICA implemented the spec,
>
> So that should be fixed in ACPICA eventually and linux/acpi.h is not
> an ACPICA file even.
>
> It is possible to apply an ACPICA fix to Linux before it goes to
> upstream ACPICA if it fixes a real problem in Linux. We've done
> things like that.
Fair enough. I've been reluctant to add further divergence, personally.
>> which in turn was ambiguous (and an errata is forthcoming to fix that).
>>
>> That being said, though, I'm definitely open to other possibilities.
>
> So I'd prefer an ACPICA fix and if that's not viable, an ARM-specific
> fix to fill the gap while ACPICA is being updated.
>
> Thanks,
> Rafael
Hrm. I'll look into the ACPICA fix. I'm sure it's possible, but it may
be messy. I will talk to Bob Moore and Lv Zheng about that, too. This
sort of thing has surely happened before, though.
In the meantime, I'll put together a new version of this patch that is
ARM-specific to fill the gap. Using linux/irqchip/arm-gic-acpi.h does
make sense.
Thanks for all the feedback, Rafael.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
Hi Al,
On Tue, Jun 30, 2015 at 9:45 PM, Al Stone <[email protected]> wrote:
> On 06/30/2015 12:25 PM, Rafael J. Wysocki wrote:
>> Hi Al,
>>
>> On Tue, Jun 30, 2015 at 7:29 PM, Al Stone <[email protected]> wrote:
>>> On 06/30/2015 11:07 AM, Sudeep Holla wrote:
>>>> Hi Al,
>>>>
>>>> On 18/06/15 23:36, Al Stone wrote:
>>>>> 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.
>>>>>
>>>>
>>>> We need to get this series or a patch to remove the check(similar to
>>>> ia64) based on what Rafael prefers. Without that, platforms using ACPI
>>>> on ARM64 fails to boot with latest mainline. This blocks any testing on
>>>> ARM64/ACPI systems.
>>>>
>>>> Regards,
>>>> Sudeep
>>>
>>> I have not received any other feedback than some Reviewed-bys from
>>> Hanjun and an ACK from Will for the arm64 patch.
>>>
>>> And absolutely agreed: this is a blocker for arm64/ACPI, starting with
>>> the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
>>>
>>> Rafael? Ping?
>>
>> I overlooked the fact that this was needed to fix a recent regression,
>> sorry about that.
>>
>> Actually, if your patch fixes an error introduced by a specific
>> commit, it is good to use the Fixes: tag to indicate that. Which I
>> still would like to do, so which commit is fixed by this?
>
> Ah, right. Sorry about missing the tag. On the other hand, we're not
> really fixing anything so much as working around a problem in the ACPI
> specification. IA64 has seen the same problem, but the choice there
> was to just remove the use of BAD_MADT_ENTRY(); my preference was to keep
> the safety check the macro represents, but do it properly for the MADT
> subtable involved.
>
> So, the commit that I see as the trigger is actually correct:
>
> commit aeb823bbacc2 ("ACPICA: ACPI 6.0: Add changes for FADT table.")
>
> That commit implements a change to the GICC subtable that is new for
> ACPI 6.0, and this is the correct change. However, this commit changes
> the length of the struct for the subtable. The problem is that both the
> old ACPI 5.1 length field value *and* the new ACPI 6.0 length field are
> now valid, but ACPICA 20150515 only has the ACPI 6.0 definition.
>
> The right long term change is for the spec to disambiguate the different
> definitions of the GICC subtable so that ACPICA knows what to implement --
> and that spec change is in progress and should be noted in the next errata.
> ACPICA will then pick up the errata change, I presume.
>
> In the meantime, however, BAD_MADT_ENTRY() compares the length field of the
> GICC subtable, which is now allowed to have multiple different values, with
> the length of the struct holding that data, which is only the proper length
> for ACPI 6.0. The macro makes no distinction between spec versions or even
> MADT versions, and hence fails when it compares an ACPI 5.1 length field with
> an ACPI 6.0 sized struct.
>
> So I guess that's why the Fixes: tag did not immediately pop to mind. ACPICA
> is not really broken, and the commit that triggers the problem is actually
> correct.
Well, Linux is technically broken if it doesn't boot on systems where
it worked previously and the "Fixes:" tag refers to Linux commits
(which might have come from ACPICA, but they are Linux commits
nevertheless).
So if a given commit breaks Linux boot on any systems, putting it in
the "Fixes:" tag is definitely justified even though the commit might
be regarded as valid in a different context.
That said I still don't think that include/linux/acpi.h is the right
place for the new macro.
You're working around the problem specifically for ARM64, so the
workaround should be contained withing the ARM64 code (and I'm not
talking about patch [1/3] which adds macros that make sense in
general).
Thanks,
Rafael
Hi Al,
On Fri, Jun 19, 2015 at 12:36 AM, Al Stone <[email protected]> wrote:
> Add the ACPI_SPEC_VERSION() macro to build a proper version number from
> a major and minor revision number. Add also the ACPI_FADT_SPEC_VERSION
> that constructs a proper version number from the entries in the current
> FADT.
>
> 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.
>
> Signed-off-by: Al Stone <[email protected]>
> ---
> include/linux/acpi.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index a4acb55..33ed313 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -48,6 +48,11 @@
> #include <acpi/acpi_io.h>
> #include <asm/acpi.h>
>
> +#define ACPI_SPEC_VERSION(major, minor) ((major<<8)|minor)
One nit here.
acpi_gbl_FADT.header.revision is of type u8 originally, so shifting it
by 8 bit positions only works due to some implicit type casting I
suppose.
Moreover, it is not entirely clear why the macro is specific to the
computation of the ACPI spec version.
So I'd drop ACPI_SPEC_VERSION and only define ACPI_FADT_SPEC_VERSION
as something like
#define ACPI_FADT_SPEC_VERSION (((unsigned
int)acpi_gbl_FADT.header.revision << 8) | (unsigned
int)acpi_gbl_FADT.minor_revision)
Thanks,
Rafael
On 06/30/2015 02:12 PM, Rafael J. Wysocki wrote:
> Hi Al,
>
> On Fri, Jun 19, 2015 at 12:36 AM, Al Stone <[email protected]> wrote:
>> Add the ACPI_SPEC_VERSION() macro to build a proper version number from
>> a major and minor revision number. Add also the ACPI_FADT_SPEC_VERSION
>> that constructs a proper version number from the entries in the current
>> FADT.
>>
>> 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.
>>
>> Signed-off-by: Al Stone <[email protected]>
>> ---
>> include/linux/acpi.h | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index a4acb55..33ed313 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -48,6 +48,11 @@
>> #include <acpi/acpi_io.h>
>> #include <asm/acpi.h>
>>
>> +#define ACPI_SPEC_VERSION(major, minor) ((major<<8)|minor)
>
> One nit here.
>
> acpi_gbl_FADT.header.revision is of type u8 originally, so shifting it
> by 8 bit positions only works due to some implicit type casting I
> suppose.
Bah. That was being sloppy on my part. Sorry about that. Will fix.
> Moreover, it is not entirely clear why the macro is specific to the
> computation of the ACPI spec version.
As far as I know, that's the only way to extract the spec version from
tables; I don't recall there being any other table with that info. Since
I will likely use this again, it seemed to make sense at the time.
> So I'd drop ACPI_SPEC_VERSION and only define ACPI_FADT_SPEC_VERSION
> as something like
>
> #define ACPI_FADT_SPEC_VERSION (((unsigned
> int)acpi_gbl_FADT.header.revision << 8) | (unsigned
> int)acpi_gbl_FADT.minor_revision)
>
> Thanks,
> Rafael
>
Sure. That makes sense. It makes it clearer that this is the version
just from the FADT. I'll do that.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
[email protected]
-----------------------------------
On 2015/7/1 2:35, Rafael J. Wysocki wrote:
> On Tue, Jun 30, 2015 at 8:25 PM, Rafael J. Wysocki <[email protected]> wrote:
>> Hi Al,
>>
>> On Tue, Jun 30, 2015 at 7:29 PM, Al Stone <[email protected]> wrote:
>>> On 06/30/2015 11:07 AM, Sudeep Holla wrote:
>>>> Hi Al,
>>>>
>>>> On 18/06/15 23:36, Al Stone wrote:
>>>>> 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.
>>>>>
>>>> We need to get this series or a patch to remove the check(similar to
>>>> ia64) based on what Rafael prefers. Without that, platforms using ACPI
>>>> on ARM64 fails to boot with latest mainline. This blocks any testing on
>>>> ARM64/ACPI systems.
>>>>
>>>> Regards,
>>>> Sudeep
>>> I have not received any other feedback than some Reviewed-bys from
>>> Hanjun and an ACK from Will for the arm64 patch.
>>>
>>> And absolutely agreed: this is a blocker for arm64/ACPI, starting with
>>> the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
>>>
>>> Rafael? Ping?
>> I overlooked the fact that this was needed to fix a recent regression,
>> sorry about that.
>>
>> Actually, if your patch fixes an error introduced by a specific
>> commit, it is good to use the Fixes: tag to indicate that. Which I
>> still would like to do, so which commit is fixed by this?
>>
>>> Do we need these to go through your tree or the arm64
>>> tree? Without this series (or an ia64-like solution), we have ACPI
>>> systems in the field that cannot boot.
>> I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go
>> into include/linux/acpi.h. Why is it necessary in there?
> Like what about defining it in linux/irqchip/arm-gic-acpi.h for example?
>
This BAD_MADT_GICC_ENTRY is both used by SMP init and GIC irqchip init for
ARM64, would it be good to put BAD_MADT_GICC_ENTRY in arch/arm64/include/asm/acpi.h?
Thanks
Hanjun
On 2015/7/1 5:15, Al Stone wrote:
> On 06/30/2015 02:12 PM, Rafael J. Wysocki wrote:
>> Hi Al,
>>
>> On Fri, Jun 19, 2015 at 12:36 AM, Al Stone <[email protected]> wrote:
>>> Add the ACPI_SPEC_VERSION() macro to build a proper version number from
>>> a major and minor revision number. Add also the ACPI_FADT_SPEC_VERSION
>>> that constructs a proper version number from the entries in the current
>>> FADT.
>>>
>>> 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.
>>>
>>> Signed-off-by: Al Stone <[email protected]>
>>> ---
>>> include/linux/acpi.h | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>>> index a4acb55..33ed313 100644
>>> --- a/include/linux/acpi.h
>>> +++ b/include/linux/acpi.h
>>> @@ -48,6 +48,11 @@
>>> #include <acpi/acpi_io.h>
>>> #include <asm/acpi.h>
>>>
>>> +#define ACPI_SPEC_VERSION(major, minor) ((major<<8)|minor)
>> One nit here.
>>
>> acpi_gbl_FADT.header.revision is of type u8 originally, so shifting it
>> by 8 bit positions only works due to some implicit type casting I
>> suppose.
> Bah. That was being sloppy on my part. Sorry about that. Will fix.
>
>> Moreover, it is not entirely clear why the macro is specific to the
>> computation of the ACPI spec version.
> As far as I know, that's the only way to extract the spec version from
> tables; I don't recall there being any other table with that info. Since
> I will likely use this again, it seemed to make sense at the time.
That's true from ACPI 5.1, as we discussed in ACPI spec working group,
FADT Major Version and FADT Minor Version are recognized as ACPI Major/Minor
version. yes, the spec itself didn't state that explicitly, maybe a ECR
to make it explicit will be good, I can prepare one if that makes sense.
Thanks
Hanjun
On 06/30/2015 08:06 PM, Hanjun Guo wrote:
> On 2015/7/1 2:35, Rafael J. Wysocki wrote:
>> On Tue, Jun 30, 2015 at 8:25 PM, Rafael J. Wysocki <[email protected]> wrote:
>>> Hi Al,
>>>
>>> On Tue, Jun 30, 2015 at 7:29 PM, Al Stone <[email protected]> wrote:
>>>> On 06/30/2015 11:07 AM, Sudeep Holla wrote:
>>>>> Hi Al,
>>>>>
>>>>> On 18/06/15 23:36, Al Stone wrote:
>>>>>> 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.
>>>>>>
>>>>> We need to get this series or a patch to remove the check(similar to
>>>>> ia64) based on what Rafael prefers. Without that, platforms using ACPI
>>>>> on ARM64 fails to boot with latest mainline. This blocks any testing on
>>>>> ARM64/ACPI systems.
>>>>>
>>>>> Regards,
>>>>> Sudeep
>>>> I have not received any other feedback than some Reviewed-bys from
>>>> Hanjun and an ACK from Will for the arm64 patch.
>>>>
>>>> And absolutely agreed: this is a blocker for arm64/ACPI, starting with
>>>> the ACPICA 20150515 patches which appear to have gone in with 4.2-rc1.
>>>>
>>>> Rafael? Ping?
>>> I overlooked the fact that this was needed to fix a recent regression,
>>> sorry about that.
>>>
>>> Actually, if your patch fixes an error introduced by a specific
>>> commit, it is good to use the Fixes: tag to indicate that. Which I
>>> still would like to do, so which commit is fixed by this?
>>>
>>>> Do we need these to go through your tree or the arm64
>>>> tree? Without this series (or an ia64-like solution), we have ACPI
>>>> systems in the field that cannot boot.
>>> I'm not quite sure why the definition of BAD_MADT_GICC_ENTRY has to go
>>> into include/linux/acpi.h. Why is it necessary in there?
>> Like what about defining it in linux/irqchip/arm-gic-acpi.h for example?
>>
>
> This BAD_MADT_GICC_ENTRY is both used by SMP init and GIC irqchip init for
> ARM64, would it be good to put BAD_MADT_GICC_ENTRY in arch/arm64/include/asm/acpi.h?
>
> Thanks
> Hanjun
Ah, right. Good point. Let me try it in that file, then. It is -- for the
time being -- arm64 specific.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------