Conversation
0620311 to
2429af6
Compare
2429af6 to
544858e
Compare
544858e to
add6252
Compare
phip1611
left a comment
There was a problem hiding this comment.
for better reviewability, please split into multiple commits.
For example:
- add new types
- delete old types
9d5490b to
4365f65
Compare
4365f65 to
04d502c
Compare
|
The packing requirement of all the structs seems to be offhandedly declared by these statements in 33.3.8 Forms Package (33.3.8.1 Binary Encoding).
|
04d502c to
a978e45
Compare
There was a problem hiding this comment.
Thank you very much! I checked (with the help of an LLM) the bindings against UefiInternalFormRepresentation.h at
edk2-stable202508.01: all 100 opcodes and all 111 struct layouts match, and
packed is right (edk2 wraps that header in #pragma pack(1)). No layout bugs.
But I have six remarks, please address them.
-
Missing
**Breaking**changelog entry.HiiDate/HiiTime/HiiRefmove
tohii,IfrTypeValuetohii::ifr. All four werepubin v0.16.0, so
downstreamusestatements break. -
IfrCheckboxFlagsshould bebitflags!(ifr.rs:285).DEFAULT(0x01)
andDEFAULT_MFG(0x02) combine; as anewtype_enum,IfrCheckboxFlags(3)
equals neither, soflags == IfrCheckboxFlags::DEFAULTsilently fails. Did I at first recommended to use newtype_enum here? I am not sure anymore, I lost context. Sorry. -
IfrDateFlags::STORAGE_NORMAL = empty()is a trap (ifr.rs:324).SORRY THIS IS COPIED FROM AN LLM. I am currently having lots on my desk, but I value your work! I hope this helps.
bitflagsimplementscontains(other)as(self.bits & other.bits) == other.bits. For a constant with no bits set that is0 == 0, i.e. always
true. Soflags.contains(IfrDateFlags::STORAGE_NORMAL)returnstruefor
a date stored in the wakeup-time register - the one check a caller would
naturally write is the one that cannot fail.The underlying reason is that
EFI_QF_DATE_STORAGE(0x30) is not two
independent bits but a 2-bit field with three defined values:NORMAL(0x00),
STORAGE_TIME(0x10),STORAGE_WAKEUP(0x20). Modelling it as bitflags also
makesfrom_bits_retain(0x30)reportcontains(STORAGE_TIME) && contains(STORAGE_WAKEUP), which has no meaning in the spec.Minimal fix: drop the
impl IfrDateFlags { pub const STORAGE_NORMAL }block.
That alone removes the trap - callers then have to write the mask themselves,
which at least fails loudly.Pragmatic fix: keep the suppress bits as flags, and expose the selector as
its own type. Roughly:bitflags::bitflags! { pub struct IfrDateFlags: u8 { const YEAR_SUPPRESS = 1 << 0; const MONTH_SUPPRESS = 1 << 1; const DAY_SUPPRESS = 1 << 2; /// Mask of the storage selector, see [`IfrDateFlags::storage`]. const STORAGE = 0x30; } } newtype_enum! { /// Storage selector of [`IfrDateFlags`] (`EFI_QF_DATE_STORAGE`). pub enum IfrDateStorage: u8 => { NORMAL = 0x00, TIME = 0x10, WAKEUP = 0x20, } } impl IfrDateFlags { /// Returns where the date is stored. #[must_use] pub const fn storage(self) -> IfrDateStorage { IfrDateStorage(self.bits() & Self::STORAGE.bits()) } }
IfrTimehas the same shape (EFI_QF_TIME_STORAGE, also 0x30), so whatever
you pick here should apply there too ifflags: u8ever gets a type. -
IfrDefaultignores the types this PR adds (ifr.rs:341). Suggestion:
default_id: DefaultId,ifr_type: IfrType.ifr_typeselects the active
IfrTypeValuearm, so it is untyped exactly where it matters most. Same for
IfrDefaultStore::default_id. -
IfrNumeric/IfrOneOf:flagsis theIfrNumericDatadiscriminant
(ifr.rs:687, 711).flags & EFI_IFR_NUMERIC_SIZE(0x03) picks the arm, but
the constant is not exported and nothing says so. Suggestion: export the mask
(andEFI_IFR_DISPLAY) and document it on the field. -
IfrDate::quest(ifr.rs:332) - the only question-header field not named
question, across 15 structs. Suggestion:pub question: IfrQuestionHeader,
Minor, feel free to skip: bitflags::bitflags! fully qualified vs the crate's
use bitflags::bitflags;; IfrFormMap/IfrFormSet flexible members commented
out while others use [T; 0]; HiiFormPackageHdr in mod.rs without an ABI
assert; HiiPackageListHeader missing the Clone, Copy its sibling got;
VarstoreInfo/IfrNumeric* undocumented; commit trailers say
edk2-stable202608, the PR description edk2-stable202508.01.
`HiiDate`, `HiiTime`, and `HiiRef` are not specific to the config protocol. Move these definitions to the HII root. Signed-off-by: Tim Crawford <tcrawford@system76.com>
Match the edk2 definitions and make these structs packed. Ref: tianocore/edk2@edk2-stable202608 Signed-off-by: Tim Crawford <tcrawford@system76.com>
a978e45 to
37875c0
Compare
Done.
Done.
TODO
Done. Also:
I've added constants for the mask and values to match UEFI. Probably can make a
Done.
Done.
There's also something like Then there's Should they all be added as
Done.
Done.
I've tried to add basic documentation for them.
Updated PR message. |
The Internal Forms Representation is a binary encoding of HII objects, used to construct UI elements of UEFI modules. Ref: UEFI 2.11: 33.3.8 Forms Package Ref: tianocore/edk2@edk2-stable202608 Signed-off-by: Tim Crawford <tcrawford@system76.com>
37875c0 to
070bfd3
Compare
I've had this change sitting for a while, so I'll just push it.
This is a follow up to my HII work in #1822.
The Internal Forms Representation is a binary encoding of HII objects, used to construct UI elements of UEFI modules.
Ref: UEFI 2.11: 33.3.8 Forms Package
Ref: edk2@edk2-stable202608
Checklist