Replace the hardcoded 47-bit DMA mask with per-architecture values.
Add Architecture::dma_mask() with an exhaustive match, so new
architectures get a compile-time reminder to specify their width.

Move Spec creation into probe() so the architecture is known before
setting the DMA mask, and pass Spec into Gpu::new().

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/nova-core/driver.rs | 28 ++++++++++++----------------
 drivers/gpu/nova-core/gpu.rs    | 29 ++++++++++++++++++++---------
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index 84b0e1703150..41227d29934e 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -5,7 +5,6 @@
     device::Core,
     devres::Devres,
     dma::Device,
-    dma::DmaMask,
     pci,
     pci::{
         Class,
@@ -23,7 +22,10 @@
     },
 };
 
-use crate::gpu::Gpu;
+use crate::gpu::{
+    Gpu,
+    Spec, //
+};
 
 /// Counter for generating unique auxiliary device IDs.
 static AUXILIARY_ID_COUNTER: Atomic<u32> = Atomic::new(0);
@@ -38,14 +40,6 @@ pub(crate) struct NovaCore {
 
 const BAR0_SIZE: usize = SZ_16M;
 
-// For now we only support Ampere which can use up to 47-bit DMA addresses.
-//
-// TODO: Add an abstraction for this to support newer GPUs which may support
-// larger DMA addresses. Limiting these GPUs to smaller address widths won't
-// have any adverse affects, unless installed on systems which require larger
-// DMA addresses. These systems should be quite rare.
-const GPU_DMA_BITS: u32 = 47;
-
 pub(crate) type Bar0 = pci::Bar<BAR0_SIZE>;
 
 kernel::pci_device_table!(
@@ -84,18 +78,20 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
             pdev.enable_device_mem()?;
             pdev.set_master();
 
-            // SAFETY: No concurrent DMA allocations or mappings can be made because
-            // the device is still being probed and therefore isn't being used by
-            // other threads of execution.
-            unsafe { pdev.dma_set_mask_and_coherent(DmaMask::new::<GPU_DMA_BITS>())? };
-
             let bar = Arc::pin_init(
                 pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0"),
                 GFP_KERNEL,
             )?;
+            let spec = Spec::new(pdev.as_ref(), bar.access(pdev.as_ref())?)?;
+            dev_info!(pdev, "NVIDIA ({})\n", spec);
+
+            // SAFETY: No concurrent DMA allocations or mappings can be made because
+            // the device is still being probed and therefore isn't being used by
+            // other threads of execution.
+            unsafe { pdev.dma_set_mask_and_coherent(spec.chipset().arch().dma_mask())? };
 
             Ok(try_pin_init!(Self {
-                gpu <- Gpu::new(pdev, bar.clone(), bar.access(pdev.as_ref())?),
+                gpu <- Gpu::new(pdev, bar.clone(), bar.access(pdev.as_ref())?, spec),
                 _reg <- auxiliary::Registration::new(
                     pdev.as_ref(),
                     c"nova-drm",
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 3cd7709883be..e7c3860cfb28 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -3,6 +3,7 @@
 use kernel::{
     device,
     devres::Devres,
+    dma::DmaMask,
     fmt,
     pci,
     prelude::*,
@@ -162,6 +163,16 @@ pub(crate) enum Architecture {
     Blackwell = 0x1b,
 }
 
+impl Architecture {
+    /// Returns the DMA mask supported by this architecture.
+    pub(crate) const fn dma_mask(&self) -> DmaMask {
+        match self {
+            Self::Turing | Self::Ampere | Self::Ada => DmaMask::new::<47>(),
+            Self::Hopper | Self::Blackwell => DmaMask::new::<52>(),
+        }
+    }
+}
+
 impl TryFrom<u8> for Architecture {
     type Error = Error;
 
@@ -213,7 +224,7 @@ pub(crate) struct Spec {
 }
 
 impl Spec {
-    fn new(dev: &device::Device, bar: &Bar0) -> Result<Spec> {
+    pub(crate) fn new(dev: &device::Device, bar: &Bar0) -> Result<Spec> {
         // Some brief notes about boot0 and boot42, in chronological order:
         //
         // NV04 through NV50:
@@ -295,32 +306,32 @@ pub(crate) fn new<'a>(
         pdev: &'a pci::Device<device::Bound>,
         devres_bar: Arc<Devres<Bar0>>,
         bar: &'a Bar0,
+        spec: Spec,
     ) -> impl PinInit<Self, Error> + 'a {
-        try_pin_init!(Self {
-            spec: Spec::new(pdev.as_ref(), bar).inspect(|spec| {
-                dev_info!(pdev, "NVIDIA ({})\n", spec);
-            })?,
+        let chipset = spec.chipset();
 
+        try_pin_init!(Self {
             _: {
                 gfw::wait_gfw_boot_completion(bar)
                     .inspect_err(|_| dev_err!(pdev, "GFW boot did not complete\n"))?;
             },
 
-            sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?,
+            sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, chipset)?,
 
             gsp_falcon: Falcon::new(
                 pdev.as_ref(),
-                spec.chipset,
+                chipset,
             )
             .inspect(|falcon| falcon.clear_swgen0_intr(bar))?,
 
-            sec2_falcon: Falcon::new(pdev.as_ref(), spec.chipset)?,
+            sec2_falcon: Falcon::new(pdev.as_ref(), chipset)?,
 
             gsp <- Gsp::new(pdev),
 
-            _: { gsp.boot(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)? },
+            _: { gsp.boot(pdev, bar, chipset, gsp_falcon, sec2_falcon)? },
 
             bar: devres_bar,
+            spec,
         })
     }
 
-- 
2.53.0

On Wed Mar 25, 2026 at 4:52 AM CET, John Hubbard wrote:
> Move Spec creation into probe() so the architecture is known before
> setting the DMA mask, and pass Spec into Gpu::new().

Hm...this is not what we discussed in [1]. You implemented the change in v5 and
v6 and v7 reverted it. Why?

[1] https://lore.kernel.org/all/DGC28K8J39E2.1X173NHT9ZJDI@kernel.org/

On Wed Mar 25, 2026 at 8:31 PM JST, Danilo Krummrich wrote:
> On Wed Mar 25, 2026 at 4:52 AM CET, John Hubbard wrote:
>> Move Spec creation into probe() so the architecture is known before
>> setting the DMA mask, and pass Spec into Gpu::new().
>
> Hm...this is not what we discussed in [1]. You implemented the change in v5 and
> v6 and v7 reverted it. Why?
>
> [1] https://lore.kernel.org/all/DGC28K8J39E2.1X173NHT9ZJDI@kernel.org/

That was on my feedback [1].

`Gpu::new` carries no guarantee in itself that no DMA ops have been
performed so far; whereas in `probe` this is much clearer.

[1] https://lore.kernel.org/rust-for-linux/DGYYH7GCIKJ6.2AICU9NFSKY92@nvidia.com/

On Wed Mar 25, 2026 at 12:45 PM CET, Alexandre Courbot wrote:
> On Wed Mar 25, 2026 at 8:31 PM JST, Danilo Krummrich wrote:
>> On Wed Mar 25, 2026 at 4:52 AM CET, John Hubbard wrote:
>>> Move Spec creation into probe() so the architecture is known before
>>> setting the DMA mask, and pass Spec into Gpu::new().
>>
>> Hm...this is not what we discussed in [1]. You implemented the change in v5 and
>> v6 and v7 reverted it. Why?
>>
>> [1] https://lore.kernel.org/all/DGC28K8J39E2.1X173NHT9ZJDI@kernel.org/
>
> That was on my feedback [1].
>
> `Gpu::new` carries no guarantee in itself that no DMA ops have been
> performed so far; whereas in `probe` this is much clearer.

That's not what has to be justified. Having DMA allocations before setting the
mask is not a violation of the safety contract; it is about concurrent
allocations.

That said, all DMA allocations are on the Gpu object, so there can't be
concurrent DMA allocations while we are still in the constructor of the Gpu
object.

Unless...one would try to create a second Gpu object from probe() (which
obviously makes zero sense). nova-core already relies on Gpu to be unique per
device, as attempting to create a second Gpu object causes UB and actual memory
corruption when there is no IOMMU. :)

[ 123.966391] pci 0000:01:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x000e address=0xf7fef000 flags=0x0000]

So, I think technically Gpu::new() already has the safety requirement that it
must not be constructed more than once per device.

I wonder if we want probe() to give out a ProbeToken<'a>, so a driver type can
consume it by value to guarantee uniqueness of an object without unsafe.

On Wed Mar 25, 2026 at 10:38 PM JST, Danilo Krummrich wrote:
> On Wed Mar 25, 2026 at 12:45 PM CET, Alexandre Courbot wrote:
>> On Wed Mar 25, 2026 at 8:31 PM JST, Danilo Krummrich wrote:
>>> On Wed Mar 25, 2026 at 4:52 AM CET, John Hubbard wrote:
>>>> Move Spec creation into probe() so the architecture is known before
>>>> setting the DMA mask, and pass Spec into Gpu::new().
>>>
>>> Hm...this is not what we discussed in [1]. You implemented the change in v5 and
>>> v6 and v7 reverted it. Why?
>>>
>>> [1] https://lore.kernel.org/all/DGC28K8J39E2.1X173NHT9ZJDI@kernel.org/
>>
>> That was on my feedback [1].
>>
>> `Gpu::new` carries no guarantee in itself that no DMA ops have been
>> performed so far; whereas in `probe` this is much clearer.
>
> That's not what has to be justified. Having DMA allocations before setting the
> mask is not a violation of the safety contract; it is about concurrent
> allocations.
>
> That said, all DMA allocations are on the Gpu object, so there can't be
> concurrent DMA allocations while we are still in the constructor of the Gpu
> object.
>
> Unless...one would try to create a second Gpu object from probe() (which
> obviously makes zero sense). nova-core already relies on Gpu to be unique per
> device, as attempting to create a second Gpu object causes UB and actual memory
> corruption when there is no IOMMU. :)
>
> [ 123.966391] pci 0000:01:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x000e address=0xf7fef000 flags=0x0000]
>
> So, I think technically Gpu::new() already has the safety requirement that it
> must not be constructed more than once per device.

Yeah, I guess that's sufficient indeed. John, my apologies for the
back-and-forth.

On 3/25/26 6:56 AM, Alexandre Courbot wrote:
> On Wed Mar 25, 2026 at 10:38 PM JST, Danilo Krummrich wrote:
>> On Wed Mar 25, 2026 at 12:45 PM CET, Alexandre Courbot wrote:
>>> On Wed Mar 25, 2026 at 8:31 PM JST, Danilo Krummrich wrote:
>>>> On Wed Mar 25, 2026 at 4:52 AM CET, John Hubbard wrote:
>>>>> Move Spec creation into probe() so the architecture is known before
>>>>> setting the DMA mask, and pass Spec into Gpu::new().
>>>>
>>>> Hm...this is not what we discussed in [1]. You implemented the change in v5 and
>>>> v6 and v7 reverted it. Why?
>>>>
>>>> [1] https://lore.kernel.org/all/DGC28K8J39E2.1X173NHT9ZJDI@kernel.org/
>>>
>>> That was on my feedback [1].
>>>
>>> `Gpu::new` carries no guarantee in itself that no DMA ops have been
>>> performed so far; whereas in `probe` this is much clearer.
>>
>> That's not what has to be justified. Having DMA allocations before setting the
>> mask is not a violation of the safety contract; it is about concurrent
>> allocations.
>>
>> That said, all DMA allocations are on the Gpu object, so there can't be
>> concurrent DMA allocations while we are still in the constructor of the Gpu
>> object.
>>
>> Unless...one would try to create a second Gpu object from probe() (which
>> obviously makes zero sense). nova-core already relies on Gpu to be unique per
>> device, as attempting to create a second Gpu object causes UB and actual memory
>> corruption when there is no IOMMU. :)
>>
>> [ 123.966391] pci 0000:01:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x000e address=0xf7fef000 flags=0x0000]
>>
>> So, I think technically Gpu::new() already has the safety requirement that it
>> must not be constructed more than once per device.
> 
> Yeah, I guess that's sufficient indeed. John, my apologies for the
> back-and-forth.

Not a problem! :)

thanks,
-- 
John Hubbard

On Tue, 24 Mar 2026 20:52:16 -0700, John Hubbard <jhubbard@nvidia.com> wrote:
> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
> index 84b0e1703150..41227d29934e 100644
> --- a/drivers/gpu/nova-core/driver.rs
> +++ b/drivers/gpu/nova-core/driver.rs
> @@ -84,18 +78,20 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
> [ ... skip 13 lines ... ]
> +            dev_info!(pdev, "NVIDIA ({})\n", spec);
> +
> +            // SAFETY: No concurrent DMA allocations or mappings can be made because
> +            // the device is still being probed and therefore isn't being used by
> +            // other threads of execution.
> +            unsafe { pdev.dma_set_mask_and_coherent(spec.chipset().arch().dma_mask())? };

To limit the unsafe block to unsafe operations, we should store the DMA
mask into a local variable.

>
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 3cd7709883be..e7c3860cfb28 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -162,6 +163,16 @@ pub(crate) enum Architecture {
>      Blackwell = 0x1b,
>  }
>  
> +impl Architecture {
> +    /// Returns the DMA mask supported by this architecture.
> +    pub(crate) const fn dma_mask(&self) -> DmaMask {
> +        match self {
> +            Self::Turing | Self::Ampere | Self::Ada => DmaMask::new::<47>(),
> +            Self::Hopper | Self::Blackwell => DmaMask::new::<52>(),
> +        }
> +    }
> +}

Eventually we may want to move this to the `Gpu` HAL, but let's do that
as a follow-up patch if we find it is relevant (which I am not sure
about yet).

> @@ -295,32 +306,32 @@ pub(crate) fn new<'a>(
>          pdev: &'a pci::Device<device::Bound>,
>          devres_bar: Arc<Devres<Bar0>>,
>          bar: &'a Bar0,
> +        spec: Spec,
>      ) -> impl PinInit<Self, Error> + 'a {
> -        try_pin_init!(Self {
> -            spec: Spec::new(pdev.as_ref(), bar).inspect(|spec| {
> -                dev_info!(pdev, "NVIDIA ({})\n", spec);
> -            })?,
> +        let chipset = spec.chipset();

`spec` is now `Copy`, so we don't need this local variable anymore.
Removing it and accessing `spec.chipset` directly simplifies the
remainder of the diff.

-- 
Alexandre Courbot <acourbot@nvidia.com>

On 3/25/26 3:53 AM, Alexandre Courbot wrote:
> On Tue, 24 Mar 2026 20:52:16 -0700, John Hubbard <jhubbard@nvidia.com> wrote:
>> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
>> index 84b0e1703150..41227d29934e 100644
>> --- a/drivers/gpu/nova-core/driver.rs
>> +++ b/drivers/gpu/nova-core/driver.rs
>> @@ -84,18 +78,20 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
>> [ ... skip 13 lines ... ]
>> +            dev_info!(pdev, "NVIDIA ({})\n", spec);
>> +
>> +            // SAFETY: No concurrent DMA allocations or mappings can be made because
>> +            // the device is still being probed and therefore isn't being used by
>> +            // other threads of execution.
>> +            unsafe { pdev.dma_set_mask_and_coherent(spec.chipset().arch().dma_mask())? };
> 
> To limit the unsafe block to unsafe operations, we should store the DMA
> mask into a local variable.

Done, thanks.

> 
>>
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index 3cd7709883be..e7c3860cfb28 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -162,6 +163,16 @@ pub(crate) enum Architecture {
>>      Blackwell = 0x1b,
>>  }
>>  
>> +impl Architecture {
>> +    /// Returns the DMA mask supported by this architecture.
>> +    pub(crate) const fn dma_mask(&self) -> DmaMask {
>> +        match self {
>> +            Self::Turing | Self::Ampere | Self::Ada => DmaMask::new::<47>(),
>> +            Self::Hopper | Self::Blackwell => DmaMask::new::<52>(),
>> +        }
>> +    }
>> +}
> 
> Eventually we may want to move this to the `Gpu` HAL, but let's do that
> as a follow-up patch if we find it is relevant (which I am not sure
> about yet).

Ack. 

> 
>> @@ -295,32 +306,32 @@ pub(crate) fn new<'a>(
>>          pdev: &'a pci::Device<device::Bound>,
>>          devres_bar: Arc<Devres<Bar0>>,
>>          bar: &'a Bar0,
>> +        spec: Spec,
>>      ) -> impl PinInit<Self, Error> + 'a {
>> -        try_pin_init!(Self {
>> -            spec: Spec::new(pdev.as_ref(), bar).inspect(|spec| {
>> -                dev_info!(pdev, "NVIDIA ({})\n", spec);
>> -            })?,
>> +        let chipset = spec.chipset();
> 
> `spec` is now `Copy`, so we don't need this local variable anymore.
> Removing it and accessing `spec.chipset` directly simplifies the
> remainder of the diff.
> 

Fixed, thanks.

thanks,
-- 
John Hubbard