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

On Tue Jun 3, 2025 at 12:15 AM JST, Joel Fernandes
wrote:
> 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.
Would it then make sense to make `FwSecBiosImage` public, add an `fn
fwsec_image(&self) -> &FwSecBiosImage` method and have the caller
call
its methods directly (maybe renamed to `header`, `ucode` and `sigs`)?

On 6/3/2025 4:12 AM, Alexandre Courbot wrote:
> On Tue Jun 3, 2025 at 12:15 AM JST, Joel Fernandes wrote:
>> 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.
> 
> Would it then make sense to make `FwSecBiosImage` public, add an `fn
> fwsec_image(&self) -> &FwSecBiosImage` method and have the
caller call
> its methods directly (maybe renamed to `header`, `ucode` and `sigs`)?
Yeah, that seems better. Danilo, you're good with that idea too?

If yes, I can just do it that way (with suggestion attribution to Alex).

Thanks.

On 6/3/25 3:47 PM, Joel Fernandes wrote:
> On 6/3/2025 4:12 AM, Alexandre Courbot wrote:
>> Would it then make sense to make `FwSecBiosImage` public, add an `fn
>> fwsec_image(&self) -> &FwSecBiosImage` method and have the
caller call
>> its methods directly (maybe renamed to `header`, `ucode` and `sigs`)?
> 
> Yeah, that seems better. Danilo, you're good with that idea too?
Thanks, that sounds good to me!