Re: [PATCH 1/9] sandbox: efi_loader: Correct use of addresses as pointers

[Heinrich Schuchardt]

Am 4. November 2024 14:39:45 MEZ schrieb Simon Glass <sjg at 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.
>
>As discussed some time ago, it would be good to update the way
>EFI_LOADER uses addresses and pointers, particularly around memory
>allocation. Ideally we would use addresses internally, with pointers
>only exposed via the external API, even where pointers are in fact
>u64 values.

No, we should get rid of sandbox virtual addresses where possible, e.g. in LMB.

They are are only relevant for the sandbox's command line interface and device-tree.

This will minimize the number of conversions, which only satisfy the needs of the sandbox and have no relevance on real systems including QEMU.

Do not wag the dog with the tail.

Best regards

Heinrich

>
>Signed-off-by: Simon Glass <sjg at chromium.org>
>Fixes: 3286d223fd7 ("sandbox: implement invalidate_icache_all()")
>---
>
> 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),
> 		    ALIGN(virt_size, EFI_CACHELINE_SIZE));
> 
> 	/*