[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Heinrich Schuchardt]

Addresses in state->ram_buf must be in the low 4 GiB of the address space.
Otherwise we cannot correctly fill SMBIOS tables. This shows up in warnings
like:

    WARNING: SMBIOS table_address overflow 7f752735e020

Ensure that state->ram_buf is initialized by the first invocation of
os_malloc().

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 arch/sandbox/cpu/start.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
index e87365e800..1388dba895 100644
--- a/arch/sandbox/cpu/start.c
+++ b/arch/sandbox/cpu/start.c
@@ -439,6 +439,14 @@ int main(int argc, char *argv[])
 	int size;
 	int ret;

+	/*
+	 * This must be the first invocation of os_malloc() to have
+	 * state->ram_buf in the low 4 GiB.
+	 */
+	ret = state_init();
+	if (ret)
+		goto err;
+
 	/*
 	 * Copy argv[] so that we can pass the arguments in the original
 	 * sequence when resetting the sandbox.
@@ -453,10 +461,6 @@ int main(int argc, char *argv[])
 	gd = &data;
 	gd->arch.text_base = os_find_text_base();

-	ret = state_init();
-	if (ret)
-		goto err;
-
 	state = state_get_current();
 	if (os_parse_args(state, argc, argv))
 		return 1;
--
2.30.2

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Simon Glass]

Hi Heinrich,

On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Addresses in state->ram_buf must be in the low 4 GiB of the address space.
> Otherwise we cannot correctly fill SMBIOS tables. This shows up in warnings
> like:
>
>     WARNING: SMBIOS table_address overflow 7f752735e020

This sounds like a bug in the smbios-table code. For sandbox it should
perhaps use addresses instead of pointers.

I think that code (that I unfortunately wrote) was an expeditious way
of getting it running, but is not correct.

>
> Ensure that state->ram_buf is initialized by the first invocation of
> os_malloc().
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  arch/sandbox/cpu/start.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>

Regards,
Simon

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Heinrich Schuchardt]

Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Heinrich,
>
>On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt <xypron.glpk@gmx.de>
>wrote:
>>
>> Addresses in state->ram_buf must be in the low 4 GiB of the address
>space.
>> Otherwise we cannot correctly fill SMBIOS tables. This shows up in
>warnings
>> like:
>>
>>     WARNING: SMBIOS table_address overflow 7f752735e020
>
>This sounds like a bug in the smbios-table code. For sandbox it should
>perhaps use addresses instead of pointers.
>
>I think that code (that I unfortunately wrote) was an expeditious way
>of getting it running, but is not correct.

The field you are filling is only 32bit wide. I wonder how that table is meant to work on systems where the lowest memory address is above 4 GiB. Such ARMv8 systems exist.

Best regards

Heinrich

>
>>
>> Ensure that state->ram_buf is initialized by the first invocation of
>> os_malloc().
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  arch/sandbox/cpu/start.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>
>Regards,
>Simon

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Simon Glass]

Hi Heinrich,

On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Heinrich,
> >
> >On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >wrote:
> >>
> >> Addresses in state->ram_buf must be in the low 4 GiB of the address
> >space.
> >> Otherwise we cannot correctly fill SMBIOS tables. This shows up in
> >warnings
> >> like:
> >>
> >>     WARNING: SMBIOS table_address overflow 7f752735e020
> >
> >This sounds like a bug in the smbios-table code. For sandbox it should
> >perhaps use addresses instead of pointers.
> >
> >I think that code (that I unfortunately wrote) was an expeditious way
> >of getting it running, but is not correct.
>
> The field you are filling is only 32bit wide. I wonder how that table is meant to work on systems where the lowest memory address is above 4 GiB. Such ARMv8 systems exist.

map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS is
legacy and designed for 4GB.

Regards,
Simon

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Heinrich Schuchardt]

On 5/13/21 4:36 PM, Simon Glass wrote:
> Hi Heinrich,
>
> On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass <sjg@chromium.org>:
>>> Hi Heinrich,
>>>
>>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> wrote:
>>>>
>>>> Addresses in state->ram_buf must be in the low 4 GiB of the address
>>> space.
>>>> Otherwise we cannot correctly fill SMBIOS tables. This shows up in
>>> warnings
>>>> like:
>>>>
>>>>      WARNING: SMBIOS table_address overflow 7f752735e020
>>>
>>> This sounds like a bug in the smbios-table code. For sandbox it should
>>> perhaps use addresses instead of pointers.
>>>
>>> I think that code (that I unfortunately wrote) was an expeditious way
>>> of getting it running, but is not correct.
>>
>> The field you are filling is only 32bit wide. I wonder how that table is meant to work on systems where the lowest memory address is above 4 GiB. Such ARMv8 systems exist.
>
> map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS is
> legacy and designed for 4GB.

I know map_to_sysmem(). But you wrote in lib/smbios.c:487:

/*
 ?* We must use a pointer here so things work correctly on sandbox. The
 ?* user of this table is not aware of the mapping of addresses to
 ?* sandbox's DRAM buffer.
  */

For testing you could start a binary with command 'bootefi' or 'booti'
and that binary would analyze the table. So yes, your comment holds true
and you must not use map_to_sysmem() here.

Best regards

Heinrich

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Simon Glass]

Hi Heinrich,

On Thu, 13 May 2021 at 08:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 5/13/21 4:36 PM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >>> Hi Heinrich,
> >>>
> >>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>> wrote:
> >>>>
> >>>> Addresses in state->ram_buf must be in the low 4 GiB of the address
> >>> space.
> >>>> Otherwise we cannot correctly fill SMBIOS tables. This shows up in
> >>> warnings
> >>>> like:
> >>>>
> >>>>      WARNING: SMBIOS table_address overflow 7f752735e020
> >>>
> >>> This sounds like a bug in the smbios-table code. For sandbox it should
> >>> perhaps use addresses instead of pointers.
> >>>
> >>> I think that code (that I unfortunately wrote) was an expeditious way
> >>> of getting it running, but is not correct.
> >>
> >> The field you are filling is only 32bit wide. I wonder how that table is meant to work on systems where the lowest memory address is above 4 GiB. Such ARMv8 systems exist.
> >
> > map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS is
> > legacy and designed for 4GB.
>
> I know map_to_sysmem(). But you wrote in lib/smbios.c:487:
>
> /*
>   * We must use a pointer here so things work correctly on sandbox. The
>   * user of this table is not aware of the mapping of addresses to
>   * sandbox's DRAM buffer.
>   */
>
> For testing you could start a binary with command 'bootefi' or 'booti'
> and that binary would analyze the table. So yes, your comment holds true
> and you must not use map_to_sysmem() here.

Yes, that is a good point. Perhaps I was not mad when I wrote that.
What is using the tables on sandbox?

