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

Sat May 15 17:33:40 CEST 2021

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?

Best regards

Heinrich

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