Messages in this thread | | | Subject | Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region | From | Richard Gong <> | Date | Thu, 29 Apr 2021 09:56:54 -0500 |
| |
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 >>
| |