[v6 01/12] sandbox: efi_loader: Correct use of addresses as pointers
Heinrich Schuchardt
xypron.glpk at gmx.de
Wed Nov 27 20:13:34 CET 2024
On 27.11.24 19:51, Tom Rini wrote:
> 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 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.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> Fixes: 3286d223fd7 ("sandbox: implement invalidate_icache_all()")
>>> Acked-by: Heinrich Schuchardt <xypron.glpk at 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.
>
Looking at
boot/bootm.c:647: flush_cache(flush_start, ALIGN(load_end,
ARCH_DMA_MINALIGN) - flush_start);
where flush_start is derived the struct image_info field load. Would you
be able to tell if flush_start is a sandbox virtual address or a
pointer? I would not as struct image_info is not documented.
The sandbox implementation assumes that it is a pointer but elsewhere we
use ulong for sandbox virtual addresses (phys_addr_t).
For me this patch starts at the wrong end.
We should first properly describe the parameters of flush_cache(). Then
we can check if the code aligns with this.
Best regards
Heinrich
More information about the U-Boot
mailing list