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;
IronPort-SDR: /Avq0B02llL/HtSP9g//q/S9mWcSvc1sL0iLkl1OoMNPTOhMoH8qyIqtundFXnhXx2HmoGBU52
 Rtz63MCo4eBQ==
IronPort-SDR: gJSrSv+nHsc47oysalmZkSpUzWooU42+BJ64KvM4XJJc/bUKl3ShAzDZg8/AzII7N0HaZWKT8n
 oWLT2/bg0pKw==
Subject: Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
From:   Richard Gong <richard.gong@linux.intel.com>
To:     Moritz Fischer <mdf@kernel.org>, Tom Rix <trix@redhat.com>
Cc:     "Gong, Richard" <richard.gong@intel.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        russell.h.weight@intel.com
References: <1612909233-13867-1-git-send-email-richard.gong@linux.intel.com>
 <MWHPR11MB001577B17723C8A046398249879E9@MWHPR11MB0015.namprd11.prod.outlook.com>
 <YF90y3Di4RbuJvr0@epycbox.lan>
 <496aa871-cfb0-faf4-4b1c-b53e56b58030@redhat.com>
 <YGC619DmLM0AAQ5p@epycbox.lan>
 <8c0fcdde-540b-2ae3-01f2-30cdb87ac037@linux.intel.com>
Message-ID: <a5e26661-388d-152b-7904-9c39522f7b14@linux.intel.com>
Date:   Thu, 29 Apr 2021 09:56:54 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <8c0fcdde-540b-2ae3-01f2-30cdb87ac037@linux.intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


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
>>