Skip to content

Commit 4da879a

Browse files
nbdd0121Danilo Krummrich
nbdd0121
authored and
Danilo Krummrich
committed
rust: dma: use pointer projection infra for dma_{read,write} macro
Current `dma_read!`, `dma_write!` macros also use a custom `addr_of!()`-based implementation for projecting pointers, which has soundness issue as it relies on absence of `Deref` implementation on types. It also has a soundness issue where it does not protect against unaligned fields (when `#[repr(packed)]` is used) so it can generate misaligned accesses. This commit migrates them to use the general pointer projection infrastructure, which handles these cases correctly. As part of migration, the macro is updated to have an improved surface syntax. The current macro have dma_read!(a.b.c[d].e.f) to mean `a.b.c` is a DMA coherent allocation and it should project into it with `[d].e.f` and do a read, which is confusing as it makes the indexing operator integral to the macro (so it will break if you have an array of `CoherentAllocation`, for example). This also is problematic as we would like to generalize `CoherentAllocation` from just slices to arbitrary types. Make the macro expects `dma_read!(path.to.dma, .path.inside.dma)` as the canonical syntax. The index operator is no longer special and is just one type of projection (in additional to field projection). Similarly, make `dma_write!(path.to.dma, .path.inside.dma, value)` become the canonical syntax for writing. Another issue of the current macro is that it is always fallible. This makes sense with existing design of `CoherentAllocation`, but once we support fixed size arrays with `CoherentAllocation`, it is desirable to have the ability to perform infallible indexing as well, e.g. doing a `[0]` index of `[Foo; 2]` is okay and can be checked at build-time, so forcing falliblity is non-ideal. To capture this, the macro is changed to use `[idx]` as infallible projection and `[idx]?` as fallible index projection (those syntax are part of the general projection infra). A benefit of this is that while individual indexing operation may fail, the overall read/write operation is not fallible. Fixes: ad2907b ("rust: add dma coherent allocator abstraction") Reviewed-by: Benno Lossin <lossin@kernel.org> Signed-off-by: Gary Guo <gary@garyguo.net> Link: https://patch.msgid.link/20260302164239.284084-4-gary@kernel.org [ Capitalize safety comments; slightly improve wording in doc-comments. - Danilo ] Signed-off-by: Danilo Krummrich <dakr@kernel.org>
1 parent f41941a commit 4da879a

5 files changed

Lines changed: 81 additions & 89 deletions

File tree

‎drivers/gpu/nova-core/gsp.rs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -143,14 +143,14 @@ impl Gsp {
143143
// _kgspInitLibosLoggingStructures (allocates memory for buffers)
144144
// kgspSetupLibosInitArgs_IMPL (creates pLibosInitArgs[] array)
145145
dma_write!(
146-
libos[0] = LibosMemoryRegionInitArgument::new("LOGINIT", &loginit.0)
147-
)?;
146+
libos, [0]?, LibosMemoryRegionInitArgument::new("LOGINIT", &loginit.0)
147+
);
148148
dma_write!(
149-
libos[1] = LibosMemoryRegionInitArgument::new("LOGINTR", &logintr.0)
150-
)?;
151-
dma_write!(libos[2] = LibosMemoryRegionInitArgument::new("LOGRM", &logrm.0))?;
152-
dma_write!(rmargs[0].inner = fw::GspArgumentsCached::new(cmdq))?;
153-
dma_write!(libos[3] = LibosMemoryRegionInitArgument::new("RMARGS", rmargs))?;
149+
libos, [1]?, LibosMemoryRegionInitArgument::new("LOGINTR", &logintr.0)
150+
);
151+
dma_write!(libos, [2]?, LibosMemoryRegionInitArgument::new("LOGRM", &logrm.0));
152+
dma_write!(rmargs, [0]?.inner, fw::GspArgumentsCached::new(cmdq));
153+
dma_write!(libos, [3]?, LibosMemoryRegionInitArgument::new("RMARGS", rmargs));
154154
},
155155
}))
156156
})

