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.g...@linux.intel.com <richard.g...@linux.intel.com> > >> Sent: Tuesday, February 9, 2021 4:20 PM > >> To: m...@kernel.org; t...@redhat.com; gre...@linuxfoundation.org; > >> linux-f...@vger.kernel.org; linux-kernel@vger.kernel.org > >> Cc: Gong, Richard <richard.g...@intel.com> > >> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region > >> > >> From: Richard Gong <richard.g...@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 :) 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
