[PATCH v3 1/9] sandbox: efi_loader: Correct use of addresses as pointers
Simon Glass
sjg at chromium.org
Fri Nov 15 15:27:57 CET 2024
Hi Ilias,
On Fri, 15 Nov 2024 at 06:58, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
>
>
> 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
Yes, that's OK with me and is clearer, as you say.
Regards,
Simon
More information about the U-Boot
mailing list