Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v17 0/5] FPGA Image Load (previously Security Manager)
To:     Xu Yilun <yilun.xu@intel.com>,
        Russ Weight <russell.h.weight@intel.com>
Cc:     mdf@kernel.org, linux-fpga@vger.kernel.org,
        linux-kernel@vger.kernel.org, lgoncalv@redhat.com,
        hao.wu@intel.com, matthew.gerlach@intel.com
References: <20210929230025.68961-1-russell.h.weight@intel.com>
 <20211009080859.GA85181@yilunxu-OptiPlex-7050>
From:   Tom Rix <trix@redhat.com>
Message-ID: <450ed897-f726-9671-26b7-2a24bb046e89@redhat.com>
Date:   Sat, 9 Oct 2021 05:11:20 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <20211009080859.GA85181@yilunxu-OptiPlex-7050>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


On 10/9/21 1:08 AM, Xu Yilun wrote:
> On Wed, Sep 29, 2021 at 04:00:20PM -0700, Russ Weight wrote:
>> The FPGA Image Load framework provides an API to upload image
>> files to an FPGA device. Image files are self-describing. They could
>> contain FPGA images, BMC images, Root Entry Hashes, or other device
>> specific files. It is up to the lower-level device driver and the
>> target device to authenticate and disposition the file data.
> I've reconsider the FPGA persistent image update again, and think we
> may include it in FPGA manager framework.
>
> Sorry I raised this topic again when it is already at patch v17, but now
> I need to consider more seriously than before.
>
> We have consensus the FPGA persistent image update is just like a normal
> firmware update which finally writes the nvmem like flash or eeprom,
> while the current FPGA manager deals with the active FPGA region update
> and re-activation. Could we just expand the FPGA manager and let it handle
> the nvmem update as well? Many FPGA cards have nvmem and downloaders
> supports updating both FPGA region and nvmem.
>
> According to the patchset, the basic workflow of the 2 update types are
> quite similar, get the data, prepare for the HW, write and complete.
> They are already implemented in FPGA manager. We've discussed some
> differences like threading or canceling the update, which are
> not provided by FPGA manager but they may also nice to have for FPGA
> region update. An FPGA region update may also last for a long time??
> So I think having 2 sets of similar frameworks in FPGA is unnecessary.
>
> My quick mind is that we add some flags in struct fpga_mgr & struct
> fpga_image_info to indicate the HW capability (support FPGA region
> update or nvmem update or both) of the download engine and the provided
> image type. Then the low-level driver knows how to download if it
> supports both image types.
>
> An char device could be added for each fpga manager dev, providing the
> user APIs for nvmem update. We may not use the char dev for FPGA region
> update cause it changes the system HW devices and needs device hotplug
> in FPGA region. We'd better leave it to FPGA region class, this is
> another topic.
>
> More discussion is appreciated.

I also think fpga_mgr could be extended.

In this patchset,

https://lore.kernel.org/linux-fpga/20210625195849.837976-1-trix@redhat.com/

A second, similar set of write ops was added to fpga_manger_ops,

new bit/flag was added to fpga_image_info

The intent was for dfl to add their specific ops to cover what is done 
in this patchset.

Any other driver would do similar.

Is this close to what you are thinking ?

Tom

>
> Thanks,
> Yilun
>