[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 at 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 at 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 at gmx.de> wrote:
> >>>>
> >>>> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass <sjg at chromium.org>:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt <xypron.glpk at 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 at chromium.org>

Regards,
Simon