| 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: <DDK7N52VX059.202D7SPGFV8A9@nvidia.com>
Date: Fri, 17 Oct 2025 10:36:10 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Daniel del Castillo" <delcastillodelarosadaniel@...il.com>, "Danilo
Krummrich" <dakr@...nel.org>, "Alexandre Courbot" <acourbot@...dia.com>,
"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]
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 `---`).
>
> 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.
<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?
> + } => 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?
Powered by blists - more mailing lists