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

[Tom Rini <trini@konsulko.com>]

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

On Wed, Nov 27, 2024 at 02:09:42PM -0700, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, 27 Nov 2024 at 11:40, Heinrich Schuchardt <xypron.glpk@gmx.de> 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)
> 
> Patches are welcome :-)
> 
> They should all be ulong

Please note that they are all consistent in prototypes and
functionality. arc just says "start" rather than "addr" and everyone
uses them in the same way. The issue, as the Fixes tag shows, is that
sandbox's isn't complete.

-- 
Tom

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