Re: [v6 01/12] sandbox: efi_loader: Correct use of addresses as pointers

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 6024 bytes --]

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@chromium.org/#3412975

And I'm not even sure that re-working changing this to void* instead is
a good idea.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]