Re: [PATCH 0/5] Add support for loading SMBIOS OEM strings from a file

[Laszlo Ersek <lersek@redhat.com>]

On 09/09/20 11:50, Daniel P. Berrangé wrote:
> On Wed, Sep 09, 2020 at 11:44:40AM +0200, Laszlo Ersek wrote:
>> On 09/08/20 18:54, Daniel P. Berrangé wrote:
>>> I previously added support for SMBIOS OEM strings tables but only
>>> allowed for data to be passed inline. Potential users indicated they
>>> wanted to pass some quite large data blobs which is inconvenient todo
>>> inline. Thus I'm adding support for passing the data from a file.
>>>
>>> In testing this I discovered the hard way that on x86 we're limited to
>>> using the SMBIOS 2.1 entry point currently. This has a maximum size of
>>> 0xffff, and if you exceed this all sorts of wierd behaviour happens.
>>>
>>> QEMU forces SMBIOS 2.1 on x86 because the default SeaBIOS firmware
>>> does not support SMBIOS 3.0. The EDK2 firmware supports SMBIOS 3.0 and
>>> QEMU defaults to this on the ARM virt machine type.
>>>
>>> This series adds support for checking the SMBIOS 2.1 limits to protect
>>> users from impossible to diagnose problems.
>>>
>>> There is also a fix needed to SeaBIOS which fails to check for
>>> integer overflow when it appends the type 0 table.
>>>
>>>   https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/3EMIOY6YS6MG5UQN3JJJS2A3DJZOVFR6/
>>>
>>> IIUC, SMBIOS 3.0 should onlky be limited by what you can fit into RAM,
>>> but in testing, EDK2 appears to hang shortly after the SMBIOS 3.0 data
>>> size exceeds 128 KB. I've not spotted an obvious flaw in EDK2 or QEMU,
>>> nor do I attempt to enforce a limit in QEMU for SMBIOS 3.0.
> 
> snip
> 
>> So we're exceeding "__brk_limit".
>>
>> ... I'm quite getting out of my league here, but "__brk_limit" seems to
>> be controlled by "brk_reservation" in "arch/x86/kernel/vmlinux.lds.S"...
>> and ultimately through the RESERVE_BRK() macro:
>>
>> [arch/x86/include/asm/setup.h]
>>
>>> /*
>>>  * Reserve space in the brk section.  The name must be unique within
>>>  * the file, and somewhat descriptive.  The size is in bytes.  Must be
>>>  * used at file scope.
>>>  *
>>>  * (This uses a temp function to wrap the asm so we can pass it the
>>>  * size parameter; otherwise we wouldn't be able to.  We can't use a
>>>  * "section" attribute on a normal variable because it always ends up
>>>  * being @progbits, which ends up allocating space in the vmlinux
>>>  * executable.)
>>>  */
>>> #define RESERVE_BRK(name,sz)                                            \
>>
>> OK, so let's see RESERVE_BRK() invocations... The relevant match is
>> likely the one below:
>>
>>> arch/x86/kernel/setup.c:RESERVE_BRK(dmi_alloc, 65536);
>>
>> ... Then see kernel commits:
>>
>> - 6de6cb442e76 ("x86: use brk allocation for DMI", 2009-03-14)
>>
>> - 796216a57fe4 ("x86: allow extend_brk users to reserve brk space",
>> 2009-03-14)
>>
>> - e808bae2407a ("x86: Do not reserve brk for DMI if it's not going to be
>> used", 2010-02-25)
>>
>> Commit 796216a57fe4 is helpful:
>>
>>>     Add RESERVE_BRK(name, size) macro to reserve space in the brk
>>>     area.  This should be a conservative (ie, larger) estimate of
>>>     how much space might possibly be required from the brk area.
>>>     Any unused space will be freed, so there's no real downside
>>>     on making the reservation too large (within limits).
>>
>> So it seems like the 64K limit could be increased, but still
>> - it requires guest kernels to be rebuilt,
>> - it doesn't seem suitable for passing MBs of data (on x86 anyway).
> 
> Yeah, this feels like we're just venturing into a bad part of town.
> Simplest is probably to just document that applications should never
> expect more than 64kb of total SMBIOS data to be viable regardless
> of the SMBIOS entry point.

Sounds OK to me personally.

In your experience, would that limit satisfy (for example) the CoreOS /
Ignition use case?

> Given this, I'm thinking it might be overkill to even both with
> supporting SMBIOS 3.0 for x86, unless it offers some other compelling
> benefit over SMBIOS 2.1 that you know of ?

I think the 32-bit entry point is sufficient for x86.

If memory serves, we only started to care about the 64-bit entry point
for aarch64. See for example

https://github.com/tianocore/edk2/commit/ca6d61b22658

x86 always has RAM under 4GB though.

Thanks
Laszlo

Thanks
Laszlo