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

From: Tom Rix
Date: Thu Oct 28 2021 - 12:08:13 EST



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