Danilo Krummrich
2025-May-20 15:01 UTC
[PATCH v3 16/19] nova-core: Add support for VBIOS ucode extraction for boot
On Tue, May 20, 2025 at 09:43:42AM -0400, Joel Fernandes wrote:> On 5/20/2025 5:30 AM, Danilo Krummrich wrote: > > On Tue, May 20, 2025 at 03:55:06AM -0400, Joel Fernandes wrote: > >> On 5/13/2025 1:19 PM, Danilo Krummrich wrote: > >>> On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot wrote: > > So the code here now looks like the below, definitely better, thanks! : > > if let (Some(second_ref), Some(first), Some(pci_at)) > (second.as_mut(), first_fwsec_image, pci_at_image) > { > second_ref > .setup_falcon_data(pdev, &pci_at, &first) > .inspect_err(|e| { > dev_err!(..) > })?; > Ok(Vbios { fwsec_image: second.take().ok_or(EINVAL)? }) > } else { > dev_err!( > pdev.as_ref(), > "Missing required images for falcon data setup, skipping\n" > ); > Err(EINVAL) > }Sorry, my code-snipped was incorrect indeed. Let me paste what I actually intended (and this time properly compile checked) and should be even better: if let (Some(mut second), Some(first), Some(pci_at)) (second_fwsec_image, first_fwsec_image, pci_at_image) { second .setup_falcon_data(pdev, &pci_at, &first) .inspect_err(|e| { dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e) })?; Ok(Vbios(second)) } else { dev_err!( pdev.as_ref(), "Missing required images for falcon data setup, skipping\n" ); Err(EINVAL) } So, with this second is the actual value and not just a reference. :) And the methods can become: pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> Result<&FalconUCodeDescV3> { self.0.fwsec_header(pdev) } pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]> { self.0.fwsec_ucode(pdev, self.fwsec_header(pdev)?) } pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> Result<&[u8]> { self.0.fwsec_sigs(pdev, self.fwsec_header(pdev)?) } However, I don't understand why they're not just implemented for FwSecBiosImage itself this way. You can just implement Deref for Vbios then.> > In general, I feel like a lot of those Option come from a programming pattern > > that is very common in C, i.e. allocate a structure (stack or heap) and then > > initialize its fields. > > > > In Rust you should aim to initialize all the fields of a structure when you > > create the instance. Option as a return type of a function is common, but it's > > always a bit suspicious when there is an Option field in a struct. > > I looked into it, I could not git rid of those ones because we need to > initialize in the "impl TryFrom<BiosImageBase> for BiosImage {" > > 0xE0 => Ok(BiosImage::FwSec(FwSecBiosImage { > base, > falcon_data_offset: None, > pmu_lookup_table: None, > falcon_ucode_offset: None, > })), > > And these fields will not be determined until much later, because as is the case > with the earlier example, these fields cannot be determined until all the images > are parsed.You should not use TryFrom, but instead use a normal constructor, such as BiosImage::new(base_bios_image) and do the parsing within this constructor. If you want a helper type with Options while parsing that's totally fine, but the final result can clearly be without Options. For instance: struct Data { image: KVec<u8>, } impl Data { fn new() -> Result<Self> { let parser = DataParser::new(); Self { image: parser.parse()? } } fn load_image(&self) { ... } } struct DataParser { // Only some images have a checksum. checksum: Option<u64>, // Some images have an extra offset. offset: Option<u64>, // Some images need to be patched. patch: Option<KVec<u8>>, image: KVec<u8>, } impl DataParser { fn new() -> Self { Self { checksum: None, offset: None, patch: None, bytes: KVec::new(), } } fn parse(self) -> Result<KVec<u8>> { // Fetch all the required data. self.fetch_checksum()?; self.fetch_offset()?; self.fetch_patch()?; self.fetch_byes()?; // Doesn't do anything if `checksum == None`. self.validate_checksum()?; // Doesn't do anything if `offset == None`. self.apply_offset()?; // Doesn't do anything if `patch == None`. self.apply_patch()?; // Return the final image. self.image } } I think the pattern here is the same, but in this example you keep working with the DataParser, instead of a new instance of Data.> > I understand that there are cases where we can't omit it, and for obvious > > reasons the Vbios code is probably a perfect example for that. > > > > However, I recommend looking at this from top to bottom: Do the "final" > > structures that we expose to the driver from the Vbios module have fields that > > are *really* optional? Or is the Option type just a result from the parsing > > process? > > > > If it's the latter, we should get rid of it and work with a different type > > during the parsing process and then create the final instance that is exposed to > > the driver at the end. > > > > For instance FwSecBiosImage is defined as: > > > > pub(crate) struct FwSecBiosImage { > > base: BiosImageBase, > > falcon_data_offset: Option<usize>, > > pmu_lookup_table: Option<PmuLookupTable>, > > falcon_ucode_offset: Option<usize>, > > } > > > > Do only *some* FwSecBiosImage instances have a falcon_ucode_offset? > > > > If the answer is 'no' then it shouldn't be an Option. If the answer is 'yes', > > then this indicates that FwSecBiosImage is probably too generic and should be > > split into more specific types of a FwSecBiosImage which instead share a common > > trait in order to treat the different types generically. > > Understood, thanks.
Joel Fernandes
2025-May-20 15:11 UTC
[PATCH v3 16/19] nova-core: Add support for VBIOS ucode extraction for boot
On 5/20/2025 11:01 AM, Danilo Krummrich wrote:> On Tue, May 20, 2025 at 09:43:42AM -0400, Joel Fernandes wrote: >> On 5/20/2025 5:30 AM, Danilo Krummrich wrote: >>> On Tue, May 20, 2025 at 03:55:06AM -0400, Joel Fernandes wrote: >>>> On 5/13/2025 1:19 PM, Danilo Krummrich wrote: >>>>> On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot wrote: >> >> So the code here now looks like the below, definitely better, thanks! : >> >> if let (Some(second_ref), Some(first), Some(pci_at)) >> (second.as_mut(), first_fwsec_image, pci_at_image) >> { >> second_ref >> .setup_falcon_data(pdev, &pci_at, &first) >> .inspect_err(|e| { >> dev_err!(..) >> })?; >> Ok(Vbios { fwsec_image: second.take().ok_or(EINVAL)? }) >> } else { >> dev_err!( >> pdev.as_ref(), >> "Missing required images for falcon data setup, skipping\n" >> ); >> Err(EINVAL) >> } > > Sorry, my code-snipped was incorrect indeed. Let me paste what I actually > intended (and this time properly compile checked) and should be even better: > > if let (Some(mut second), Some(first), Some(pci_at)) > (second_fwsec_image, first_fwsec_image, pci_at_image) > { > second > .setup_falcon_data(pdev, &pci_at, &first) > .inspect_err(|e| { > dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e) > })?; > > Ok(Vbios(second)) > } else { > dev_err!( > pdev.as_ref(), > "Missing required images for falcon data setup, skipping\n" > ); > > Err(EINVAL) > } > > So, with this second is the actual value and not just a reference. :) > > And the methods can become: > > pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> Result<&FalconUCodeDescV3> { > self.0.fwsec_header(pdev) > } > > pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]> { > self.0.fwsec_ucode(pdev, self.fwsec_header(pdev)?) > } > > pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> Result<&[u8]> { > self.0.fwsec_sigs(pdev, self.fwsec_header(pdev)?) > }I made this change and it LGTM. Thanks! I did not do the '.0' though since I want to keep the readability, lets see in the next revision if that looks good.>>> In general, I feel like a lot of those Option come from a programming pattern >>> that is very common in C, i.e. allocate a structure (stack or heap) and then >>> initialize its fields. >>> >>> In Rust you should aim to initialize all the fields of a structure when you >>> create the instance. Option as a return type of a function is common, but it's >>> always a bit suspicious when there is an Option field in a struct. >> >> I looked into it, I could not git rid of those ones because we need to >> initialize in the "impl TryFrom<BiosImageBase> for BiosImage {" >> >> 0xE0 => Ok(BiosImage::FwSec(FwSecBiosImage { >> base, >> falcon_data_offset: None, >> pmu_lookup_table: None, >> falcon_ucode_offset: None, >> })), >> >> And these fields will not be determined until much later, because as is the case >> with the earlier example, these fields cannot be determined until all the images >> are parsed. > > You should not use TryFrom, but instead use a normal constructor, such as > > BiosImage::new(base_bios_image) > > and do the parsing within this constructor. > > If you want a helper type with Options while parsing that's totally fine, but > the final result can clearly be without Options. For instance: > > struct Data { > image: KVec<u8>, > } > > impl Data { > fn new() -> Result<Self> { > let parser = DataParser::new(); > > Self { image: parser.parse()? } > } > > fn load_image(&self) { > ... > } > } > > struct DataParser { > // Only some images have a checksum. > checksum: Option<u64>, > // Some images have an extra offset. > offset: Option<u64>, > // Some images need to be patched. > patch: Option<KVec<u8>>, > image: KVec<u8>, > } > > impl DataParser { > fn new() -> Self { > Self { > checksum: None, > offset: None, > patch: None, > bytes: KVec::new(), > } > } > > fn parse(self) -> Result<KVec<u8>> { > // Fetch all the required data. > self.fetch_checksum()?; > self.fetch_offset()?; > self.fetch_patch()?; > self.fetch_byes()?; > > // Doesn't do anything if `checksum == None`. > self.validate_checksum()?; > > // Doesn't do anything if `offset == None`. > self.apply_offset()?; > > // Doesn't do anything if `patch == None`. > self.apply_patch()?; > > // Return the final image. > self.image > } > } > > I think the pattern here is the same, but in this example you keep working with > the DataParser, instead of a new instance of Data.I think this would be a fundamental rewrite of the patch. I am Ok with looking into it as a future item, but right now I am not sure if it justifies not using Option for these few. There's a lot of immediate work we have to do for boot, lets please not block the patch on just this if that's Ok with you. If you want, I could add a TODO here. thanks, - Joel
Danilo Krummrich
2025-May-20 15:36 UTC
[PATCH v3 16/19] nova-core: Add support for VBIOS ucode extraction for boot
On Tue, May 20, 2025 at 11:11:12AM -0400, Joel Fernandes wrote:> On 5/20/2025 11:01 AM, Danilo Krummrich wrote: > > I made this change and it LGTM. Thanks! I did not do the '.0' though since I > want to keep the readability, lets see in the next revision if that looks good.I think readability, is just as good with `.0`, but I'm fine with either.> >>> In general, I feel like a lot of those Option come from a programming pattern > >>> that is very common in C, i.e. allocate a structure (stack or heap) and then > >>> initialize its fields. > >>> > >>> In Rust you should aim to initialize all the fields of a structure when you > >>> create the instance. Option as a return type of a function is common, but it's > >>> always a bit suspicious when there is an Option field in a struct. > >> > >> I looked into it, I could not git rid of those ones because we need to > >> initialize in the "impl TryFrom<BiosImageBase> for BiosImage {" > >> > >> 0xE0 => Ok(BiosImage::FwSec(FwSecBiosImage { > >> base, > >> falcon_data_offset: None, > >> pmu_lookup_table: None, > >> falcon_ucode_offset: None, > >> })), > >> > >> And these fields will not be determined until much later, because as is the case > >> with the earlier example, these fields cannot be determined until all the images > >> are parsed. > > > > You should not use TryFrom, but instead use a normal constructor, such as > > > > BiosImage::new(base_bios_image) > > > > and do the parsing within this constructor. > > > > If you want a helper type with Options while parsing that's totally fine, but > > the final result can clearly be without Options. For instance: > > > > struct Data { > > image: KVec<u8>, > > } > > > > impl Data { > > fn new() -> Result<Self> { > > let parser = DataParser::new(); > > > > Self { image: parser.parse()? } > > } > > > > fn load_image(&self) { > > ... > > } > > } > > > > struct DataParser { > > // Only some images have a checksum. > > checksum: Option<u64>, > > // Some images have an extra offset. > > offset: Option<u64>, > > // Some images need to be patched. > > patch: Option<KVec<u8>>, > > image: KVec<u8>, > > } > > > > impl DataParser { > > fn new() -> Self { > > Self { > > checksum: None, > > offset: None, > > patch: None, > > bytes: KVec::new(), > > } > > } > > > > fn parse(self) -> Result<KVec<u8>> { > > // Fetch all the required data. > > self.fetch_checksum()?; > > self.fetch_offset()?; > > self.fetch_patch()?; > > self.fetch_byes()?; > > > > // Doesn't do anything if `checksum == None`. > > self.validate_checksum()?; > > > > // Doesn't do anything if `offset == None`. > > self.apply_offset()?; > > > > // Doesn't do anything if `patch == None`. > > self.apply_patch()?; > > > > // Return the final image. > > self.image > > } > > } > > > > I think the pattern here is the same, but in this example you keep working with > > the DataParser, instead of a new instance of Data. > > I think this would be a fundamental rewrite of the patch. I am Ok with looking > into it as a future item, but right now I am not sure if it justifies not using > Option for these few. There's a lot of immediate work we have to do for boot, > lets please not block the patch on just this if that's Ok with you. If you want, > I could add a TODO here.Honestly, I don't think it'd be too bad to fix this up. It's "just" a bit of juggling fields and moving code around. The actual code should not change much. Having Option<T> where the corresponding value T isn't actually optional is extremely confusing and makes it hard for everyone, but especially new contributors, to understand the code and can easily trick people into taking wrong assumptions. Making the code reasonably accessible for (new) contributors is one of the objectives of nova and one of the learnings from nouveau. Hence, let's get this right from the get-go please.