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@nvidia.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)?,
_: { 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)?;
+
self.cmdq
- .send_command(bar, commands::SetSystemInfo::new(pdev))?;
+ .send_command(bar, commands::SetSystemInfo::new(pdev, vf_info))?;
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,
}
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)
}
}
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,
+}
+
+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);
+ }
+ Ok(vf_info)
+ }
+}
+
+// SAFETY: Padding is explicit and does not contain uninitialized data.
+unsafe impl AsBytes for GspVfInfo {}
+
+// SAFETY: This struct only contains integer types for which all bit patterns are valid.
+unsafe impl FromBytes for GspVfInfo {}
diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
index 21be44199693..3b5c05704b2d 100644
--- a/drivers/gpu/nova-core/gsp/fw/commands.rs
+++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
@@ -4,7 +4,10 @@
use kernel::transmute::{AsBytes, FromBytes};
use kernel::{device, pci};
-use crate::gsp::GSP_PAGE_SIZE;
+use crate::gsp::{
+ fw::GspVfInfo,
+ GSP_PAGE_SIZE, //
+};
use super::bindings;
@@ -18,7 +21,10 @@ pub(crate) struct GspSetSystemInfo {
impl GspSetSystemInfo {
/// Returns an in-place initializer for the `GspSetSystemInfo` command.
#[allow(non_snake_case)]
- pub(crate) fn init<'a>(dev: &'a pci::Device<device::Bound>) -> impl Init<Self, Error> + 'a {
+ pub(crate) fn init<'a>(
+ dev: &'a pci::Device<device::Bound>,
+ info: GspVfInfo,
+ ) -> impl Init<Self, Error> + 'a {
type InnerGspSystemInfo = bindings::GspSystemInfo;
let init_inner = try_init!(InnerGspSystemInfo {
gpuPhysAddr: dev.resource_start(0)?,
@@ -38,6 +44,7 @@ pub(crate) fn init<'a>(dev: &'a pci::Device<device::Bound>) -> impl Init<Self, E
PCIRevisionID: u32::from(dev.revision_id()),
bIsPrimary: 0,
bPreserveVideoMemoryAllocations: 0,
+ gspVFInfo: info.inner,
..Zeroable::init_zeroed()
});
--
2.51.0On 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@nvidia.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.
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@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. >
Hi Zhi, On Tue, Dec 09, 2025 at 03:41:14PM +0200, Zhi Wang wrote: > On Sat, 6 Dec 2025 21:32:51 -0500 > Joel Fernandes <joelagnelf@nvidia.com> wrote: [..] > > > 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. :) I think these magic numbers are PCIe config space related. I found a couple of references [1] [2] [3] [1] In Open GPU docs, I see 0x00088000 is NV_PCFG but this is on Turing, lets confirm what it is on other architectures (if not common, should it go through a HAL?). https://github.com/NVIDIA/open-gpu-kernel-modules/blob/a5bfb10e75a4046c5d991c65f49b5d29151e68cf/src/common/inc/swref/published/turing/tu102/dev_nv_xve.h#L4 and 0xbf4 is SRIOV capability headers, per the same header file: NV_XVE_SRIOV_CAP_HDR10 Also the bit definition is not documented in that public header, but I find from internal sources that what you're trying to do with the "& 0x6" is determine whether the VF BAR is capable of 64-bit addressing: Bits [2:1] is VF_BAR1_ADR_TYPE and = 2 means the BAR is capable of 64-bit addressing, and = 0 means 32-bit. I wonder if the format of these capability headers are present in the PCI specification? It is worth checking, I find some very similar mentions of the value 2 being 64-bit in https://wiki.osdev.org/PCI as well. [2] In Nouveau I found the 0x88000 drivers/gpu/drm/nouveau/nouveau_reg.h +684 With a bunch of ids and such which is typical of what is in config space: # define NV50_PBUS_PCI_ID 0x00088000 # define NV50_PBUS_PCI_ID_VENDOR_ID 0x0000ffff # define NV50_PBUS_PCI_ID_VENDOR_ID__SHIFT 0 # define NV50_PBUS_PCI_ID_DEVICE_ID 0xffff0000 # define NV50_PBUS_PCI_ID_DEVICE_ID__SHIFT 16 Perhaps this is something pdev.config_read_dword() should be giving? [3] This one I am not sure off, but the link https://envytools.readthedocs.io/en/latest/hw/bus/pci.html says that on NV40+ cards, all 0x1000 bytes of PCIE config space are mapped to MMIO register space at addresses 0x88000-0x88fff. This matches exactly the magic number in your patch. Also, I wonder if we need to determine if the BARs can be 64-bit addressed, do we have requirements for BAR sizes > 4GB for vGPU and if not, do we need to determine the BAR size addressability? Also, shouldn't the PCI core subsystem be automatically determining if the BARs are 64-bit addressable? Not sure if that belongs in the driver. It would be good to understand how this is supposed to work. thanks, - Joel
On 12/11/25 5:36 PM, Joel Fernandes wrote: > Hi Zhi, > > On Tue, Dec 09, 2025 at 03:41:14PM +0200, Zhi Wang wrote: >> On Sat, 6 Dec 2025 21:32:51 -0500 >> Joel Fernandes <joelagnelf@nvidia.com> wrote: > [..] >>>> 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. :) > > > I think these magic numbers are PCIe config space related. I found a couple of references [1] [2] [3] > > [1] > In Open GPU docs, I see 0x00088000 is NV_PCFG but this is on Turing, lets > confirm what it is on other architectures (if not common, should it go > through a HAL?). It changed on Hopper. My Hopper/Blackwell series handles this. thanks, John Hubbard > > https://github.com/NVIDIA/open-gpu-kernel-modules/blob/a5bfb10e75a4046c5d991c65f49b5d29151e68cf/src/common/inc/swref/published/turing/tu102/dev_nv_xve.h#L4 > > and 0xbf4 is SRIOV capability headers, per the same header file: > NV_XVE_SRIOV_CAP_HDR10 > > Also the bit definition is not documented in that public header, but I find > from internal sources that what you're trying to do with the "& 0x6" is > determine whether the VF BAR is capable of 64-bit addressing: > > Bits [2:1] is VF_BAR1_ADR_TYPE and = 2 means the BAR is capable of 64-bit > addressing, and = 0 means 32-bit. > > I wonder if the format of these capability headers are present in the PCI > specification? It is worth checking, I find some very similar mentions of the > value 2 being 64-bit in https://wiki.osdev.org/PCI as well. > > [2] > In Nouveau I found the 0x88000 > drivers/gpu/drm/nouveau/nouveau_reg.h +684 > > With a bunch of ids and such which is typical of what is in config space: > > # define NV50_PBUS_PCI_ID 0x00088000 > # define NV50_PBUS_PCI_ID_VENDOR_ID 0x0000ffff > # define NV50_PBUS_PCI_ID_VENDOR_ID__SHIFT 0 > # define NV50_PBUS_PCI_ID_DEVICE_ID 0xffff0000 > # define NV50_PBUS_PCI_ID_DEVICE_ID__SHIFT 16 > > Perhaps this is something pdev.config_read_dword() should be giving? > > [3] This one I am not sure off, but the link > https://envytools.readthedocs.io/en/latest/hw/bus/pci.html says that on NV40+ > cards, all 0x1000 bytes of PCIE config space are mapped to MMIO register > space at addresses 0x88000-0x88fff. This matches exactly the magic number in > your patch. > > Also, I wonder if we need to determine if the BARs can be 64-bit addressed, do > we have requirements for BAR sizes > 4GB for vGPU and if not, do we need to > determine the BAR size addressability? > > Also, shouldn't the PCI core subsystem be automatically determining if the > BARs are 64-bit addressable? Not sure if that belongs in the driver. It would > be good to understand how this is supposed to work. > > thanks, > > - Joel >
> On Dec 12, 2025, at 9:17 AM, John Hubbard <jhubbard@nvidia.com> wrote: > > On 12/11/25 5:36 PM, Joel Fernandes wrote: >> Hi Zhi, >>> On Tue, Dec 09, 2025 at 03:41:14PM +0200, Zhi Wang wrote: >>> On Sat, 6 Dec 2025 21:32:51 -0500 >>> Joel Fernandes <joelagnelf@nvidia.com> wrote: >> [..] >>>>> 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. :) >> I think these magic numbers are PCIe config space related. I found a couple of references [1] [2] [3] >> [1] >> In Open GPU docs, I see 0x00088000 is NV_PCFG but this is on Turing, lets >> confirm what it is on other architectures (if not common, should it go >> through a HAL?). > > It changed on Hopper. My Hopper/Blackwell series handles this. Great, thanks for confirming, John. Zhi, you should probably rebase on the series from John then. thanks, - Joel > > thanks, > John Hubbard > >> https://github.com/NVIDIA/open-gpu-kernel-modules/blob/a5bfb10e75a4046c5d991c65f49b5d29151e68cf/src/common/inc/swref/published/turing/tu102/dev_nv_xve.h#L4 >> and 0xbf4 is SRIOV capability headers, per the same header file: >> NV_XVE_SRIOV_CAP_HDR10 >> Also the bit definition is not documented in that public header, but I find >> from internal sources that what you're trying to do with the "& 0x6" is >> determine whether the VF BAR is capable of 64-bit addressing: >> Bits [2:1] is VF_BAR1_ADR_TYPE and = 2 means the BAR is capable of 64-bit >> addressing, and = 0 means 32-bit. >> I wonder if the format of these capability headers are present in the PCI >> specification? It is worth checking, I find some very similar mentions of the >> value 2 being 64-bit in https://wiki.osdev.org/PCI as well. >> [2] >> In Nouveau I found the 0x88000 >> drivers/gpu/drm/nouveau/nouveau_reg.h +684 >> With a bunch of ids and such which is typical of what is in config space: >> # define NV50_PBUS_PCI_ID 0x00088000 >> # define NV50_PBUS_PCI_ID_VENDOR_ID 0x0000ffff >> # define NV50_PBUS_PCI_ID_VENDOR_ID__SHIFT 0 >> # define NV50_PBUS_PCI_ID_DEVICE_ID 0xffff0000 >> # define NV50_PBUS_PCI_ID_DEVICE_ID__SHIFT 16 >> Perhaps this is something pdev.config_read_dword() should be giving? >> [3] This one I am not sure off, but the link >> https://envytools.readthedocs.io/en/latest/hw/bus/pci.html says that on NV40+ >> cards, all 0x1000 bytes of PCIE config space are mapped to MMIO register >> space at addresses 0x88000-0x88fff. This matches exactly the magic number in >> your patch. >> Also, I wonder if we need to determine if the BARs can be 64-bit addressed, do >> we have requirements for BAR sizes > 4GB for vGPU and if not, do we need to >> determine the BAR size addressability? >> Also, shouldn't the PCI core subsystem be automatically determining if the >> BARs are 64-bit addressable? Not sure if that belongs in the driver. It would >> be good to understand how this is supposed to work. >> thanks, >> - Joel >
© 2016 - 2025 Red Hat, Inc.