Re: [RFC PATCH 1/3] dt-bindings: fpga: Add support for user-key encrypted bitstream loading

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3390 bytes --]

On Fri, Nov 24, 2023 at 06:35:19AM +0000, Manne, Nava kishore wrote:
> Hi Conor,
> 
> 	Thanks for providing the review comments.
> Please find my response inline.
> 
> > -----Original Message-----
> > From: Conor Dooley <conor@kernel.org>
> > Sent: Wednesday, November 22, 2023 10:21 PM
> > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > Cc: mdf@kernel.org; hao.wu@intel.com; yilun.xu@intel.com;
> > trix@redhat.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > conor+dt@kernel.org; Simek, Michal <michal.simek@amd.com>;
> > mathieu.poirier@linaro.org; Levinsky, Ben <ben.levinsky@amd.com>;
> > Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Shah, Tanmay
> > <tanmay.shah@amd.com>; dhaval.r.shah@amd.com; arnd@arndb.de;
> > Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>; linux-
> > fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > Subject: Re: [RFC PATCH 1/3] dt-bindings: fpga: Add support for user-key
> > encrypted bitstream loading
> > 
> > On Wed, Nov 22, 2023 at 11:14:02AM +0530, Nava kishore Manne wrote:
> > > Adds ‘encrypted-key-name’ property to support user-key encrypted
> > > bitstream loading use case.
> > >
> > > Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> > > ---
> > >  .../devicetree/bindings/fpga/fpga-region.txt  | 32
> > > +++++++++++++++++++
> > 
> > Is there a reason that this has not yet been converted to yaml?
> > 
> I am not sure about the complication involved here why it's not converted to yaml format.
> Due to time constraints, I couldn’t spend much time so I have used this existing legacy format
> to add my changes.
> 
> > >  1 file changed, 32 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > index 528df8a0e6d8..309334558b3f 100644
> > > --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > @@ -177,6 +177,9 @@ Optional properties:
> > >  	it indicates that the FPGA has already been programmed with this
> > image.
> > >  	If this property is in an overlay targeting an FPGA region, it is a
> > >  	request to program the FPGA with that image.
> > > +- encrypted-key-name : should contain the name of an encrypted key file
> > located
> > > +	on the firmware search path. It will be used to decrypt the FPGA
> > image
> > > +	file with user-key.
> > 
> > I might be misreading things, but your driver code seems to assume that this
> > is an aes key. Nothing here seems to document that this is supposed to be a
> > key of a particular type.
> > 
> 
> Yes, these changes are intended to add the support for Aes user-key encrypted bitstream loading use case.
> Will fix it in v2, something like below.
> aes-key-file-name : Should contain the AES key file name on the firmware search path.
> 		      The key file contains the AES key and it will be used to decrypt the FPGA image.

Then when someone comes along looking for a different type of encryption
we will end up with national-pride-foo-file-name etc. I think I'd rather
have a second property that notes what type of cipher is being used and
if that property is not present default to AES.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]