Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1220271 > unrolled thread

Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually  replace the macro Sudeep Holla <sudeep.holla@arm.com> - 2015-09-07 17:40 +0200
    Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to  eventually replace the macro Al Stone <ahs3@redhat.com> - 2015-09-09 01:10 +0200
    Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to  eventually replace the macro Al Stone <ahs3@redhat.com> - 2015-09-09 22:00 +0200
      Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to  eventually replace the macro Sudeep Holla <sudeep.holla@arm.com> - 2015-09-10 18:30 +0200
        Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to  eventually replace the macro Al Stone <ahs3@redhat.com> - 2015-09-10 22:50 +0200
          Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to  eventually replace the macro Sudeep Holla <sudeep.holla@arm.com> - 2015-09-11 10:50 +0200

#1220271 — Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

Hi Al,

On 19/08/15 23:07, Al Stone wrote:

I finally got a chance to try this series on Juno. Well it exposed a 
firmware bug in MADT table :)

[..]

>                  acpi_tbl_entry_handler handler,
> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>                 table_end) {
>                  if (entry->type == entry_id
>                      && (!max_entries || count < max_entries)) {
> +                       if (bad_madt_entry(table_header, entry))
> +                               return -EINVAL;

Not sure if we can have the above check here unconditionally.
Currently I can see there are 2 other users of acpi_parse_entries i.e.
PCC and NUMA. So may be it can be made conditional or return success for
non-MADT tables from bad_madt_entry ?

Other than that, you can add for ARM64 specific parts:
Reviewed-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1221133 — Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

On 09/07/2015 09:32 AM, Sudeep Holla wrote:
> Hi Al,
> 
> On 19/08/15 23:07, Al Stone wrote:
> 
> I finally got a chance to try this series on Juno. Well it exposed a firmware
> bug in MADT table :)

What?  The code did what it was supposed to do :-)?  Very cool.  Good to know.

I talked to Graeme a bit, too, and he had some good suggestions for clean up.
I'll post a v3 tomorrow.

