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 <[email protected]> > > > > 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 <[email protected]> > > Fixes: 3286d223fd7 ("sandbox: implement invalidate_icache_all()") > > Acked-by: Heinrich Schuchardt <[email protected]> > > > > 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/[email protected]/#3412975 And I'm not even sure that re-working changing this to void* instead is a good idea. -- Tom
signature.asc
Description: PGP signature