Regards,
SImon

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Heinrich Schuchardt]

On 5/14/21 1:56 AM, Simon Glass wrote:
> Hi Heinrich,
>
> On Thu, 13 May 2021 at 08:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 5/13/21 4:36 PM, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass <sjg@chromium.org>:
>>>>> Hi Heinrich,
>>>>>
>>>>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> wrote:
>>>>>>
>>>>>> Addresses in state->ram_buf must be in the low 4 GiB of the address
>>>>> space.
>>>>>> Otherwise we cannot correctly fill SMBIOS tables. This shows up in
>>>>> warnings
>>>>>> like:
>>>>>>
>>>>>>       WARNING: SMBIOS table_address overflow 7f752735e020
>>>>>
>>>>> This sounds like a bug in the smbios-table code. For sandbox it should
>>>>> perhaps use addresses instead of pointers.
>>>>>
>>>>> I think that code (that I unfortunately wrote) was an expeditious way
>>>>> of getting it running, but is not correct.
>>>>
>>>> The field you are filling is only 32bit wide. I wonder how that table is meant to work on systems where the lowest memory address is above 4 GiB. Such ARMv8 systems exist.
>>>
>>> map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS is
>>> legacy and designed for 4GB.
>>
>> I know map_to_sysmem(). But you wrote in lib/smbios.c:487:
>>
>> /*
>>    * We must use a pointer here so things work correctly on sandbox. The
>>    * user of this table is not aware of the mapping of addresses to
>>    * sandbox's DRAM buffer.
>>    */
>>
>> For testing you could start a binary with command 'bootefi' or 'booti'
>> and that binary would analyze the table. So yes, your comment holds true
>> and you must not use map_to_sysmem() here.
>
> Yes, that is a good point. Perhaps I was not mad when I wrote that.
> What is using the tables on sandbox?

The UEFI shell has a command smbiosview to display the SMBIOS table.

With the current U-Boot sandbox it crashes. I do not know what the cause is:

FS0:\> smbiosview

Segmentation violation
pc = 0x7fb0df88d17e, pc_reloc = 0x7fb0cf88d17e

UEFI image [0x00007fb0df836000:0x00007fb0df920c5f] pc=0x5717e
'/Shell_x64.efi'

Here is some of the output for my laptop:

SMBIOS Entry Point Structure:
Anchor String:        _SM_
EPS Checksum:         0xEE
Entry Point Len:      31
Version:              3.1
Number of Structures: 62
Max Struct size:      145
Table Address:        0x986E9000
Table Length:         2316
Entry Point revision: 0x0
SMBIOS BCD Revision:  0x31
Inter Anchor:         _DMI_
Inter Checksum:       0x4E
Formatted Area:
   00000000: 00 00 00 00 00                                   *.....*

=========================================================
Query Structure, conditions are:
QueryType   = Random
QueryHandle = Random
ShowType    = SHOW_DETAIL


Enter to continue, :q to exit, :[0-3] to change mode, /? for help
$
=========================================================
Type=18, Handle=0x0
Dump Structure as:
Index=0,Length=0x19,Addr=0x986E9019
00000000: 12 17 00 00 03 02 02 00-00 00 00 00 00 00 80 00
*................*
00000010: 00 00 80 00 00 00 80 00-00                       *.........*

Enter to continue, :q to exit, :[0-3] to change mode, /? for help
$Structure Type: 32-bit Memory Error Information
Format part Len : 23
Structure Handle: 0
32-bit Memory Error Information - Type:   OK
Memory Error - Error granularity:   Unknown
Memory Error - Error Operation:   Unknown
VendorSyndrome: 0x0
MemoryArrayErrorAddress: 0x80000000
DeviceErrorAddress: 0x80000000
ErrorResolution: 0x80000000

Best regards

Heinrich

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Simon Glass]

Hi Heinrich,

On Fri, 14 May 2021 at 00:11, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 5/14/21 1:56 AM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Thu, 13 May 2021 at 08:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 5/13/21 4:36 PM, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>
> >>>> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>> wrote:
> >>>>>>
> >>>>>> Addresses in state->ram_buf must be in the low 4 GiB of the address
> >>>>> space.
> >>>>>> Otherwise we cannot correctly fill SMBIOS tables. This shows up in
> >>>>> warnings
> >>>>>> like:
> >>>>>>
> >>>>>>       WARNING: SMBIOS table_address overflow 7f752735e020
> >>>>>
> >>>>> This sounds like a bug in the smbios-table code. For sandbox it should
> >>>>> perhaps use addresses instead of pointers.
> >>>>>
> >>>>> I think that code (that I unfortunately wrote) was an expeditious way
> >>>>> of getting it running, but is not correct.
> >>>>
> >>>> The field you are filling is only 32bit wide. I wonder how that table is meant to work on systems where the lowest memory address is above 4 GiB. Such ARMv8 systems exist.
> >>>
> >>> map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS is
> >>> legacy and designed for 4GB.
> >>
> >> I know map_to_sysmem(). But you wrote in lib/smbios.c:487:
> >>
> >> /*
> >>    * We must use a pointer here so things work correctly on sandbox. The
> >>    * user of this table is not aware of the mapping of addresses to
> >>    * sandbox's DRAM buffer.
> >>    */
> >>
> >> For testing you could start a binary with command 'bootefi' or 'booti'
> >> and that binary would analyze the table. So yes, your comment holds true
> >> and you must not use map_to_sysmem() here.
> >
> > Yes, that is a good point. Perhaps I was not mad when I wrote that.
> > What is using the tables on sandbox?
>
> The UEFI shell has a command smbiosview to display the SMBIOS table.
>
> With the current U-Boot sandbox it crashes. I do not know what the cause is:
>
> FS0:\> smbiosview
>
> Segmentation violation
> pc = 0x7fb0df88d17e, pc_reloc = 0x7fb0cf88d17e
>
> UEFI image [0x00007fb0df836000:0x00007fb0df920c5f] pc=0x5717e
> '/Shell_x64.efi'
>
> Here is some of the output for my laptop:
>
> SMBIOS Entry Point Structure:
> Anchor String:        _SM_
> EPS Checksum:         0xEE
> Entry Point Len:      31
> Version:              3.1
> Number of Structures: 62
> Max Struct size:      145
> Table Address:        0x986E9000
> Table Length:         2316
> Entry Point revision: 0x0
> SMBIOS BCD Revision:  0x31
> Inter Anchor:         _DMI_
> Inter Checksum:       0x4E
> Formatted Area:
>    00000000: 00 00 00 00 00                                   *.....*
>
> =========================================================
> Query Structure, conditions are:
> QueryType   = Random
> QueryHandle = Random
> ShowType    = SHOW_DETAIL
>
>
> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
> $
> =========================================================
> Type=18, Handle=0x0
> Dump Structure as:
> Index=0,Length=0x19,Addr=0x986E9019
> 00000000: 12 17 00 00 03 02 02 00-00 00 00 00 00 00 80 00
> *................*
> 00000010: 00 00 80 00 00 00 80 00-00                       *.........*
>
> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
> $Structure Type: 32-bit Memory Error Information
> Format part Len : 23
> Structure Handle: 0
> 32-bit Memory Error Information - Type:   OK
> Memory Error - Error granularity:   Unknown
> Memory Error - Error Operation:   Unknown
> VendorSyndrome: 0x0
> MemoryArrayErrorAddress: 0x80000000
> DeviceErrorAddress: 0x80000000
> ErrorResolution: 0x80000000

