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

Sat May 15 18:09:23 CEST 2021

Hi Heinrich,

On Sat, 15 May 2021 at 09:33, Heinrich Schuchardt <xypron.glpk at 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 at gmx.de> wrote:
> >>
> >> Am 14. Mai 2021 22:44:32 MESZ schrieb Simon Glass <sjg at chromium.org>:
> >>> 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.
> >>
> >> 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