| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <72cfbe83-e587-441e-abfb-b50155a326ab@gmail.com>
Date: Sun, 19 Oct 2025 13:57:29 +0200
From: Daniel del Castillo <delcastillodelarosadaniel@...il.com>
To: Alexandre Courbot <acourbot@...dia.com>,
Danilo Krummrich <dakr@...nel.org>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>
Cc: nouveau@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, Boqun Feng <boqun.feng@...il.com>,
Gary Guo <gary@...yguo.net>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>,
Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
Trevor Gross <tmgross@...ch.edu>, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation`
improvements [COHA]
Hi Alexandre,
Thanks for your comments!
On 10/17/25 03:36, Alexandre Courbot wrote:
> On Thu Oct 16, 2025 at 4:49 AM JST, Daniel del Castillo wrote:
>> This patch solves the existing mentions of COHA, a task
>> in the Nova task list about improving the `CoherentAllocation` API.
>> I confirmed by talking to Alexandre Courbot, that the reading/writing
>> methods in `CoherentAllocation` can never be safe, so
>> this patch doesn't actually change `CoherentAllocation`, but rather
>> tries to solve the existing references to [COHA].
>
> This mention of background discussions should be in the comment part of
> your commit (below the `---`).
Noted, will do for the next version of the patch.
>>
>> Signed-off-by: Daniel del Castillo <delcastillodelarosadaniel@...il.com>
>> ---
>> drivers/gpu/nova-core/dma.rs | 20 ++---
>> drivers/gpu/nova-core/firmware/fwsec.rs | 104 ++++++++++--------------
>> 2 files changed, 50 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/dma.rs b/drivers/gpu/nova-core/dma.rs
>> index 94f44bcfd748..639a99cf72c4 100644
>> --- a/drivers/gpu/nova-core/dma.rs
>> +++ b/drivers/gpu/nova-core/dma.rs
>> @@ -25,21 +25,11 @@ pub(crate) fn new(dev: &device::Device<device::Bound>, len: usize) -> Result<Sel
>> }
>>
>> pub(crate) fn from_data(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
>> - Self::new(dev, data.len()).map(|mut dma_obj| {
>> - // TODO[COHA]: replace with `CoherentAllocation::write()` once available.
>> - // SAFETY:
>> - // - `dma_obj`'s size is at least `data.len()`.
>> - // - We have just created this object and there is no other user at this stage.
>> - unsafe {
>> - core::ptr::copy_nonoverlapping(
>> - data.as_ptr(),
>> - dma_obj.dma.start_ptr_mut(),
>> - data.len(),
>> - );
>> - }
>> -
>> - dma_obj
>> - })
>> + let mut dma_obj = Self::new(dev, data.len())?;
>> + // SAFETY: We have just created this object and there is no other user at this stage.
>> + unsafe { dma_obj.write(data, 0)? };
>> +
>> + Ok(dma_obj)
>
> Can you preserve the use of `map`? This removes the need for the
> temporary variable.
>
Sure.> <snip>
>> /// The FWSEC microcode, extracted from the BIOS and to be run on the GSP falcon.
>> @@ -260,32 +245,38 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>>
>> // Find the DMEM mapper section in the firmware.
>> for i in 0..hdr.entry_count as usize {
>> - let app: &FalconAppifV1 =
>> // SAFETY: we have exclusive access to `dma_object`.
>> - unsafe {
>> + let app: &FalconAppifV1 = unsafe {
>> transmute(
>> &dma_object,
>> - hdr_offset + hdr.header_size as usize + i * hdr.entry_size as usize
>> + hdr_offset + hdr.header_size as usize + i * hdr.entry_size as usize,
>> )
>> }?;
>>
>> if app.id != NVFW_FALCON_APPIF_ID_DMEMMAPPER {
>> continue;
>> }
>> + let dmem_base = app.dmem_base;
>>
>> // SAFETY: we have exclusive access to `dma_object`.
>> let dmem_mapper: &mut FalconAppifDmemmapperV3 = unsafe {
>> - transmute_mut(
>> - &mut dma_object,
>> - (desc.imem_load_size + app.dmem_base) as usize,
>> - )
>> + transmute_mut(&mut dma_object, (desc.imem_load_size + dmem_base) as usize)
>> }?;
>>
>> + dmem_mapper.init_cmd = match cmd {
>> + FwsecCommand::Frts {
>> + frts_addr: _,
>> + frts_size: _,
>
> Can the `{ .. }` syntax be used here?
>
Yes! I didn't remember that syntax.
>> + } => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS,
>> + FwsecCommand::Sb => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB,
>> + };
>> + let cmd_in_buffer_offset = dmem_mapper.cmd_in_buffer_offset;
>> +
>> // SAFETY: we have exclusive access to `dma_object`.
>> let frts_cmd: &mut FrtsCmd = unsafe {
>> transmute_mut(
>> &mut dma_object,
>> - (desc.imem_load_size + dmem_mapper.cmd_in_buffer_offset) as usize,
>> + (desc.imem_load_size + cmd_in_buffer_offset) as usize,
>> )
>> }?;
>>
>> @@ -296,24 +287,19 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>> size: 0,
>> flags: 2,
>> };
>> -
>> - dmem_mapper.init_cmd = match cmd {
>> - FwsecCommand::Frts {
>> - frts_addr,
>> - frts_size,
>> - } => {
>> - frts_cmd.frts_region = FrtsRegion {
>> - ver: 1,
>> - hdr: size_of::<FrtsRegion>() as u32,
>> - addr: (frts_addr >> 12) as u32,
>> - size: (frts_size >> 12) as u32,
>> - ftype: NVFW_FRTS_CMD_REGION_TYPE_FB,
>> - };
>> -
>> - NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS
>> - }
>> - FwsecCommand::Sb => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB,
>> - };
>> + if let FwsecCommand::Frts {
>> + frts_addr,
>> + frts_size,
>> + } = cmd
>> + {
>> + frts_cmd.frts_region = FrtsRegion {
>> + ver: 1,
>> + hdr: size_of::<FrtsRegion>() as u32,
>> + addr: (frts_addr >> 12) as u32,
>> + size: (frts_size >> 12) as u32,
>> + ftype: NVFW_FRTS_CMD_REGION_TYPE_FB,
>> + };
>> + }
>
> I liked that the original code updated both `init_cmd` and `frts_region`
> in the same match block. I understand it might be difficult to preserve
> due to the borrowing rules, but can you try to preserve it if that's
> possible at all?
I agree it was nicer. I tried to preserve it, but I don't see a way to
do it cleanly, as I can't keep both mutable references at the same time.
What I could do is only check `cmd` once, set `init_cmd` and store an
`Option<FrtsRegion>` that I will later use to set `frts_region` if it's
not `None`. Let me know if you prefer that.
Cheers,
Daniel
Powered by blists - more mailing lists