Backport: Propagate SMBIOS fields for sysinfo and chassis#164
Conversation
Open question (no strong opinion): what do we gain from having that in our fork? I wonder if increasing our patchset makes sense - given that I do not expect bugfixes in that part of the code and it doesn't fixes or improves functionality (right?). We'll rebase to CH v53 probaly in July |
Wouldn't this make rebasing later easier? We revert our version of the implementation and include upstream's version so when we have to rebase on the next release, we can squash the reverted commits and drop upstream's commits that are already present. Any new additions we add are then also already based on upstream's code. |
I just had a similar offline discussion with @Coffeeri - sorry 😀. Let's continue the discussion here. When I updated the patchset from one version to the next, for example v49 → v50 or v50 → v51, I never actually used If I were to do the v52 → v53 update right now, I would simply omit the old SMBIOS commits and ignore them. I would handle the commits we are discussing here in the same way. That said, if someone else performs the next update using an actual rebase, they would first have to rebase the old SMBIOS commits, since those appear earlier in the patchset. But that would not make any sense as the functionality is always upstream and the commits would not be needed.
EDIT: I have a tendency to just incorporate the fixes from upstream in a new commit here. I think they are only fairly minor? |
There have been a couple of upstream changes, so either way, I think we should:
Overall, I see benefit of generally staying closer to upstream and no downside. |
This reverts commit 9e3afb7. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
This reverts commit 29fab19. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
This reverts commit 287c2c0. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
This reverts commit 7a1d331. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
This reverts commit 59763b3. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
The allocator lists are sized from the fixed PCI segment count. They are only indexed afterwards, so boxed slices fit the use. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
The PCI segment list is created once from the configured count. Later code mutates entries, but does not add or remove segments. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
Each affinity host CPU list is copied from configuration. It is only iterated afterwards, so a boxed slice is enough. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
I started by looking at all `Option<Vec<T>>` values in config.rs and vm_config.rs, and replaced them with `Option<Box<[T]>>`. This has the advantage that one now can see at a glance if this field will ever resize during operation or not, reducing cognitive load and increasing maintainability. All fields that need the properties of a Ver or where this change was not trivial are kept intact. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
Split the System Information write into helper functions and reuse the string writer so the table layout and inputs are unchanged. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Add a small SMBIOS config that carries serial_number, uuid, and OEM strings, and pass it from platform config into x86_64 setup. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Extend SMBIOS System Information with manufacturer, product, version, family, sku, serial, and uuid fields, add a chassis asset tag, and pass a structured SMBIOS config from --platform into arch setup. Keep OEM strings and legacy serial_number/uuid options working for compatibility. The platform option naming follows `dmidecode -s <field>`. Fields: - system_manufacturer - system_product_name - system_version - system_family - system_serial_number - system_uuid - chassis_asset_tag On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Mark serial_number/uuid as deprecated in the OpenAPI schema and emit warnings when those legacy --platform keys are used, while continuing to accept them for compatibility. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Add unit tests that walk the SMBIOS binary layout in guest memory and
verify structure ordering, string-set encoding, and error paths.
Tests added:
- smbios_chassis_empty_string_set_has_double_null: verify that
a chassis with no strings emits the double-NUL terminator required
by SMBIOS DSP0134 §6.1.3.
- smbios_chassis_oem_strings_layout: verify the full chain
(BIOS → System → Chassis → OEM → End) when a chassis asset tag and
OEM strings are configured.
- smbios_strings_terminators_default: verify the default table chain
(BIOS → System → End) and check that string indices and string-set
contents match for both structures.
- smbios_strings_too_many: exercise alloc_index up to the u8 limit
(255 strings) and verify the 256th is rejected.
- smbios_uuid_invalid_rejected: ensure a malformed UUID string is
rejected with Error::ParseUuid.
- smbios_uuid_written_le: ensure the UUID is stored in little-endian
byte order as required by SMBIOS Spec 7.2.1.
- smbios_write_fails_with_too_small_memory: verify that setup_smbios
fails with Error::WriteData when guest memory is too small to hold
anything beyond the entry point.
All tests also succeed when run with miri:
cargo +nightly miri test -p arch smbios
On-behalf-of: SAP leander.kohler@sap.com
Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
The structured SMBIOS platform config introduced six new keys (system_manufacturer, system_product_name, system_version, system_family, system_sku_number, chassis_asset_tag), but integration coverage only existed for serial_number, uuid, and oem_strings. Add _test_dmi_system_and_chassis, which boots a guest with all six keys set and checks each value via `dmidecode -s` using the same leaf name as the CLI key. Execute it in both the regular and SEV-SNP integration suites. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
That seems like the biggest advantage to me. I'm in favor of backporting.
I'm not sure if I understand correctly. The commits you added seem unrelated to the SMBIOS backport. If I'm not missing anything and they are unrelated, please drop them and create a separate PR. |
The changes from cloud-hypervisor#8231 are required for the upstream SMBIOS version to compile, because some types have changed. I’d like to keep this simple, and I don’t see much benefit in the overhead of creating a separate PR. |
I agree - thanks for the discussion! Fine with me, we can go forward Beware that every backport will increase the cognitive load during the next patchset bump |
|
@Coffeeri Could you run the libvirt tests for the PR? |
3 participants
This backports the upstreamed SMBIOS changes cloud-hypervisor#8259, which were first introduced in #78.
We revert our initial implementation to be in-sync with upstream.
As the upstream change relies on cloud-hypervisor#8231, we backport this as well.
Pipeline: https://gitlab.cyberus-technology.de/cyberus/cloud/libvirt/-/pipelines/229081