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

[Tom Rix <trix@redhat.com>]

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

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