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

[Richard Gong]

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@xxxxxxxxxxxxxxx <richard.gong@xxxxxxxxxxxxxxx>
> > > > > Sent: Tuesday, February 9, 2021 4:20 PM
> > > > > To: mdf@xxxxxxxxxx; trix@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; 
> > > > > linux-fpga@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > > > Cc: Gong, Richard <richard.gong@xxxxxxxxx>
> > > > > Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and 
> > > > > region
> > > > >
> > > > > From: Richard Gong <richard.gong@xxxxxxxxx>
> > > > >
> > > > > 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