[PATCH 0/5] sandbox: Fix memory corruption around 1MB
From: Simon Glass <sjg@chromium.org> After many hours of debugging, it turns out that the PCI EA driver is mapping itself into RAM at 1MB. This happens to be where the kernel ends up, with the vbe_abrec_os bootmeth. Since measurement is enabled, bootm_measure() calls map_sysmem() on the kernel in order to measure it. easurement takes place, although of course using the wrong data. Then, through a strange sequence of events, which I have found very hard to narrow down, the malloc() heap is corrupted. This series provides a fix. To repeat this problem: NO_LTO=1 ./test/py/test.py --bd sandbox --build -k "(ut or vbe) and not efi and not slow and not dm" which dies when running vbe_test_abrec_oem_norun: ... => echo $? 0 => host bind 0 [...]/build-sandbox/persistent-data/vbe1.img => ut -f bootstd vbe_test_abrec_oem_norun Test: bootstd_setup_for_tests: bootstd_common.c common/dlmalloc.c:793: do_check_free_chunk: Assertion `next->prev_size == sz' failed. resetting ... With this series, the above now passes. Simon Glass (5): sandbox: Swap the wanted and got messages in map_physmem() sandbox: Move PCI EA space out of RAM sandbox: Use the exact mapping size for PCI in pmc test: pci: Use the exact mapping size for PCI EA tests sandbox: Abort if a partition memory map is detected arch/sandbox/cpu/mem.c | 13 +++++++++++-- arch/sandbox/include/asm/test.h | 8 ++++---- doc/arch/sandbox/sandbox.rst | 1 + drivers/power/acpi_pmc/sandbox.c | 2 +- test/dm/pci.c | 14 +++++++++----- 5 files changed, 26 insertions(+), 12 deletions(-) -- 2.43.0 base-commit: f85258951dfb3f818e19daeaaddf543ddfc433bc branch: maked
From: Simon Glass <sjg@chromium.org> The ordering is reversed. Fix it, to avoid confusion. Signed-off-by: Simon Glass <sjg@chromium.org> --- arch/sandbox/cpu/mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/sandbox/cpu/mem.c b/arch/sandbox/cpu/mem.c index 010dc9c16a0..4737a45b002 100644 --- a/arch/sandbox/cpu/mem.c +++ b/arch/sandbox/cpu/mem.c @@ -139,7 +139,7 @@ void *map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags) if (enable_pci_map && !pci_map_physmem(paddr, &len, &map_dev, &ptr)) { if (plen != len) { printf("%s: Warning: partial map at %x, wanted %lx, got %lx\n", - __func__, (uint)paddr, len, plen); + __func__, (uint)paddr, plen, len); } map_len = len; log_debug("pci map %lx -> %p\n", (ulong)paddr, ptr); -- 2.43.0
From: Simon Glass <sjg@chromium.org> The address chosen for testing this feature is 1MB which is part of the sandbox RAM. When devices access this, e.g. with map_sysmem(), the memory is mapped to a PCI device. Any changes then apply to that device and are not written to memory. Reads also come from the device. It is not safe to use RAM space in this way. A symptom that something is wrong is the log message: map_physmem: Warning: partial map at 100000, wanted 4, got 2000 Move the memory out of the way and document it. Signed-off-by: Simon Glass <sjg@chromium.org> Fixes: 21ebbafde8d ("test: dm: Add a test for PCI Enhanced Allocation") --- arch/sandbox/include/asm/test.h | 8 ++++---- doc/arch/sandbox/sandbox.rst | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/arch/sandbox/include/asm/test.h b/arch/sandbox/include/asm/test.h index 0e8d19ce232..4df39fac1da 100644 --- a/arch/sandbox/include/asm/test.h +++ b/arch/sandbox/include/asm/test.h @@ -38,10 +38,10 @@ struct unit_test_state; #define SANDBOX_CLK_RATE 32768 /* Macros used to test PCI EA capability structure */ -#define PCI_CAP_EA_BASE_LO0 0x00100000 -#define PCI_CAP_EA_BASE_LO1 0x00110000 -#define PCI_CAP_EA_BASE_LO2 0x00120000 -#define PCI_CAP_EA_BASE_LO4 0x00140000 +#define PCI_CAP_EA_BASE_LO0 0x20100000 +#define PCI_CAP_EA_BASE_LO1 0x20110000 +#define PCI_CAP_EA_BASE_LO2 0x20120000 +#define PCI_CAP_EA_BASE_LO4 0x20140000 #define PCI_CAP_EA_BASE_HI2 0x00020000ULL #define PCI_CAP_EA_BASE_HI4 0x00040000ULL #define PCI_CAP_EA_SIZE_LO 0x0000ffff diff --git a/doc/arch/sandbox/sandbox.rst b/doc/arch/sandbox/sandbox.rst index 3bde6bdbafc..89255cb14dd 100644 --- a/doc/arch/sandbox/sandbox.rst +++ b/doc/arch/sandbox/sandbox.rst @@ -785,6 +785,7 @@ Addr Config Usage 200000 CONFIG_TRACE_EARLY_ADDR Early trace buffer (if enabled). Also used 400000 CONFIG_TEXT_BASE Load buffer for U-Boot (sandbox_spl only) 10000000 PCI address space (see test.dts) +20000000 PCI EA space (see PCI_CAP_EA_BASE_LO0) ff000000 Memory-mapping tags start here ======== ======================== =============================== -- 2.43.0
From: Simon Glass <sjg@chromium.org> Update the size to the expected size of the device's mapping, to avoid a warning in map_physmem() about a partial map. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/power/acpi_pmc/sandbox.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/power/acpi_pmc/sandbox.c b/drivers/power/acpi_pmc/sandbox.c index ed1bb198093..e48cdf8200e 100644 --- a/drivers/power/acpi_pmc/sandbox.c +++ b/drivers/power/acpi_pmc/sandbox.c @@ -70,7 +70,7 @@ static int sandbox_pmc_probe(struct udevice *dev) base = dm_pci_read_bar32(dev, 0); if (base == FDT_ADDR_T_NONE) return log_msg_ret("No base address", -EINVAL); - upriv->pmc_bar0 = map_sysmem(base, 0x2000); + upriv->pmc_bar0 = map_sysmem(base, 0x80); upriv->gpe_cfg = (u32 *)(upriv->pmc_bar0 + GPIO_GPE_CFG); return pmc_ofdata_to_uc_plat(dev); -- 2.43.0
From: Simon Glass <sjg@chromium.org> Update the sizes to the expected size of the device's mapping in each case, to avoid a warning in map_physmem() about a partial map. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/dm/pci.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/test/dm/pci.c b/test/dm/pci.c index 6eb19f6fea3..bcd274d1879 100644 --- a/test/dm/pci.c +++ b/test/dm/pci.c @@ -267,27 +267,31 @@ static int dm_test_pci_ea(struct unit_test_state *uts) ut_asserteq(PCI_CAP_ID_EA_OFFSET, cap); /* test swap case in BAR 1 */ - bar = dm_pci_map_bar(swap, PCI_BASE_ADDRESS_0, 0, 0, PCI_REGION_TYPE, 0); + bar = dm_pci_map_bar(swap, PCI_BASE_ADDRESS_0, 0, 4, PCI_REGION_TYPE, 0); ut_assertnonnull(bar); *(int *)bar = 2; /* swap upper/lower */ - bar = dm_pci_map_bar(swap, PCI_BASE_ADDRESS_1, 0, 0, PCI_REGION_TYPE, 0); + bar = dm_pci_map_bar(swap, PCI_BASE_ADDRESS_1, 0, 0xff, PCI_REGION_TYPE, + 0); ut_assertnonnull(bar); strcpy(bar, "ea TEST"); unmap_sysmem(bar); - bar = dm_pci_map_bar(swap, PCI_BASE_ADDRESS_1, 0, 0, PCI_REGION_TYPE, 0); + bar = dm_pci_map_bar(swap, PCI_BASE_ADDRESS_1, 0, 0xff, PCI_REGION_TYPE, + 0); ut_assertnonnull(bar); ut_asserteq_str("EA test", bar); /* test magic values in BARs2, 4; BAR 3 is n/a */ - bar = dm_pci_map_bar(swap, PCI_BASE_ADDRESS_2, 0, 0, PCI_REGION_TYPE, 0); + bar = dm_pci_map_bar(swap, PCI_BASE_ADDRESS_2, 0, 0xffff, + PCI_REGION_TYPE, 0); ut_assertnonnull(bar); ut_asserteq(PCI_EA_BAR2_MAGIC, *(u32 *)bar); bar = dm_pci_map_bar(swap, PCI_BASE_ADDRESS_3, 0, 0, PCI_REGION_TYPE, 0); ut_assertnull(bar); - bar = dm_pci_map_bar(swap, PCI_BASE_ADDRESS_4, 0, 0, PCI_REGION_TYPE, 0); + bar = dm_pci_map_bar(swap, PCI_BASE_ADDRESS_4, 0, 0x100000ffff, + PCI_REGION_TYPE, 0); ut_assertnonnull(bar); ut_asserteq(PCI_EA_BAR4_MAGIC, *(u32 *)bar); -- 2.43.0
From: Simon Glass <sjg@chromium.org> This can indicate that something is horribly wrong. It seems better to abort, rather than just print a message which might not be noticed. If the mapping does not map exactly, abort. Signed-off-by: Simon Glass <sjg@chromium.org> --- arch/sandbox/cpu/mem.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/arch/sandbox/cpu/mem.c b/arch/sandbox/cpu/mem.c index 4737a45b002..4e9b629e3aa 100644 --- a/arch/sandbox/cpu/mem.c +++ b/arch/sandbox/cpu/mem.c @@ -138,8 +138,17 @@ void *map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags) map_dev = NULL; if (enable_pci_map && !pci_map_physmem(paddr, &len, &map_dev, &ptr)) { if (plen != len) { - printf("%s: Warning: partial map at %x, wanted %lx, got %lx\n", + /* + * This may actually be harmless, but since this feature + * is only used in tests, it is better to fix the text + * to request the correct size. Aborting here enables + * use of gdb to figure out what went wrong. It may mask + * a very hard-to-debug problem, if sandbox's RAM is + * inadvertently mapped in. + */ + printf("%s: Fatal: partial map at %x, wanted %lx, got %lx\n", __func__, (uint)paddr, plen, len); + os_abort(); } map_len = len; log_debug("pci map %lx -> %p\n", (ulong)paddr, ptr); -- 2.43.0
participants (1)
-
Simon Glass