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/seab...@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