OK, I am not sure what to make of that except that it is every bit as
impenetrable as I would expect from UEFI.

Is this on a Risc-V host? I am not sure why the first alloc would get
the requested address and not the second. Is it because we specify
0x10000000 as the requested address? Is that what you actually get in
this case?

If we drop that value on subsequent calls to mmap(), does it fix the problem?

I think we should update the commit message to say we are hoping to
get 0x10000000 for the RAM buffer (although of course there is a 4K
region at the start of it) and that xx arch provides large addresses
for subsequent mmap()s.

Also, how about upgrading the smbios warning to an error. I think that
is what it is and it should stop execution.

With that said:

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Heinrich Schuchardt]

Am 14. Mai 2021 22:44:32 MESZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Heinrich,
>
>On Fri, 14 May 2021 at 00:11, Heinrich Schuchardt <xypron.glpk@gmx.de>
>wrote:
>>
>> On 5/14/21 1:56 AM, Simon Glass wrote:
>> > Hi Heinrich,
>> >
>> > On Thu, 13 May 2021 at 08:53, Heinrich Schuchardt
><xypron.glpk@gmx.de> wrote:
>> >>
>> >> On 5/13/21 4:36 PM, Simon Glass wrote:
>> >>> Hi Heinrich,
>> >>>
>> >>> On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt
><xypron.glpk@gmx.de> wrote:
>> >>>>
>> >>>> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass
><sjg@chromium.org>:
>> >>>>> Hi Heinrich,
>> >>>>>
>> >>>>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt
><xypron.glpk@gmx.de>
>> >>>>> wrote:
>> >>>>>>
>> >>>>>> Addresses in state->ram_buf must be in the low 4 GiB of the
>address
>> >>>>> space.
>> >>>>>> Otherwise we cannot correctly fill SMBIOS tables. This shows
>up in
>> >>>>> warnings
>> >>>>>> like:
>> >>>>>>
>> >>>>>>       WARNING: SMBIOS table_address overflow 7f752735e020
>> >>>>>
>> >>>>> This sounds like a bug in the smbios-table code. For sandbox it
>should
>> >>>>> perhaps use addresses instead of pointers.
>> >>>>>
>> >>>>> I think that code (that I unfortunately wrote) was an
>expeditious way
>> >>>>> of getting it running, but is not correct.
>> >>>>
>> >>>> The field you are filling is only 32bit wide. I wonder how that
>table is meant to work on systems where the lowest memory address is
>above 4 GiB. Such ARMv8 systems exist.
>> >>>
>> >>> map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS
>is
>> >>> legacy and designed for 4GB.
>> >>
>> >> I know map_to_sysmem(). But you wrote in lib/smbios.c:487:
>> >>
>> >> /*
>> >>    * We must use a pointer here so things work correctly on
>sandbox. The
>> >>    * user of this table is not aware of the mapping of addresses
>to
>> >>    * sandbox's DRAM buffer.
>> >>    */
>> >>
>> >> For testing you could start a binary with command 'bootefi' or
>'booti'
>> >> and that binary would analyze the table. So yes, your comment
>holds true
>> >> and you must not use map_to_sysmem() here.
>> >
>> > Yes, that is a good point. Perhaps I was not mad when I wrote that.
>> > What is using the tables on sandbox?
>>
>> The UEFI shell has a command smbiosview to display the SMBIOS table.
>>
>> With the current U-Boot sandbox it crashes. I do not know what the
>cause is:
>>
>> FS0:\> smbiosview
>>
>> Segmentation violation
>> pc = 0x7fb0df88d17e, pc_reloc = 0x7fb0cf88d17e
>>
>> UEFI image [0x00007fb0df836000:0x00007fb0df920c5f] pc=0x5717e
>> '/Shell_x64.efi'
>>
>> Here is some of the output for my laptop:
>>
>> SMBIOS Entry Point Structure:
>> Anchor String:        _SM_
>> EPS Checksum:         0xEE
>> Entry Point Len:      31
>> Version:              3.1
>> Number of Structures: 62
>> Max Struct size:      145
>> Table Address:        0x986E9000
>> Table Length:         2316
>> Entry Point revision: 0x0
>> SMBIOS BCD Revision:  0x31
>> Inter Anchor:         _DMI_
>> Inter Checksum:       0x4E
>> Formatted Area:
>>    00000000: 00 00 00 00 00                                   *.....*
>>
>> =========================================================
>> Query Structure, conditions are:
>> QueryType   = Random
>> QueryHandle = Random
>> ShowType    = SHOW_DETAIL
>>
>>
>> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
>> $
>> =========================================================
>> Type=18, Handle=0x0
>> Dump Structure as:
>> Index=0,Length=0x19,Addr=0x986E9019
>> 00000000: 12 17 00 00 03 02 02 00-00 00 00 00 00 00 80 00
>> *................*
>> 00000010: 00 00 80 00 00 00 80 00-00                      
>*.........*
>>
>> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
>> $Structure Type: 32-bit Memory Error Information
>> Format part Len : 23
>> Structure Handle: 0
>> 32-bit Memory Error Information - Type:   OK
>> Memory Error - Error granularity:   Unknown
>> Memory Error - Error Operation:   Unknown
>> VendorSyndrome: 0x0
>> MemoryArrayErrorAddress: 0x80000000
>> DeviceErrorAddress: 0x80000000
>> ErrorResolution: 0x80000000
>
>OK, I am not sure what to make of that except that it is every bit as
>impenetrable as I would expect from UEFI.

This is a dump of the SMBIOS tables. Why do you refer to UEFI?

>
>Is this on a Risc-V host? I am not sure why the first alloc would get
>the requested address and not the second. Is it because we specify
>0x10000000 as the requested address? Is that what you actually get in
>this case?

The observation is reproducible on x86_64.

Two mmap calls cannot receive the same address without calling munmap in between.

The first call *might* return the address 0x10000000 that you ask for.

