* Re: [PATCH 2/2] arm64: acpi: Prepare for longer MADTs
2018-10-12 19:29 [PATCH 2/2] arm64: acpi: Prepare for longer MADTs Jeremy Linton
@ 2018-10-15 9:29 ` Sudeep Holla
2018-11-01 11:31 ` Lorenzo Pieralisi
2018-11-27 15:01 ` Lorenzo Pieralisi
2 siblings, 0 replies; 6+ messages in thread
From: Sudeep Holla @ 2018-10-15 9:29 UTC (permalink / raw)
To: Jeremy Linton
Cc: linux-arm-kernel, catalin.marinas, will.deacon, ard.biesheuvel,
james.morse, rafael.j.wysocki, takahiro.akashi, lorenzo.pieralisi,
ahs3, linux-acpi, linux-kernel, Sudeep Holla
On Fri, Oct 12, 2018 at 02:29:37PM -0500, Jeremy Linton wrote:
> The BAD_MADT_GICC_ENTRY check is a little too strict because
> it rejects MADT entries that don't match the currently known
> lengths. We should remove this restriction to avoid problems
> if the table length changes. Future code which might depend on
> additional fields should be written to validate those fields
> before using them, rather than trying to globally check
> known MADT version lengths.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> arch/arm64/include/asm/acpi.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 709208dfdc8b..4d0946bd485a 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -22,12 +22,12 @@
> #include <asm/tlbflush.h>
>
> /* Macros for consistency checks of the GICC subtable of MADT */
> -#define ACPI_MADT_GICC_LENGTH \
> - (acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
> +#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \
> + struct acpi_madt_generic_interrupt, efficiency_class)
>
This makes it 76 always which is fine, just that the first user of
efficiency_class should check for the length before accessing it.
No user of efficiency_class yet, so I am fine with this change.
> #define BAD_MADT_GICC_ENTRY(entry, end) \
> - (!(entry) || (entry)->header.length != ACPI_MADT_GICC_LENGTH || \
> - (unsigned long)(entry) + ACPI_MADT_GICC_LENGTH > (end))
> + (!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
> + (unsigned long)(entry) + (entry)->header.length > (end))
>
Acked-by: Sudeep Holla <sudeep.holla@arm.com>
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] arm64: acpi: Prepare for longer MADTs
2018-10-12 19:29 [PATCH 2/2] arm64: acpi: Prepare for longer MADTs Jeremy Linton
2018-10-15 9:29 ` Sudeep Holla
@ 2018-11-01 11:31 ` Lorenzo Pieralisi
2018-11-01 14:22 ` Sudeep Holla
2018-11-27 15:01 ` Lorenzo Pieralisi
2 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Pieralisi @ 2018-11-01 11:31 UTC (permalink / raw)
To: Jeremy Linton
Cc: linux-arm-kernel, catalin.marinas, will.deacon, ard.biesheuvel,
james.morse, rafael.j.wysocki, takahiro.akashi, sudeep.holla,
ahs3, linux-acpi, linux-kernel
On Fri, Oct 12, 2018 at 02:29:37PM -0500, Jeremy Linton wrote:
> The BAD_MADT_GICC_ENTRY check is a little too strict because
> it rejects MADT entries that don't match the currently known
> lengths. We should remove this restriction to avoid problems
> if the table length changes. Future code which might depend on
> additional fields should be written to validate those fields
> before using them, rather than trying to globally check
> known MADT version lengths.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> arch/arm64/include/asm/acpi.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Hi Jeremy,
I assume there is no [PATCH 1/2] (to make sure I have not missed
anything).
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 709208dfdc8b..4d0946bd485a 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -22,12 +22,12 @@
> #include <asm/tlbflush.h>
>
> /* Macros for consistency checks of the GICC subtable of MADT */
> -#define ACPI_MADT_GICC_LENGTH \
> - (acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
> +#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \
> + struct acpi_madt_generic_interrupt, efficiency_class)
I would add a comment explaining why the efficiency_class offset
corresponds to the min length, I will do it myself before sending it
upstream.
I will take this patch and send it to Will/Catalin with ARM64 ACPI
material for v4.21.
Thanks,
Lorenzo
>
> #define BAD_MADT_GICC_ENTRY(entry, end) \
> - (!(entry) || (entry)->header.length != ACPI_MADT_GICC_LENGTH || \
> - (unsigned long)(entry) + ACPI_MADT_GICC_LENGTH > (end))
> + (!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
> + (unsigned long)(entry) + (entry)->header.length > (end))
>
> /* Basic configuration for ACPI */
> #ifdef CONFIG_ACPI
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] arm64: acpi: Prepare for longer MADTs
2018-11-01 11:31 ` Lorenzo Pieralisi
@ 2018-11-01 14:22 ` Sudeep Holla
0 siblings, 0 replies; 6+ messages in thread
From: Sudeep Holla @ 2018-11-01 14:22 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Jeremy Linton, linux-arm-kernel, catalin.marinas, will.deacon,
ard.biesheuvel, james.morse, rafael.j.wysocki, takahiro.akashi,
Sudeep Holla, ahs3, linux-acpi, linux-kernel
On Thu, Nov 01, 2018 at 11:31:36AM +0000, Lorenzo Pieralisi wrote:
> On Fri, Oct 12, 2018 at 02:29:37PM -0500, Jeremy Linton wrote:
> > The BAD_MADT_GICC_ENTRY check is a little too strict because
> > it rejects MADT entries that don't match the currently known
> > lengths. We should remove this restriction to avoid problems
> > if the table length changes. Future code which might depend on
> > additional fields should be written to validate those fields
> > before using them, rather than trying to globally check
> > known MADT version lengths.
> >
> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > ---
> > arch/arm64/include/asm/acpi.h | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
>
> Hi Jeremy,
>
> I assume there is no [PATCH 1/2] (to make sure I have not missed
> anything).
>
IIRC, it's a patch fixing module device table entry for SPE. They are
independent, just happened to be put out together.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] arm64: acpi: Prepare for longer MADTs
2018-10-12 19:29 [PATCH 2/2] arm64: acpi: Prepare for longer MADTs Jeremy Linton
2018-10-15 9:29 ` Sudeep Holla
2018-11-01 11:31 ` Lorenzo Pieralisi
@ 2018-11-27 15:01 ` Lorenzo Pieralisi
2018-11-27 16:32 ` Jeremy Linton
2 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Pieralisi @ 2018-11-27 15:01 UTC (permalink / raw)
To: Jeremy Linton
Cc: linux-arm-kernel, catalin.marinas, will.deacon, ard.biesheuvel,
james.morse, rafael.j.wysocki, takahiro.akashi, sudeep.holla,
ahs3, linux-acpi, linux-kernel
On Fri, Oct 12, 2018 at 02:29:37PM -0500, Jeremy Linton wrote:
> The BAD_MADT_GICC_ENTRY check is a little too strict because
> it rejects MADT entries that don't match the currently known
> lengths. We should remove this restriction to avoid problems
> if the table length changes. Future code which might depend on
> additional fields should be written to validate those fields
> before using them, rather than trying to globally check
> known MADT version lengths.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> arch/arm64/include/asm/acpi.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Hi Jeremy,
as mentioned, I added a comment to the macro so that in the future
we will be able to remember the logic behind it.
I pushed the commit out:
git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git acpi/madt
Please have a look, if I hear no objections I shall be sending it to
Will and Catalin to queue it for v4.21.
Thanks,
Lorenzo
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 709208dfdc8b..4d0946bd485a 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -22,12 +22,12 @@
> #include <asm/tlbflush.h>
>
> /* Macros for consistency checks of the GICC subtable of MADT */
> -#define ACPI_MADT_GICC_LENGTH \
> - (acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
> +#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \
> + struct acpi_madt_generic_interrupt, efficiency_class)
>
> #define BAD_MADT_GICC_ENTRY(entry, end) \
> - (!(entry) || (entry)->header.length != ACPI_MADT_GICC_LENGTH || \
> - (unsigned long)(entry) + ACPI_MADT_GICC_LENGTH > (end))
> + (!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
> + (unsigned long)(entry) + (entry)->header.length > (end))
>
> /* Basic configuration for ACPI */
> #ifdef CONFIG_ACPI
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] arm64: acpi: Prepare for longer MADTs
2018-11-27 15:01 ` Lorenzo Pieralisi
@ 2018-11-27 16:32 ` Jeremy Linton
0 siblings, 0 replies; 6+ messages in thread
From: Jeremy Linton @ 2018-11-27 16:32 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: linux-arm-kernel, catalin.marinas, will.deacon, ard.biesheuvel,
james.morse, rafael.j.wysocki, takahiro.akashi, sudeep.holla,
ahs3, linux-acpi, linux-kernel
Hi Lorenzo,
On 11/27/2018 09:01 AM, Lorenzo Pieralisi wrote:
> On Fri, Oct 12, 2018 at 02:29:37PM -0500, Jeremy Linton wrote:
>> The BAD_MADT_GICC_ENTRY check is a little too strict because
>> it rejects MADT entries that don't match the currently known
>> lengths. We should remove this restriction to avoid problems
>> if the table length changes. Future code which might depend on
>> additional fields should be written to validate those fields
>> before using them, rather than trying to globally check
>> known MADT version lengths.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>> arch/arm64/include/asm/acpi.h | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> Hi Jeremy,
>
> as mentioned, I added a comment to the macro so that in the future
> we will be able to remember the logic behind it.
It all looks good to me, Thanks!
>
> I pushed the commit out:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git acpi/madt
>
> Please have a look, if I hear no objections I shall be sending it to
> Will and Catalin to queue it for v4.21.
>
> Thanks,
> Lorenzo
>
>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
>> index 709208dfdc8b..4d0946bd485a 100644
>> --- a/arch/arm64/include/asm/acpi.h
>> +++ b/arch/arm64/include/asm/acpi.h
>> @@ -22,12 +22,12 @@
>> #include <asm/tlbflush.h>
>>
>> /* Macros for consistency checks of the GICC subtable of MADT */
>> -#define ACPI_MADT_GICC_LENGTH \
>> - (acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
>> +#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \
>> + struct acpi_madt_generic_interrupt, efficiency_class)
>>
>> #define BAD_MADT_GICC_ENTRY(entry, end) \
>> - (!(entry) || (entry)->header.length != ACPI_MADT_GICC_LENGTH || \
>> - (unsigned long)(entry) + ACPI_MADT_GICC_LENGTH > (end))
>> + (!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
>> + (unsigned long)(entry) + (entry)->header.length > (end))
>>
>> /* Basic configuration for ACPI */
>> #ifdef CONFIG_ACPI
>> --
>> 2.14.3
>>
^ permalink raw reply [flat|nested] 6+ messages in thread