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:     Tom Rix <trix@redhat.com>, Xu Yilun <yilun.xu@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>
 <450ed897-f726-9671-26b7-2a24bb046e89@redhat.com>
 <20211011014154.GA82360@yilunxu-OptiPlex-7050>
 <79350773-3629-2734-21c0-0314a762e722@redhat.com>
From:   Russ Weight <russell.h.weight@intel.com>
Message-ID: <336e4827-b09a-e1ab-b67d-d8755012d71c@intel.com>
Date:   Mon, 11 Oct 2021 18:00:16 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.13.0
In-Reply-To: <79350773-3629-2734-21c0-0314a762e722@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZkJsNWxDQkFrUTZhaGwwWnpSaFN0SkVUTE1PY3NjVnlFcnZPSjdOSWxLU3ZX?=
 =?utf-8?B?WDdRVVo2WHVub1RNbWR4eFVndjVOTGtKRGhsSHJLUmRHK0gyN01PaVpLSlc5?=
 =?utf-8?B?aUozcVgyVXN6clFTSzlYNEJ0YlNmR1krejhSZ2RmRjAvREVGdDRSaDh0ekRj?=
 =?utf-8?B?Wll0Y0MybEtNcVB1RnVqRThDTFpyZ2thTlMvajhoazFIREhaQ1lFa2lhcXdU?=
 =?utf-8?B?YWxOMW53UGRoUVBEYlNHdEk4YW5TSEhFT3RreE44WXRicWlNNXFPdjhYdWM3?=
 =?utf-8?B?V1FLK3dEVlVaREw3NXYzWE5aZ3ViWlA1cks3U3JPRWYyMW1vRWhMQW9PRE1o?=
 =?utf-8?B?TS85UzJKQ2U4OE1scE5nUU83c29JWGFTRnJJZmFWL04xNzNwYzBMZUxmZzBa?=
 =?utf-8?B?SENRUkYxVXhobStqK0VUTTJldUtCQktSWXRpVXBKdFhiUWxpVkVjQnI1bEJG?=
 =?utf-8?B?c09naGc1VDJQbWo4cll2MExpTURoWnhXYnFNSk1rc1VES0djRUpEQTNudy9y?=
 =?utf-8?B?UkpBS3JXanB4SitRQVlpMU5YNlF0OVhlY0x0a21heXFqRWVPQWVMOFlMNEh5?=
 =?utf-8?B?NjZtcS9VU2xIdEdBOGJQSXlYd0o0RzRGTWdpSTJsK1JSYjZTRkg2WmY3bHhQ?=
 =?utf-8?B?dHI2TkoxRE44MFJzZWZsRkVIZ3JiOUxaNlhpZXJPcVZETFozLzFGMnhqeGpP?=
 =?utf-8?B?MnpJeDErL3hxSzUwOXAyWHh3TlNxSDFCSU11c0U4ZXZyRVYxbEpTLzNQME9r?=
 =?utf-8?B?ajh1TS9jS2lFRmV6UDRTSlM2VFRpYk1RYlgxaVROWnZ1TTJvMTdPZHJ0Q3VN?=
 =?utf-8?B?MXBzQmNmWjRlbTd6emhKdXNHUS9zNXZkMk1mQ1FKNWZ0UmhxT2VtSGtFNkIy?=
 =?utf-8?B?aU1raG1NTDNYSER2dVBBY3FzUG1BSFR1bWdrWHduc0ZLUHdIdkUraG1BYTZn?=
 =?utf-8?B?RjR0RDd1Tm9LRWdxU2xtMG1JVmk5cG1vTWFYT0lvVEV4dnFFWGRLWGtwQWZO?=
 =?utf-8?B?RHhGWnV6R3Nld0c4VzdiaUVYaW5FWE4wTGh2cnZRMHB5ZUhuT1lkbEZvc2tl?=
 =?utf-8?B?bEJOQnBDMDFXM0VxUk9HanlzTXJnT2RnYjJIb0dWd1FVbjFyYmVxdnJidGlD?=
 =?utf-8?B?TXRXdnZHVU1Ialc1L25qVFpnNGJSVi9ZaU9uaFBKRkl1VFJBb1ptYXRYRjd2?=
 =?utf-8?B?SlZOOXpabkFiVkJWY01XS2FFOXNXQ040S2RGclBTQ2xHWm9lMHB6a0pNaXpN?=
 =?utf-8?B?VXE3cG5Gd2s5VXZpNVYvQ3lJV1FaaGhZS1lGam9OaGtUaXk3dXFxaW10bUd5?=
 =?utf-8?B?VzRtR0JncEJ6b1N5ckV3Nmw2YTk0bHo2Q1RwN3hXVzcyMXhxTmZzUXVYRnRk?=
 =?utf-8?B?bnJaMTVFYjJIdGk1REtJcDFpa2xLanZxOC9QMDNHdDNyaGFnYWhUSnh1Q1pi?=
 =?utf-8?B?K3Y0aDNBbEpKM1RvZi9Vb2lIbVY5NFdna0R2aDlKd1FKS29GTng1STcyUU8y?=
 =?utf-8?B?VnEzaHduWnNUUUowSll6dThCMHdnMU9aNDNIM2hxSTVIblJLcHRMM2dJaWJP?=
 =?utf-8?B?Wk5pUUh6NUtxM3FPbWFVSGlKMjVFQm45T1gwOVhIQ2JXU0gzWHczYXZXYS9X?=
 =?utf-8?B?blJqSFhXU0xicjJZbCtoSk9DUlJmNzV4RXlMTDlUWmJoT0QyWUttOXZKYWth?=
 =?utf-8?B?WmJ3VERXZWc4YVIwaHNFSDFTT3RyeURPZUUrVFNVOEtNQXJEZHIycDZMMndV?=
 =?utf-8?Q?xQ8XuR3cCKa31Eg4X7SNUWlzjOKUrWbHqhU8s7g?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d1002fb2-1051-4253-d485-08d98d1baa7d
