[PATCH v3 1/9] sandbox: efi_loader: Correct use of addresses as pointers
Ilias Apalodimas
ilias.apalodimas at linaro.org
Fri Nov 15 14:57:27 CET 2024
On Fri, 15 Nov 2024 at 15:54, Ilias Apalodimas <ilias.apalodimas at linaro.org>
wrote:
>
>
> On Fri, 15 Nov 2024 at 15:53, Simon Glass <sjg at chromium.org> wrote:
>
>> Hi Ilias,
>>
>> On Thu, 14 Nov 2024 at 00:04, Ilias Apalodimas
>> <ilias.apalodimas at linaro.org> wrote:
>> >
>> > Hi Simon,
>> >
>> > On Tue, 12 Nov 2024 at 16:11, Simon Glass <sjg at chromium.org> 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 <sjg at chromium.org>
>> > > 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
>>
>
More information about the U-Boot
mailing list