It ran on the machine from the same set that we were able to reproduce
it on previously. If you or anyone else have an idea on how to stabilize
the reproducibility or have a debug patch we'll be happy to try it.
Possibly it depends on the individual machines' firmware exactly how the
relevant bits of their ACPI tables are aligned in memory?
I've started digging into that callstack - it may not be a "weird
module" but it's definitely crusty ACPI code... a238317ce818 ("ACPI:
Clean up acpi_os_map/unmap_memory() to eliminate __iomem.") looks
frankly a bit questionable in its decision to blithely cast away
__iomem, but then the rationale in aafc65c731fe ("ACPI: add arm64 to the
platforms that use ioremap") seems particularly dubious on top of that
(especially given this end result).
At a wild guess, I'm wondering if this may be sufficient:
----->8-----
@@ -277,7 +277,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)returnNULL;}-#if defined(CONFIG_IA64) || defined(CONFIG_ARM64)+#if defined(CONFIG_IA64)/* ioremap will take care of cache attributes */#define should_use_kmap(pfn) 0#else-----8<-----
otherwise we might need to refactor acpi_os_map_memory() to properly use
memremap() instead of bodging the ioremap() path.
My actual ACPI-capable dev board has a trashed filesystem at the moment,
but I guess I can try an ACPI VM with QEMU to poke further.
Robin.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
It ran on the machine from the same set that we were able to reproduce
it on previously. If you or anyone else have an idea on how to stabilize
the reproducibility or have a debug patch we'll be happy to try it.
Possibly it depends on the individual machines' firmware exactly how the
relevant bits of their ACPI tables are aligned in memory?
I've started digging into that callstack - it may not be a "weird module"
but it's definitely crusty ACPI code... a238317ce818 ("ACPI: Clean up
acpi_os_map/unmap_memory() to eliminate __iomem.") looks frankly a bit
questionable in its decision to blithely cast away __iomem, but then the
rationale in aafc65c731fe ("ACPI: add arm64 to the platforms that use
ioremap") seems particularly dubious on top of that (especially given this
end result).
At a wild guess, I'm wondering if this may be sufficient:
----->8-----
@@ -277,7 +277,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)returnNULL;}-#if defined(CONFIG_IA64) || defined(CONFIG_ARM64)+#if defined(CONFIG_IA64)/* ioremap will take care of cache attributes */#define should_use_kmap(pfn) 0#else-----8<-----
I thought the same but shouldn't acpi_os_ioremap() map it with the right
attributes? It uses the EFI maps to check what kind of memory this is.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
It ran on the machine from the same set that we were able to reproduce
it on previously. If you or anyone else have an idea on how to stabilize
the reproducibility or have a debug patch we'll be happy to try it.
Possibly it depends on the individual machines' firmware exactly how the
relevant bits of their ACPI tables are aligned in memory?
I've started digging into that callstack - it may not be a "weird module"
but it's definitely crusty ACPI code... a238317ce818 ("ACPI: Clean up
acpi_os_map/unmap_memory() to eliminate __iomem.") looks frankly a bit
questionable in its decision to blithely cast away __iomem, but then the
rationale in aafc65c731fe ("ACPI: add arm64 to the platforms that use
ioremap") seems particularly dubious on top of that (especially given this
end result).
At a wild guess, I'm wondering if this may be sufficient:
----->8-----
@@ -277,7 +277,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)returnNULL;}-#if defined(CONFIG_IA64) || defined(CONFIG_ARM64)+#if defined(CONFIG_IA64)/* ioremap will take care of cache attributes */#define should_use_kmap(pfn) 0#else-----8<-----
I thought the same but shouldn't acpi_os_ioremap() map it with the right
attributes? It uses the EFI maps to check what kind of memory this is.
Oh crikey, I missed that branch of the rabbit hole... I guess that must
mean that the tables being poked here are *not* covered by the EFI
memory map, so page_is_ram() is unlikely to help either :(
Robin.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
It ran on the machine from the same set that we were able to reproduce
it on previously. If you or anyone else have an idea on how to
stabilize
the reproducibility or have a debug patch we'll be happy to try it.
Possibly it depends on the individual machines' firmware exactly how the
relevant bits of their ACPI tables are aligned in memory?
I've started digging into that callstack - it may not be a "weird
module"
but it's definitely crusty ACPI code... a238317ce818 ("ACPI: Clean up
acpi_os_map/unmap_memory() to eliminate __iomem.") looks frankly a bit
questionable in its decision to blithely cast away __iomem, but then the
rationale in aafc65c731fe ("ACPI: add arm64 to the platforms that use
ioremap") seems particularly dubious on top of that (especially given
this
end result).
At a wild guess, I'm wondering if this may be sufficient:
----->8-----
acpi_size size)
return NULL;
}
-#if defined(CONFIG_IA64) || defined(CONFIG_ARM64)
+#if defined(CONFIG_IA64)
/* ioremap will take care of cache attributes */
#define should_use_kmap(pfn) 0
#else
-----8<-----
I thought the same but shouldn't acpi_os_ioremap() map it with the right
attributes? It uses the EFI maps to check what kind of memory this is.
Oh crikey, I missed that branch of the rabbit hole... I guess that must
mean that the tables being poked here are *not* covered by the EFI
memory map, so page_is_ram() is unlikely to help either :(
After picking through the UEFI spec I think I've now got a clearer
picture of what's happening, but I'm not sure where it goes from here...
The spec implies that it *is* legitimate for runtime-loaded ACPI tables
to lie outside the EFI memory map, and that case they must be assumed to
be uncached, so the behaviour of acpi_os_ioremap() is correct. Given the
definition of uncached for arm64 memory types though, that means that
callers of acpi_os_map_memory() still have to be prepared to get an
__iomem pointer back even if they know they're mapping a table rather
than some random bit of MMIO for an AML method.
Therefore in this case it seems the blame lies partway between
acpi_os_map_memory() for casting away __iomem and acpi_data_show() for
letting an arbitrary offset lead to an arbitrarily-aligned memcpy(), but
I don't know what the best way to fix it is. Either way I've satisfied
myself that it's not an issue with the arm64 code itself - I do wonder
whether this might also be a problem on IA-64 given
ACPI_MISALIGNMENT_NOT_SUPPORTED, and I guess RISC-V may have alignment
concerns as well.
Robin.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
It ran on the machine from the same set that we were able to reproduce
it on previously. If you or anyone else have an idea on how
to stabilize
the reproducibility or have a debug patch we'll be happy to try it.
Possibly it depends on the individual machines' firmware exactly how the
relevant bits of their ACPI tables are aligned in memory?
I've started digging into that callstack - it may not be a
"weird module"
but it's definitely crusty ACPI code... a238317ce818 ("ACPI: Clean up
acpi_os_map/unmap_memory() to eliminate __iomem.") looks frankly a bit
questionable in its decision to blithely cast away __iomem, but then the
rationale in aafc65c731fe ("ACPI: add arm64 to the platforms that use
ioremap") seems particularly dubious on top of that (especially
given this
end result).
At a wild guess, I'm wondering if this may be sufficient:
----->8-----
acpi_size size)
return NULL;
}
-#if defined(CONFIG_IA64) || defined(CONFIG_ARM64)
+#if defined(CONFIG_IA64)
/* ioremap will take care of cache attributes */
#define should_use_kmap(pfn) 0
#else
-----8<-----
I thought the same but shouldn't acpi_os_ioremap() map it with the right
attributes? It uses the EFI maps to check what kind of memory this is.
Oh crikey, I missed that branch of the rabbit hole... I guess that must
mean that the tables being poked here are *not* covered by the EFI
memory map, so page_is_ram() is unlikely to help either :(
After picking through the UEFI spec I think I've now got a clearer picture
of what's happening, but I'm not sure where it goes from here...
The spec implies that it *is* legitimate for runtime-loaded ACPI tables to
lie outside the EFI memory map, and that case they must be assumed to be
uncached, so the behaviour of acpi_os_ioremap() is correct.
I'd agree with the reasoning, it would be good to pinpoint whether
that's what actually triggers the issue.
I'd like to replicate it if possible (it is TX2 HW but firmware
config is likely to differ from the HW I have at hand), the
test command line that triggers the fault would be useful as
a starting point.
Furthermore, is this a v5.13-rc* regression ? If so it would be
good to bisect it - I can't recollect arm64 changes that could
have introduced this regression in the last cycle but I may have
missed something.
Given the definition of uncached for arm64 memory types though, that
means that callers of acpi_os_map_memory() still have to be prepared
to get an __iomem pointer back even if they know they're mapping a
table rather than some random bit of MMIO for an AML method.
Therefore in this case it seems the blame lies partway between
acpi_os_map_memory() for casting away __iomem and acpi_data_show() for
letting an arbitrary offset lead to an arbitrarily-aligned memcpy(), but I
don't know what the best way to fix it is. Either way I've satisfied myself
that it's not an issue with the arm64 code itself - I do wonder whether this
might also be a problem on IA-64 given ACPI_MISALIGNMENT_NOT_SUPPORTED, and
I guess RISC-V may have alignment concerns as well.
Yes agreed but see above, this code has been there for aeons if it
is a v5.13-rc* regression it must be something else that actually
triggered it (test/FW config).
Thanks for looking into this.
Lorenzo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
It ran on the machine from the same set that we were able to reproduce
it on previously. If you or anyone else have an idea on how
to stabilize
the reproducibility or have a debug patch we'll be happy to try it.
Possibly it depends on the individual machines' firmware exactly how the
relevant bits of their ACPI tables are aligned in memory?
I've started digging into that callstack - it may not be a
"weird module"
but it's definitely crusty ACPI code... a238317ce818 ("ACPI: Clean up
acpi_os_map/unmap_memory() to eliminate __iomem.") looks frankly a bit
questionable in its decision to blithely cast away __iomem, but then the
rationale in aafc65c731fe ("ACPI: add arm64 to the platforms that use
ioremap") seems particularly dubious on top of that (especially
given this
end result).
At a wild guess, I'm wondering if this may be sufficient:
----->8-----
acpi_size size)
return NULL;
}
-#if defined(CONFIG_IA64) || defined(CONFIG_ARM64)
+#if defined(CONFIG_IA64)
/* ioremap will take care of cache attributes */
#define should_use_kmap(pfn) 0
#else
-----8<-----
I thought the same but shouldn't acpi_os_ioremap() map it with the right
attributes? It uses the EFI maps to check what kind of memory this is.
Oh crikey, I missed that branch of the rabbit hole... I guess that must
mean that the tables being poked here are *not* covered by the EFI
memory map, so page_is_ram() is unlikely to help either :(
After picking through the UEFI spec I think I've now got a clearer picture
of what's happening, but I'm not sure where it goes from here...
The spec implies that it *is* legitimate for runtime-loaded ACPI tables to
lie outside the EFI memory map, and that case they must be assumed to be
uncached, so the behaviour of acpi_os_ioremap() is correct.
I'd agree with the reasoning, it would be good to pinpoint whether
that's what actually triggers the issue.
I'd like to replicate it if possible (it is TX2 HW but firmware
config is likely to differ from the HW I have at hand), the
test command line that triggers the fault would be useful as
a starting point.
Furthermore, is this a v5.13-rc* regression ? If so it would be
good to bisect it - I can't recollect arm64 changes that could
have introduced this regression in the last cycle but I may have
missed something.
The actual change which has brought this to light is the update to
arm64's memcpy() routine for 5.13 - the new version is more aggressive
at making unaligned loads from the source buffer, so now triggers
alignment faults more readily when (wrongly) used on iomem mappings in
places that were getting away with it by chance under the previous
implementation (see also [1], for example).
Thanks,
Robin.
[1]
https://lore.kernel.org/linux-arm-kernel/20210608153344.3813661-1-narmstrong@baylibre.com/
quoted
Given the definition of uncached for arm64 memory types though, that
means that callers of acpi_os_map_memory() still have to be prepared
to get an __iomem pointer back even if they know they're mapping a
table rather than some random bit of MMIO for an AML method.
Therefore in this case it seems the blame lies partway between
acpi_os_map_memory() for casting away __iomem and acpi_data_show() for
letting an arbitrary offset lead to an arbitrarily-aligned memcpy(), but I
don't know what the best way to fix it is. Either way I've satisfied myself
that it's not an issue with the arm64 code itself - I do wonder whether this
might also be a problem on IA-64 given ACPI_MISALIGNMENT_NOT_SUPPORTED, and
I guess RISC-V may have alignment concerns as well.
Yes agreed but see above, this code has been there for aeons if it
is a v5.13-rc* regression it must be something else that actually
triggered it (test/FW config).
Thanks for looking into this.
Lorenzo
It ran on the machine from the same set that we were able to reproduce
it on previously. If you or anyone else have an idea on how
to stabilize the reproducibility or have a debug patch we'll be happy to try it.
Possibly it depends on the individual machines' firmware exactly how the
relevant bits of their ACPI tables are aligned in memory?
I've started digging into that callstack - it may not be a "weird module"
but it's definitely crusty ACPI code... a238317ce818 ("ACPI: Clean up
acpi_os_map/unmap_memory() to eliminate __iomem.") looks frankly a bit
questionable in its decision to blithely cast away __iomem, but then the
rationale in aafc65c731fe ("ACPI: add arm64 to the platforms that use
ioremap") seems particularly dubious on top of that (especially
given this end result).
[...]
quoted
quoted
After picking through the UEFI spec I think I've now got a clearer picture
of what's happening, but I'm not sure where it goes from here...
The spec implies that it *is* legitimate for runtime-loaded ACPI tables to
lie outside the EFI memory map, and that case they must be assumed to be
uncached, so the behaviour of acpi_os_ioremap() is correct.
I'd agree with the reasoning, it would be good to pinpoint whether
that's what actually triggers the issue.
I'd like to replicate it if possible (it is TX2 HW but firmware
config is likely to differ from the HW I have at hand), the
test command line that triggers the fault would be useful as
a starting point.
Furthermore, is this a v5.13-rc* regression ? If so it would be
good to bisect it - I can't recollect arm64 changes that could
have introduced this regression in the last cycle but I may have
missed something.
The actual change which has brought this to light is the update to arm64's
memcpy() routine for 5.13 - the new version is more aggressive at making
unaligned loads from the source buffer, so now triggers alignment faults
more readily when (wrongly) used on iomem mappings in places that were
getting away with it by chance under the previous implementation (see also
[1], for example).
I wouldn't revert any of the memcpy() stuff as it just uncovered an
existing bug in how the ACPI tables are handled. Could we actually hit
a similar issue with C code parsing the ACPI tables?
Is there a way to map the ACPI tables as Normal Noncacheable
(ioremap_wc)? Presumably no-one sane would place ACPI tables in memory
that's sensitive to the access size.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
It ran on the machine from the same set that we were able to reproduce
it on previously. If you or anyone else have an idea on how
to stabilize the reproducibility or have a debug patch we'll be happy to try it.
Possibly it depends on the individual machines' firmware exactly how the
relevant bits of their ACPI tables are aligned in memory?
I've started digging into that callstack - it may not be a "weird module"
but it's definitely crusty ACPI code... a238317ce818 ("ACPI: Clean up
acpi_os_map/unmap_memory() to eliminate __iomem.") looks frankly a bit
questionable in its decision to blithely cast away __iomem, but then the
rationale in aafc65c731fe ("ACPI: add arm64 to the platforms that use
ioremap") seems particularly dubious on top of that (especially
given this end result).
[...]
quoted
quoted
quoted
After picking through the UEFI spec I think I've now got a clearer picture
of what's happening, but I'm not sure where it goes from here...
The spec implies that it *is* legitimate for runtime-loaded ACPI tables to
lie outside the EFI memory map, and that case they must be assumed to be
uncached, so the behaviour of acpi_os_ioremap() is correct.
I'd agree with the reasoning, it would be good to pinpoint whether
that's what actually triggers the issue.
I'd like to replicate it if possible (it is TX2 HW but firmware
config is likely to differ from the HW I have at hand), the
test command line that triggers the fault would be useful as
a starting point.
Furthermore, is this a v5.13-rc* regression ? If so it would be
good to bisect it - I can't recollect arm64 changes that could
have introduced this regression in the last cycle but I may have
missed something.
The actual change which has brought this to light is the update to arm64's
memcpy() routine for 5.13 - the new version is more aggressive at making
unaligned loads from the source buffer, so now triggers alignment faults
more readily when (wrongly) used on iomem mappings in places that were
getting away with it by chance under the previous implementation (see also
[1], for example).
I wouldn't revert any of the memcpy() stuff as it just uncovered an
existing bug in how the ACPI tables are handled. Could we actually hit
a similar issue with C code parsing the ACPI tables?
I agree - I don't think a revert should be considered, this looks like
a long standing ACPI bug.
This needs debugging but I believe that it all depends on the table
being in the EFI map or not. I'd help a lot if I managed to reproduce
the bug for a given set-up so that we can check which table is causing
it.
Is there a way to map the ACPI tables as Normal Noncacheable
(ioremap_wc)?
That's a good point. IIUC UEFI 2.9 (2.3.6) requires tables loaded at
runtime (see above - I really would like to understand what table
is triggering this bug) that are not in the EFI memory map and whose
attributes cannot be retrieved through ACPI descriptors to be considered
non-cacheable.
The question is whether [arm64] acpi_os_ioremap() can be changed so that
the above is mapped to Normal NC rather than device-nGnRnE; this may
cause surprises the other way around (given that dev-nGnRnE is an
all encompassing fallback - again IIUC, I believe Ard knows better
than me if he has time to chime in).
We need a reproducer and some tracing in the ACPI code.
Lorenzo
Presumably no-one sane would place ACPI tables in memory that's
sensitive to the access size.
It ran on the machine from the same set that we were able to reproduce
it on previously. If you or anyone else have an idea on how
to stabilize the reproducibility or have a debug patch we'll be happy to try it.
Possibly it depends on the individual machines' firmware exactly how the
relevant bits of their ACPI tables are aligned in memory?
I've started digging into that callstack - it may not be a "weird module"
but it's definitely crusty ACPI code... a238317ce818 ("ACPI: Clean up
acpi_os_map/unmap_memory() to eliminate __iomem.") looks frankly a bit
questionable in its decision to blithely cast away __iomem, but then the
rationale in aafc65c731fe ("ACPI: add arm64 to the platforms that use
ioremap") seems particularly dubious on top of that (especially
given this end result).
[...]
quoted
quoted
quoted
After picking through the UEFI spec I think I've now got a clearer picture
of what's happening, but I'm not sure where it goes from here...
The spec implies that it *is* legitimate for runtime-loaded ACPI tables to
lie outside the EFI memory map, and that case they must be assumed to be
uncached, so the behaviour of acpi_os_ioremap() is correct.
I'd agree with the reasoning, it would be good to pinpoint whether
that's what actually triggers the issue.
I'd like to replicate it if possible (it is TX2 HW but firmware
config is likely to differ from the HW I have at hand), the
test command line that triggers the fault would be useful as
a starting point.
Furthermore, is this a v5.13-rc* regression ? If so it would be
good to bisect it - I can't recollect arm64 changes that could
have introduced this regression in the last cycle but I may have
missed something.
The actual change which has brought this to light is the update to arm64's
memcpy() routine for 5.13 - the new version is more aggressive at making
unaligned loads from the source buffer, so now triggers alignment faults
more readily when (wrongly) used on iomem mappings in places that were
getting away with it by chance under the previous implementation (see also
[1], for example).
I wouldn't revert any of the memcpy() stuff as it just uncovered an
existing bug in how the ACPI tables are handled. Could we actually hit
a similar issue with C code parsing the ACPI tables?
I agree - I don't think a revert should be considered, this looks like
a long standing ACPI bug.
This needs debugging but I believe that it all depends on the table
being in the EFI map or not. I'd help a lot if I managed to reproduce
the bug for a given set-up so that we can check which table is causing
it.
quoted
Is there a way to map the ACPI tables as Normal Noncacheable
(ioremap_wc)?
That's a good point. IIUC UEFI 2.9 (2.3.6) requires tables loaded at
runtime (see above - I really would like to understand what table
is triggering this bug) that are not in the EFI memory map and whose
attributes cannot be retrieved through ACPI descriptors to be considered
non-cacheable.
The question is whether [arm64] acpi_os_ioremap() can be changed so that
the above is mapped to Normal NC rather than device-nGnRnE; this may
cause surprises the other way around (given that dev-nGnRnE is an
all encompassing fallback - again IIUC, I believe Ard knows better
than me if he has time to chime in).
We need a reproducer and some tracing in the ACPI code.
Having looked even more at the sysfs code, I think this might not
actually be an ACPI table per se, but specifically only the Generic
Error Status Block pointed to by the BERT (so maybe it also requires the
machine to have experienced a boot-time error to be present?). ACPI
merely says that this is "a range of addressable memory" and "System
firmware must report this memory range as firmware reserved", so I have
no idea whether there's any specific expectation of how it's supposed to
be mapped.
Robin.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel