gpu: nova-core: arm32 build errors

gpu: nova-core: arm32 build errors

[Miguel Ojeda]

Hi Danilo et al.,

In Linus' tree and -next, for an arm32 LLVM defconfig plus Rust build, I see:

    error[E0308]: mismatched types
      --> drivers/gpu/nova-core/fb.rs:49:59
       |
    49 |         hal::fb_hal(chipset).write_sysmem_flush_page(bar, page.dma_handle())?;
       |                              -----------------------      ^^^^^^^^^^^^^^^^^ expected `u64`, found `u32`
       |                              |
       |                              arguments to this method are incorrect
       |
    note: method defined here
      --> drivers/gpu/nova-core/fb/hal.rs:19:8
       |
    19 |     fn write_sysmem_flush_page(&self, bar: &Bar0, addr: u64) -> Result;
       |        ^^^^^^^^^^^^^^^^^^^^^^^
    help: you can convert a `u32` to a `u64`
       |
    49 |         hal::fb_hal(chipset).write_sysmem_flush_page(bar, page.dma_handle().into())?;
       |                                                                            +++++++


    error[E0308]: mismatched types
      --> drivers/gpu/nova-core/fb.rs:65:47
       |
    65 |         if hal.read_sysmem_flush_page(bar) == self.page.dma_handle() {
       |            -------------------------------    ^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found `u32`
       |            |
       |            expected because this is `u64`
       |
    help: you can convert a `u32` to a `u64`
       |
    65 |         if hal.read_sysmem_flush_page(bar) == self.page.dma_handle().into() {
       |                                                                     +++++++


    error: this arithmetic operation will overflow
       --> drivers/gpu/nova-core/falcon.rs:469:23
        |
    469 |             .set_base((dma_start >> 40) as u16)
        |                       ^^^^^^^^^^^^^^^^^ attempt to shift right by `40_i32`, which would overflow
        |
        = note: `#[deny(arithmetic_overflow)]` on by default

Similar to another one I sent, I hope it helps -- it may be useful to make it
build in 32-bit as a test for those kinds of platforms.

Thanks!

Cheers,
Miguel

Re: gpu: nova-core: arm32 build errors

[Danilo Krummrich]

Hi Miguel,

Thanks for reporting!

On 8/28/25 6:02 PM, Miguel Ojeda wrote:
> Hi Danilo et al.,
> 
> In Linus' tree and -next, for an arm32 LLVM defconfig plus Rust build, I see:
> 
>      error[E0308]: mismatched types
>        --> drivers/gpu/nova-core/fb.rs:49:59
>         |
>      49 |         hal::fb_hal(chipset).write_sysmem_flush_page(bar, page.dma_handle())?;
>         |                              -----------------------      ^^^^^^^^^^^^^^^^^ expected `u64`, found `u32`
>         |                              |
>         |                              arguments to this method are incorrect
>         |
>      note: method defined here
>        --> drivers/gpu/nova-core/fb/hal.rs:19:8
>         |
>      19 |     fn write_sysmem_flush_page(&self, bar: &Bar0, addr: u64) -> Result;
>         |        ^^^^^^^^^^^^^^^^^^^^^^^
>      help: you can convert a `u32` to a `u64`
>         |
>      49 |         hal::fb_hal(chipset).write_sysmem_flush_page(bar, page.dma_handle().into())?;
>         |                                                                            +++++++
> 
> 
>      error[E0308]: mismatched types
>        --> drivers/gpu/nova-core/fb.rs:65:47
>         |
>      65 |         if hal.read_sysmem_flush_page(bar) == self.page.dma_handle() {
>         |            -------------------------------    ^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found `u32`
>         |            |
>         |            expected because this is `u64`
>         |
>      help: you can convert a `u32` to a `u64`
>         |
>      65 |         if hal.read_sysmem_flush_page(bar) == self.page.dma_handle().into() {

I think those should just use the DMA address type introduced in [1].
Unfortunately, for a fix we have to stick to bindings::dma_addr_t.

[1] https://lore.kernel.org/lkml/20250828133323.53311-3-dakr@kernel.org/

>         |                                                                     +++++++
> 
> 
>      error: this arithmetic operation will overflow
>         --> drivers/gpu/nova-core/falcon.rs:469:23
>          |
>      469 |             .set_base((dma_start >> 40) as u16)
>          |                       ^^^^^^^^^^^^^^^^^ attempt to shift right by `40_i32`, which would overflow
>          |
>          = note: `#[deny(arithmetic_overflow)]` on by default

Should probably just be

	val.checked_shr(shift).unwrap_or(0)

I'll send a patch to fix this up.

> Similar to another one I sent, I hope it helps -- it may be useful to make it
> build in 32-bit as a test for those kinds of platforms.

Agreed.

Thanks,
Danilo

Re: gpu: nova-core: arm32 build errors

[Danilo Krummrich]

On Thu Aug 28, 2025 at 7:54 PM CEST, Danilo Krummrich wrote:
> On 8/28/25 6:02 PM, Miguel Ojeda wrote:
>> Similar to another one I sent, I hope it helps -- it may be useful to make it
>> build in 32-bit as a test for those kinds of platforms.
>
> Agreed.

Maybe I spoke too soon, it's actually pretty painful to keep 32-bit
compatibility, even though it would be nice for testing purposes.

I'll paste the diff to fix it below, I think that makes it obvious why I say
that.

Instead, we should really just depend on CONFIG_64BIT (which implies
ARCH_DMA_ADDR_T_64BIT).

--

diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 50437c67c14a..e6a22834e317 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -466,7 +466,7 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
             .set_base((dma_start >> 8) as u32)
             .write(bar, E::BASE);
         regs::NV_PFALCON_FALCON_DMATRFBASE1::default()
-            .set_base((dma_start >> 40) as u16)
+            .set_base((dma_start.checked_shr(40).unwrap_or(0)) as u16)
             .write(bar, E::BASE);

         let cmd = regs::NV_PFALCON_FALCON_DMATRFCMD::default()
diff --git a/drivers/gpu/nova-core/fb/hal.rs b/drivers/gpu/nova-core/fb/hal.rs
index 2f914948bb9a..710491ee445a 100644
--- a/drivers/gpu/nova-core/fb/hal.rs
+++ b/drivers/gpu/nova-core/fb/hal.rs
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0

+use kernel::bindings::dma_addr_t as DmaAddress;
 use kernel::prelude::*;

 use crate::driver::Bar0;
@@ -11,12 +12,12 @@

 pub(crate) trait FbHal {
     /// Returns the address of the currently-registered sysmem flush page.
-    fn read_sysmem_flush_page(&self, bar: &Bar0) -> u64;
+    fn read_sysmem_flush_page(&self, bar: &Bar0) -> DmaAddress;

     /// Register `addr` as the address of the sysmem flush page.
     ///
     /// This might fail if the address is too large for the receiving register.
-    fn write_sysmem_flush_page(&self, bar: &Bar0, addr: u64) -> Result;
+    fn write_sysmem_flush_page(&self, bar: &Bar0, addr: DmaAddress) -> Result;

     /// Returns `true` is display is supported.
     fn supports_display(&self, bar: &Bar0) -> bool;
diff --git a/drivers/gpu/nova-core/fb/hal/ga100.rs b/drivers/gpu/nova-core/fb/hal/ga100.rs
index 871c42bf033a..c4ae172b4ed4 100644
--- a/drivers/gpu/nova-core/fb/hal/ga100.rs
+++ b/drivers/gpu/nova-core/fb/hal/ga100.rs
@@ -2,6 +2,7 @@

 struct Ga100;

+use kernel::bindings::dma_addr_t as DmaAddress;
 use kernel::prelude::*;

 use crate::driver::Bar0;
@@ -10,13 +11,22 @@

 use super::tu102::FLUSH_SYSMEM_ADDR_SHIFT;

-pub(super) fn read_sysmem_flush_page_ga100(bar: &Bar0) -> u64 {
-    u64::from(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::read(bar).adr_39_08()) << FLUSH_SYSMEM_ADDR_SHIFT
+pub(super) fn read_sysmem_flush_page_ga100(bar: &Bar0) -> DmaAddress {
+    let addr = u64::from(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::read(bar).adr_39_08())
+        << FLUSH_SYSMEM_ADDR_SHIFT
         | u64::from(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI::read(bar).adr_63_40())
-            << FLUSH_SYSMEM_ADDR_SHIFT_HI
+            << FLUSH_SYSMEM_ADDR_SHIFT_HI;
+
+    addr.try_into().unwrap_or_else(|_| {
+        kernel::warn_on!(true);
+
+        0
+    })
 }

-pub(super) fn write_sysmem_flush_page_ga100(bar: &Bar0, addr: u64) {
+pub(super) fn write_sysmem_flush_page_ga100(bar: &Bar0, addr: DmaAddress) {
+    let addr = Into::<u64>::into(addr);
+
     regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI::default()
         .set_adr_63_40((addr >> FLUSH_SYSMEM_ADDR_SHIFT_HI) as u32)
         .write(bar);
@@ -34,11 +44,11 @@ pub(super) fn display_enabled_ga100(bar: &Bar0) -> bool {
 const FLUSH_SYSMEM_ADDR_SHIFT_HI: u32 = 40;

 impl FbHal for Ga100 {
-    fn read_sysmem_flush_page(&self, bar: &Bar0) -> u64 {
+    fn read_sysmem_flush_page(&self, bar: &Bar0) -> DmaAddress {
         read_sysmem_flush_page_ga100(bar)
     }

-    fn write_sysmem_flush_page(&self, bar: &Bar0, addr: u64) -> Result {
+    fn write_sysmem_flush_page(&self, bar: &Bar0, addr: DmaAddress) -> Result {
         write_sysmem_flush_page_ga100(bar, addr);

         Ok(())
diff --git a/drivers/gpu/nova-core/fb/hal/ga102.rs b/drivers/gpu/nova-core/fb/hal/ga102.rs
index a73b77e39715..271dfd522b3c 100644
--- a/drivers/gpu/nova-core/fb/hal/ga102.rs
+++ b/drivers/gpu/nova-core/fb/hal/ga102.rs
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0

+use kernel::bindings::dma_addr_t as DmaAddress;
 use kernel::prelude::*;

 use crate::driver::Bar0;
@@ -13,11 +14,11 @@ fn vidmem_size_ga102(bar: &Bar0) -> u64 {
 struct Ga102;

 impl FbHal for Ga102 {
-    fn read_sysmem_flush_page(&self, bar: &Bar0) -> u64 {
+    fn read_sysmem_flush_page(&self, bar: &Bar0) -> DmaAddress {
         super::ga100::read_sysmem_flush_page_ga100(bar)
     }

-    fn write_sysmem_flush_page(&self, bar: &Bar0, addr: u64) -> Result {
+    fn write_sysmem_flush_page(&self, bar: &Bar0, addr: DmaAddress) -> Result {
         super::ga100::write_sysmem_flush_page_ga100(bar, addr);

         Ok(())
diff --git a/drivers/gpu/nova-core/fb/hal/tu102.rs b/drivers/gpu/nova-core/fb/hal/tu102.rs
index b022c781caf4..157342411596 100644
--- a/drivers/gpu/nova-core/fb/hal/tu102.rs
+++ b/drivers/gpu/nova-core/fb/hal/tu102.rs
@@ -3,17 +3,28 @@
 use crate::driver::Bar0;
 use crate::fb::hal::FbHal;
 use crate::regs;
+
+use kernel::bindings::dma_addr_t as DmaAddress;
 use kernel::prelude::*;

 /// Shift applied to the sysmem address before it is written into `NV_PFB_NISO_FLUSH_SYSMEM_ADDR`,
 /// to be used by HALs.
 pub(super) const FLUSH_SYSMEM_ADDR_SHIFT: u32 = 8;

-pub(super) fn read_sysmem_flush_page_gm107(bar: &Bar0) -> u64 {
-    u64::from(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::read(bar).adr_39_08()) << FLUSH_SYSMEM_ADDR_SHIFT
+pub(super) fn read_sysmem_flush_page_gm107(bar: &Bar0) -> DmaAddress {
+    let addr = u64::from(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::read(bar).adr_39_08())
+        << FLUSH_SYSMEM_ADDR_SHIFT;
+
+    addr.try_into().unwrap_or_else(|_| {
+        kernel::warn_on!(true);
+
+        0
+    })
 }

-pub(super) fn write_sysmem_flush_page_gm107(bar: &Bar0, addr: u64) -> Result {
+pub(super) fn write_sysmem_flush_page_gm107(bar: &Bar0, addr: DmaAddress) -> Result {
+    let addr = Into::<u64>::into(addr);
+
     // Check that the address doesn't overflow the receiving 32-bit register.
     if addr >> (u32::BITS + FLUSH_SYSMEM_ADDR_SHIFT) == 0 {
         regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::default()
@@ -37,11 +48,11 @@ pub(super) fn vidmem_size_gp102(bar: &Bar0) -> u64 {
 struct Tu102;

 impl FbHal for Tu102 {
-    fn read_sysmem_flush_page(&self, bar: &Bar0) -> u64 {
+    fn read_sysmem_flush_page(&self, bar: &Bar0) -> DmaAddress {
         read_sysmem_flush_page_gm107(bar)
     }

-    fn write_sysmem_flush_page(&self, bar: &Bar0, addr: u64) -> Result {
+    fn write_sysmem_flush_page(&self, bar: &Bar0, addr: DmaAddress) -> Result {
         write_sysmem_flush_page_gm107(bar, addr)
     }

Re: gpu: nova-core: arm32 build errors

[Miguel Ojeda]

On Thu, Aug 28, 2025 at 9:24 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Maybe I spoke too soon, it's actually pretty painful to keep 32-bit
> compatibility, even though it would be nice for testing purposes.
>
> I'll paste the diff to fix it below, I think that makes it obvious why I say
> that.
>
> Instead, we should really just depend on CONFIG_64BIT (which implies
> ARCH_DMA_ADDR_T_64BIT).

Yeah, it isn't great.

If it were just that, maybe it it is worth it (and a `DmaAddress`
newtype, not just a typedef, could perhaps be nice anyway?), but if
you think it will become increasingly painful later, then it may be
best to focus on what matters.

It is unlikely there is going to be actual users on a 32-bit platform, right?

Cheers,
Miguel

Re: gpu: nova-core: arm32 build errors

[Miguel Ojeda]

On Thu, Aug 28, 2025 at 9:31 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> and a `DmaAddress`
> newtype, not just a typedef, could perhaps be nice anyway?

The one from your linked patch is not a newtype though, so I guess
there is a reason for that.

Cheers,
Miguel

Re: gpu: nova-core: arm32 build errors

[Danilo Krummrich]

On Thu Aug 28, 2025 at 9:31 PM CEST, Miguel Ojeda wrote:
> On Thu, Aug 28, 2025 at 9:24 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> Maybe I spoke too soon, it's actually pretty painful to keep 32-bit
>> compatibility, even though it would be nice for testing purposes.
>>
>> I'll paste the diff to fix it below, I think that makes it obvious why I say
>> that.
>>
>> Instead, we should really just depend on CONFIG_64BIT (which implies
>> ARCH_DMA_ADDR_T_64BIT).
>
> Yeah, it isn't great.
>
> If it were just that, maybe it it is worth it (and a `DmaAddress`
> newtype, not just a typedef, could perhaps be nice anyway?)

What do you have in mind what the newtype can do?

I assume the idea is to make it provide methods {to,from}_u64, where to_u64()
has to be fallible? This would be an improvement, but not really solve the issue
entirely.

The annoying part really is

	pub(super) fn read_sysmem_flush_page_ga100(bar: &Bar0) -> DmaAddress {
	    let addr = u64::from(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::read(bar).adr_39_08())
	        << FLUSH_SYSMEM_ADDR_SHIFT
	        | u64::from(regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI::read(bar).adr_63_40())
	            << FLUSH_SYSMEM_ADDR_SHIFT_HI;
	
	    addr.try_into().unwrap_or_else(|_| {
	        kernel::warn_on!(true);
	
	        0
	    })
	}

i.e. when we read a u64 from registers, but we know that what we've orignally
written there is a DmaAddress, so we can assume that a cast to DmaAddress is
fine.

But this really depends on driver specific semantics.

> but if
> you think it will become increasingly painful later, then it may be
> best to focus on what matters.

We'll have a couple more such cases for sure; I think being able to assume that
DmaAddress is always 64-bit will result in simpler and less distracting code.

But if we can come up with a good idea to deal with this with a DmaAddress type;
I'm open to that.

> It is unlikely there is going to be actual users on a 32-bit platform, right?

Yeah, I doubt that someone could think it's a great idea to run a Turing+ GPU on
some 32-bit machine.

Re: gpu: nova-core: arm32 build errors

[Danilo Krummrich]

On Thu Aug 28, 2025 at 9:36 PM CEST, Miguel Ojeda wrote:
> On Thu, Aug 28, 2025 at 9:31 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> and a `DmaAddress`
>> newtype, not just a typedef, could perhaps be nice anyway?
>
> The one from your linked patch is not a newtype though, so I guess
> there is a reason for that.

No specific reason, I didn't see a lot of value in a newtype in the first
place, depending on you answer in the other thread, may we just found some
value. :)

Re: gpu: nova-core: arm32 build errors

[Danilo Krummrich]

On Thu Aug 28, 2025 at 9:58 PM CEST, Danilo Krummrich wrote:
> On Thu Aug 28, 2025 at 9:36 PM CEST, Miguel Ojeda wrote:
>> On Thu, Aug 28, 2025 at 9:31 PM Miguel Ojeda
>> <miguel.ojeda.sandonis@gmail.com> wrote:
>>>
>>> and a `DmaAddress`
>>> newtype, not just a typedef, could perhaps be nice anyway?
>>
>> The one from your linked patch is not a newtype though, so I guess
>> there is a reason for that.
>
> No specific reason, I didn't see a lot of value in a newtype in the first
> place, depending on you answer in the other thread, may we just found some
> value. :)

To expand a bit, the typdef is also for simplicity. Eventually, drivers will do
some arithmetic on the DMA address, etc.

So, if we have a new type, we'd probably want to provide methods for doing the
most common arithmetic operations, because we don't want to convert to/from the
corresponding primitive type all the time.

At the same time we could take this further and also provide a DmaRange type,
which also considers the size for those operations.

DmaRange is actually something that I had in mind to implement subsequently,
because I'm not too happy with CoherentAllocation::dma_handle_with_offset(),
it's just too specific and insufficient.

Given that, I thought there's not that much value in making DmaAddress a new
type. (Mybe saying "no specific reason" was a slight understatement. :)

So, if the idea was to have from/to helpers, we can also do them on DmaRange.

However, given that also the above only helps in a limited way for the cases
discussed in the other thread, I feel like the best option might still be to
depend on 64-bit for Nova.

Re: gpu: nova-core: arm32 build errors

[John Hubbard]

On 8/28/25 12:31 PM, Miguel Ojeda wrote:
> On Thu, Aug 28, 2025 at 9:24 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> Maybe I spoke too soon, it's actually pretty painful to keep 32-bit
>> compatibility, even though it would be nice for testing purposes.
>>
>> I'll paste the diff to fix it below, I think that makes it obvious why I say
>> that.
>>
>> Instead, we should really just depend on CONFIG_64BIT (which implies
>> ARCH_DMA_ADDR_T_64BIT).

Yes yes yes. :)

> 
> Yeah, it isn't great.
> 
> If it were just that, maybe it it is worth it (and a `DmaAddress`
> newtype, not just a typedef, could perhaps be nice anyway?), but if
> you think it will become increasingly painful later, then it may be

Oh yes, this is just the tip of the iceberg.

> best to focus on what matters.
> 
> It is unlikely there is going to be actual users on a 32-bit platform, right?

Completely not going to happen, actually. The Open RM driver dropped
support for 32-bit platforms in *2018*, and Nova and Open RM have...a
relationship. For example, they use the same identical firmware (GSP etc).
And so it is inconceivable that we would attempt 32-bit support in
Nova.

So Nova should definitely depend upon 64-bit configs.


thanks,
-- 
John Hubbard

Re: gpu: nova-core: arm32 build errors

[Danilo Krummrich]

On 8/28/25 11:45 PM, John Hubbard wrote:
> So Nova should definitely depend upon 64-bit configs.

Yeah, I fully agree.

But I think it would have been good to have a consumer for the generic APIs that
has to deal with it, which I think was also Miguel's point.

I will send a patch for Nova to depend on 64-bit. Independent of that, I think
the code requires a few cleanups (e.g. avoiding `as` casts or using the DMA
type).

I will also further look into how we can help other drivers from the perspective
of the DMA layer to deal with this, e.g. in the context of a DmaRage type, etc.

- Danilo