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

[Heinrich Schuchardt]

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    */

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