rust: dma: remove DMA_ATTR_NO_KERNEL_MAPPING from public attrs
Subsystems (4)
Danilo Krummrich
maintainerPATCH 1/2 rust: dma: remove DMA_ATTR_NO_KERNEL_MAPPING from public attrs
When DMA_ATTR_NO_KERNEL_MAPPING is passed to dma_alloc_attrs(), the returned CPU address is not a pointer to the allocated memory but an opaque handle (e.g. struct page *).
Coherent<T> (or CoherentAllocation<T> respectively) stores this value as NonNull<T> and exposes methods that dereference it and even modify its contents.
Remove the flag from the public attrs module such that drivers cannot pass it to Coherent<T> (or CoherentAllocation<T> respectively) in the first place.
Instead DMA_ATTR_NO_KERNEL_MAPPING can be supported with an additional opaque type (e.g. CoherentHandle) which does not provide access to the allocated memory.
--- rust/kernel/dma.rs | 3 --- 1 file changed, 3 deletions(-)
rust/kernel/dma.rs +0 -3
@@ -264,9 +264,6 @@ pub mod attrs {/// Specifies that writes to the mapping may be buffered to improve performance. pub const DMA_ATTR_WRITE_COMBINE :Attrs =Attrs (bindings::DMA_ATTR_WRITE_COMBINE ); -/// Lets the platform to avoid creating a kernel virtual mapping for the allocated buffer. -pub const DMA_ATTR_NO_KERNEL_MAPPING :Attrs =Attrs (bindings::DMA_ATTR_NO_KERNEL_MAPPING ); -/// Allows platform code to skip synchronization of the CPU cache for the given buffer assuming /// that it has been already transferred to 'device' domain. pub const DMA_ATTR_SKIP_CPU_SYNC :Attrs =Attrs (bindings::DMA_ATTR_SKIP_CPU_SYNC );
Signature · 1 line
2.53.0
Danilo Krummrich
maintainerPATCH 2/2 rust: dma: add CoherentHandle for DMA allocations without kernel mapping
Add CoherentHandle, an opaque DMA allocation type for buffers that are only ever accessed by hardware. Unlike Coherent<T>, it does not provide CPU access to the allocated memory.
CoherentHandle implicitly sets DMA_ATTR_NO_KERNEL_MAPPING and stores the value returned by dma_alloc_attrs() as an opaque handle (NonNull<c_void>) rather than a typed pointer, since with this flag the C API returns an opaque cookie (e.g. struct page *), not a CPU pointer to the allocated memory.
Only the DMA bus address is exposed to drivers; the opaque handle is used solely to free the allocation on drop.
This commit is for reference only; there is currently no in-tree user.
--- rust/kernel/dma.rs | 119 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+)
rust/kernel/dma.rs +119 -0
@@ -1011,6 +1011,125 @@ fn drop(&mut self) {// can be sent to another thread. unsafe impl <T :KnownSize +Send + ?Sized >Send for Coherent <T > {} +/// An opaque DMA allocation without a kernel virtual mapping. +/// +/// Unlike [`Coherent`], a `CoherentHandle` does not provide CPU access to the allocated memory. +/// The allocation is always performed with `DMA_ATTR_NO_KERNEL_MAPPING`, meaning no kernel +/// virtual mapping is created for the buffer. The value returned by the C API as the CPU +/// address is an opaque handle used only to free the allocation. +/// +/// This is useful for buffers that are only ever accessed by hardware. +/// +/// # Invariants +/// +/// - `cpu_handle` holds the opaque handle returned by `dma_alloc_attrs` with +/// `DMA_ATTR_NO_KERNEL_MAPPING` set, and is only valid for passing back to `dma_free_attrs`. +/// - `dma_handle` is the corresponding bus address for device DMA. +/// - `size` is the allocation size in bytes as passed to `dma_alloc_attrs`. +/// - `dma_attrs` contains the attributes used for the allocation, always including +/// `DMA_ATTR_NO_KERNEL_MAPPING`. +pub struct CoherentHandle { + dev:ARef <device::Device >, + dma_handle:DmaAddress , + cpu_handle:NonNull <c_void >, + size:usize , + dma_attrs:Attrs , +} + +impl CoherentHandle { +/// Allocates `size` bytes of coherent DMA memory without creating a kernel virtual mapping. +/// +/// Additional DMA attributes may be passed via `dma_attrs`; `DMA_ATTR_NO_KERNEL_MAPPING` is +/// always set implicitly. +/// +/// Returns `EINVAL` if `size` is zero, `ENOMEM` if the allocation fails. +pub fn alloc_with_attrs ( + dev: &device::Device <Bound >, + size:usize , + gfp_flags: kernel::alloc::Flags , + dma_attrs:Attrs , + ) ->Result <Self > { +if size ==0 { +return Err (EINVAL ); + } + +let dma_attrs = dma_attrs |Attrs (bindings::DMA_ATTR_NO_KERNEL_MAPPING ); +let mut dma_handle =0 ; +// SAFETY: `dev.as_raw()` is valid by the type invariant on `device::Device`. +let cpu_handle =unsafe { + bindings::dma_alloc_attrs ( + dev.as_raw (), + size, + &mut dma_handle, + gfp_flags.as_raw (), + dma_attrs.as_raw (), + ) + }; + +let cpu_handle =NonNull ::new (cpu_handle).ok_or (ENOMEM )?; + +// INVARIANT: `cpu_handle` is the opaque handle from a successful `dma_alloc_attrs` call +// with `DMA_ATTR_NO_KERNEL_MAPPING`, `dma_handle` is the corresponding DMA address, +// and we hold a refcounted reference to the device. +Ok (Self { + dev: dev.into (), + dma_handle, + cpu_handle, + size, + dma_attrs, + }) + } + +/// Allocates `size` bytes of coherent DMA memory without creating a kernel virtual mapping. +#[inline] +pub fn alloc ( + dev: &device::Device <Bound >, + size:usize , + gfp_flags: kernel::alloc::Flags , + ) ->Result <Self > { +Self ::alloc_with_attrs (dev, size, gfp_flags,Attrs (0 )) + } + +/// Returns the DMA handle for this allocation. +/// +/// This address can be programmed into device hardware for DMA access. +#[inline] +pub fn dma_handle (&self) ->DmaAddress { + self.dma_handle + } + +/// Returns the size in bytes of this allocation. +#[inline] +pub fn size (&self) ->usize { + self.size + } +} + +impl Drop for CoherentHandle { +fn drop (&mut self) { +// SAFETY: All values are valid by the type invariants on `CoherentHandle`. +// `cpu_handle` is the opaque handle from `dma_alloc_attrs` and is passed back unchanged. +unsafe { + bindings::dma_free_attrs ( + self.dev.as_raw (), + self.size, + self.cpu_handle.as_ptr (), + self.dma_handle, + self.dma_attrs.as_raw (), + ) + } + } +} + +// SAFETY: `CoherentHandle` only holds a device reference, a DMA handle, an opaque CPU handle, +// and a size. None of these are tied to a specific thread. +unsafe impl Send for CoherentHandle {} + +// SAFETY: `CoherentHandle` provides no CPU access to the underlying allocation. The only +// operations on `&CoherentHandle` are reading the DMA handle and size, both of which are +// plain `Copy` values. +unsafe impl Sync for CoherentHandle {} +/// Reads a field of an item from an allocated region of structs. /// /// The syntax is of the form `kernel::dma_read!(dma, proj)` where `dma` is an expression evaluating
Signature · 1 line
2.53.0
Re: [PATCH 2/2] rust: dma: add CoherentHandle for DMA allocations without kernel mapping
Quoted 10 lines · Danilo Krummrich
Add CoherentHandle, an opaque DMA allocation type for buffers that are only ever accessed by hardware. Unlike Coherent<T>, it does not provide CPU access to the allocated memory.
CoherentHandle implicitly sets
DMA_ATTR_NO_KERNEL_MAPPINGand stores the value returned bydma_alloc_attrs()as an opaque handle (NonNull<c_void>) rather than a typed pointer, since with this flag the C API returns an opaque cookie (e.g. struct page *), not a CPU pointer to the allocated memory.
Quoted 4 lines
Only the DMA bus address is exposed to drivers; the opaque handle is used solely to free the allocation on drop.
This commit is for reference only; there is currently no in-tree user.
nova-core's sysmem flush memory page would be a prime candidate to use this, I'll add this patch as a dependency to [1] and use it.
(one question below)
[1] https://lore.kernel.org/all/[email protected]/
Quoted 21 lines
Signed-off-by Danilo Krummrich <[email protected]>--- rust/kernel/dma.rs | 119 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+)rust/kernel/dma.rs +7
-0@@ -1011,6 +1011,125 @@ fn drop(&mut self) {// can be sent to another thread. unsafe impl <T :KnownSize +Send + ?Sized >Send for Coherent <T > {} +/// An opaque DMA allocation without a kernel virtual mapping. +/// +/// Unlike [`Coherent`], a `CoherentHandle` does not provide CPU access to the allocated memory. +/// The allocation is always performed with `DMA_ATTR_NO_KERNEL_MAPPING`, meaning no kernel +/// virtual mapping is created for the buffer. The value returned by the C API as the CPU +/// address is an opaque handle used only to free the allocation. +///
rust/kernel/dma.rs fn drop(&mut self) { · line 1030
Quoted 10 lines
rust/kernel/dma.rs +10
-0+/// This is useful for buffers that are only ever accessed by hardware. +/// +/// # Invariants +/// +/// - `cpu_handle` holds the opaque handle returned by `dma_alloc_attrs` with +/// `DMA_ATTR_NO_KERNEL_MAPPING` set, and is only valid for passing back to `dma_free_attrs`. +/// - `dma_handle` is the corresponding bus address for device DMA. +/// - `size` is the allocation size in bytes as passed to `dma_alloc_attrs`. +/// - `dma_attrs` contains the attributes used for the allocation, always including +/// `DMA_ATTR_NO_KERNEL_MAPPING`.
Quick question for my erudition: I understand all the invariants are referred to by drop, but some of them (size notably) really read more like doccomments. Do we need to be that exhaustive every time we call a C API?
Danilo Krummrich
maintainerRe: [PATCH 2/2] rust: dma: add CoherentHandle for DMA allocations without kernel mapping
Quoted 11 lines · Alexandre Courbot
Quoted 10 lines · Danilo Krummrich
Add CoherentHandle, an opaque DMA allocation type for buffers that are only ever accessed by hardware. Unlike Coherent<T>, it does not provide CPU access to the allocated memory.
CoherentHandle implicitly sets
DMA_ATTR_NO_KERNEL_MAPPINGand stores the value returned bydma_alloc_attrs()as an opaque handle (NonNull<c_void>) rather than a typed pointer, since with this flag the C API returns an opaque cookie (e.g. struct page *), not a CPU pointer to the allocated memory.
Quoted 7 lines
Quoted 4 lines
Only the DMA bus address is exposed to drivers; the opaque handle is used solely to free the allocation on drop.
This commit is for reference only; there is currently no in-tree user.
nova-core's sysmem flush memory page would be a prime candidate to use this, I'll add this patch as a dependency to [1] and use it.
Sure, please do.
Quoted 34 lines
Reviewed-by Alexandre Courbot <[email protected]>(one question below)
[1] https://lore.kernel.org/all/[email protected]/
Quoted 28 lines
Signed-off-by Danilo Krummrich <[email protected]>--- rust/kernel/dma.rs | 119 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+)rust/kernel/dma.rs +14
-0@@ -1011,6 +1011,125 @@ fn drop(&mut self) {// can be sent to another thread. unsafe impl <T :KnownSize +Send + ?Sized >Send for Coherent <T > {} +/// An opaque DMA allocation without a kernel virtual mapping. +/// +/// Unlike [`Coherent`], a `CoherentHandle` does not provide CPU access to the allocated memory. +/// The allocation is always performed with `DMA_ATTR_NO_KERNEL_MAPPING`, meaning no kernel +/// virtual mapping is created for the buffer. The value returned by the C API as the CPU +/// address is an opaque handle used only to free the allocation. +/// +/// This is useful for buffers that are only ever accessed by hardware. +/// +/// # Invariants +/// +/// - `cpu_handle` holds the opaque handle returned by `dma_alloc_attrs` with +/// `DMA_ATTR_NO_KERNEL_MAPPING` set, and is only valid for passing back to `dma_free_attrs`. +/// - `dma_handle` is the corresponding bus address for device DMA.
Quoted 8 lines
rust/kernel/dma.rs fn drop(&mut self) { · line 1030
Quoted 3 lines
rust/kernel/dma.rs +3
-0+/// - `size` is the allocation size in bytes as passed to `dma_alloc_attrs`. +/// - `dma_attrs` contains the attributes used for the allocation, always including +/// `DMA_ATTR_NO_KERNEL_MAPPING`. Quick question for my erudition: I understand all the invariants are referred to by
drop, but some of them (sizenotably) really read more like doccomments. Do we need to be that exhaustive every time we call a C API?
We have the same on dma::Coherent / dma::CoherentAllocation, as the destructor relies on all those to be invariant.
I don't think we have to do this for every C API call, but in this case the question is what the safety justification in the destructor would look like if we'd drop those invariants.
Gary Guo
reviewerRe: [PATCH 2/2] rust: dma: add CoherentHandle for DMA allocations without kernel mapping
In reply to Danilo KrummrichQuoted 10 lines · Danilo Krummrich
Add CoherentHandle, an opaque DMA allocation type for buffers that are only ever accessed by hardware. Unlike Coherent<T>, it does not provide CPU access to the allocated memory.
CoherentHandle implicitly sets
DMA_ATTR_NO_KERNEL_MAPPINGand stores the value returned bydma_alloc_attrs()as an opaque handle (NonNull<c_void>) rather than a typed pointer, since with this flag the C API returns an opaque cookie (e.g. struct page *), not a CPU pointer to the allocated memory.
Quoted 4 lines
Only the DMA bus address is exposed to drivers; the opaque handle is used solely to free the allocation on drop.
This commit is for reference only; there is currently no in-tree user.
Thinking about this a bit more, instead of creating new separate types, we can probably just have multiple flavour/kind of Coherent, and we default to the most common one.
Something like
pub struct Normal;
pub struct NoMapping;
struct Coherent<T: KnonwSize + ?Sized, Kind = Normal> {
...
}Where for the common functions, they're implemented on Coherent<T, Kind> while the ones that require CPU mapping have impl Coherent<T>.
The benefit is that there's no code duplication. Also, you could also implement Io but leave out all IoCapable impls, so you may do I/O projections on these and be able to get offseted DMA addresses without having to do manual computation.
Best,
Gary
Quoted 39 lines
Signed-off-by Danilo Krummrich <[email protected]>--- rust/kernel/dma.rs | 119 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+)rust/kernel/dma.rs +25
-0@@ -1011,6 +1011,125 @@ fn drop(&mut self) {// can be sent to another thread. unsafe impl <T :KnownSize +Send + ?Sized >Send for Coherent <T > {} +/// An opaque DMA allocation without a kernel virtual mapping. +/// +/// Unlike [`Coherent`], a `CoherentHandle` does not provide CPU access to the allocated memory. +/// The allocation is always performed with `DMA_ATTR_NO_KERNEL_MAPPING`, meaning no kernel +/// virtual mapping is created for the buffer. The value returned by the C API as the CPU +/// address is an opaque handle used only to free the allocation. +/// +/// This is useful for buffers that are only ever accessed by hardware. +/// +/// # Invariants +/// +/// - `cpu_handle` holds the opaque handle returned by `dma_alloc_attrs` with +/// `DMA_ATTR_NO_KERNEL_MAPPING` set, and is only valid for passing back to `dma_free_attrs`. +/// - `dma_handle` is the corresponding bus address for device DMA. +/// - `size` is the allocation size in bytes as passed to `dma_alloc_attrs`. +/// - `dma_attrs` contains the attributes used for the allocation, always including +/// `DMA_ATTR_NO_KERNEL_MAPPING`. +pub struct CoherentHandle { + dev:ARef <device::Device >, + dma_handle:DmaAddress , + cpu_handle:NonNull <c_void >, + size:usize , + dma_attrs:Attrs , +} +
Danilo Krummrich
maintainerRe: [PATCH 2/2] rust: dma: add CoherentHandle for DMA allocations without kernel mapping
Quoted 25 lines · Gary Guo
Quoted 10 lines · Danilo Krummrich
Add CoherentHandle, an opaque DMA allocation type for buffers that are only ever accessed by hardware. Unlike Coherent<T>, it does not provide CPU access to the allocated memory.
CoherentHandle implicitly sets
DMA_ATTR_NO_KERNEL_MAPPINGand stores the value returned bydma_alloc_attrs()as an opaque handle (NonNull<c_void>) rather than a typed pointer, since with this flag the C API returns an opaque cookie (e.g. struct page *), not a CPU pointer to the allocated memory.Quoted 4 lines
Only the DMA bus address is exposed to drivers; the opaque handle is used solely to free the allocation on drop.
This commit is for reference only; there is currently no in-tree user.
Thinking about this a bit more, instead of creating new separate types, we can probably just have multiple flavour/kind of
Coherent, and we default to the most common one.Something like
pub struct Normal;
pub struct NoMapping;
Quoted 8 lines
struct Coherent<T: KnonwSize + ?Sized, Kind = Normal> { ... }Where for the common functions, they're implemented on
Coherent<T, Kind>while the ones that require CPU mapping haveimpl Coherent<T>.The benefit is that there's no code duplication.
This was my first thought as well.
But I found it a bit odd that users would still have to define T, then I thought we could have a type alias, e.g.
type CoherentHandle = Coherent<[u8], NoMapping>;
In this case the constructor would be CoherentHandle::zeroed_slice(), which I find a bit odd as well.
Alternatively, we could have a const SIZE generic on CoherentHandle wrapping Coherent<[u8; SIZE], NoMapping>.
Then we'd get CoherentHandle::<SIZE>::zeroed() (which I find much better), but we would still have Coherent<T, NoMapping> publically available, which I still find a bit odd.
And then I thought, it's probably not worth the additional complexity and a separate CoherentHandle type is probably good enough.
Quoted 3 lines
Also, you could also implement
Iobut leave out allIoCapableimpls, so you may do I/O projections on these and be able to get offseted DMA addresses without having to do manual computation.
This is a reason for having Coherent<T, NoMapping>, but I'm not sure it is an improvement in the first place.
If the memory is not mapped on the CPU side it is probably just an opaque buffer. And I'm not sure there is a huge advantage in defining a structure representing any relevant offsets and then do an I/O projection compared to just having constants or an enum defining those offsets.
I also think there's not a huge difference in terms of robustness. In the former case, if you mess up the offsets, the buffer size will be wrong (unless multiple offsets are wrong and compensate each other) and the device will not properly work on runtime.
In the latter case, you either also pass a wrong offset to the device, or you get a compile-time error if the offset is out of bounds (which is probably even better).
Also, consider the case where the offset into the opaque buffer is dynamic, then a type and I/O projections won't help either.
Gary Guo
reviewerRe: [PATCH 2/2] rust: dma: add CoherentHandle for DMA allocations without kernel mapping
Quoted 45 lines · Danilo Krummrich
Quoted 25 lines · Gary Guo
Quoted 10 lines · Danilo Krummrich
Add CoherentHandle, an opaque DMA allocation type for buffers that are only ever accessed by hardware. Unlike Coherent<T>, it does not provide CPU access to the allocated memory.
CoherentHandle implicitly sets
DMA_ATTR_NO_KERNEL_MAPPINGand stores the value returned bydma_alloc_attrs()as an opaque handle (NonNull<c_void>) rather than a typed pointer, since with this flag the C API returns an opaque cookie (e.g. struct page *), not a CPU pointer to the allocated memory.Quoted 4 lines
Only the DMA bus address is exposed to drivers; the opaque handle is used solely to free the allocation on drop.
This commit is for reference only; there is currently no in-tree user.
Thinking about this a bit more, instead of creating new separate types, we can probably just have multiple flavour/kind of
Coherent, and we default to the most common one.Something like
pub struct Normal;
pub struct NoMapping;Quoted 8 lines
struct Coherent<T: KnonwSize + ?Sized, Kind = Normal> { ... }Where for the common functions, they're implemented on
Coherent<T, Kind>while the ones that require CPU mapping haveimpl Coherent<T>.The benefit is that there's no code duplication.
This was my first thought as well.
But I found it a bit odd that users would still have to define T, then I thought we could have a type alias, e.g.
type CoherentHandle = Coherent<[u8], NoMapping>;In this case the constructor would be
CoherentHandle::zeroed_slice(), which I find a bit odd as well.
Quoted 6 lines
Alternatively, we could have a const SIZE generic on CoherentHandle wrapping Coherent<[u8; SIZE], NoMapping>.
Then we'd get CoherentHandle::<SIZE>::zeroed() (which I find much better), but we would still have Coherent<T, NoMapping> publically available, which I still find a bit odd.
This only gives you fixed size, though. If you want the same type that supports both a fixed size and dynamic size, then a generic that is either array/slice is the way to go.
Quoted 21 lines
And then I thought, it's probably not worth the additional complexity and a separate CoherentHandle type is probably good enough.
Quoted 3 lines
Also, you could also implement
Iobut leave out allIoCapableimpls, so you may do I/O projections on these and be able to get offseted DMA addresses without having to do manual computation.This is a reason for having Coherent<T, NoMapping>, but I'm not sure it is an improvement in the first place.
If the memory is not mapped on the CPU side it is probably just an opaque buffer. And I'm not sure there is a huge advantage in defining a structure representing any relevant offsets and then do an I/O projection compared to just having constants or an enum defining those offsets.
I also think there's not a huge difference in terms of robustness. In the former case, if you mess up the offsets, the buffer size will be wrong (unless multiple offsets are wrong and compensate each other) and the device will not properly work on runtime.
Quoted 6 lines
In the latter case, you either also pass a wrong offset to the device, or you get a compile-time error if the offset is out of bounds (which is probably even better).
Also, consider the case where the offset into the opaque buffer is dynamic, then a type and I/O projections won't help either.
You still can use io_project!(handle, [start..end]?) to do the bounds checking. But as you said, the benefit might be minimal.
Best,
Gary
Danilo Krummrich
maintainerRe: [PATCH 2/2] rust: dma: add CoherentHandle for DMA allocations without kernel mapping
Quoted 3 lines · Gary Guo
This only gives you fixed size, though. If you want the same type that supports both a fixed size and dynamic size, then a generic that is either array/slice is the way to go.
[...]
Quoted 2 lines
You still can use
io_project!(handle, [start..end]?)to do the bounds checking. But as you said, the benefit might be minimal.
I think we shouldn't overengineer this for now, and wait for actual use-cases that require any of that.
The only thing we need right now in practice is an API that takes the size and returns a handle to the buffer of this size exposing the DMA address that is subsequently passed to the device without further modification.
So, if we want to re-use dma::Coherent<T>, we can just do this
pub struct CoherentHandle(Coherent<[u8]>);
impl CoherentHandle {
pub fn alloc_with_attrs(
dev: &device::Device<Bound>,
size: usize,
gfp_flags: kernel::alloc::Flags,
dma_attrs: Attrs,
) -> Result<Self> {
let dma_attrs = dma_attrs | Attrs(bindings::DMA_ATTR_NO_KERNEL_MAPPING);
Coherent::<u8>::alloc_slice_with_attrs(dev, size, gfp_flags, dma_attrs).map(Self)
}
#[inline]
pub fn alloc(
dev: &device::Device<Bound>,
size: usize,
gfp_flags: kernel::alloc::Flags,
) -> Result<Self> {
Self::alloc_with_attrs(dev, size, gfp_flags, Attrs(0))
}
#[inline]
pub fn dma_handle(&self) -> DmaAddress {
self.0.dma_handle()
}
#[inline]
pub fn size(&self) -> usize {
self.0.size()
}
}and add a guarantee that Coherent::alloc_with_attrs() and Coherent::drop() never touch the cpu_ptr.
Gary Guo
reviewerRe: [PATCH 2/2] rust: dma: add CoherentHandle for DMA allocations without kernel mapping
Quoted 52 lines · Danilo Krummrich
Quoted 3 lines · Gary Guo
This only gives you fixed size, though. If you want the same type that supports both a fixed size and dynamic size, then a generic that is either array/slice is the way to go.
[...]
Quoted 2 lines
You still can use
io_project!(handle, [start..end]?)to do the bounds checking. But as you said, the benefit might be minimal.I think we shouldn't overengineer this for now, and wait for actual use-cases that require any of that.
The only thing we need right now in practice is an API that takes the size and returns a handle to the buffer of this size exposing the DMA address that is subsequently passed to the device without further modification.
So, if we want to re-use dma::Coherent<T>, we can just do this
pub struct CoherentHandle(Coherent<[u8]>); impl CoherentHandle { pub fn alloc_with_attrs( dev: &device::Device<Bound>, size: usize, gfp_flags: kernel::alloc::Flags, dma_attrs: Attrs, ) -> Result<Self> { let dma_attrs = dma_attrs | Attrs(bindings::DMA_ATTR_NO_KERNEL_MAPPING); Coherent::<u8>::alloc_slice_with_attrs(dev, size, gfp_flags, dma_attrs).map(Self) } #[inline] pub fn alloc( dev: &device::Device<Bound>, size: usize, gfp_flags: kernel::alloc::Flags, ) -> Result<Self> { Self::alloc_with_attrs(dev, size, gfp_flags, Attrs(0)) } #[inline] pub fn dma_handle(&self) -> DmaAddress { self.0.dma_handle() } #[inline] pub fn size(&self) -> usize { self.0.size() } }
Quoted 2 lines
and add a guarantee that
Coherent::alloc_with_attrs()andCoherent::drop()never touch thecpu_ptr.
Hmm, it sounds like we *could* actually have Coherent support Drop types and have drop() run the destructor for it. We don't need to for FromBytes + AsBytes, but I think we'd better not have that as a guarantee.
So the original approach that you have is better.
Best,
Gary
Quoted 12 lines · Danilo Krummrich
When
DMA_ATTR_NO_KERNEL_MAPPINGis passed todma_alloc_attrs(), the returned CPU address is not a pointer to the allocated memory but an opaque handle (e.g. struct page *).Coherent<T> (or CoherentAllocation<T> respectively) stores this value as NonNull<T> and exposes methods that dereference it and even modify its contents.
Remove the flag from the public attrs module such that drivers cannot pass it to Coherent<T> (or CoherentAllocation<T> respectively) in the first place.
Quoted 7 lines
Instead
DMA_ATTR_NO_KERNEL_MAPPINGcan be supported with an additional opaque type (e.g. CoherentHandle) which does not provide access to the allocated memory.Fixes ad2907b4e308 ("rust: add dma coherent allocator abstraction")Signed-off-by Danilo Krummrich <[email protected]>
Quoted 3 lines
--- rust/kernel/dma.rs | 3 --- 1 file changed, 3 deletions(-)
Quoted 13 lines · Danilo Krummrich
When
DMA_ATTR_NO_KERNEL_MAPPINGis passed todma_alloc_attrs(), the returned CPU address is not a pointer to the allocated memory but an opaque handle (e.g. struct page *).Coherent<T> (or CoherentAllocation<T> respectively) stores this value as NonNull<T> and exposes methods that dereference it and even modify its contents.
Remove the flag from the public attrs module such that drivers cannot pass it to Coherent<T> (or CoherentAllocation<T> respectively) in the first place.
Quoted 7 lines
Instead
DMA_ATTR_NO_KERNEL_MAPPINGcan be supported with an additional opaque type (e.g. CoherentHandle) which does not provide access to the allocated memory.Fixes ad2907b4e308 ("rust: add dma coherent allocator abstraction")Signed-off-by Danilo Krummrich <[email protected]>
Quoted 12 lines · Danilo Krummrich
When
DMA_ATTR_NO_KERNEL_MAPPINGis passed todma_alloc_attrs(), the returned CPU address is not a pointer to the allocated memory but an opaque handle (e.g. struct page *).Coherent<T> (or CoherentAllocation<T> respectively) stores this value as NonNull<T> and exposes methods that dereference it and even modify its contents.
Remove the flag from the public attrs module such that drivers cannot pass it to Coherent<T> (or CoherentAllocation<T> respectively) in the first place.
Quoted 7 lines
Instead
DMA_ATTR_NO_KERNEL_MAPPINGcan be supported with an additional opaque type (e.g. CoherentHandle) which does not provide access to the allocated memory.Fixes ad2907b4e308 ("rust: add dma coherent allocator abstraction")Signed-off-by Danilo Krummrich <[email protected]>
Alexandre Courbot
reviewerQuoted 12 lines · Danilo Krummrich
When
DMA_ATTR_NO_KERNEL_MAPPINGis passed todma_alloc_attrs(), the returned CPU address is not a pointer to the allocated memory but an opaque handle (e.g. struct page *).Coherent<T> (or CoherentAllocation<T> respectively) stores this value as NonNull<T> and exposes methods that dereference it and even modify its contents.
Remove the flag from the public attrs module such that drivers cannot pass it to Coherent<T> (or CoherentAllocation<T> respectively) in the first place.
Quoted 7 lines
Instead
DMA_ATTR_NO_KERNEL_MAPPINGcan be supported with an additional opaque type (e.g. CoherentHandle) which does not provide access to the allocated memory.Fixes ad2907b4e308 ("rust: add dma coherent allocator abstraction")Signed-off-by Danilo Krummrich <[email protected]>
As agreed with Danilo [1], pushed the series to drm-rust-next - thanks!