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 <[email protected]>

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 <[email protected]>
Fixes: 3286d223fd7 ("sandbox: implement invalidate_icache_all()")
Acked-by: Heinrich Schuchardt <[email protected]>

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/[email protected]/#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

Reply via email to