> [..]
> 
>>                  acpi_tbl_entry_handler handler,
>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>                 table_end) {
>>                  if (entry->type == entry_id
>>                      && (!max_entries || count < max_entries)) {
>> +                       if (bad_madt_entry(table_header, entry))
>> +                               return -EINVAL;
> 
> Not sure if we can have the above check here unconditionally.
> Currently I can see there are 2 other users of acpi_parse_entries i.e.
> PCC and NUMA. So may be it can be made conditional or return success for
> non-MADT tables from bad_madt_entry ?

I'll double check these uses.  I thought I had before, and based on what I
saw the check would be reasonable.  It never hurts to check again, though.

> Other than that, you can add for ARM64 specific parts:
> Reviewed-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks!

> Regards,
> Sudeep
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221672 — Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

On 09/07/2015 09:32 AM, Sudeep Holla wrote:
> Hi Al,
> 
> On 19/08/15 23:07, Al Stone wrote:
> 
> I finally got a chance to try this series on Juno. Well it exposed a firmware
> bug in MADT table :)
> 
> [..]
> 
>>                  acpi_tbl_entry_handler handler,
>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>                 table_end) {
>>                  if (entry->type == entry_id
>>                      && (!max_entries || count < max_entries)) {
>> +                       if (bad_madt_entry(table_header, entry))
>> +                               return -EINVAL;
> 
> Not sure if we can have the above check here unconditionally.
> Currently I can see there are 2 other users of acpi_parse_entries i.e.
> PCC and NUMA. So may be it can be made conditional or return success for
> non-MADT tables from bad_madt_entry ?

So, I went back and double checked the other users and they're looking at
the return value for acpi_parse_entries properly; adding in the check above
unconditionally should not cause any behavior change.  Further, despite the
name, acpi_parse_entries is only used to examine MADT subtables.  Granted,
we should probably make the name clearer at some point (too ambiguous as to
which entries are parsed right now).  Nonetheless, current usage seems to
be in order.


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222281 — Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

On 09/09/15 20:57, Al Stone wrote:
> On 09/07/2015 09:32 AM, Sudeep Holla wrote:
>> Hi Al,
>>
>> On 19/08/15 23:07, Al Stone wrote:
>>
>> I finally got a chance to try this series on Juno. Well it exposed a firmware
>> bug in MADT table :)
>>
>> [..]
>>
>>>                   acpi_tbl_entry_handler handler,
>>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>>                  table_end) {
>>>                   if (entry->type == entry_id
>>>                       && (!max_entries || count < max_entries)) {
>>> +                       if (bad_madt_entry(table_header, entry))
>>> +                               return -EINVAL;
>>
>> Not sure if we can have the above check here unconditionally.
>> Currently I can see there are 2 other users of acpi_parse_entries i.e.
>> PCC and NUMA. So may be it can be made conditional or return success for
>> non-MADT tables from bad_madt_entry ?
>
> So, I went back and double checked the other users and they're looking at
> the return value for acpi_parse_entries properly; adding in the check above
> unconditionally should not cause any behavior change.

I disagree. I populated PCCT table on Juno to get this error for
PCCT(PCCT header gets interpreted as MADT header):
"
ACPI: undefined version for either FADT 5.1 or MADT 1
Error parsing PCC subspaces from PCCT
"
And here the stacktrace:
[<ffffffc000881e58>] bad_madt_entry+0x90/0x16c
[<ffffffc000882030>] acpi_table_parse_entries+0xfc/0x180
[<ffffffc000895af8>] pcc_init+0x70/0x148

> Further, despite the name, acpi_parse_entries is only used to examine MADT
> subtables.  Granted, we should probably make the name clearer at some point
> (too ambiguous as to which entries are parsed right now).  Nonetheless, current
> usage seems to be in order.
>

 From the code inspection, I can see we have 3 users of 
acpi_parse_entries not just MADT but also PCC and NUMA/SRAT

Something like this solves this issue:
-              if (bad_madt_entry(table_header, entry))
+              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
+                      bad_madt_entry(table_header, entry)


Or am I still missing something ?

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222387 — Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

On 09/10/2015 10:20 AM, Sudeep Holla wrote:
> 
> 
> On 09/09/15 20:57, Al Stone wrote:
>> On 09/07/2015 09:32 AM, Sudeep Holla wrote:
>>> Hi Al,
>>>
>>> On 19/08/15 23:07, Al Stone wrote:
>>>
>>> I finally got a chance to try this series on Juno. Well it exposed a firmware
>>> bug in MADT table :)
>>>
>>> [..]
>>>
>>>>                   acpi_tbl_entry_handler handler,
>>>> @@ -245,6 +484,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>>>                  table_end) {
>>>>                   if (entry->type == entry_id
>>>>                       && (!max_entries || count < max_entries)) {
>>>> +                       if (bad_madt_entry(table_header, entry))
>>>> +                               return -EINVAL;
>>>
>>> Not sure if we can have the above check here unconditionally.
>>> Currently I can see there are 2 other users of acpi_parse_entries i.e.
>>> PCC and NUMA. So may be it can be made conditional or return success for
>>> non-MADT tables from bad_madt_entry ?
>>
>> So, I went back and double checked the other users and they're looking at
>> the return value for acpi_parse_entries properly; adding in the check above
>> unconditionally should not cause any behavior change.
> 
> I disagree. I populated PCCT table on Juno to get this error for
> PCCT(PCCT header gets interpreted as MADT header):
> "
> ACPI: undefined version for either FADT 5.1 or MADT 1
> Error parsing PCC subspaces from PCCT
> "
> And here the stacktrace:
> [<ffffffc000881e58>] bad_madt_entry+0x90/0x16c
> [<ffffffc000882030>] acpi_table_parse_entries+0xfc/0x180
> [<ffffffc000895af8>] pcc_init+0x70/0x148
> 
>> Further, despite the name, acpi_parse_entries is only used to examine MADT
>> subtables.  Granted, we should probably make the name clearer at some point
>> (too ambiguous as to which entries are parsed right now).  Nonetheless, current
>> usage seems to be in order.
>>
> 
> From the code inspection, I can see we have 3 users of acpi_parse_entries not
> just MADT but also PCC and NUMA/SRAT
> 
> Something like this solves this issue:
> -              if (bad_madt_entry(table_header, entry))
> +              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
> +                      bad_madt_entry(table_header, entry)
> 
> 
> Or am I still missing something ?
> 
> Regards,
> Sudeep

Nope, I missed it.  Your fix above will solve the problem; I misunderstood
how acpi_parse_entries() was being used -- somehow I had it in my head that
only MADT was in use, and just not seeing that it's being used for several
other subtable traversals also.  Sorry about that, Sudeep.  My mistake.

I'll add this fix for a v4, but I'll wait for a few days to see if I get any
additional comments -- I haven't heard from any x86, ia64 or ACPI maintainers
yet.  OTOH, it's nice to know we've already found and fixed two sets of arm64
ACPI tables that are in error by using these patches, even with the flaws :).

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222631 — Re: [PATCH v2 1/5] ACPI: add in a bad_madt_entry() function to eventually replace the macro

On 10/09/15 21:43, Al Stone wrote:
> On 09/10/2015 10:20 AM, Sudeep Holla wrote:
>>

[...]

>>
>>  From the code inspection, I can see we have 3 users of acpi_parse_entries not
>> just MADT but also PCC and NUMA/SRAT
>>
>> Something like this solves this issue:
>> -              if (bad_madt_entry(table_header, entry))
>> +              if (!strncmp(id, ACPI_SIG_MADT, 4) &&
>> +                      bad_madt_entry(table_header, entry)
>>
>>
>> Or am I still missing something ?
>
> Nope, I missed it.  Your fix above will solve the problem; I misunderstood
> how acpi_parse_entries() was being used -- somehow I had it in my head that
> only MADT was in use, and just not seeing that it's being used for several
> other subtable traversals also.  Sorry about that, Sudeep.  My mistake.
>

No worries.

> I'll add this fix for a v4, but I'll wait for a few days to see if I get any
> additional comments -- I haven't heard from any x86, ia64 or ACPI maintainers

Makes sense.

> yet.  OTOH, it's nice to know we've already found and fixed two sets of arm64
> ACPI tables that are in error by using these patches, even with the flaws :).
>

Very much true indeed :)

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web