[PATCH 1/1] cmd: fix loads, saves on sandbox

[Simon Glass]

Hi Heinrich,

On Mon, 26 Jun 2023 at 11:35, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 6/26/23 11:07, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Sun, 25 Jun 2023 at 10:54, Heinrich Schuchardt
> > <heinrich.schuchardt at 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 at canonical.com>
> >> ---
> >>   cmd/load.c | 16 ++++++++++++----
> >>   1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg at 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    */

Ah yes, the const problem. Perhaps we should have an
unmap_sysmem_const() as a work-around? But this patch is fine.

Regards,
Simon