On 06/30/2015 01:05 PM, Rafael J. Wysocki wrote:

Hi Al,

On Tue, Jun 30, 2015 at 8:39 PM, Al Stone [off-list ref] wrote:

quoted

On 06/30/2015 12:25 PM, Rafael J. Wysocki wrote:

quoted

Hi Al,

On Tue, Jun 30, 2015 at 7:29 PM, Al Stone [off-list ref] wrote:

quoted

On 06/30/2015 11:07 AM, Sudeep Holla wrote:

quoted

Hi Al,

On 18/06/15 23:36, Al Stone wrote:

quoted

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?

quoted

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.

quoted

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.

quoted

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.
ahs3 at redhat.com
-----------------------------------