On 6/26/23 11:07, Simon Glass wrote:

Hi Heinrich,

On Sun, 25 Jun 2023 at 10:54, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

...

The loads and saves commands crash on the sandbox due to illegal memory access.

For command line arguments the sandbox uses a virtual address space which does not equal the addresses of the memory allocated with memmap(). Add the missing address translations for the loads and saves commands.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

cmd/load.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

...

diff --git a/cmd/load.c b/cmd/load.c index 5c4f34781d..2715cf5957 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -181,13 +181,17 @@ static ulong load_serial(long offset) } else #endif {

•                   void *dst;
  

•                    ret = lmb_reserve(&lmb, store_addr, binlen);
                     if (ret) {
                             printf("\nCannot overwrite reserved area (%08lx..%08lx)\n",
                                     store_addr, store_addr + binlen);
                             return ret;
                     }
  

•                   memcpy((char *)(store_addr), binbuf, binlen);
  

•                   dst = map_sysmem(store_addr, binlen);
  

•                   memcpy(dst, binbuf, binlen);
  

•                   unmap_sysmem(dst);
                     lmb_free(&lmb, store_addr, binlen);
                 }
                 if ((store_addr) < start_addr)
  

@@ -350,15 +354,19 @@ static int save_serial(ulong address, ulong count) if(write_record(SREC3_START)) /* write the header */ return (-1); do {

•           if(count) {                                             /* collect hex data in the buffer  */
  

•                   c = *(volatile uchar*)(address + reclen);       /* get one byte    */
  

•                   checksum += c;                                                  /* accumulate checksum */
  

•           volatile uchar *src;
  

•           src = map_sysmem(address, count);
  

•           if (count) {                            /* collect hex data in the buffer */
  

•                   c = src[reclen];                /* get one byte */
  

•                   checksum += c;                  /* accumulate checksum */
                     data[2*reclen]   = hex[(c>>4)&0x0f];
                     data[2*reclen+1] = hex[c & 0x0f];
                     data[2*reclen+2] = '\0';
                     ++reclen;
                     --count;
             }
  

•           unmap_sysmem((void *)src);
  

nit: You should not need the cast.

Without the cast I get a build warning:

cmd/load.c: In function ‘save_serial’: cmd/load.c:369:30: warning: passing argument 1 of ‘unmap_sysmem’ discards ‘volatile’ qualifier from pointer target type [-Wdiscarded-qualifiers] 369 | unmap_sysmem(src); | ^~~ In file included from include/mapmem.h:14, from cmd/load.c:22: ./arch/sandbox/include/asm/io.h:40:45: note: expected ‘const void *’ but argument is of type ‘volatile uchar *’ {aka ‘volatile unsigned char *’} 40 | static inline void unmap_sysmem(const void *vaddr) | ~~~~~~~~~~~~^~~~~

Why Wolfgang introduced volatile in the original code is unclear to me. Would anybody try to save register contents as S-records?

See commit fe8c2806cdba ("Initial revision") common/cmd_boot.c:484: c = *(volatile uchar*)(address + reclen); /* get one byte */

Best regards

Heinrich

...

             if(reclen == SREC_BYTES_PER_RECORD || count == 0) {
                     /* enough data collected for one record: dump it */
                     if(reclen) {    /* build & write a data record: */

-- 2.40.1

Regards, Simon