On Fri, 15 Nov 2024 at 15:54, Ilias Apalodimas <[email protected]> wrote:
> > > On Fri, 15 Nov 2024 at 15:53, Simon Glass <[email protected]> wrote: > >> Hi Ilias, >> >> On Thu, 14 Nov 2024 at 00:04, Ilias Apalodimas >> <[email protected]> wrote: >> > >> > Hi Simon, >> > >> > On Tue, 12 Nov 2024 at 16:11, Simon Glass <[email protected]> wrote: >> > > >> > > 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()") >> > > --- >> > > >> > > (no changes since v2) >> > > >> > > 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(-) >> > > >> > > diff --git a/arch/sandbox/cpu/cache.c b/arch/sandbox/cpu/cache.c >> > > index c8a5e64214b..96b3da47e8e 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 ab1836b3f07..0b3941b7798 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 0ddf69a0918..bb58cf1badb 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), >> > >> > Shouldn't we now umap that address on the err: tag? >> >> This address should be in sandbox's DRAM, so there is no mapping >> created. The mapping is needed for things that are not in the emulated >> DRAM, such as stack variables. >> > > Isn't there a ref counter that gets increased for these though? > Why would we not want to decrease it properly? > Ah scratch that, the refcnt is not for the sandbox DRAM. But in any case, I don't like seeing that magic of people knowing what they should unmap or not. I would prefer calling the unmap call unconditionally, since the function deals with sandbox memory correctly already Thanks /Ilias > > > Thanks > /Ilias > >> >> > >> > Thanks >> > /Ilias >> > > ALIGN(virt_size, EFI_CACHELINE_SIZE)); >> > > >> > > /* >> > > -- >> > > 2.34.1 >> > > >> >> Regards, >> Simon >> >
