Re: [PATCH v4 16/20] nova-core: Add support for VBIOS ucode extraction for boot

[Joel Fernandes]

On Mon, Jun 02, 2025 at 03:33:56PM +0200, Danilo Krummrich wrote:
> On Wed, May 21, 2025 at 03:45:11PM +0900, Alexandre Courbot wrote:
> > +impl Vbios {
> 
> <snip>
> 
> > +    pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> 
> > Result<&FalconUCodeDescV3> {
> > +        self.fwsec_image.fwsec_header(pdev)
> > +    }
> > +
> > +    pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> 
> > Result<&[u8]> {
> > +        self.fwsec_image.fwsec_ucode(pdev, self.fwsec_header(pdev)?)
> > +    }
> > +
> > +    pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> 
> > Result<&[u8]> {
> > +        self.fwsec_image.fwsec_sigs(pdev, self.fwsec_header(pdev)?)
> > +    }
> 
> Can't we just implement Deref here? Why do we need this indirection?

We could, but it seems weird to deref a Vbios struct to an FwsecBiosImage
struct. Conceptually a Vbios is a collection of things and it could have
future extensions to its struct.

The win with using Deref is also not that much, just 2 lines fewer since the
deleted functions are replaced by the the impl Deref block. But I am Ok with
it either way, here is the diff on top of this patch.
Or did I miss something about the suggestion? Will respond to the other
comments, soon, Thanks.
---8<-----------------------
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 346d48c4820c..ccf83b206758 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -6,6 +6,7 @@
 use crate::firmware::fwsec::Bcrt30Rsa3kSignature;
 use crate::firmware::FalconUCodeDescV3;
 use core::convert::TryFrom;
+use core::ops::Deref;
 use kernel::device;
 use kernel::error::Result;
 use kernel::num::NumExt;
@@ -247,17 +248,13 @@ pub(crate) fn new(pdev: &pci::Device, bar0: &Bar0) -> 
Result<Vbios> {
             Err(EINVAL)
         }
     }
+}
 
-    pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> 
Result<&FalconUCodeDescV3> {
-        self.fwsec_image.fwsec_header(pdev)
-    }
-
-    pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]> {
-        self.fwsec_image.fwsec_ucode(pdev, self.fwsec_header(pdev)?)
-    }
+impl Deref for Vbios {
+    type Target = FwSecBiosImage;
 
-    pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> 
Result<&[Bcrt30Rsa3kSignature]> {
-        self.fwsec_image.fwsec_sigs(pdev, self.fwsec_header(pdev)?)
+    fn deref(&self) -> &Self::Target {
+        &self.fwsec_image
     }
 }
 
@@ -735,7 +732,7 @@ struct FwSecBiosPartial {
     falcon_ucode_offset: Option<usize>,
 }
 
-struct FwSecBiosImage {
+pub(crate) struct FwSecBiosImage {
     base: BiosImageBase,
     // The offset of the Falcon ucode
     falcon_ucode_offset: usize,
@@ -1091,7 +1088,7 @@ fn new(pdev: &pci::Device, data: FwSecBiosPartial) -> 
Result<Self> {
     }
 
     /// Get the FwSec header (FalconUCodeDescV3)
-    fn fwsec_header(&self, dev: &device::Device) -> Result<&FalconUCodeDescV3> 
{
+    pub(crate) fn fwsec_header(&self, dev: &device::Device) -> 
Result<&FalconUCodeDescV3> {
         // Get the falcon ucode offset that was found in setup_falcon_data
         let falcon_ucode_offset = self.falcon_ucode_offset;
 
@@ -1119,9 +1116,11 @@ fn fwsec_header(&self, dev: &device::Device) -> 
Result<&FalconUCodeDescV3> {
             &*(self.base.data.as_ptr().add(falcon_ucode_offset) as *const 
FalconUCodeDescV3)
         })
     }
+
     /// Get the ucode data as a byte slice
-    fn fwsec_ucode(&self, dev: &device::Device, desc: &FalconUCodeDescV3) -> 
Result<&[u8]> {
+    pub(crate) fn fwsec_ucode(&self, dev: &device::Device) -> Result<&[u8]> {
         let falcon_ucode_offset = self.falcon_ucode_offset;
+        let desc = self.fwsec_header(dev)?;
 
         // The ucode data follows the descriptor
         let ucode_data_offset = falcon_ucode_offset + desc.size();
@@ -1136,17 +1135,17 @@ fn fwsec_ucode(&self, dev: &device::Device, desc: 
&FalconUCodeDescV3) -> Result<
     }
 
     /// Get the FWSEC signatures.
-    fn fwsec_sigs(
+    pub(crate) fn fwsec_sigs(
         &self,
         dev: &device::Device,
-        v3_desc: &FalconUCodeDescV3,
     ) -> Result<&[Bcrt30Rsa3kSignature]> {
         let falcon_ucode_offset = self.falcon_ucode_offset;
+        let desc = self.fwsec_header(dev)?;
 
         // The signatures data follows the descriptor
         let sigs_data_offset = falcon_ucode_offset + 
core::mem::size_of::<FalconUCodeDescV3>();
         let sigs_size =
-            v3_desc.signature_count as usize * 
core::mem::size_of::<Bcrt30Rsa3kSignature>();
+            desc.signature_count as usize * 
core::mem::size_of::<Bcrt30Rsa3kSignature>();
 
         // Make sure the data is within bounds
         if sigs_data_offset + sigs_size > self.base.data.len() {
@@ -1166,9 +1165,8 @@ fn fwsec_sigs(
                     .as_ptr()
                     .add(sigs_data_offset)
                     .cast::<Bcrt30Rsa3kSignature>(),
-                v3_desc.signature_count as usize,
+                desc.signature_count as usize,
             )
         })
     }
-}
-
+}
\ No newline at end of file