Re: [PATCH 2/2] gpu: nova-core: falcon: use I/O projection to check transfer bounds

[John Hubbard <jhubbard@nvidia.com>]

On 7/24/26 4:10 AM, Alexandre Courbot wrote:
> The DMA transfer routine was computing the start of the DMA area by
> taking the handle to the beginning of the coherent allocation, and then
> adding the transfer's start offset. It then checked manually that the
> upper bound was valid.
> 
> Convert this to an I/O projection of the same region, which returns
> `ERANGE` if the passed range does not fit within the coherent
> allocation. This removes the need to perform arithmetic on DMA handles
> and to explicitly check for the bounds' validity.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>   drivers/gpu/nova-core/falcon.rs | 55 +++++++++++++++++------------------------
>   1 file changed, 23 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index cd05985f5ee6..344cb1487295 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -12,6 +12,7 @@
>           DmaAddress, //
>       },
>       io::{
> +        io_project,
>           poll::read_poll_timeout,
>           register::{
>               RegisterBase,
> @@ -511,20 +512,31 @@ fn dma_wr(
>       ) -> Result {
>           const DMA_LEN: u32 = num::usize_into_u32::<{ MEM_BLOCK_ALIGNMENT }>();
>   
> +        // DMA transfers can only be done in units of 256 bytes. Compute how many such transfers we
> +        // need to perform.
> +        let num_transfers = load_offsets.len.div_ceil(DMA_LEN);
> +
>           // For IMEM, we want to use the start offset as a virtual address tag for each page, since
>           // code addresses in the firmware (and the boot vector) are virtual.
>           //
> -        // For DMEM we can fold the start offset into the DMA handle.
> +        // For DMEM, the start offset is folded into the DMA address.
>           let (src_start, dma_start) = match target_mem {
> -            FalconMem::ImemSecure | FalconMem::ImemNonSecure => {
> -                (load_offsets.src_start, dma_obj.dma_handle())
> -            }
> -            FalconMem::Dmem => (
> -                0,
> -                dma_obj.dma_handle() + DmaAddress::from(load_offsets.src_start),
> -            ),
> +            FalconMem::ImemSecure | FalconMem::ImemNonSecure => (load_offsets.src_start, 0),
> +            FalconMem::Dmem => (0, usize::from_safe_cast(load_offsets.src_start)),
>           };
> -        if dma_start % DmaAddress::from(DMA_LEN) > 0 {
> +
> +        let dma_handle = {

Sort of "conceptually pre-existing" problem, but "handle" doesn't
quite work as a name, because handles are supposed to be opaque
items that one just uses to find and refer to things.


And below, that is violated:

> +            // Upper limit of transfer is `(num_transfers * DMA_LEN) + load_offsets.src_start`.
> +            let dma_end = num_transfers
> +                .checked_mul(DMA_LEN)
> +                .and_then(|size| size.checked_add(load_offsets.src_start))
> +                .map(usize::from_safe_cast)
> +                .ok_or(EOVERFLOW)?;
> +
> +            io_project!(dma_obj, [try: dma_start..dma_end]).dma_handle()
> +        };
> +
> +        if dma_handle % DmaAddress::from(DMA_LEN) > 0 {
>               dev_err!(
>                   self.dev,
>                   "DMA transfer start addresses must be a multiple of {}\n",
> @@ -533,27 +545,6 @@ fn dma_wr(
>               return Err(EINVAL);
>           }
>   
> -        // DMA transfers can only be done in units of 256 bytes. Compute how many such transfers we
> -        // need to perform.
> -        let num_transfers = load_offsets.len.div_ceil(DMA_LEN);
> -
> -        // Check that the area we are about to transfer is within the bounds of the DMA object.
> -        // Upper limit of transfer is `(num_transfers * DMA_LEN) + load_offsets.src_start`.
> -        match num_transfers
> -            .checked_mul(DMA_LEN)
> -            .and_then(|size| size.checked_add(load_offsets.src_start))
> -        {
> -            None => {
> -                dev_err!(self.dev, "DMA transfer length overflow\n");
> -                return Err(EOVERFLOW);
> -            }
> -            Some(upper_bound) if usize::from_safe_cast(upper_bound) > dma_obj.size() => {
> -                dev_err!(self.dev, "DMA transfer goes beyond range of DMA object\n");
> -                return Err(EINVAL);
> -            }
> -            Some(_) => (),
> -        };
> -
>           // Set up the base source DMA address.
>   
>           self.bar.write(
> @@ -561,12 +552,12 @@ fn dma_wr(
>               regs::NV_PFALCON_FALCON_DMATRFBASE::zeroed().with_base(
>                   // CAST: `as u32` is used on purpose since we do want to strip the upper bits,
>                   // which will be written to `NV_PFALCON_FALCON_DMATRFBASE1`.
> -                (dma_start >> 8) as u32,
> +                (dma_handle >> 8) as u32,

This is a "whaaat?" moment: shifting dma_start makes sense,
but shifting a handle does not.

Thoughts?


thanks,
-- 
John Hubbard