Re: [PATCH v3 5/8] gpu: nova-core: add vGPU preludes

On Wed Jul 1, 2026 at 3:26 PM JST, Zhi Wang wrote:
<...>
> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
> index c347558aa8e5..5abab54639a4 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -26,6 +26,7 @@
>          commands,
>          GspFwWprMeta, //
>      },
> +    vgpu::VgpuState, //
>  };
>  
>  impl super::Gsp {
> @@ -48,6 +49,18 @@ pub(crate) fn boot(
>          let dev = pdev.as_ref();
>          let hal = super::hal::gsp_hal(chipset);
>  
> +        let (vgpu_enabled, total_vfs) = match ctx.vgpu.state() {
> +            VgpuState::Disabled => (false, 0),
> +            VgpuState::Enabled { total_vfs } => (true, total_vfs),
> +        };
> +
> +        dev_dbg!(
> +            dev,
> +            "vGPU enabled: {}, total VFs: {}\n",
> +            vgpu_enabled,
> +            total_vfs
> +        );

This whole hunk is dedicated to collecting debugging information that we
most likely won't ever display. Can you instead derive `Debug` on
`VgpuState` and use something like:

  dev_dbg!(dev, "vGPU state: {:?}\n", ctx.vgpu.state());

That statement would also be better located in `VgpuManager::new` imho,
right before returning the constructed value.

> +
>          let gsp_fw = KBox::pin_init(GspFirmware::new(dev, chipset, FIRMWARE_VERSION), GFP_KERNEL)?;
>  
>          let fb_layout = FbLayout::new(chipset, bar, &gsp_fw)?;
> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> index 735b8e17c6b6..2df2f773ec8e 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -26,6 +26,7 @@
>  mod regs;
>  mod sbuffer;
>  mod vbios;
> +mod vgpu;
>  
>  pub(crate) const MODULE_NAME: &core::ffi::CStr = <LocalModule as kernel::ModuleMetadata>::NAME;
>  
> diff --git a/drivers/gpu/nova-core/vgpu.rs b/drivers/gpu/nova-core/vgpu.rs
> new file mode 100644
> index 000000000000..d2f311a8b2d5
> --- /dev/null
> +++ b/drivers/gpu/nova-core/vgpu.rs
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use kernel::{
> +    device,
> +    pci,
> +    prelude::*, //
> +};
> +
> +use crate::{
> +    fsp::{
> +        Fsp,
> +        VgpuMode, //
> +    },
> +    gpu::Chipset, //
> +};
> +
> +mod hal;
> +
> +/// vGPU state detected during GPU construction.
> +#[derive(Clone, Copy)]
> +pub(crate) enum VgpuState {
> +    /// vGPU mode is not enabled for this boot.
> +    Disabled,
> +    /// vGPU mode is enabled for this boot.
> +    Enabled {
> +        /// Total number of SR-IOV VFs supported by this device.
> +        total_vfs: u16,
> +    },
> +}
> +
> +/// vGPU state manager.
> +pub(crate) struct VgpuManager {
> +    state: VgpuState,
> +}

Just to confirm: right now `VgpuManager` is a wrapper for `VgpuState`,
but it is going to grow further in the future, right?

> +
> +impl VgpuManager {
> +    /// Creates a vGPU manager by querying SR-IOV and the FSP PRC vGPU knob.
> +    pub(crate) fn new(
> +        pdev: &pci::Device<device::Bound>,
> +        chipset: Chipset,
> +        fsp: Option<&mut Fsp<'_>>,
> +    ) -> Result<Self> {
> +        let state = Self::detect_state(pdev, chipset, fsp)?;

Do we want to return an error (and make probe fail) if the vGPU state
cannot be detected for some reason? Or should we prefer printing a
warning and returning a `Disabled` state? I don't have a particular
preference but thought it is worth pondering.

> +
> +        Ok(Self { state })
> +    }
> +
> +    fn detect_state(

Let's grant a short documentation to this method.

> +        pdev: &pci::Device<device::Bound>,
> +        chipset: Chipset,
> +        fsp: Option<&mut Fsp<'_>>,
> +    ) -> Result<VgpuState> {
> +        if !hal::supports_vgpu(chipset) {
> +            return Ok(VgpuState::Disabled);
> +        }
> +
> +        let total_vfs = pdev.sriov_get_totalvfs();
> +        if total_vfs < 2 {
> +            // The current vGPU path does not support single-VF SR-IOV devices yet.
> +            // Treat 0 or 1 total VFs as vGPU-disabled for now; single-VF support can
> +            // relax this gate once the manager handles that topology.
> +            return Ok(VgpuState::Disabled);
> +        }
> +
> +        let Some(fsp) = fsp else {
> +            return Ok(VgpuState::Disabled);
> +        };
> +
> +        match fsp.read_vgpu_mode(pdev.as_ref())? {
> +            VgpuMode::Enabled => Ok(VgpuState::Enabled { total_vfs }),
> +            VgpuMode::Disabled => Ok(VgpuState::Disabled),
> +        }
> +    }
> +
> +    /// Returns the detected vGPU state for this boot.
> +    pub(crate) fn state(&self) -> VgpuState {
> +        self.state
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/vgpu/hal.rs b/drivers/gpu/nova-core/vgpu/hal.rs
> new file mode 100644
> index 000000000000..e6fb6cec2599
> --- /dev/null
> +++ b/drivers/gpu/nova-core/vgpu/hal.rs
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use crate::gpu::{
> +    Architecture,
> +    Chipset, //
> +};
> +
> +trait VgpuHal {
> +    /// Returns whether this chipset can support vGPU.
> +    fn supports_vgpu(&self) -> bool;
> +}
> +
> +struct Tu102;
> +
> +impl VgpuHal for Tu102 {
> +    fn supports_vgpu(&self) -> bool {
> +        false
> +    }
> +}
> +
> +struct Gb202;
> +
> +impl VgpuHal for Gb202 {
> +    fn supports_vgpu(&self) -> bool {
> +        true
> +    }
> +}

`Tu102` and `Gb202` should be in their own sub-modules, even if they are
small.

> +
> +const TU102: Tu102 = Tu102;
> +const GB202: Gb202 = Gb202;
> +
> +fn vgpu_hal(chipset: Chipset) -> &'static dyn VgpuHal {
> +    match chipset.arch() {
> +        Architecture::BlackwellGB20x => &GB202,
> +        Architecture::Turing
> +        | Architecture::Ampere
> +        | Architecture::Hopper
> +        | Architecture::Ada
> +        | Architecture::BlackwellGB10x => &TU102,
> +    }
> +}
> +
> +pub(super) fn supports_vgpu(chipset: Chipset) -> bool {
> +    vgpu_hal(chipset).supports_vgpu()
> +}

We typically don't create functions for each hal method - let's make
`vgpu_hal` and `VgpuHal` `pub(super)` and use them from their caller,
like the other HALs are doing - `fsp/hal.rs` is a good model to follow.