On Wed, Nov 27, 2024 at 07:40:05PM +0100, Heinrich Schuchardt wrote:
On 27.11.24 18:17, Tom Rini wrote:
From: Simon Glass sjg@chromium.org
The cache-flush function is incorrect which causes a crash in the remoteproc tests with arm64.
Fix both problems by using map_sysmem() to convert an address to a pointer and map_to_sysmem() to convert a pointer to an address.
Also update the image-loader's cache-flushing logic.
Signed-off-by: Simon Glass sjg@chromium.org Fixes: 3286d223fd7 ("sandbox: implement invalidate_icache_all()") Acked-by: Heinrich Schuchardt xypron.glpk@gmx.de
Changes in v6:
- Re-introduce
Changes in v2:
Drop message about EFI_LOADER
arch/sandbox/cpu/cache.c | 8 +++++++- drivers/remoteproc/rproc-elf-loader.c | 18 +++++++++++------- lib/efi_loader/efi_image_loader.c | 3 ++- 3 files changed, 20 insertions(+), 9 deletions(-)
arch/sandbox/cpu/cache.c | 8 +++++++- drivers/remoteproc/rproc-elf-loader.c | 18 +++++++++++------- lib/efi_loader/efi_image_loader.c | 3 ++- 3 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/arch/sandbox/cpu/cache.c b/arch/sandbox/cpu/cache.c index c8a5e64214b6..96b3da47e8ed 100644 --- a/arch/sandbox/cpu/cache.c +++ b/arch/sandbox/cpu/cache.c @@ -4,12 +4,18 @@ */
#include <cpu_func.h> +#include <mapmem.h> #include <asm/state.h>
void flush_cache(unsigned long addr, unsigned long size) {
- void *ptr;
- ptr = map_sysmem(addr, size);
- /* Clang uses (char *) parameters, GCC (void *) */
- __builtin___clear_cache((void *)addr, (void *)(addr + size));
__builtin___clear_cache(map_sysmem(addr, size), ptr + size);
unmap_sysmem(ptr); }
void invalidate_icache_all(void)
diff --git a/drivers/remoteproc/rproc-elf-loader.c b/drivers/remoteproc/rproc-elf-loader.c index ab1836b3f078..0b3941b7798d 100644 --- a/drivers/remoteproc/rproc-elf-loader.c +++ b/drivers/remoteproc/rproc-elf-loader.c @@ -6,6 +6,7 @@ #include <dm.h> #include <elf.h> #include <log.h> +#include <mapmem.h> #include <remoteproc.h> #include <asm/cache.h> #include <dm/device_compat.h> @@ -180,6 +181,7 @@ int rproc_elf32_load_image(struct udevice *dev, unsigned long addr, ulong size) for (i = 0; i < ehdr->e_phnum; i++, phdr++) { void *dst = (void *)(uintptr_t)phdr->p_paddr; void *src = (void *)addr + phdr->p_offset;
ulong dst_addr;if (phdr->p_type != PT_LOAD) continue;
@@ -195,10 +197,11 @@ int rproc_elf32_load_image(struct udevice *dev, unsigned long addr, ulong size) if (phdr->p_filesz != phdr->p_memsz) memset(dst + phdr->p_filesz, 0x00, phdr->p_memsz - phdr->p_filesz);
flush_cache(rounddown((unsigned long)dst, ARCH_DMA_MINALIGN),roundup((unsigned long)dst + phdr->p_filesz,
dst_addr = map_to_sysmem(dst);flush_cache(rounddown(dst_addr, ARCH_DMA_MINALIGN),roundup(dst_addr + phdr->p_filesz, ARCH_DMA_MINALIGN) -
rounddown((unsigned long)dst, ARCH_DMA_MINALIGN));
rounddown(dst_addr, ARCH_DMA_MINALIGN));}
return 0;
@@ -377,6 +380,7 @@ int rproc_elf32_load_rsc_table(struct udevice *dev, ulong fw_addr, const struct dm_rproc_ops *ops; Elf32_Shdr *shdr; void *src, *dst;
ulong dst_addr;
shdr = rproc_elf32_find_rsc_table(dev, fw_addr, fw_size); if (!shdr)
@@ -398,10 +402,10 @@ int rproc_elf32_load_rsc_table(struct udevice *dev, ulong fw_addr, (ulong)dst, *rsc_size);
memcpy(dst, src, *rsc_size);
- flush_cache(rounddown((unsigned long)dst, ARCH_DMA_MINALIGN),
roundup((unsigned long)dst + *rsc_size,ARCH_DMA_MINALIGN) -rounddown((unsigned long)dst, ARCH_DMA_MINALIGN));
dst_addr = map_to_sysmem(dst);
flush_cache(rounddown(dst_addr, ARCH_DMA_MINALIGN),
roundup(dst_addr + *rsc_size, ARCH_DMA_MINALIGN) -rounddown(dst_addr, ARCH_DMA_MINALIGN));return 0; }
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 0ddf69a09183..bb58cf1badb7 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -13,6 +13,7 @@ #include <efi_loader.h> #include <log.h> #include <malloc.h> +#include <mapmem.h> #include <pe.h> #include <sort.h> #include <crypto/mscode.h> @@ -977,7 +978,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, }
/* Flush cache */
- flush_cache((ulong)efi_reloc,
- flush_cache(map_to_sysmem(efi_reloc), ALIGN(virt_size, EFI_CACHELINE_SIZE));
It would be nice if we could be consistent:
include/cpu_func.h:66: void flush_cache(unsigned long addr, unsigned long size);
arch/arc/lib/cache.c:773: void flush_cache(unsigned long start, unsigned long size)
arch/sandbox/cpu/cache.c:9: void flush_cache(unsigned long addr, unsigned long size)
Here sandbox calls __builtin___clear_cache() which needs a pointer.
The correct thing would be to change the signature of flush_cache to
flush_cache(void *addr, size_t size)
in all locations and do away with ulong here.
NAK to this patch.
I'm going to refer you back to yourself when Acked-by'ing this: https://patchwork.ozlabs.org/project/uboot/patch/20241112141105.640189-2-sjg...
And I'm not even sure that re-working changing this to void* instead is a good idea.