* [PATCH 1/1] cmd: fix loads, saves on sandbox
@ 2023-06-25 9:54 Heinrich Schuchardt
2023-06-26 9:07 ` Simon Glass
0 siblings, 1 reply; 5+ messages in thread
From: Heinrich Schuchardt @ 2023-06-25 9:54 UTC (permalink / raw)
To: Tom Rini; +Cc: Simon Glass, Pali Rohár, u-boot, Heinrich Schuchardt
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(-)
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);
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] cmd: fix loads, saves on sandbox
2023-06-25 9:54 [PATCH 1/1] cmd: fix loads, saves on sandbox Heinrich Schuchardt
@ 2023-06-26 9:07 ` Simon Glass
2023-06-26 10:35 ` Heinrich Schuchardt
0 siblings, 1 reply; 5+ messages in thread
From: Simon Glass @ 2023-06-26 9:07 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Tom Rini, Pali Rohár, u-boot
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.
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] cmd: fix loads, saves on sandbox
2023-06-26 9:07 ` Simon Glass
@ 2023-06-26 10:35 ` Heinrich Schuchardt
2023-06-26 11:24 ` Simon Glass
2023-07-23 13:23 ` Simon Glass
0 siblings, 2 replies; 5+ messages in thread
From: Heinrich Schuchardt @ 2023-06-26 10:35 UTC (permalink / raw)
To: Simon Glass; +Cc: Tom Rini, Pali Rohár, u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] cmd: fix loads, saves on sandbox
2023-06-26 10:35 ` Heinrich Schuchardt
@ 2023-06-26 11:24 ` Simon Glass
2023-07-23 13:23 ` Simon Glass
1 sibling, 0 replies; 5+ messages in thread
From: Simon Glass @ 2023-06-26 11:24 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Tom Rini, Pali Rohár, u-boot
Hi Heinrich,
On Mon, 26 Jun 2023 at 11:35, Heinrich Schuchardt
<heinrich.schuchardt@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@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 */
Ah yes, the const problem. Perhaps we should have an
unmap_sysmem_const() as a work-around? But this patch is fine.
Regards,
Simon
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] cmd: fix loads, saves on sandbox
2023-06-26 10:35 ` Heinrich Schuchardt
2023-06-26 11:24 ` Simon Glass
@ 2023-07-23 13:23 ` Simon Glass
1 sibling, 0 replies; 5+ messages in thread
From: Simon Glass @ 2023-07-23 13:23 UTC (permalink / raw)
To: Simon Glass; +Cc: Tom Rini, Pali Rohár, u-boot, Heinrich Schuchardt
Hi Heinrich,
On Mon, 26 Jun 2023 at 11:35, Heinrich Schuchardt
<heinrich.schuchardt@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@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>
> >
> >>
Applied to u-boot-dm, thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-07-23 13:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-25 9:54 [PATCH 1/1] cmd: fix loads, saves on sandbox Heinrich Schuchardt
2023-06-26 9:07 ` Simon Glass
2023-06-26 10:35 ` Heinrich Schuchardt
2023-06-26 11:24 ` Simon Glass
2023-07-23 13:23 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.