diff mbox series

[v4,08/16] drm/tyr: specify base type for registers

Message ID 20260901-typed_register-v4-8-5552b1d59525@garyguo.net
State New
Headers show
Series rust: io: support register projections and remove relative registers | expand

Commit Message

Gary Guo Sept. 1, 2026, 4:50 p.m. UTC 
All registers use the same base type, which is `<IoMem as IO>::Target`.
Thus add the base parameter to `register!` invocation.

Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/gpu/drm/tyr/driver.rs |  1 +
 drivers/gpu/drm/tyr/fw.rs     |  5 ++++-
 drivers/gpu/drm/tyr/regs.rs   | 45 ++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 49 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 1, 2026, 5:04 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Out-of-bounds Register Definition: The `DOORBELL` register array exceeds the maximum size of its specified base type `TyrRegisters`.
- [Low] API Misuse: Applying `base: TyrRegisters;` to purely in-memory bitfields allows them to silently alias hardware MMIO registers.
--

commit 1d550d1a42720086769c2c8dc9fd2f019102d497
Author: Gary Guo <gary@garyguo.net>

drm/tyr: specify base type for registers

This commit updates register definitions to specify TyrRegisters as the base
type for all register! invocations.

> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index bfb0ba19caff9..730b84e37a544 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -46,6 +46,7 @@
>  };
>  
>  pub(crate) type IoMem<'a> = kernel::io::mem::IoMem<'a, SZ_2M>;
> +pub(crate) type TyrRegisters = kernel::io::Region<SZ_2M>;

[ ... ]