If the first call cannot receive it because it is not available, why should subsequent calls succeed?

>
>If we drop that value on subsequent calls to mmap(), does it fix the
>problem?

If you drop that value you get a random address. What would be better in this case?

>
>I think we should update the commit message to say we are hoping to
>get 0x10000000 for the RAM buffer (although of course there is a 4K
>region at the start of it) and that xx arch provides large addresses
>for subsequent mmap()s.

What 4 KiB region are you referring to?

Why do you expect any specific address in subsequent calls? The man-page states "If another mapping already exists there, the kernel picks a new address that may or may not depend on the hint."

For performance reasons the kernel should not care about the hint in this case.

>
>Also, how about upgrading the smbios warning to an error. I think that
>is what it is and it should stop execution.

For errors we use log_err(). But where is the error? mmap() comes with absolutely no guarantee that it will assign the address that you are requesting or anything close to it.

Smbios is not important enough to make the sandbox unusable in this case.

Best regards

Heinrich

>
>With that said:
>
>Reviewed-by: Simon Glass <sjg@chromium.org>
>
>Regards,
>Simon

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Simon Glass]

Hi Heinrich,

On Fri, 14 May 2021 at 18:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 14. Mai 2021 22:44:32 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Heinrich,
> >
> >On Fri, 14 May 2021 at 00:11, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >wrote:
> >>
> >> On 5/14/21 1:56 AM, Simon Glass wrote:
> >> > Hi Heinrich,
> >> >
> >> > On Thu, 13 May 2021 at 08:53, Heinrich Schuchardt
> ><xypron.glpk@gmx.de> wrote:
> >> >>
> >> >> On 5/13/21 4:36 PM, Simon Glass wrote:
> >> >>> Hi Heinrich,
> >> >>>
> >> >>> On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt
> ><xypron.glpk@gmx.de> wrote:
> >> >>>>
> >> >>>> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass
> ><sjg@chromium.org>:
> >> >>>>> Hi Heinrich,
> >> >>>>>
> >> >>>>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt
> ><xypron.glpk@gmx.de>
> >> >>>>> wrote:
> >> >>>>>>
> >> >>>>>> Addresses in state->ram_buf must be in the low 4 GiB of the
> >address
> >> >>>>> space.
> >> >>>>>> Otherwise we cannot correctly fill SMBIOS tables. This shows
> >up in
> >> >>>>> warnings
> >> >>>>>> like:
> >> >>>>>>
> >> >>>>>>       WARNING: SMBIOS table_address overflow 7f752735e020
> >> >>>>>
> >> >>>>> This sounds like a bug in the smbios-table code. For sandbox it
> >should
> >> >>>>> perhaps use addresses instead of pointers.
> >> >>>>>
> >> >>>>> I think that code (that I unfortunately wrote) was an
> >expeditious way
> >> >>>>> of getting it running, but is not correct.
> >> >>>>
> >> >>>> The field you are filling is only 32bit wide. I wonder how that
> >table is meant to work on systems where the lowest memory address is
> >above 4 GiB. Such ARMv8 systems exist.
> >> >>>
> >> >>> map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS
> >is
> >> >>> legacy and designed for 4GB.
> >> >>
> >> >> I know map_to_sysmem(). But you wrote in lib/smbios.c:487:
> >> >>
> >> >> /*
> >> >>    * We must use a pointer here so things work correctly on
> >sandbox. The
> >> >>    * user of this table is not aware of the mapping of addresses
> >to
> >> >>    * sandbox's DRAM buffer.
> >> >>    */
> >> >>
> >> >> For testing you could start a binary with command 'bootefi' or
> >'booti'
> >> >> and that binary would analyze the table. So yes, your comment
> >holds true
> >> >> and you must not use map_to_sysmem() here.
> >> >
> >> > Yes, that is a good point. Perhaps I was not mad when I wrote that.
> >> > What is using the tables on sandbox?
> >>
> >> The UEFI shell has a command smbiosview to display the SMBIOS table.
> >>
> >> With the current U-Boot sandbox it crashes. I do not know what the
> >cause is:
> >>
> >> FS0:\> smbiosview
> >>
> >> Segmentation violation
> >> pc = 0x7fb0df88d17e, pc_reloc = 0x7fb0cf88d17e
> >>
> >> UEFI image [0x00007fb0df836000:0x00007fb0df920c5f] pc=0x5717e
> >> '/Shell_x64.efi'
> >>
> >> Here is some of the output for my laptop:
> >>
> >> SMBIOS Entry Point Structure:
> >> Anchor String:        _SM_
> >> EPS Checksum:         0xEE
> >> Entry Point Len:      31
> >> Version:              3.1
> >> Number of Structures: 62
> >> Max Struct size:      145
> >> Table Address:        0x986E9000
> >> Table Length:         2316
> >> Entry Point revision: 0x0
> >> SMBIOS BCD Revision:  0x31
> >> Inter Anchor:         _DMI_
> >> Inter Checksum:       0x4E
> >> Formatted Area:
> >>    00000000: 00 00 00 00 00                                   *.....*
> >>
> >> =========================================================
> >> Query Structure, conditions are:
> >> QueryType   = Random
> >> QueryHandle = Random
> >> ShowType    = SHOW_DETAIL
> >>
> >>
> >> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
> >> $
> >> =========================================================
> >> Type=18, Handle=0x0
> >> Dump Structure as:
> >> Index=0,Length=0x19,Addr=0x986E9019
> >> 00000000: 12 17 00 00 03 02 02 00-00 00 00 00 00 00 80 00
> >> *................*
> >> 00000010: 00 00 80 00 00 00 80 00-00
> >*.........*
> >>
> >> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
> >> $Structure Type: 32-bit Memory Error Information
> >> Format part Len : 23
> >> Structure Handle: 0
> >> 32-bit Memory Error Information - Type:   OK
> >> Memory Error - Error granularity:   Unknown
> >> Memory Error - Error Operation:   Unknown
> >> VendorSyndrome: 0x0
> >> MemoryArrayErrorAddress: 0x80000000
> >> DeviceErrorAddress: 0x80000000
> >> ErrorResolution: 0x80000000
> >
> >OK, I am not sure what to make of that except that it is every bit as
> >impenetrable as I would expect from UEFI.
>
> This is a dump of the SMBIOS tables. Why do you refer to UEFI?

How do you fit so many questions in one email? :-)

I assumed smbiosview is a UEFI command because of the prompt. I have
never heard of the command.

>
> >
> >Is this on a Risc-V host? I am not sure why the first alloc would get
> >the requested address and not the second. Is it because we specify
> >0x10000000 as the requested address? Is that what you actually get in
> >this case?
>
> The observation is reproducible on x86_64.
>
> Two mmap calls cannot receive the same address without calling munmap in between.
>
> The first call *might* return the address 0x10000000 that you ask for.
>
> If the first call cannot receive it because it is not available, why should subsequent calls succeed?

