Re: [PATCH v6 09/10] drm/tyr: support write-combine BO mappings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Critical] drm/tyr: invalid downcast in lookup_handle() for PRIME or
  dumb GEM BOs
- [High] drm/tyr: cached BO mappings on coherent devices without GPU
  coherency
- [Medium] drm/tyr: unconditional sg_table() call in new_object() when
  map_wc is false
--

commit 6624593ccd32e8779801a0fe4fb33fa8b31a7440
Author: Sami Tolvanen <samitolvanen@google.com>

drm/tyr: support write-combine BO mappings

This commit caches device DMA coherency at probe time and threads it to
GEM shmem object creation to configure write-combine vs write-back CPU
mappings, while calling sg_table() at BO creation to flush cached zero-init
pages.

> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index ba03cc4c52539..6869ab345d8fd 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
[ ... ]
> @@ -151,6 +158,8 @@ fn probe<'bound>(
>              // other threads of execution.
>              unsafe { pdev.dma_set_mask_and_coherent(DmaMask::try_new(pa_bits)?)? };
>  
> +            let coherent = pdev.as_ref().dma_attr() == DmaAttr::COHERENT;
> +
>              let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, Ok(()))?;
>  
>              let mmu = Mmu::new(pdev.as_ref(), iomem.as_arc_borrow(), &gpu_info)?;
> @@ -161,6 +170,7 @@ fn probe<'bound>(
>                  &unreg_dev,
>                  mmu.as_arc_borrow(),
>                  &gpu_info,
> +                coherent,
>              )?;

[Severity: High]
Does setting coherent from dma_attr() break MCU firmware boot and BO
mappings on platforms whose Device Tree or ACPI tables mark the GPU
DMA-coherent?

When coherent is true, gem::should_map_wc() returns false, so firmware
KernelBo sections and user BOs are mapped write-back cached (PAGE_KERNEL) on
the CPU, and dma_map_sgtable() performs no cache maintenance because the
device is marked coherent.  However, Tyr does not yet enable GPU or MMU
coherency: GpuInfo::new() hardcodes selected_coherency to
DRM_PANTHOR_GPU_COHERENCY_NONE, gpu::l2_power_on() never programs
COHERENCY_ENABLE, VmAsData::new() sets coherent_walk to false, and
MEMATTR::attribute_from_mair() always encodes Coherency::MidgardInnerDomain:

drivers/gpu/drm/tyr/regs.rs:MEMATTR::attribute_from_mair() {
    ...
            // TODO: Add a `coherent` parameter like panthor's mair_to_memattr().
            // For now, assume a non-coherent system and always encode write-back
            // memory with MidgardInnerDomain coherency.
            Self::try_new_memattr(
                 UseRecipe::AttributeMode,
                 Coherency::MidgardInnerDomain,
    ...
}

As a result, CPU writes to the cached vmap in Firmware::init_section_mem()
can remain in dirty CPU cache lines while the non-coherent MCU and GPU read
stale DRAM.

> diff --git a/drivers/gpu/drm/tyr/gem.rs b/drivers/gpu/drm/tyr/gem.rs
> index 60f6363995a5a..55e2d67b96d98 100644
> --- a/drivers/gpu/drm/tyr/gem.rs
> +++ b/drivers/gpu/drm/tyr/gem.rs
[ ... ]
> @@ -38,6 +43,14 @@ pub(crate) struct BoData {
>      flags: u32,
>  }
>  
> +impl BoData {
> +    /// Returns the flags the BO was created with.
> +    #[expect(dead_code)]
> +    pub(crate) fn flags(&self) -> u32 {
> +        self.flags
> +    }
> +}

[Severity: Critical]
Can looking up a GEM handle via gem::lookup_handle() and accessing Bo fields
such as BoData::flags() or Object::sg_table() read and write out of bounds on
PRIME-imported or dumb GEM objects?

In rust/kernel/drm/gem/shmem.rs, Object::ALLOC_OPS leaves gem_create_object
as None while wiring gem_prime_import_sg_table and dumb_create to the C
drm_gem_shmem helpers:

rust/kernel/drm/gem/shmem.rs:Object::ALLOC_OPS {
    const ALLOC_OPS: driver::AllocOps = driver::AllocOps {
        gem_create_object: None,
        prime_handle_to_fd: None,
        prime_fd_to_handle: None,
        gem_prime_import: None,
        gem_prime_import_sg_table: Some(bindings::drm_gem_shmem_prime_import_sg_table),
        dumb_create: Some(bindings::drm_gem_shmem_dumb_create),
        dumb_map_offset: None,
    };
}

When userspace creates a handle via DRM_IOCTL_PRIME_FD_TO_HANDLE or
DRM_IOCTL_MODE_CREATE_DUMB, __drm_gem_shmem_create() sees a NULL
gem_create_object callback and allocates only sizeof(struct
drm_gem_shmem_object) bytes without the trailing Rust gem::shmem::Object<T>
fields (parent_resv_obj, sgt_res, sgt_lock, and inner: BoData).  Passing
that handle to DRM_IOCTL_PANTHOR_VM_BIND (or a subsequent BO ioctl) resolves
it through Bo::lookup_handle(), which uncheckedly casts the bare struct
drm_gem_shmem_object pointer to &gem::shmem::Object<BoData> and accesses
sgt_lock, sgt_res, or inner.flags past the end of the slab allocation.

> @@ -56,24 +69,48 @@ fn new(_dev: &TyrDrmDevice, _size: usize, args: BoCreateArgs) -> impl PinInit<Se
[ ... ]
>  /// Create a new GEM buffer object.
>  #[expect(dead_code)]
> -pub(crate) fn new_object(ddev: &TyrDrmDevice, size: usize, flags: u32) -> Result<ARef<Bo>> {
> +pub(crate) fn new_object(
> +    ddev: &TyrDrmDevice,
> +    pdev: &Device<Bound>,
> +    size: usize,
> +    flags: u32,
> +    coherent: bool,
> +) -> Result<ARef<Bo>> {
>      if size == 0 {
>          return Err(EINVAL);
>      }
>  
>      let aligned_size = size.checked_next_multiple_of(PAGE_SIZE).ok_or(EINVAL)?;
>  
> -    Bo::new(
> +    let map_wc = should_map_wc(coherent, flags);
> +    let bo = Bo::new(
>          ddev,
>          aligned_size,
>          shmem::ObjectConfig {
> -            map_wc: true,
> +            map_wc,
>              parent_resv_obj: None,
>          },
>          BoCreateArgs { flags },
> -    )
> +    )?;
> +
> +    bo.sg_table(pdev)?;
> +
> +    Ok(bo)
>  }

[Severity: Medium]
Should bo.sg_table(pdev) only be called when map_wc is true?

Object::sg_table() caches its result in a SetOnce (self.sgt_res).  On a
non-coherent device when userspace sets DRM_PANTHOR_BO_WB_MMAP (map_wc is
false), calling bo.sg_table(pdev) at BO creation immediately pins the pages
and consumes the one-time dma_map_sgtable() cache flush before userspace
mmaps and writes to the cached buffer.  When the BO is later bound via
DRM_IOCTL_PANTHOR_VM_BIND, GpuVmData::sm_step_map() -> bo.sg_table() hits the
already-populated SetOnce and skips dma_map_sgtable(), leaving CPU writes
made before VM_BIND unflushed in the CPU cache.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-tyr-ioctls-v6-0-b2896bf2b3c9@kylinos.cn?part=9