Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Richard Gong <richard.gong@linux.intel.com>]

Hi Moritz,

Not sure if you have chance to view Russ's comments and my comments. 
Pleas let us know what you think so I can act accordingly.

I had a few discussions with Russ, and we all realized that the goals we 
were trying to achieve were not as similar as they seemed.

Regards,
Richard

On 4/12/21 8:41 PM, Richard Gong wrote:
> 
> Hi Moritz,
> 
> On 3/28/21 12:20 PM, Moritz Fischer wrote:
>> Tom,
>>
>> On Sun, Mar 28, 2021 at 08:40:24AM -0700, Tom Rix wrote:
>>>
>>> On 3/27/21 11:09 AM, Moritz Fischer wrote:
>>>> Hi Richard, Russ,
>>>>
>>>> On Thu, Feb 25, 2021 at 01:07:14PM +0000, Gong, Richard wrote:
>>>>> Hi Moritz,
>>>>>
>>>>> Sorry for asking.
>>>>>
>>>>> When you have chance, can you help review the version 5 patchset 
>>>>> submitted on 02/09/21?
>>>>>
>>>>> Regards,
>>>>> Richard
>>>>>
>>>>> -----Original Message-----
>>>>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com>
>>>>> Sent: Tuesday, February 9, 2021 4:20 PM
>>>>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; 
>>>>> linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
>>>>> Cc: Gong, Richard <richard.gong@intel.com>
>>>>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and 
>>>>> region
>>>>>
>>>>> From: Richard Gong <richard.gong@intel.com>
>>>>>
>>>>> This is 5th submission of Intel service layer and FPGA patches, 
>>>>> which includes the missing standalone patch in the 4th submission.
>>>>>
>>>>> This submission includes additional changes for Intel service layer 
>>>>> driver to get the firmware version running at FPGA SoC device. Then 
>>>>> FPGA manager driver, one of Intel service layer driver's client, 
>>>>> can decide whether to handle the newly added bitstream 
>>>>> authentication function based on the retrieved firmware version. So 
>>>>> that we can maintain FPGA manager driver the back compatible.
>>>>>
>>>>> Bitstream authentication makes sure a signed bitstream has valid 
>>>>> signatures.
>>>>>
>>>>> The customer sends the bitstream via FPGA framework and overlay, 
>>>>> the firmware will authenticate the bitstream but not program the 
>>>>> bitstream to device. If the authentication passes, the bitstream 
>>>>> will be programmed into QSPI flash and will be expected to boot 
>>>>> without issues.
>>>>>
>>>>> Extend Intel service layer, FPGA manager and region drivers to 
>>>>> support the bitstream authentication feature.
>>>>>
>>>>> Richard Gong (7):
>>>>>    firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>>>>>    firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>>>>>    firmware: stratix10-svc: extend SVC driver to get the firmware 
>>>>> version
>>>>>    fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>>>>>    fpga: of-fpga-region: add authenticate-fpga-config property
>>>>>    dt-bindings: fpga: add authenticate-fpga-config property
>>>>>    fpga: stratix10-soc: extend driver for bitstream authentication
>>>>>
>>>>>   .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>>>>>   drivers/firmware/stratix10-svc.c                   | 12 ++++-
>>>>>   drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>>>>>   drivers/fpga/stratix10-soc.c                       | 62 
>>>>> +++++++++++++++++++---
>>>>>   include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>>>>>   .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>>>>>   include/linux/fpga/fpga-mgr.h                      |  3 ++
>>>>>   7 files changed, 125 insertions(+), 18 deletions(-)
>>>>>
>>>>> -- 
>>>>> 2.7.4
>>>>>
>>>> Apologies for the epic delay in getting back to this, I took another
>>>> look at this patchset and Russ' patchset.
>>>>
>>>> TL;DR I'm not really a fan of using device-tree overlays for this (and
>>>> again, apologies, I should've voiced this earlier ...).
>>>>
>>>> Anyways, let's find a common API for this and Russ' work, they're 
>>>> trying
>>>> to achieve the same / similar thing, they should use the same API.
>>>>
>>>> I'd like to re-invetigate the possiblity to extend FPGA Manager with
>>>> 'secure update' ops that work for both these use-cases (and I susspect
>>>> hte XRT patchset will follow with a similar requirement, right after).
>>>
>>> The xrt patchset makes heavy use of device trees.
>>>
>>> What is the general guidance for device tree usage ?
>>
>> I'm not generally against using device tree, it has its place. To
>> describe hardware (and hardware *changes* with overlays) :)
>>
>> What I don't like about this particular implementation w.r.t device-tree
>> usage is that it uses DT overlays as a mechanism to program the flash --
>> in place of having an API to do so.
>>
>> One could add device-nodes during the DT overlay application, while the
>> FPGA doesn't actually get programmed with a new runtime image -- meaning
>> live DT and actual hardware state diverged -- worst case it'd crash.
>>
>> So when roughly at the same time (from the same company even) we have two
>> patchsets that do similar things with radically different APIs I think
>> we should pause, and reflect on whether we can come up with something
>> that works for both :)
>>
> 
> I discussed with Russ and studies his patches, came to realize that the 
> work we had to accomplish was not same or similar. What I want to 
> achieve is to verify the identity of the bitstream, which is like doing 
> a "dry-run" to FPGA configuration.
> 
> Performing FPGA configuration (full or partial) through the device tree 
> overlay is a method widely used by our customers.
> 
> Russ's approach utilizes a different user API which is a set of sysfs 
> files.
> 
> If we depart from device tree overlay, then the end-user must utilize 2 
> different mechanism or APIs (device tree overlay is used for 
> full/partial configuration, and sysfs is used for bitstream 
> authentication). Similarly low-level FPGA manager driver also needs to 
> add additional codes. For the end-user the single and simple mechanism 
> is always better choice, device tree overlay should be a better way to 
> achieve that goal.
> 
> Regards,
> Richard
> 
>> TL;DR the firmware parts to authenticate the bitstream look fine to 
>> me, the
>> way we tie it into the FPGA region I'm not a fan of.
>>
>> - Moritz
>>