Nouveau - Dec 2025 - [RFC 4/7] gpu: nova-core: populate GSP_VF_INFO when vGPU is enabled

Hi Zhi,

On 12/6/2025 7:42 AM, Zhi Wang wrote:
[...]
> +/// VF information - gspVFInfo in SetSystemInfo.
> +#[derive(Clone, Copy, Zeroable)]
> +#[repr(transparent)]
> +pub(crate) struct GspVfInfo {
> +    inner: bindings::GSP_VF_INFO,
> +}
> +
> +impl GspVfInfo {
> +    /// Creates a new GspVfInfo structure.
> +    pub(crate) fn new<'a>(
> +        pdev: &'a pci::Device<device::Bound>,
> +        bar: &Bar0,
> +        vgpu_support: bool,
> +    ) -> Result<GspVfInfo> {
> +        let mut vf_info = GspVfInfo::zeroed();
> +        let info = &mut vf_info.inner;
> +
> +        if vgpu_support {
> +            let val = pdev.sriov_get_totalvfs()?;
> +            info.totalVFs = u32::try_from(val)?;
> +
> +            let pos = pdev
> +               
.find_ext_capability(kernel::bindings::PCI_EXT_CAP_ID_SRIOV as i32)
> +                .ok_or(ENODEV)?;
> +
> +            let val = pdev.config_read_word(
> +                i32::from(pos) +
i32::from(kernel::bindings::PCI_SRIOV_VF_OFFSET as i32),
> +            )?;
> +            info.firstVFOffset = u32::from(val);
> +
> +            let val = pdev.config_read_dword(
> +                i32::from(pos) + i32::from(kernel::bindings::PCI_SRIOV_BAR
as i32),
> +            )?;
> +            info.FirstVFBar0Address = u64::from(val);
> +
> +            let bar1_lo = pdev.config_read_dword(
> +                i32::from(pos) + i32::from(kernel::bindings::PCI_SRIOV_BAR
as i32 + 4),
> +            )?;
> +            let bar1_hi = pdev.config_read_dword(
> +                i32::from(pos) + i32::from(kernel::bindings::PCI_SRIOV_BAR
as i32 + 8),
> +            )?;
> +
> +            let addr_mask =
u64::try_from(kernel::bindings::PCI_BASE_ADDRESS_MEM_MASK)?;
> +
> +            info.FirstVFBar1Address > +               
(u64::from(bar1_hi) << 32) | ((u64::from(bar1_lo)) & addr_mask);
> +
> +            let bar2_lo = pdev.config_read_dword(
> +                i32::from(pos) + i32::from(kernel::bindings::PCI_SRIOV_BAR
as i32 + 12),
> +            )?;
> +            let bar2_hi = pdev.config_read_dword(
> +                i32::from(pos) + i32::from(kernel::bindings::PCI_SRIOV_BAR
as i32 + 16),
> +            )?;
> +
> +            info.FirstVFBar2Address = (u64::from(bar2_hi) << 32) |
(u64::from(bar2_lo) & addr_mask);
> +
> +            let val = bar.read32(0x88000 + 0xbf4);
> +            info.b64bitBar1 = u8::from((val & 0x00000006) ==
0x00000004);
> +
> +            let val = bar.read32(0x88000 + 0xbfc);
> +            info.b64bitBar2 = u8::from((val & 0x00000006) ==
0x00000004);
Please no magic numbers, please use proper named constants with documentation
comments explaining the values.

Also BAR reads here need proper register macro definitions/access.

Also the above code is lacking in comments. All the steps above need proper
comments IMO.

General philosophy of Nova is it is a well documented, cleanly written driver
with minimal/no magic numbers and abundant comments. :)

Thanks.

On Sat, 6 Dec 2025 21:32:51 -0500
Joel Fernandes <joelagnelf at nvidia.com> wrote:
> Hi Zhi,
> 
> On 12/6/2025 7:42 AM, Zhi Wang wrote:
snip

 => > 0x00000004); +
> > +            let val = bar.read32(0x88000 + 0xbfc);
> > +            info.b64bitBar2 = u8::from((val & 0x00000006) =>
> 0x00000004);
> 
> Please no magic numbers, please use proper named constants with
> documentation comments explaining the values.
> 
> Also BAR reads here need proper register macro definitions/access.
> 
That is true. :) But this is because there is no register definition in
the OpenRM code/non OpenRM code as well. I have no idea about the name
and bit definitions of this register.

Suppose I will have to find some clues from some folks then document
them here when going to patches request for merged. :)
> Also the above code is lacking in comments. All the steps above need
> proper comments IMO.
> 
> General philosophy of Nova is it is a well documented, cleanly
> written driver with minimal/no magic numbers and abundant comments. :)
> 
Agree. :)
> Thanks.
>