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

[Tom Rix <trix@redhat.com>]

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