‎drivers/gpu/nova-core/gsp/boot.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ impl super::Gsp {
157157

158158
let wpr_meta =
159159
CoherentAllocation::<GspFwWprMeta>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?;
160-
dma_write!(wpr_meta[0] = GspFwWprMeta::new(&gsp_fw, &fb_layout))?;
160+
dma_write!(wpr_meta, [0]?, GspFwWprMeta::new(&gsp_fw, &fb_layout));
161161

162162
self.cmdq
163163
.send_command(bar, commands::SetSystemInfo::new(pdev))?;

‎drivers/gpu/nova-core/gsp/cmdq.rs‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -201,9 +201,13 @@ impl DmaGspMem {
201201

202202
let gsp_mem =
203203
CoherentAllocation::<GspMem>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?;
204-
dma_write!(gsp_mem[0].ptes = PteArray::new(gsp_mem.dma_handle())?)?;
205-
dma_write!(gsp_mem[0].cpuq.tx = MsgqTxHeader::new(MSGQ_SIZE, RX_HDR_OFF, MSGQ_NUM_PAGES))?;
206-
dma_write!(gsp_mem[0].cpuq.rx = MsgqRxHeader::new())?;
204+
dma_write!(gsp_mem, [0]?.ptes, PteArray::new(gsp_mem.dma_handle())?);
205+
dma_write!(
206+
gsp_mem,
207+
[0]?.cpuq.tx,
208+
MsgqTxHeader::new(MSGQ_SIZE, RX_HDR_OFF, MSGQ_NUM_PAGES)
209+
);
210+
dma_write!(gsp_mem, [0]?.cpuq.rx, MsgqRxHeader::new());
207211

208212
Ok(Self(gsp_mem))
209213
}

‎rust/kernel/dma.rs‎

Lines changed: 50 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,19 @@ impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
461461
self.count * core::mem::size_of::<T>()
462462
}
463463

464+
/// Returns the raw pointer to the allocated region in the CPU's virtual address space.
465+
#[inline]
466+
pub fn as_ptr(&self) -> *const [T] {
467+
core::ptr::slice_from_raw_parts(self.cpu_addr.as_ptr(), self.count)
468+
}
469+
470+
/// Returns the raw pointer to the allocated region in the CPU's virtual address space as
471+
/// a mutable pointer.
472+
#[inline]
473+
pub fn as_mut_ptr(&self) -> *mut [T] {
474+
core::ptr::slice_from_raw_parts_mut(self.cpu_addr.as_ptr(), self.count)
475+
}
476+
464477
/// Returns the base address to the allocated region in the CPU's virtual address space.
465478
pub fn start_ptr(&self) -> *const T {
466479
self.cpu_addr.as_ptr()
@@ -581,23 +594,6 @@ impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
581594
Ok(())
582595
}
583596

584-
/// Returns a pointer to an element from the region with bounds checking. `offset` is in
585-
/// units of `T`, not the number of bytes.
586-
///
587-
/// Public but hidden since it should only be used from [`dma_read`] and [`dma_write`] macros.
588-
#[doc(hidden)]
589-
pub fn item_from_index(&self, offset: usize) -> Result<*mut T> {
590-
if offset >= self.count {
591-
return Err(EINVAL);
592-
}
593-
// SAFETY:
594-
// - The pointer is valid due to type invariant on `CoherentAllocation`
595-
// and we've just checked that the range and index is within bounds.
596-
// - `offset` can't overflow since it is smaller than `self.count` and we've checked
597-
// that `self.count` won't overflow early in the constructor.
598-
Ok(unsafe { self.cpu_addr.as_ptr().add(offset) })
599-
}
600-
601597
/// Reads the value of `field` and ensures that its type is [`FromBytes`].
602598
///
603599
/// # Safety
@@ -670,6 +666,9 @@ unsafe impl<T: AsBytes + FromBytes + Send> Send for CoherentAllocation<T> {}
670666