X-MS-Exchange-CrossTenant-AuthSource: SA2PR11MB5163.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Oct 2021 01:00:21.5125
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: FcoyLTs9/UmpzVyRFgo/wZvh6oDLlhgKzFCSqcFC+WhEUVHU9NwIe6Ba7GxSf+/IgMiTN4jhc4q5sZI9apeYMsmKgKMniMlzZql4D/oivN4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4576
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 10/11/21 5:35 AM, Tom Rix wrote:
>
> On 10/10/21 6:41 PM, Xu Yilun wrote:
>> On Sat, Oct 09, 2021 at 05:11:20AM -0700, Tom Rix wrote:
>>> 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.
The fpga-image-load driver is actually just a data transfer. The class
driver has no knowledge about what the data is or where/if the data will
be stored.

This functionality could, of course, be merged into the fpga-mgr. I did
a proof of concept of this a while back and we discussed the pros and cons.
See this email for a recap:

https://marc.info/?l=linux-fpga&m=161998085507374&w=2

Things have changed some with the evolution of the driver. The IOCTL
approach probably fits better than the sysfs implementation. At the time
it seemed that a merge would add unnecessary complexity without adding value.

>>>>
>>>> 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.
I'll give this some more thought and see if I can come up with some RFC
patches.

- Russ
>>>>
>>>> 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.
>> I think we don't have to add 2 ops for reconfig & reimage in framework,
>> the 2 processes are almost the same.
>>
>> Just add the _REIMAGE (or something else, NVMEM?) flag for
>> fpga_image_info, and low level drivers handle it as they do for other
>> flags.
>>
>> How do you think?
>
> A single set is fine.
>
> A difficult part of is the length of  time to do the write. The existing write should be improved to use a worker thread.
>
> Tom
>
>>
>> Thanks,
>> Yilun
>>
>>> Any other driver would do similar.
>>>
>>> Is this close to what you are thinking ?
>>>
>>> Tom
>>>
>>>> Thanks,
>>>> Yilun
>>>>
>