Re: [PATCH v2 12/13] gpu: nova-core: add PIO support for loading firmware images

[Timur Tabi <ttabi@nvidia.com>]

On Tue, 2025-12-02 at 16:23 -0500, Joel Fernandes wrote:
> 
> 
> On 12/1/2025 6:39 PM, Timur Tabi wrote:
> >  
> > +
> > +    /// See nvkm_falcon_pio_wr - takes a byte array instead of a FalconFirmware
> > +    fn pio_wr_bytes(
> > +        &self,
> > +        bar: &Bar0,
> > +        img: &[u8],
> > +        mem_base: u16,
> > +        target_mem: FalconMem,
> > +        port: u8,
> > +        tag: u16
> > +    ) {
> > +        let port = usize::from(port);
> > +
> > +        match target_mem {
> > +            FalconMem::ImemSecure | FalconMem::ImemNonSecure => {
> > +                regs::NV_PFALCON_FALCON_IMEMC::default()
> > +                    .set_secure(target_mem == FalconMem::ImemSecure)
> > +                    .set_aincw(true)
> > +                    .set_offs(mem_base)
> > +                    .write(bar, &E::ID, port);
> > +
> > +                let mut tag = tag;
> > +                for block in img.chunks(256) {
> > +                    regs::NV_PFALCON_FALCON_IMEMT::default()
> > +                        .set_tag(tag)
> > +                        .write(bar, &E::ID, port);
> > +                    for word in block.chunks(4) {
> > +                        let w = u32::from_le_bytes(word.try_into().unwrap());
> 
> If img.size is not a multiple of 4 bytes, this can panic right?

I think so.  I just noticed that I used chunks(4) here and chunks_exact(4) in the Dmem loop below. 
I need to make it consistent.

chunks(4) will return &[u8; 3] if the buffer is shy one byte.  chunks_exact(4) will simply skip the
last 3 bytes.

The problem is that it is an error for these images to not be a multiple of 4.  Such an image is
just not valid.

So it's a lot simpler to just reject these misaligned images.  The previous version of this function
did return a Result, maybe I should put that back.  It just seems wasteful to test for misalignment
on every pass of the loop.

What we really need is for from_le_bytes() to be less picky about the slice size.  If I give it
&[u8; 3], then it should be able to handle that.

> Even if it is unlikely, unwrap() is quite frowned up due to possibility of
> panic. I'd recommend something like the following since the function cannot
> return an error:
> 
>                         let w = if let Ok(bytes) = word.try_into() {
>                             u32::from_le_bytes(bytes)
>                         } else {
> 			    // can print a warning here too if needed.
>                             let mut buf = [0u8; 4];
>                             buf[..word.len()].copy_from_slice(word);
>                             u32::from_le_bytes(buf)
>                         };

Wouldn't it be simpler to use chunks_exact() and then remainder()?  That way, we wouldn't need a
test inside the loop?

> Btw, I wish we could encode the slice length constraint in the slice type itself
> (i.e., the slice length ought to be a certain multiple). But I think there's no
> way to do that without introducing a new type.

Wouldn't it be a run-time constraint anyway? With the exception of the BootloaderDmemDescV2 write, 
all of the calls to pio_wr_bytes() have lengths only known at runtime.