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

[Zhi Wang <zhiw@nvidia.com>]

On Thu, 09 Jul 2026 14:25:25 +0900
"Alexandre Courbot" <acourbot@nvidia.com> wrote:

> On Wed Jul 1, 2026 at 3:26 PM JST, Zhi Wang wrote:
> <...>
snip

> > +/// 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?
> 

Yes. I have some example code here:

https://github.com/zhiwang-nvidia/nova-core/blob/zhi/nova-vgpu-wip/drivers/gpu/nova-core/vgpu/mod.rs#L62

It should extend more when more features are coming besides the current
functions.

> > +
> > +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.
> 

Good question. After thinking for a while, I think it should be
'Disabled'. E.g. if the user has updated the vBIOS and the PRC is not
there, we don't wanna disturb the booting flow. Let me update this in
v7.

> > +
> > +        Ok(Self { state })
> > +    }
> > +
> > +    fn detect_state(  
> 
> Let's grant a short documentation to this method.
> 

Sure.

> > +        pdev: &pci::Device<device::Bound>,
> > +        chipset: Chipset,
> > +        fsp: Option<&mut Fsp<'_>>,
> > +    ) -> Result<VgpuState> {
> > +        if !hal::supports_vgpu(chipset) {

snip

> 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.

Sure.