Re: [PATCH v17 0/5] FPGA Image Load (previously Security Manager)

[Tom Rix]

On 10/28/21 8:09 AM, Xu Yilun wrote:
> On Wed, Oct 27, 2021 at 08:34:16AM -0700, Tom Rix wrote:
> > On 10/27/21 8:11 AM, Russ Weight wrote:
> > > On 10/26/21 8:29 PM, Wu, Hao wrote:
> > > > > > > > The API should not only define what it won't do, but also define what
> > > > > > > > it will do. But the "image load" just specifies the top half of the
> > > > > > > > process. So I don't think this API would be accepted.
> > > > > > > So what is the path forward. It seems like you are saying
> > > > > > > that the self-describing files do not fit in the fpga-mgr.
> > > > > > > Can we reconsider the FPGA Image Load Framework, which does
> > > > > > > not make any assumptions about the contents of the image
> > > > > > > files?
> > > > > > Why we need such "generic data transfer" interface in FPGA
> > > > > > framework?
> > > > > Are you referring to the use of self-describing files?
> > > > > or the generic nature of this class driver?
> > > > Yes, why this is under FPGA framework? Per your description that
> > > > it can be used to transfer any data, e.g. BMC images, some device
> > > > specific data (self-describing image?). Let's take this as example,
> > > > if FPGA device is replaced with ASIC on N3000, do you still want
> > > > to use FPGA image load framework to transfer your device specific
> > > > data, e.g. BMC images? I really hope that FPGA framework code only
> > > > focus on common usage of FPGA.
> > > >
> > > > > > we need to handle the common need for FPGA
> > > > > > devices only, not all devices, like programming FPGA images.
> > > > > > So far we even don't know, what's the hardware response on
> > > > > > these self-describing files, how we define it as a common need
> > > > > > interface in the framework?
> > > > > The class driver does not _need_ to reside in the FPGA
> > > > > framework. I sent an inquiry to the maintainer of the
> > > > > Firmware update subsystem (and cc'd the kernel mailing list)
> > > > > and received no responses. I placed it under the FPGA
> > > > > framework only because the first user of the class driver
> > > > > is an FPGA driver.
> > > > You must have enough justifications why this needs to be included
> > > > for everybody not for our own case.
> > > How do we justify it when there are currently no other known
> > > users? I can go ahead and work up some patches for the firmware
> > > subsystem, if we can resolve the other concerns below.
> > >
> > > > > > If you just want to reuse the
> > > > > > fpga-mgr/framework code for your own purpose, Yes, it seems
> > > > > > saving some code for you, but finally it loses flexibility, as it's
> > > > > > not possible to extend common framework for your own
> > > > > > purpose in the future.
> > > > > If I understand correctly, you are saying that it doesn't
> > > > > fit well in the FPGA manager, because not all file types
> > > > > fit the definition of a firmware update? And future file
> > > > > types may not fit in fpga-mgr context?
> > > > Let's split the use cases, I think the use case that update a persistent
> > > > storage for FPGA image, and later use hardware logic (FPGA loader)
> > > > to load it into FPGA. This sounds like a common usage for FPGA
> > > > devices, so I think this is why Yilun propose to have this part to be
> > > > covered by fpga-mgr. But for other cases in your description, e.g.
> > > > BMC images, device specific data, self-describing image and etc,
> > > > they are out of scope of FPGA.
> > > Self-describing files are not something new to us; _ALL_ of the image
> > > files that we send to our FPGA cards, including the N3000 FPGA and BMC
> > > images, root-entry hashes, key cancellations, etc. are self-describing
> > > files. They always have been.
> > >
> > > > Actually I don't fully understand why we need to introduce the
> > > > "self-describing image" as a common data transfer interface, if
> > > > I remember correctly, for N3000, different sub drivers will own
> > > > different hardware sub function blocks, why expose such a new
> > > > shared communication channel?
> > > There is no change here. The N3000 files are self describing. The
> > > secure-update sub-driver of the MAX10 BMC invokes the class driver,
> > > funnels image data to the BMC, performs handshakes with the BMC,
> > > and ultimately returns status through the class driver. All images
> > > that are sent to the FPGA card follow this same path - and it works
> > > fine.
> > >
> > > To try to split out the purposes of each self-describing file to
> > > use different kernel APIs means interfacing multiple class drivers
> > > to the same MAX10 sub-driver. I think it also means replicating
> > > code.
> > Could the split be ?
> >
> > add max10 bits mfd/
> >
> > move image updating out of the kernel and into an uio driver
> I'm afraid an uio driver doesn't help in this case. The image updating
> is not an independent device, it may dynamically change other hardwares.
> So it is better the image updating driver works as an low level driver
> which provides services to other feature drivers.

Ok.

Since this is dfl specific could a 'write' op be added to fme_fops ?

Tom

> Thanks,
> Yilun
>
> > Tom
> >
> > > - Russ
> > > > If "self-describing image" is a
> > > > request to one of the sub function block, why not just expose
> > > > new interface in such hardware block per modularization? I
> > > > have some concern that this new requirement may break
> > > > current driver architecture for N3000.
> > > >
> > > > Hao
> > > >
> > > > > - Russ
> > > > > > Thanks
> > > > > > Hao