(sorry, took a while to come back to this)
On Fri Feb 13, 2026 at 11:47 PM JST, Danilo Krummrich wrote:
> On Thu Feb 12, 2026 at 9:26 AM CET, Alexandre Courbot wrote:
>> + fn try_as_pio_loadable(&self) -> Result<FalconDmaFirmwarePioAdapter<'_,
>> Self>> {
>
> [...]
>
>> + let dmem = {
>> + let params = self.dmem_load_params();
>> +
>> + // SAFETY: we keep a reference to `self` for as long as this
>> slice is alive, and the
>> + // device will not access this DMA object since we are using
>> PIO.
>
> How is this guaranteed by this function? I.e. how is it prevented that this
> function is never called when the device acesses the DMA memory?
This relies on the fact that firmware loading and running is
synchronous, and that the firmware does not touch these objects once it
is done running. But yes, this is a pretty weak guarantee for Rust
standards.
>
>> + let data = unsafe {
>> + self.as_slice(
>> + usize::from_safe_cast(params.src_start),
>> + usize::from_safe_cast(params.len),
>> + )?
>> + };
>> +
>> + let dst_start = u16::try_from(params.dst_start).map_err(|_|
>> EINVAL)?;
>> +
>> + FalconPioDmemLoadTarget { data, dst_start }
>> + };
>> +
>> + Ok(FalconDmaFirmwarePioAdapter {
>> + fw: self,
>> + imem_sec,
>> + imem_ns,
>> + dmem,
>> + })
>> + }
>> +}
>
> <snip>
>
>> +/// Adapter type that makes any DMA-loadable firmware also loadable via PIO.
>> +///
>> +/// Created using [`FalconDmaLoadable::try_as_pio_loadable`].
>> +pub(crate) struct FalconDmaFirmwarePioAdapter<'a, T: FalconDmaLoadable +
>> ?Sized> {
>> + /// Reference to the DMA firmware.
>> + fw: &'a T,
>
> In v6 [1] I wrote:
>
> > @@ -221,6 +286,8 @@ pub(crate) struct FwsecFirmware {
> > desc: FalconUCodeDesc,
> > /// GPU-accessible DMA object containing the firmware.
> > ucode: FirmwareDmaObject<Self, Signed>,
> > + /// Generic bootloader
> > + gen_bootloader: Option<GenericBootloader>,
>
> I'm not convinced this is a good idea. We probably want a HAL here and
> have different FwsecFirmware types:
>
> One with a DMA object and one with a system memory object when the
> architecture uses PIO. In the latter case the object can have a
> GenericBootloader field, i.e. this also gets us rid of the Option and
> all the subsequent 'if chipset < Chipset::GA102' checks and 'match
> gbl_fw' matches below.
>
> So, I still wonder, why use an Adapter impl on top of DMA memory for PIO
> rather
> than different base types with a common trait to avoid DMA allocations in the
> PIO case altogether?
This would require quite a lot of new (almost duplicated) code just for
handling the PIO path, and complicate things more than is worth IMHO.
All these problems (and a few others) stem from the fact that we create
the DMA object early during the loading process to avoid a copy; but
doing so also forces us to do the patching and other fun things on that
DMA object. The instance of "we cannot really guarantee that nobody else
is accessing that object" is not unique, the signature patching for
example is just as weak.
However, if we are willing to accept an extra copy of the ucode when DMA
is used, then we can change the loading process to work with a regular
KVec, and only move the ucode into a DmaObject at the last minute in
`dma_load`.
This would solve all the problems you raised while avoiding duplicating
code - it would actually *simplify* the code a bit and remove a bunch of
unsafes notably in `fwsec.rs`. WDYT? I'd say the unsafe removal alone
makes it worthwhile, and it's not like that copy would induce a
perceived slowdown anyway.
