Message-Id: <DEULURWNK2ZP.1GGKEII07Q1IE@nvidia.com>
Date: Wed, 10 Dec 2025 23:27:01 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Zhi Wang" <zhiw@...dia.com>, <rust-for-linux@...r.kernel.org>,
 <linux-pci@...r.kernel.org>, <nouveau@...ts.freedesktop.org>,
 <linux-kernel@...r.kernel.org>
Cc: <airlied@...il.com>, <dakr@...nel.org>, <aliceryhl@...gle.com>,
 <bhelgaas@...gle.com>, <kwilczynski@...nel.org>, <ojeda@...nel.org>,
 <alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
 <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>, <a.hindborg@...nel.org>,
 <tmgross@...ch.edu>, <markus.probst@...teo.de>, <helgaas@...nel.org>,
 <cjia@...dia.com>, <alex@...zbot.org>, <smitra@...dia.com>,
 <ankita@...dia.com>, <aniketa@...dia.com>, <kwankhede@...dia.com>,
 <targupta@...dia.com>, <acourbot@...dia.com>, <joelagnelf@...dia.com>,
 <jhubbard@...dia.com>, <zhiwang@...nel.org>, "Nouveau"
 <nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [RFC 4/7] gpu: nova-core: populate GSP_VF_INFO when vGPU is
 enabled

On Sat Dec 6, 2025 at 9:42 PM JST, Zhi Wang wrote:
> GSP firmware needs to know the VF BAR offsets to correctly calculate the
> VF events.
>
> The VF BAR information is stored in GSP_VF_INFO, which needs to be
> initialized and uploaded together with the GSP_SYSTEM_INFO.
>
> Populate GSP_VF_INFO when nova-core uploads the GSP_SYSTEM_INFO if NVIDIA
> vGPU is enabled.
>
> Signed-off-by: Zhi Wang <zhiw@...dia.com>
> ---
>  drivers/gpu/nova-core/gpu.rs             |  2 +-
>  drivers/gpu/nova-core/gsp.rs             |  8 ++-
>  drivers/gpu/nova-core/gsp/boot.rs        |  6 +-
>  drivers/gpu/nova-core/gsp/commands.rs    |  8 ++-
>  drivers/gpu/nova-core/gsp/fw.rs          | 75 ++++++++++++++++++++++++
>  drivers/gpu/nova-core/gsp/fw/commands.rs | 11 +++-
>  6 files changed, 102 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 10c5ae07a891..08a41e7bd982 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -285,7 +285,7 @@ pub(crate) fn new<'a>(
>  
>              sec2_falcon: Falcon::new(pdev.as_ref(), spec.chipset)?,
>  
> -            gsp <- Gsp::new(pdev)?,
> +            gsp <- Gsp::new(pdev, vgpu.vgpu_support)?,

This seems like it is making the whole `Vgpu` structure introduced in
the previous patch superfluous, since its sole purpose is to pass the
`vgpu_support` value to `Gsp::new` - we could just extract that value in
`Gsp::new`, or better in `Gsp::boot`, where we actually use it, and
avoid storing it in 3 different places.

