Skip to content

patina_smbios: Add SMBIOS types#1383

Open
ansleythompson wants to merge 1 commit intoOpenDevicePartnership:mainfrom
ansleythompson:smbios-types-cr
Open

patina_smbios: Add SMBIOS types#1383
ansleythompson wants to merge 1 commit intoOpenDevicePartnership:mainfrom
ansleythompson:smbios-types-cr

Conversation

@ansleythompson
Copy link
Copy Markdown
Contributor

Description

Define types for SMBIOS records according the SMBIOS specs 3.0+ and update records to use types. These structs/enums/bitfields force typing when creating SMBIOS records

  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
  • Includes documentation?

How This Was Tested

Integrated into patina-dxe-core-qemu with a local patch and verified with patina-qemu smbiosview

Integration Instructions

Use Smbios Types in place of magic numbers to create SMBIOS records.

Define types for SMBIOS records according the SMBIOS specs 3.0+ and update records to use types.
These structs/enums/bitfields force typing when creating SMBIOS records
@patina-automation
Copy link
Copy Markdown
Contributor

⌛ QEMU Validation Pending

QEMU validation is pending on successful CI completion.

Note: Any previous results are available in this comment's edit history.

This comment was automatically generated by the Patina QEMU PR Validation workflow.

/// Boot Up State
///
#[derive(Copy, Clone, Debug)]
pub enum BootUpState {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These values are Chassis Types (SMBIOS spec Table 17 — System Enclosure or Chassis Types), not boot-up states. Boot-up states should be

Other = 0x01,
Unknown,
Safe,
Warning,
Critical,
...

Comment on lines +339 to +342
/// Power Supply State
///
#[derive(Copy, Clone, Debug)]
pub enum PowerSupplyState {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these values look like ProcessorType values instead of PowerSupplyState Values

matches!(
segment.ident.to_string().as_str(),
"u8" | "u16" | "u32" | "u64" | "i8" | "i16" | "i32" | "i64"
| "BiosCharacteristics"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could use zerocopy::IntoBytes here instead. So we could have something like:

#[bitfield(u8)]
#[derive(zerocopy::IntoBytes)]
pub struct BiosCharacteristicsExt1 {
    pub acpi_supported: bool,
    pub usb_legacy_supported: bool,
    // ...

I can refactor the smbios_record_macro to use zerocopy::IntoBytes::as_bytes() instead of the type name string list in a separate PR though. Will update you here when that's created

spin = { workspace = true }
zerocopy = { workspace = true }
zerocopy-derive = { workspace = true }
bitfield = "0.17.0"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use bitfield-struct from the workspace? If not, can you add it to the workspace and then use bitfield = { workspace = true } here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

impact:non-functional Does not have a functional impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants