On Fri, 2026-01-16 at 22:05 +0100, Danilo Krummrich wrote:
> > + fn pio_wr_bytes(
> > + &self,
> > + bar: &Bar0,
> > + img: &[u8],
> > + mem_base: u16,
> > + target_mem: FalconMem,
> > + port: u8,
>
> This looks like we should use the Bounded type instead.
I would rather just hard-code the port to 0 since that's the only value we ever
use, and we always
pass it as a constant anyway. This looks silly to me:
self.pio_wr_bytes(bar, data, dst_start, FalconMem::ImemNonSecure,
Bounded::<u8, 2>::new::<0>(),
tag)?;
> for (n, block) in img.chunks(256).iter().enumerate() {
> regs::NV_PFALCON_FALCON_IMEMT::default()
> .set_tag(tag + n)
> .write(bar, &E::ID, port);
> }
>
> This way you don't need the mutable shadow of tag. Besides that, how do we
> know
> this doesn't overflow? Don't we need a checked_add()?
Ok.
> > + fn pio_wr<F: FalconFirmware<Target = E>>(
> > + &self,
> > + bar: &Bar0,
> > + fw: &F,
> > + target_mem: FalconMem,
> > + load_offsets: &FalconLoadTarget,
> > + port: u8,
> > + tag: u16,
> > + ) -> Result {
> > + let start = usize::from_safe_cast(load_offsets.src_start);
> > + let len = usize::from_safe_cast(load_offsets.len);
> > + let mem_base = u16::try_from(load_offsets.dst_start)?;
> > +
> > + // SAFETY: we are the only user of the firmware image at this stage
> > + let data = unsafe { fw.as_slice(start, len).map_err(|_| EINVAL)? };
>
> Why do we need the firmware image to be backed by a DMA object at this point
> when you load the firmware image through PIO anyways?
I'm not sure I understand. The firmware images are always loaded into DMA
buffers. I don't control
that.
pio_wr() is just supposed to be the PIO equivalent to dma_wr(). It's purpose
is to load FW images.
I'll add a function comment.
> > diff --git a/drivers/gpu/nova-core/falcon/hal.rs
> > b/drivers/gpu/nova-core/falcon/hal.rs
> > index 49501aa6ff7f..3a882b7d8aa8 100644
> > --- a/drivers/gpu/nova-core/falcon/hal.rs
> > +++ b/drivers/gpu/nova-core/falcon/hal.rs
> > @@ -15,6 +15,11 @@
> > mod ga102;
> > mod tu102;
> >
> > +pub(crate) enum LoadMethod {
> > + Pio,
> > + Dma,
> > +}
>
> Seems obvious, but still, please add some documentation explaining that this
> is
> the load method for falcon firmware images, etc.
Ok.
>
> > +pub(crate) struct GenericBootloader {
> > + pub desc: BootloaderDesc,
> > + pub ucode: Vec<u8, kernel::alloc::allocator::Kmalloc>,
>
> pub ucode: KVec<u8>,
>
> Also, we may want to use KVVec<u8> or just VVec<u8> instead. What's the image
> size?
The Generic Bootloader is tiny, less than a KB and hasn't changed since 2016.
-rw-r--r-- 1 root root 816 Jan 14 20:47 gen_bootloader-570.144.bin
> > /// The FWSEC microcode, extracted from the BIOS and to be run on the GSP
> > falcon.
> > ///
> > /// It is responsible for e.g. carving out the WPR2 region as the first
> > step of the GSP
> > bootflow.
> > @@ -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.
Please give me more details as to what you're thinking. I know Alex has plans
for this code as
well, and I really don't want to try to implement something complicated, just
to have it conflict
with his designs.
It also feels like the kind of change that could be made in a follow-up
patchset.