The goal is to keep the addresses small since 64-bit addresses are a
pain when debugging. I had hoped that mmap() would try to find an
address near the one requested, for subsequent calls, but perhaps that
hope is forlorn. It certainly doesn't seem to do that on x86.

>
> >
> >If we drop that value on subsequent calls to mmap(), does it fix the
> >problem?
>
> If you drop that value you get a random address. What would be better in this case?

I just tried it on x86 and we get a 'random' address anyway, whatever
is passed, once it has used the requested address. I think the address
hint only works if it works. If not, then linux starts allocating from
another address. Perhaps this linux behaviour has changed in recent
years.

>
> >
> >I think we should update the commit message to say we are hoping to
> >get 0x10000000 for the RAM buffer (although of course there is a 4K
> >region at the start of it) and that xx arch provides large addresses
> >for subsequent mmap()s.
>
> What 4 KiB region are you referring to?

If you look at os_malloc() you will see that it adds a 4KB header
before each block it allocates.

>
> Why do you expect any specific address in subsequent calls? The man-page states "If another mapping already exists there, the kernel picks a new address that may or may not depend on the hint."
>
> For performance reasons the kernel should not care about the hint in this case.

OK.

>
> >
> >Also, how about upgrading the smbios warning to an error. I think that
> >is what it is and it should stop execution.
>
> For errors we use log_err(). But where is the error? mmap() comes with absolutely no guarantee that it will assign the address that you are requesting or anything close to it.
>
> Smbios is not important enough to make the sandbox unusable in this case.

Well if the tables are invalid we should not pass them on...can we at
least do that?

Regards,
Simon
[..]

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Heinrich Schuchardt]

On 5/15/21 5:19 PM, Simon Glass wrote:
> Hi Heinrich,
>
> On Fri, 14 May 2021 at 18:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Am 14. Mai 2021 22:44:32 MESZ schrieb Simon Glass <sjg@chromium.org>:
>>> Hi Heinrich,
>>>
>>> On Fri, 14 May 2021 at 00:11, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> wrote:
>>>>
>>>> On 5/14/21 1:56 AM, Simon Glass wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> On Thu, 13 May 2021 at 08:53, Heinrich Schuchardt
>>> <xypron.glpk@gmx.de> wrote:
>>>>>>
>>>>>> On 5/13/21 4:36 PM, Simon Glass wrote:
>>>>>>> Hi Heinrich,
>>>>>>>
>>>>>>> On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt
>>> <xypron.glpk@gmx.de> wrote:
>>>>>>>>
>>>>>>>> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass
>>> <sjg@chromium.org>:
>>>>>>>>> Hi Heinrich,
>>>>>>>>>
>>>>>>>>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt
>>> <xypron.glpk@gmx.de>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> Addresses in state->ram_buf must be in the low 4 GiB of the
>>> address
>>>>>>>>> space.
>>>>>>>>>> Otherwise we cannot correctly fill SMBIOS tables. This shows
>>> up in
>>>>>>>>> warnings
>>>>>>>>>> like:
>>>>>>>>>>
>>>>>>>>>>        WARNING: SMBIOS table_address overflow 7f752735e020
>>>>>>>>>
>>>>>>>>> This sounds like a bug in the smbios-table code. For sandbox it
>>> should
>>>>>>>>> perhaps use addresses instead of pointers.
>>>>>>>>>
>>>>>>>>> I think that code (that I unfortunately wrote) was an
>>> expeditious way
>>>>>>>>> of getting it running, but is not correct.
>>>>>>>>
>>>>>>>> The field you are filling is only 32bit wide. I wonder how that
>>> table is meant to work on systems where the lowest memory address is
>>> above 4 GiB. Such ARMv8 systems exist.
>>>>>>>
>>>>>>> map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS
>>> is
>>>>>>> legacy and designed for 4GB.
>>>>>>
>>>>>> I know map_to_sysmem(). But you wrote in lib/smbios.c:487:
>>>>>>
>>>>>> /*
>>>>>>     * We must use a pointer here so things work correctly on
>>> sandbox. The
>>>>>>     * user of this table is not aware of the mapping of addresses
>>> to
>>>>>>     * sandbox's DRAM buffer.
>>>>>>     */
>>>>>>
>>>>>> For testing you could start a binary with command 'bootefi' or
>>> 'booti'
>>>>>> and that binary would analyze the table. So yes, your comment
>>> holds true
>>>>>> and you must not use map_to_sysmem() here.
>>>>>
>>>>> Yes, that is a good point. Perhaps I was not mad when I wrote that.
>>>>> What is using the tables on sandbox?
>>>>
>>>> The UEFI shell has a command smbiosview to display the SMBIOS table.
>>>>
>>>> With the current U-Boot sandbox it crashes. I do not know what the
>>> cause is:
>>>>
>>>> FS0:\> smbiosview
>>>>
>>>> Segmentation violation
>>>> pc = 0x7fb0df88d17e, pc_reloc = 0x7fb0cf88d17e
>>>>
>>>> UEFI image [0x00007fb0df836000:0x00007fb0df920c5f] pc=0x5717e
>>>> '/Shell_x64.efi'
>>>>
>>>> Here is some of the output for my laptop:
>>>>
>>>> SMBIOS Entry Point Structure:
>>>> Anchor String:        _SM_
>>>> EPS Checksum:         0xEE
>>>> Entry Point Len:      31
>>>> Version:              3.1
>>>> Number of Structures: 62
>>>> Max Struct size:      145
>>>> Table Address:        0x986E9000
>>>> Table Length:         2316
>>>> Entry Point revision: 0x0
>>>> SMBIOS BCD Revision:  0x31
>>>> Inter Anchor:         _DMI_
>>>> Inter Checksum:       0x4E
>>>> Formatted Area:
>>>>     00000000: 00 00 00 00 00                                   *.....*
>>>>
>>>> =========================================================
>>>> Query Structure, conditions are:
>>>> QueryType   = Random
>>>> QueryHandle = Random
>>>> ShowType    = SHOW_DETAIL
>>>>
>>>>
>>>> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
>>>> $
>>>> =========================================================
>>>> Type=18, Handle=0x0
>>>> Dump Structure as:
>>>> Index=0,Length=0x19,Addr=0x986E9019
>>>> 00000000: 12 17 00 00 03 02 02 00-00 00 00 00 00 00 80 00
>>>> *................*
>>>> 00000010: 00 00 80 00 00 00 80 00-00
>>> *.........*
>>>>
>>>> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
>>>> $Structure Type: 32-bit Memory Error Information
>>>> Format part Len : 23
>>>> Structure Handle: 0
>>>> 32-bit Memory Error Information - Type:   OK
>>>> Memory Error - Error granularity:   Unknown
>>>> Memory Error - Error Operation:   Unknown
>>>> VendorSyndrome: 0x0
>>>> MemoryArrayErrorAddress: 0x80000000
>>>> DeviceErrorAddress: 0x80000000
>>>> ErrorResolution: 0x80000000
>>>
>>> OK, I am not sure what to make of that except that it is every bit as
>>> impenetrable as I would expect from UEFI.
>>
>> This is a dump of the SMBIOS tables. Why do you refer to UEFI?
>
> How do you fit so many questions in one email? :-)
>
> I assumed smbiosview is a UEFI command because of the prompt. I have
> never heard of the command.
>
>>
>>>
>>> Is this on a Risc-V host? I am not sure why the first alloc would get
>>> the requested address and not the second. Is it because we specify
>>> 0x10000000 as the requested address? Is that what you actually get in
>>> this case?
>>
>> The observation is reproducible on x86_64.
>>
>> Two mmap calls cannot receive the same address without calling munmap in between.
>>
>> The first call *might* return the address 0x10000000 that you ask for.
>>
>> If the first call cannot receive it because it is not available, why should subsequent calls succeed?
>
> The goal is to keep the addresses small since 64-bit addresses are a
> pain when debugging. I had hoped that mmap() would try to find an
> address near the one requested, for subsequent calls, but perhaps that
> hope is forlorn. It certainly doesn't seem to do that on x86.
>
>>
>>>
>>> If we drop that value on subsequent calls to mmap(), does it fix the
>>> problem?
>>
>> If you drop that value you get a random address. What would be better in this case?
>
> I just tried it on x86 and we get a 'random' address anyway, whatever
> is passed, once it has used the requested address. I think the address
> hint only works if it works. If not, then linux starts allocating from
> another address. Perhaps this linux behaviour has changed in recent
> years.
>
>>
>>>
>>> I think we should update the commit message to say we are hoping to
>>> get 0x10000000 for the RAM buffer (although of course there is a 4K
>>> region at the start of it) and that xx arch provides large addresses
>>> for subsequent mmap()s.
>>
>> What 4 KiB region are you referring to?
>
> If you look at os_malloc() you will see that it adds a 4KB header
> before each block it allocates.
>
>>
>> Why do you expect any specific address in subsequent calls? The man-page states "If another mapping already exists there, the kernel picks a new address that may or may not depend on the hint."
>>
>> For performance reasons the kernel should not care about the hint in this case.
>
> OK.
>
>>
>>>
>>> Also, how about upgrading the smbios warning to an error. I think that
>>> is what it is and it should stop execution.
>>
>> For errors we use log_err(). But where is the error? mmap() comes with absolutely no guarantee that it will assign the address that you are requesting or anything close to it.
>>
>> Smbios is not important enough to make the sandbox unusable in this case.
>
> Well if the tables are invalid we should not pass them on...can we at
> least do that?