671667
/// Reads a field of an item from an allocated region of structs.
672668
///
669+
/// The syntax is of the form `kernel::dma_read!(dma, proj)` where `dma` is an expression evaluating
670+
/// to a [`CoherentAllocation`] and `proj` is a [projection specification](kernel::ptr::project!).
671+
///
673672
/// # Examples
674673
///
675674
/// ```
@@ -684,36 +683,29 @@ unsafe impl<T: AsBytes + FromBytes + Send> Send for CoherentAllocation<T> {}
684683
/// unsafe impl kernel::transmute::AsBytes for MyStruct{};
685684
///
686685
/// # fn test(alloc: &kernel::dma::CoherentAllocation<MyStruct>) -> Result {
687-
/// let whole = kernel::dma_read!(alloc[2]);
688-
/// let field = kernel::dma_read!(alloc[1].field);
686+
/// let whole = kernel::dma_read!(alloc, [2]?);
687+
/// let field = kernel::dma_read!(alloc, [1]?.field);
689688
/// # Ok::<(), Error>(()) }
690689
/// ```
691690
#[macro_export]
692691
macro_rules! dma_read {
693-
($dma:expr, $idx: expr, $($field:tt)*) => {{
694-
(|| -> ::core::result::Result<_, $crate::error::Error> {
695-
let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
696-
// SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
697-
// dereferenced. The compiler also further validates the expression on whether `field`
698-
// is a member of `item` when expanded by the macro.
699-
unsafe {
700-
let ptr_field = ::core::ptr::addr_of!((*item) $($field)*);
701-
::core::result::Result::Ok(
702-
$crate::dma::CoherentAllocation::field_read(&$dma, ptr_field)
703-
)
704-
}
705-
})()
692+
($dma:expr, $($proj:tt)*) => {{
693+
let dma = &$dma;
694+
let ptr = $crate::ptr::project!(
695+
$crate::dma::CoherentAllocation::as_ptr(dma), $($proj)*
696+
);
697+
// SAFETY: The pointer created by the projection is within the DMA region.
698+
unsafe { $crate::dma::CoherentAllocation::field_read(dma, ptr) }
706699
}};
707-
($dma:ident [ $idx:expr ] $($field:tt)* ) => {
708-
$crate::dma_read!($dma, $idx, $($field)*)
709-
};
710-
($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {
711-
$crate::dma_read!($($dma).*, $idx, $($field)*)
712-
};
713700
}
714701

715702
/// Writes to a field of an item from an allocated region of structs.
716703
///
704+
/// The syntax is of the form `kernel::dma_write!(dma, proj, val)` where `dma` is an expression
705+
/// evaluating to a [`CoherentAllocation`], `proj` is a
706+
/// [projection specification](kernel::ptr::project!), and `val` is the value to be written to the
707+
/// projected location.
708+
///
717709
/// # Examples
718710
///
719711
/// ```
@@ -728,37 +720,31 @@ macro_rules! dma_read {
728720
/// unsafe impl kernel::transmute::AsBytes for MyStruct{};
729721
///
730722
/// # fn test(alloc: &kernel::dma::CoherentAllocation<MyStruct>) -> Result {
731-
/// kernel::dma_write!(alloc[2].member = 0xf);
732-
/// kernel::dma_write!(alloc[1] = MyStruct { member: 0xf });
723+
/// kernel::dma_write!(alloc, [2]?.member, 0xf);
724+
/// kernel::dma_write!(alloc, [1]?, MyStruct { member: 0xf });
733725
/// # Ok::<(), Error>(()) }
734726
/// ```
735727
#[macro_export]
736728
macro_rules! dma_write {
737-
($dma:ident [ $idx:expr ] $($field:tt)*) => {{
738-
$crate::dma_write!($dma, $idx, $($field)*)
739-
}};
740-
($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {{
741-
$crate::dma_write!($($dma).*, $idx, $($field)*)
729+
(@parse [$dma:expr] [$($proj:tt)*] [, $val:expr]) => {{
730+
let dma = &$dma;
731+
let ptr = $crate::ptr::project!(
732+
mut $crate::dma::CoherentAllocation::as_mut_ptr(dma), $($proj)*
733+
);
734+
let val = $val;
735+
// SAFETY: The pointer created by the projection is within the DMA region.
736+
unsafe { $crate::dma::CoherentAllocation::field_write(dma, ptr, val) }
742737
}};
743-
($dma:expr, $idx: expr, = $val:expr) => {
744-
(|| -> ::core::result::Result<_, $crate::error::Error> {
745-
let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
746-
// SAFETY: `item_from_index` ensures that `item` is always a valid item.
747-
unsafe { $crate::dma::CoherentAllocation::field_write(&$dma, item, $val) }
748-
::core::result::Result::Ok(())
749-
})()
738+
(@parse [$dma:expr] [$($proj:tt)*] [.$field:tt $($rest:tt)*]) => {
739+
$crate::dma_write!(@parse [$dma] [$($proj)* .$field] [$($rest)*])
740+
};
741+
(@parse [$dma:expr] [$($proj:tt)*] [[$index:expr]? $($rest:tt)*]) => {
742+
$crate::dma_write!(@parse [$dma] [$($proj)* [$index]?] [$($rest)*])
743+
};
744+
(@parse [$dma:expr] [$($proj:tt)*] [[$index:expr] $($rest:tt)*]) => {
745+
$crate::dma_write!(@parse [$dma] [$($proj)* [$index]] [$($rest)*])
750746
};
751-
($dma:expr, $idx: expr, $(.$field:ident)* = $val:expr) => {
752-
(|| -> ::core::result::Result<_, $crate::error::Error> {
753-
let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
754-
// SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
755-
// dereferenced. The compiler also further validates the expression on whether `field`
756-
// is a member of `item` when expanded by the macro.
757-
unsafe {
758-
let ptr_field = ::core::ptr::addr_of_mut!((*item) $(.$field)*);
759-
$crate::dma::CoherentAllocation::field_write(&$dma, ptr_field, $val)
760-
}
761-
::core::result::Result::Ok(())
762-
})()
747+
($dma:expr, $($rest:tt)*) => {
748+
$crate::dma_write!(@parse [$dma] [] [$($rest)*])
763749
};
764750
}

‎samples/rust/rust_dma.rs‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ impl pci::Driver for DmaSampleDriver {
6868
CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
6969

7070
for (i, value) in TEST_VALUES.into_iter().enumerate() {
71-
kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
71+
kernel::dma_write!(ca, [i]?, MyStruct::new(value.0, value.1));
7272
}
7373

7474
let size = 4 * page::PAGE_SIZE;
@@ -85,24 +85,26 @@ impl pci::Driver for DmaSampleDriver {
8585
}
8686
}
8787

88+
impl DmaSampleDriver {
89+
fn check_dma(&self) -> Result {
90+
for (i, value) in TEST_VALUES.into_iter().enumerate() {
91+
let val0 = kernel::dma_read!(self.ca, [i]?.h);
92+
let val1 = kernel::dma_read!(self.ca, [i]?.b);
93+
94+
assert_eq!(val0, value.0);
95+
assert_eq!(val1, value.1);
96+
}
97+
98+
Ok(())
99+
}
100+
}
101+
88102
#[pinned_drop]
89103
impl PinnedDrop for DmaSampleDriver {
90104
fn drop(self: Pin<&mut Self>) {
91105
dev_info!(self.pdev, "Unload DMA test driver.\n");
92106

93-
for (i, value) in TEST_VALUES.into_iter().enumerate() {
94-
let val0 = kernel::dma_read!(self.ca[i].h);
95-
let val1 = kernel::dma_read!(self.ca[i].b);
96-
assert!(val0.is_ok());
97-
assert!(val1.is_ok());
98-
99-
if let Ok(val0) = val0 {
100-
assert_eq!(val0, value.0);
101-
}
102-
if let Ok(val1) = val1 {
103-
assert_eq!(val1, value.1);
104-
}
105-
}
107+
assert!(self.check_dma().is_ok());
106108

107109
for (i, entry) in self.sgt.iter().enumerate() {
108110
dev_info!(

0 commit comments

Comments
 (0)