Re: [PATCH REF v4 26/27] gpu: nova-core: replace ARef<Device> with &'bound Device in SysmemFlush

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The PCIe write to unregister the sysmem flush page is a posted write. Without a subsequent dummy read to flush the PCIe bus buffer, the DMA memory may be freed before the hardware processes the unregistration, risking a use-after-free by the GPU.
- [High] The sysmem flush page DMA address is restricted to 40 bits by the hardware register on some architectures, but the driver configures a 47-bit DMA mask, leading to potential driver probe failures on systems with >1TB of RAM.
--

commit 4632ef4cc9c31d8529ab1675ddf82d3a2b423cef
Author: Danilo Krummrich <dakr@kernel.org>

gpu: nova-core: replace ARef<Device> with &'bound Device in SysmemFlush

This commit updates SysmemFlush to use a lifetime-parameterized reference
to the device instead of an ARef. This removes the unnecessary reference
counting overhead since the lifetime is statically bound.

> diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
> index 64fe5f27f41ee..a1c5ccd0a5345 100644
> --- a/drivers/gpu/nova-core/fb.rs
> +++ b/drivers/gpu/nova-core/fb.rs
[ ... ]
> @@ -49,7 +48,7 @@
>  pub(crate) struct SysmemFlush<'sys> {
>      /// Chipset we are operating on.
>      chipset: Chipset,
> -    device: ARef<device::Device>,
> +    device: &'sys device::Device,
>      bar: &'sys Bar0,
>      /// Keep the page alive as long as we need it.
>      page: CoherentHandle,

[Severity: Critical]
This is a pre-existing issue, but does the Drop implementation for SysmemFlush
need a dummy read to flush the PCIe bus buffer?

When unregistering the sysmem flush page, the MMIO write is asynchronous and
posted to the PCIe bus. Without a subsequent dummy read to flush the bus,
could the Drop implementation return and free the CoherentHandle page before
the unregistration reaches the hardware?

If the GPU executes a sysmem flush before the hardware processes the write,
it might write to the freed DMA page, which could corrupt system memory or
trigger an IOMMU fault.

>  }
>  
>  impl<'sys> SysmemFlush<'sys> {
>      /// Allocate a memory page and register it as the sysmem flush page.
>      pub(crate) fn register(
> -        dev: &device::Device<device::Bound>,
> +        dev: &'sys device::Device<device::Bound>,
>          bar: &'sys Bar0,
>          chipset: Chipset,
>      ) -> Result<Self> {

[Severity: High]
This isn't a bug introduced by this patch, but is the DMA mask configured
correctly for this allocation?

The driver configures a 47-bit coherent DMA mask, which allows the page
physical or IOVA address to exceed 40 bits. However, the hardware register
only accommodates 40 bits on some architectures.

When write_sysmem_flush_page_gm107() shifts the address by 8 and attempts
to fit it into a u32 for the NV_PFB_NISO_FLUSH_SYSMEM_ADDR register, will
this conversion fail if the allocated address is 1TB or higher?

If so, this might cause driver probe failures on systems with large amounts
of RAM or IOMMUs issuing high IOVAs.

> @@ -68,7 +67,7 @@ pub(crate) fn register(
>  
>          Ok(Self {
>              chipset,
> -            device: dev.into(),
> +            device: dev,
>              bar,
>              page,
>          })

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521233501.1191842-1-dakr@kernel.org?part=26