That would be separate patch?

Best regards

Heinrich

>
> Regards,
> Simon
> [..]
>

[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Simon Glass]

Hi Heinrich,

On Sat, 15 May 2021 at 09:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 5/15/21 5:19 PM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Fri, 14 May 2021 at 18:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> Am 14. Mai 2021 22:44:32 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >>> Hi Heinrich,
> >>>
> >>> On Fri, 14 May 2021 at 00:11, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>> wrote:
> >>>>
> >>>> On 5/14/21 1:56 AM, Simon Glass wrote:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On Thu, 13 May 2021 at 08:53, Heinrich Schuchardt
> >>> <xypron.glpk@gmx.de> wrote:
> >>>>>>
> >>>>>> On 5/13/21 4:36 PM, Simon Glass wrote:
> >>>>>>> Hi Heinrich,
> >>>>>>>
> >>>>>>> On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt
> >>> <xypron.glpk@gmx.de> wrote:
> >>>>>>>>
> >>>>>>>> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass
> >>> <sjg@chromium.org>:
> >>>>>>>>> Hi Heinrich,
> >>>>>>>>>
> >>>>>>>>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt
> >>> <xypron.glpk@gmx.de>
> >>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Addresses in state->ram_buf must be in the low 4 GiB of the
> >>> address
> >>>>>>>>> space.
> >>>>>>>>>> Otherwise we cannot correctly fill SMBIOS tables. This shows
> >>> up in
> >>>>>>>>> warnings
> >>>>>>>>>> like:
> >>>>>>>>>>
> >>>>>>>>>>        WARNING: SMBIOS table_address overflow 7f752735e020
> >>>>>>>>>
> >>>>>>>>> This sounds like a bug in the smbios-table code. For sandbox it
> >>> should
> >>>>>>>>> perhaps use addresses instead of pointers.
> >>>>>>>>>
> >>>>>>>>> I think that code (that I unfortunately wrote) was an
> >>> expeditious way
> >>>>>>>>> of getting it running, but is not correct.
> >>>>>>>>
> >>>>>>>> The field you are filling is only 32bit wide. I wonder how that
> >>> table is meant to work on systems where the lowest memory address is
> >>> above 4 GiB. Such ARMv8 systems exist.
> >>>>>>>
> >>>>>>> map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS
> >>> is
> >>>>>>> legacy and designed for 4GB.
> >>>>>>
> >>>>>> I know map_to_sysmem(). But you wrote in lib/smbios.c:487:
> >>>>>>
> >>>>>> /*
> >>>>>>     * We must use a pointer here so things work correctly on
> >>> sandbox. The
> >>>>>>     * user of this table is not aware of the mapping of addresses
> >>> to
> >>>>>>     * sandbox's DRAM buffer.
> >>>>>>     */
> >>>>>>
> >>>>>> For testing you could start a binary with command 'bootefi' or
> >>> 'booti'
> >>>>>> and that binary would analyze the table. So yes, your comment
> >>> holds true
> >>>>>> and you must not use map_to_sysmem() here.
> >>>>>
> >>>>> Yes, that is a good point. Perhaps I was not mad when I wrote that.
> >>>>> What is using the tables on sandbox?
> >>>>
> >>>> The UEFI shell has a command smbiosview to display the SMBIOS table.
> >>>>
> >>>> With the current U-Boot sandbox it crashes. I do not know what the
> >>> cause is:
> >>>>
> >>>> FS0:\> smbiosview
> >>>>
> >>>> Segmentation violation
> >>>> pc = 0x7fb0df88d17e, pc_reloc = 0x7fb0cf88d17e
> >>>>
> >>>> UEFI image [0x00007fb0df836000:0x00007fb0df920c5f] pc=0x5717e
> >>>> '/Shell_x64.efi'
> >>>>
> >>>> Here is some of the output for my laptop:
> >>>>
> >>>> SMBIOS Entry Point Structure:
> >>>> Anchor String:        _SM_
> >>>> EPS Checksum:         0xEE
> >>>> Entry Point Len:      31
> >>>> Version:              3.1
> >>>> Number of Structures: 62
> >>>> Max Struct size:      145
> >>>> Table Address:        0x986E9000
> >>>> Table Length:         2316
> >>>> Entry Point revision: 0x0
> >>>> SMBIOS BCD Revision:  0x31
> >>>> Inter Anchor:         _DMI_
> >>>> Inter Checksum:       0x4E
> >>>> Formatted Area:
> >>>>     00000000: 00 00 00 00 00                                   *.....*
> >>>>
> >>>> =========================================================
> >>>> Query Structure, conditions are:
> >>>> QueryType   = Random
> >>>> QueryHandle = Random
> >>>> ShowType    = SHOW_DETAIL
> >>>>
> >>>>
> >>>> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
> >>>> $
> >>>> =========================================================
> >>>> Type=18, Handle=0x0
> >>>> Dump Structure as:
> >>>> Index=0,Length=0x19,Addr=0x986E9019
> >>>> 00000000: 12 17 00 00 03 02 02 00-00 00 00 00 00 00 80 00
> >>>> *................*
> >>>> 00000010: 00 00 80 00 00 00 80 00-00
> >>> *.........*
> >>>>
> >>>> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
> >>>> $Structure Type: 32-bit Memory Error Information
> >>>> Format part Len : 23
> >>>> Structure Handle: 0
> >>>> 32-bit Memory Error Information - Type:   OK
> >>>> Memory Error - Error granularity:   Unknown
> >>>> Memory Error - Error Operation:   Unknown
> >>>> VendorSyndrome: 0x0
> >>>> MemoryArrayErrorAddress: 0x80000000
> >>>> DeviceErrorAddress: 0x80000000
> >>>> ErrorResolution: 0x80000000
> >>>
> >>> OK, I am not sure what to make of that except that it is every bit as
> >>> impenetrable as I would expect from UEFI.
> >>
> >> This is a dump of the SMBIOS tables. Why do you refer to UEFI?
> >
> > How do you fit so many questions in one email? :-)
> >
> > I assumed smbiosview is a UEFI command because of the prompt. I have
> > never heard of the command.
> >
> >>
> >>>
> >>> Is this on a Risc-V host? I am not sure why the first alloc would get
> >>> the requested address and not the second. Is it because we specify
> >>> 0x10000000 as the requested address? Is that what you actually get in
> >>> this case?
> >>
> >> The observation is reproducible on x86_64.
> >>
> >> Two mmap calls cannot receive the same address without calling munmap in between.
> >>
> >> The first call *might* return the address 0x10000000 that you ask for.
> >>
> >> If the first call cannot receive it because it is not available, why should subsequent calls succeed?
> >
> > The goal is to keep the addresses small since 64-bit addresses are a
> > pain when debugging. I had hoped that mmap() would try to find an
> > address near the one requested, for subsequent calls, but perhaps that
> > hope is forlorn. It certainly doesn't seem to do that on x86.
> >
> >>
> >>>
> >>> If we drop that value on subsequent calls to mmap(), does it fix the
> >>> problem?
> >>
> >> If you drop that value you get a random address. What would be better in this case?
> >
> > I just tried it on x86 and we get a 'random' address anyway, whatever
> > is passed, once it has used the requested address. I think the address
> > hint only works if it works. If not, then linux starts allocating from
> > another address. Perhaps this linux behaviour has changed in recent
> > years.
> >
> >>
> >>>
> >>> I think we should update the commit message to say we are hoping to
> >>> get 0x10000000 for the RAM buffer (although of course there is a 4K
> >>> region at the start of it) and that xx arch provides large addresses
> >>> for subsequent mmap()s.
> >>
> >> What 4 KiB region are you referring to?
> >
> > If you look at os_malloc() you will see that it adds a 4KB header
> > before each block it allocates.
> >
> >>
> >> Why do you expect any specific address in subsequent calls? The man-page states "If another mapping already exists there, the kernel picks a new address that may or may not depend on the hint."
> >>
> >> For performance reasons the kernel should not care about the hint in this case.
> >
> > OK.
> >
> >>
> >>>
> >>> Also, how about upgrading the smbios warning to an error. I think that
> >>> is what it is and it should stop execution.
> >>
> >> For errors we use log_err(). But where is the error? mmap() comes with absolutely no guarantee that it will assign the address that you are requesting or anything close to it.
> >>
> >> Smbios is not important enough to make the sandbox unusable in this case.
> >
> > Well if the tables are invalid we should not pass them on...can we at
> > least do that?
>
> That would be separate patch?