>  
>              _: { gsp.boot(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)? },
>  
> diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
> index fb6f74797178..2d9352740c28 100644
> --- a/drivers/gpu/nova-core/gsp.rs
> +++ b/drivers/gpu/nova-core/gsp.rs
> @@ -115,11 +115,16 @@ pub(crate) struct Gsp {
>      pub(crate) cmdq: Cmdq,
>      /// RM arguments.
>      rmargs: CoherentAllocation<GspArgumentsCached>,
> +    /// Support vGPU.
> +    vgpu_support: bool,
>  }
>  
>  impl Gsp {
>      // Creates an in-place initializer for a `Gsp` manager for `pdev`.
> -    pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> Result<impl PinInit<Self, Error>> {
> +    pub(crate) fn new(
> +        pdev: &pci::Device<device::Bound>,
> +        vgpu_support: bool,
> +    ) -> Result<impl PinInit<Self, Error>> {
>          let dev = pdev.as_ref();
>          let libos = CoherentAllocation::<LibosMemoryRegionInitArgument>::alloc_coherent(
>              dev,
> @@ -156,6 +161,7 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> Result<impl PinInit<Self
>              logrm,
>              rmargs,
>              cmdq,
> +            vgpu_support,
>          }))
>      }
>  }
> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
> index 54937606b5b0..5016c630cec3 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -33,6 +33,7 @@
>      gpu::Chipset,
>      gsp::{
>          commands,
> +        fw::GspVfInfo,
>          sequencer::{
>              GspSequencer,
>              GspSequencerParams, //
> @@ -136,6 +137,7 @@ pub(crate) fn boot(
>          sec2_falcon: &Falcon<Sec2>,
>      ) -> Result {
>          let dev = pdev.as_ref();
> +        let vgpu_support = self.vgpu_support;
>  
>          let bios = Vbios::new(dev, bar)?;
>  
> @@ -162,8 +164,10 @@ pub(crate) fn boot(
>              CoherentAllocation::<GspFwWprMeta>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?;
>          dma_write!(wpr_meta[0] = GspFwWprMeta::new(&gsp_fw, &fb_layout))?;
>  
> +        let vf_info = GspVfInfo::new(pdev, bar, vgpu_support)?;

This is a strange constructor. It initializes the `GspVfInfo` if
`vgpu_support` is true, and returns a zeroed structure otherwise. I'd
rather have an unconditional constructor that is only called if
`vgpu_support` is true, and store the result in an `Option`:

    let vf_info = if vgpu_support {
        Some(GspVfInfo::new(pdev, bar)?)
    } else {
        None
    };

It will become clearer later why this is a better design.

> +
>          self.cmdq
> -            .send_command(bar, commands::SetSystemInfo::new(pdev))?;
> +            .send_command(bar, commands::SetSystemInfo::new(pdev, vf_info))?;

As a result of the previous comment, `SetSystemInfo::new` now takes an
`Option<GspVfInfo>`...

>          self.cmdq.send_command(bar, commands::SetRegistry::new())?;
>  
>          gsp_falcon.reset(bar)?;
> diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
> index 0425c65b5d6f..1d519c4ed232 100644
> --- a/drivers/gpu/nova-core/gsp/commands.rs
> +++ b/drivers/gpu/nova-core/gsp/commands.rs
> @@ -26,6 +26,7 @@
>          },
>          fw::{
>              commands::*,
> +            GspVfInfo,
>              MsgFunction, //
>          },
>      },
> @@ -36,12 +37,13 @@
>  /// The `GspSetSystemInfo` command.
>  pub(crate) struct SetSystemInfo<'a> {
>      pdev: &'a pci::Device<device::Bound>,
> +    vf_info: GspVfInfo,

... and this becomes an `Option` as well.

>  }
>  
>  impl<'a> SetSystemInfo<'a> {
>      /// Creates a new `GspSetSystemInfo` command using the parameters of `pdev`.
> -    pub(crate) fn new(pdev: &'a pci::Device<device::Bound>) -> Self {
> -        Self { pdev }
> +    pub(crate) fn new(pdev: &'a pci::Device<device::Bound>, vf_info: GspVfInfo) -> Self {
> +        Self { pdev, vf_info }
>      }
>  }
>  
> @@ -51,7 +53,7 @@ impl<'a> CommandToGsp for SetSystemInfo<'a> {
>      type InitError = Error;
>  
>      fn init(&self) -> impl Init<Self::Command, Self::InitError> {
> -        GspSetSystemInfo::init(self.pdev)
> +        GspSetSystemInfo::init(self.pdev, self.vf_info)

And here things become interesting: you can leave the constructor if
`GspSetSystemInfo` unchanged, since vgpu support is not strictly
required to produce a valid value. But you can also chain the
initializer to add the vgpu information when relevant:

    GspSetSystemInfo::init(self.pdev).chain(|info| {
        if let Some(vf_info) = &self.vf_info {
            info.set_vf_info(vf_info);
        }

        Ok(())
    })

(more on `set_vf_info` below)

This results in less code overall, and better conveys the fact that vgpu
support is technically optional.

>      }
>  }
>  
> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> index abffd6beec65..a0581ac34586 100644
> --- a/drivers/gpu/nova-core/gsp/fw.rs
> +++ b/drivers/gpu/nova-core/gsp/fw.rs
> @@ -9,8 +9,10 @@
>  use core::ops::Range;
>  
>  use kernel::{
> +    device,
>      dma::CoherentAllocation,
>      fmt,
> +    pci,
>      prelude::*,
>      ptr::{
>          Alignable,
> @@ -27,6 +29,7 @@
>  };
>  
>  use crate::{
> +    driver::Bar0,
>      fb::FbLayout,
>      firmware::gsp::GspFirmware,
>      gpu::Chipset,
> @@ -926,3 +929,75 @@ fn new(cmdq: &Cmdq) -> Self {
>          })
>      }
>  }
> +
> +/// VF information - gspVFInfo in SetSystemInfo.
> +#[derive(Clone, Copy, Zeroable)]
> +#[repr(transparent)]
> +pub(crate) struct GspVfInfo {
> +    inner: bindings::GSP_VF_INFO,
> +}

You can use a tuple struct here (i.e. `struct
GspVfInfo(bindings::GSP_VF_INFO`).

Also none of the derives should be needed eventually.

> +
> +impl GspVfInfo {
> +    /// Creates a new GspVfInfo structure.
> +    pub(crate) fn new<'a>(
> +        pdev: &'a pci::Device<device::Bound>,
> +        bar: &Bar0,
> +        vgpu_support: bool,

As mentioned before, we can drop this bool argument.

> +    ) -> Result<GspVfInfo> {
> +        let mut vf_info = GspVfInfo::zeroed();
> +        let info = &mut vf_info.inner;

It is generally considered better practice to avoid mutating values we
initialize. By starting with a zeroed state and then initializing the
members, you are at risk of forgetting to initialize some.

What you should do is initialize your `GspVfInfo` in its entirety in a
final statement like the other command structures do, storing
intermediate values in temporary variables if needed.

> +
> +        if vgpu_support {
> +            let val = pdev.sriov_get_totalvfs()?;
> +            info.totalVFs = u32::try_from(val)?;

We should be able to avoid this `try_from` once `sriov_get_totalvfs`
returns the correct `u16` type as suggested on the first patch. :)

> +
> +            let pos = pdev
> +                .find_ext_capability(kernel::bindings::PCI_EXT_CAP_ID_SRIOV as i32)
> +                .ok_or(ENODEV)?;

`pos` also seems to be the wrong type - it seems to me that it should be unsigned...

> +
> +            let val = pdev.config_read_word(
> +                i32::from(pos) + i32::from(kernel::bindings::PCI_SRIOV_VF_OFFSET as i32),
> +            )?;

... but I guess this comes from here - I understand that
`config_read_word` takes an `int` for its offset parameter, but when I
read the C code I also see checks that return errors if that offset is
bigger than 4095. What does the PCI spec says? It seems we can go with a
`u16` for the offset, which would simplify this code quite a bit.

Also please don't use `as` whenever possible, there are utility
functions in `crate::num` to do the conversions infallibly. You will
probably be interested in `u32_into_u16` for this method.

> +            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)?;

This `try_from` will always fail as `PCI_BASE_ADDRESS_MEM_MASK` is
negative. This is a case for a legit use of `as` (with a CAST: comment),
although this is also a case for us generating better bindings. :)

> +
> +            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);

Magic numbers baaaaaaad. Let's define these as proper registers.

Powered by blists - more mailing lists