> diff --git a/drivers/gpu/drm/tyr/fw.rs b/drivers/gpu/drm/tyr/fw.rs
> index 47d25c901bd01..7edb5eff17077 100644
> --- a/drivers/gpu/drm/tyr/fw.rs
> +++ b/drivers/gpu/drm/tyr/fw.rs
> @@ -101,6 +102,8 @@ fn from(value: CacheMode) -> Self {
>  }
>  
>  register! {
> +    base: TyrRegisters;
> +
>       #[allow(non_upper_case_globals)]
>      pub(super) SectionFlags(u32) @ 0x0 {
>          0:0 read => bool;

[Severity: Low]
Does applying an MMIO base to this firmware struct unintentionally allow it
to be used in hardware I/O operations?

By specifying base: TyrRegisters; and @ 0x0, SectionFlags gains I/O traits
tied to the hardware MMIO base. Since this appears to be an in-memory bitfield
used for parsing firmware, should the bitfield! macro be used instead to
prevent it from silently aliasing hardware registers?

[ ... ]

> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index a62724378ced2..0c419c4e1186c 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs
[ ... ]
> @@ -1104,6 +1131,8 @@ fn from(val: MemoryType) -> Self {
>          }
>  
>          register! {
> +            base: TyrRegisters;
> +
>              /// Stage 1 memory attributes (8-bit bitfield).
>              ///
>              /// This is not an actual register, but a bitfield definition used by the MEMATTR

[Severity: Low]
Similar to SectionFlags above, the comment explicitly notes this is not an
actual register. Does adding base: TyrRegisters; here mistakenly grant it
MMIO traits and allow accidental I/O operations?

[ ... ]

> @@ -1770,6 +1811,8 @@ pub(crate) mod doorbell_block {
>      const STRIDE: usize = 0x10000;
>  
>      register! {
> +        base: TyrRegisters;
> +
>          /// Doorbell request register. Write-only.
>          pub(crate) DOORBELL(u32)[NUM_DOORBELLS, stride = STRIDE] @ 0x80000 {
>              /// Doorbell set. Writing 1 triggers the doorbell.

[Severity: Medium]
Does this base type provide enough address space for the entire array?

TyrRegisters is defined in driver.rs as kernel::io::Region<SZ_2M>
(a 2MB limit). However, the DOORBELL array has 64 elements (NUM_DOORBELLS)
with a stride of 0x10000 and starts at offset 0x80000. 

Accessing higher indices in this array will exceed the 2MB limit (e.g., the
maximum offset is 0x80000 + 63 * 0x10000 = 0x470000, ~4.5MB). Will this
result in a build assertion failure or runtime out-of-bounds error when
upper doorbell indices are accessed?
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
index bfb0ba19caff..730b84e37a54 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -46,6 +46,7 @@ 
 };
 
 pub(crate) type IoMem<'a> = kernel::io::mem::IoMem<'a, SZ_2M>;
+pub(crate) type TyrRegisters = kernel::io::Region<SZ_2M>;
 
 pub(crate) struct TyrDrmDriver;
 
diff --git a/drivers/gpu/drm/tyr/fw.rs b/drivers/gpu/drm/tyr/fw.rs
index 47d25c901bd0..7edb5eff1707 100644
--- a/drivers/gpu/drm/tyr/fw.rs
+++ b/drivers/gpu/drm/tyr/fw.rs
@@ -39,7 +39,8 @@ 
 use crate::{
     driver::{
         IoMem,
-        TyrDrmDevice, //
+        TyrDrmDevice,
+        TyrRegisters, //
     },
     fw::parser::{
         FwParser,
@@ -101,6 +102,8 @@  fn from(value: CacheMode) -> Self {
 }
 
 register! {
+    base: TyrRegisters;
+
      #[allow(non_upper_case_globals)]
     pub(super) SectionFlags(u32) @ 0x0 {
         0:0 read => bool;
diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
index a62724378ced..0c419c4e1186 100644
--- a/drivers/gpu/drm/tyr/regs.rs
+++ b/drivers/gpu/drm/tyr/regs.rs
@@ -57,7 +57,11 @@  pub(crate) mod gpu_control {
         uapi, //
     };
 
+    use crate::driver::TyrRegisters;
+
     register! {
+        base: TyrRegisters;
+
         /// GPU identification register.
         pub(crate) GPU_ID(u32) @ 0x0 {
             /// Status of the GPU release.
@@ -315,6 +319,8 @@  fn from(mode: FlushMode) -> Self {
     }
 
     register! {
+        base: TyrRegisters;
+
         /// GPU command register.
         ///
         /// Use the constructor methods to create commands:
@@ -380,6 +386,8 @@  pub(crate) fn clear_fault() -> Self {
     }
 
     register! {
+        base: TyrRegisters;
+
         /// GPU status register. Read only.
         pub(crate) GPU_STATUS(u32) @ 0x34 {
             /// GPU active, a 1-bit boolean flag.
@@ -463,6 +471,8 @@  fn from(access: AccessType) -> Self {
     }
 
     register! {
+        base: TyrRegisters;
+
         /// GPU fault status register. Read only.
         pub(crate) GPU_FAULTSTATUS(u32) @ 0x3c {
             /// Exception type.
@@ -768,6 +778,8 @@  fn from(mode: CoherencyMode) -> Self {
     }
 
     register! {
+        base: TyrRegisters;
+
         /// Coherency enable. An index of which coherency protocols should be used.
         /// This register only selects the protocol for coherency messages on the
         /// interconnect. This is not to enable or disable coherency controlled by MMU.
@@ -808,6 +820,8 @@  fn from(mode: McuControlMode) -> Self {
     }
 
     register! {
+        base: TyrRegisters;
+
         /// MCU control.
         pub(crate) MCU_CONTROL(u32) @ 0x700 {
             /// Request MCU state change.
@@ -849,6 +863,8 @@  fn from(status: McuStatus) -> Self {
     }
 
     register! {
+        base: TyrRegisters;
+
         /// MCU status. Read only.
         pub(crate) MCU_STATUS(u32) @ 0x704 {
             /// Read current state of MCU.
@@ -862,7 +878,11 @@  fn from(status: McuStatus) -> Self {
 pub(crate) mod job_control {
     use kernel::register;
 
+    use crate::driver::TyrRegisters;
+
     register! {
+        base: TyrRegisters;
+
         /// Raw status of job interrupts.
         ///
         /// Write to this register to trigger these interrupts.
@@ -912,7 +932,11 @@  pub(crate) mod job_control {
 pub(crate) mod mmu_control {
     use kernel::register;
 
+    use crate::driver::TyrRegisters;
+
     register! {
+        base: TyrRegisters;
+
         /// IRQ sources raw status.
         ///
         /// This register contains the raw unmasked interrupt sources for MMU status and exception
@@ -966,9 +990,10 @@  pub(crate) mod mmu_as_control {
             prelude::*,
             register, //
         };
-
         use pin_init::Zeroable;
 
+        use crate::driver::TyrRegisters;
+
         /// Maximum number of hardware address space slots.
         /// The actual number of slots available is usually lower.
         pub(crate) const MAX_AS: usize = 16;
@@ -977,6 +1002,8 @@  pub(crate) mod mmu_as_control {
         const STRIDE: usize = 0x40;
 
         register! {
+            base: TyrRegisters;
+
             /// Translation table base address. A 64-bit pointer.
             ///
             /// This field contains the address of the top level of a translation table structure.
@@ -1104,6 +1131,8 @@  fn from(val: MemoryType) -> Self {
         }
 
         register! {
+            base: TyrRegisters;
+
             /// Stage 1 memory attributes (8-bit bitfield).
             ///
             /// This is not an actual register, but a bitfield definition used by the MEMATTR
@@ -1137,6 +1166,8 @@  fn from(val: MMU_MEMATTR_STAGE1) -> Self {
         }
 
         register! {
+            base: TyrRegisters;
+
             /// Memory attributes.
             ///
             /// Each address space can configure up to 8 different memory attribute profiles.
@@ -1292,6 +1323,8 @@  pub(crate) fn from_mair(mair: u64) -> Self {
         }
 
         register! {
+            base: TyrRegisters;
+
             /// Lock region address for each address space.
             pub(crate) LOCKADDR(u64)[MAX_AS, stride = STRIDE] @ 0x2410 {
                 /// Lock region size.
@@ -1353,6 +1386,8 @@  fn from(cmd: MmuCommand) -> Self {
         }
 
         register! {
+            base: TyrRegisters;
+
             /// MMU command register for each address space. Write only.
             pub(crate) COMMAND(u32)[MAX_AS, stride = STRIDE] @ 0x2418 {
                 7:0     command ?=> MmuCommand;
@@ -1480,6 +1515,8 @@  fn from(access: MmuAccessType) -> Self {
         }
 
         register! {
+            base: TyrRegisters;
+
             /// Fault status register for each address space. Read only.
             pub(crate) FAULTSTATUS(u32)[MAX_AS, stride = STRIDE] @ 0x241c {
                 /// Exception type.
@@ -1705,6 +1742,8 @@  fn from(sh: PtwShareability) -> Self {
         }
 
         register! {
+            base: TyrRegisters;
+
             /// Translation configuration and control.
             pub(crate) TRANSCFG(u64)[MAX_AS, stride = STRIDE] @ 0x2430 {
                 /// Address space mode.
@@ -1760,6 +1799,8 @@  fn from(sh: PtwShareability) -> Self {
 pub(crate) mod doorbell_block {
     use kernel::register;
 
+    use crate::driver::TyrRegisters;
+
     /// Number of doorbells available.
     pub(crate) const NUM_DOORBELLS: usize = 64;
 
@@ -1770,6 +1811,8 @@  pub(crate) mod doorbell_block {
     const STRIDE: usize = 0x10000;
 
     register! {
+        base: TyrRegisters;
+
         /// Doorbell request register. Write-only.
         pub(crate) DOORBELL(u32)[NUM_DOORBELLS, stride = STRIDE] @ 0x80000 {
             /// Doorbell set. Writing 1 triggers the doorbell.