Yes indeed.

Regards,
Simon

Re: [PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Simon Glass]

Hi Heinrich,

On Sat, 15 May 2021 at 09:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 5/15/21 5:19 PM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Fri, 14 May 2021 at 18:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> Am 14. Mai 2021 22:44:32 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >>> Hi Heinrich,
> >>>
> >>> On Fri, 14 May 2021 at 00:11, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>> wrote:
> >>>>
> >>>> On 5/14/21 1:56 AM, Simon Glass wrote:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On Thu, 13 May 2021 at 08:53, Heinrich Schuchardt
> >>> <xypron.glpk@gmx.de> wrote:
> >>>>>>
> >>>>>> On 5/13/21 4:36 PM, Simon Glass wrote:
> >>>>>>> Hi Heinrich,
> >>>>>>>
> >>>>>>> On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt
> >>> <xypron.glpk@gmx.de> wrote:
> >>>>>>>>
> >>>>>>>> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass
> >>> <sjg@chromium.org>:
> >>>>>>>>> Hi Heinrich,
> >>>>>>>>>
> >>>>>>>>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt
> >>> <xypron.glpk@gmx.de>
> >>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Addresses in state->ram_buf must be in the low 4 GiB of the
> >>> address
> >>>>>>>>> space.
> >>>>>>>>>> Otherwise we cannot correctly fill SMBIOS tables. This shows
> >>> up in
> >>>>>>>>> warnings
> >>>>>>>>>> like:
> >>>>>>>>>>
> >>>>>>>>>>        WARNING: SMBIOS table_address overflow 7f752735e020
> >>>>>>>>>
> >>>>>>>>> This sounds like a bug in the smbios-table code. For sandbox it
> >>> should
> >>>>>>>>> perhaps use addresses instead of pointers.
> >>>>>>>>>
> >>>>>>>>> I think that code (that I unfortunately wrote) was an
> >>> expeditious way
> >>>>>>>>> of getting it running, but is not correct.
> >>>>>>>>
> >>>>>>>> The field you are filling is only 32bit wide. I wonder how that
> >>> table is meant to work on systems where the lowest memory address is
> >>> above 4 GiB. Such ARMv8 systems exist.
> >>>>>>>
> >>>>>>> map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS
> >>> is
> >>>>>>> legacy and designed for 4GB.
> >>>>>>
> >>>>>> I know map_to_sysmem(). But you wrote in lib/smbios.c:487:
> >>>>>>
> >>>>>> /*
> >>>>>>     * We must use a pointer here so things work correctly on
> >>> sandbox. The
> >>>>>>     * user of this table is not aware of the mapping of addresses
> >>> to
> >>>>>>     * sandbox's DRAM buffer.
> >>>>>>     */
> >>>>>>
> >>>>>> For testing you could start a binary with command 'bootefi' or
> >>> 'booti'
> >>>>>> and that binary would analyze the table. So yes, your comment
> >>> holds true
> >>>>>> and you must not use map_to_sysmem() here.
> >>>>>
> >>>>> Yes, that is a good point. Perhaps I was not mad when I wrote that.
> >>>>> What is using the tables on sandbox?
> >>>>
> >>>> The UEFI shell has a command smbiosview to display the SMBIOS table.
> >>>>
> >>>> With the current U-Boot sandbox it crashes. I do not know what the
> >>> cause is:
> >>>>
> >>>> FS0:\> smbiosview
> >>>>
> >>>> Segmentation violation
> >>>> pc = 0x7fb0df88d17e, pc_reloc = 0x7fb0cf88d17e
> >>>>
> >>>> UEFI image [0x00007fb0df836000:0x00007fb0df920c5f] pc=0x5717e
> >>>> '/Shell_x64.efi'
> >>>>
> >>>> Here is some of the output for my laptop:
> >>>>
> >>>> SMBIOS Entry Point Structure:
> >>>> Anchor String:        _SM_
> >>>> EPS Checksum:         0xEE
> >>>> Entry Point Len:      31
> >>>> Version:              3.1
> >>>> Number of Structures: 62
> >>>> Max Struct size:      145
> >>>> Table Address:        0x986E9000
> >>>> Table Length:         2316
> >>>> Entry Point revision: 0x0
> >>>> SMBIOS BCD Revision:  0x31
> >>>> Inter Anchor:         _DMI_
> >>>> Inter Checksum:       0x4E
> >>>> Formatted Area:
> >>>>     00000000: 00 00 00 00 00                                   *.....*
> >>>>
> >>>> =========================================================
> >>>> Query Structure, conditions are:
> >>>> QueryType   = Random
> >>>> QueryHandle = Random
> >>>> ShowType    = SHOW_DETAIL
> >>>>
> >>>>
> >>>> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
> >>>> $
> >>>> =========================================================
> >>>> Type=18, Handle=0x0
> >>>> Dump Structure as:
> >>>> Index=0,Length=0x19,Addr=0x986E9019
> >>>> 00000000: 12 17 00 00 03 02 02 00-00 00 00 00 00 00 80 00
> >>>> *................*
> >>>> 00000010: 00 00 80 00 00 00 80 00-00
> >>> *.........*
> >>>>
> >>>> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
> >>>> $Structure Type: 32-bit Memory Error Information
> >>>> Format part Len : 23
> >>>> Structure Handle: 0
> >>>> 32-bit Memory Error Information - Type:   OK
> >>>> Memory Error - Error granularity:   Unknown
> >>>> Memory Error - Error Operation:   Unknown
> >>>> VendorSyndrome: 0x0
> >>>> MemoryArrayErrorAddress: 0x80000000
> >>>> DeviceErrorAddress: 0x80000000
> >>>> ErrorResolution: 0x80000000
> >>>
> >>> OK, I am not sure what to make of that except that it is every bit as
> >>> impenetrable as I would expect from UEFI.
> >>
> >> This is a dump of the SMBIOS tables. Why do you refer to UEFI?
> >
> > How do you fit so many questions in one email? :-)
> >
> > I assumed smbiosview is a UEFI command because of the prompt. I have
> > never heard of the command.
> >
> >>
> >>>
> >>> Is this on a Risc-V host? I am not sure why the first alloc would get
> >>> the requested address and not the second. Is it because we specify
> >>> 0x10000000 as the requested address? Is that what you actually get in
> >>> this case?
> >>
> >> The observation is reproducible on x86_64.
> >>
> >> Two mmap calls cannot receive the same address without calling munmap in between.
> >>
> >> The first call *might* return the address 0x10000000 that you ask for.
> >>
> >> If the first call cannot receive it because it is not available, why should subsequent calls succeed?
> >
> > The goal is to keep the addresses small since 64-bit addresses are a
> > pain when debugging. I had hoped that mmap() would try to find an
> > address near the one requested, for subsequent calls, but perhaps that
> > hope is forlorn. It certainly doesn't seem to do that on x86.
> >
> >>
> >>>
> >>> If we drop that value on subsequent calls to mmap(), does it fix the
> >>> problem?
> >>
> >> If you drop that value you get a random address. What would be better in this case?
> >
> > I just tried it on x86 and we get a 'random' address anyway, whatever
> > is passed, once it has used the requested address. I think the address
> > hint only works if it works. If not, then linux starts allocating from
> > another address. Perhaps this linux behaviour has changed in recent
> > years.
> >
> >>
> >>>
> >>> I think we should update the commit message to say we are hoping to
> >>> get 0x10000000 for the RAM buffer (although of course there is a 4K
> >>> region at the start of it) and that xx arch provides large addresses
> >>> for subsequent mmap()s.
> >>
> >> What 4 KiB region are you referring to?
> >
> > If you look at os_malloc() you will see that it adds a 4KB header
> > before each block it allocates.
> >
> >>
> >> Why do you expect any specific address in subsequent calls? The man-page states "If another mapping already exists there, the kernel picks a new address that may or may not depend on the hint."
> >>
> >> For performance reasons the kernel should not care about the hint in this case.
> >
> > OK.
> >
> >>
> >>>
> >>> Also, how about upgrading the smbios warning to an error. I think that
> >>> is what it is and it should stop execution.
> >>
> >> For errors we use log_err(). But where is the error? mmap() comes with absolutely no guarantee that it will assign the address that you are requesting or anything close to it.
> >>
> >> Smbios is not important enough to make the sandbox unusable in this case.
> >
> > Well if the tables are invalid we should not pass them on...can we at
> > least do that?
>
> That would be separate patch?

Yes indeed.

Regards,
Simon

Applied to u-boot-dm/next, thanks!