Re: [PATCHv2 1/5] firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag

From: Moritz Fischer <mdf@kernel.org>
To: Richard Gong <richard.gong@linux.intel.com>
Cc: Moritz Fischer <mdf@kernel.org>,
	gregkh@linuxfoundation.org, trix@redhat.com,
	linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org,
	dinguyen@kernel.org, sridhar.rajagopal@intel.com,
	richard.gong@intel.com
Subject: Re: [PATCHv2 1/5] firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
Date: Mon, 30 Nov 2020 20:31:34 -0800	[thread overview]
Message-ID: <X8XHJimPDaE/lNx0@archbook> (raw)
In-Reply-To: <771ba4f4-59e1-74b0-ba55-3f65914e2bc7@linux.intel.com>

Hi Richard,

On Mon, Nov 30, 2020 at 12:55:44PM -0600, Richard Gong wrote:
> 
> Hi Moritz,
> 
> Sorry for late reply, I was out last week.

No worries, usually I'm late with replies ;-)
> 
> On 11/21/20 7:10 PM, Moritz Fischer wrote:
> > Richard,
> > 
> > On Wed, Nov 18, 2020 at 12:16:09PM -0600, Richard Gong wrote:
> > 
> > > > > -#define COMMAND_RECONFIG_FLAG_PARTIAL	1
> > > > > +#define COMMAND_RECONFIG_FLAG_PARTIAL	0
> > > > > +#define COMMAND_AUTHENTICATE_BITSTREAM	1
> > > > 
> > > > Can you explain how this commit by itself doesn't break things?
> > > > 
> > > > Before this change firmware expected BIT(0) to be set for partial
> > > > reconfiguration, now BIT(0) suddenly means authentication? How doest his
> > > > work? :)
> > > >   > Was there a firmware version change? Did this never work before?
> > > > 
> > > > If this is version depenedent for firmware, then this might need a
> > > > different compatible string / id / some form of probing?
> > > > 
> > > > Entirely possible that I'm missing something, but it doesn't *seem*
> > > > right.
> > > 
> > > It did work before.
> > > 
> > > Before this change, firmware only checks if the received flag value is zero.
> > > If the value is zero, it preforms full reconfiguration. Otherwise it does
> > > partial reconfiguration.
> > > 
> > > To support bitstream authentication feature, firmware is updated to check
> > > the received flag value as below:
> > > 	0	--- full reconfiguration
> > > 	BIT(0) 	--- partial reconfiguration
> > > 	BIT(1) 	--- bitstream authentication
> > 
> > So there are two different versions of firmware involved that behave
> > differently?
> > 
> > Old firmware:
> > - ctype.flags  = 0x0 -> Full reconfig
> > - ctype.flags != 0 -> Partial reconfig
> > 
> > New firmware:
> > - ctype.flags = 0x0 -> Full reconfig
> > - ctype.flags = 0x1 -> Partial reconfig
> > - ctype.flags = 0x2 -> Authenticate
> > 
> > Old software:
> > - Send 0x0 for Full
> > - Send 0x1 for Partial
> > 
> > New software:
> > - Send 0x0 for Full
> > - Send 0x1 for Partial
> > - Send 0x2 for Auth
> > 
> > If I send request for authentication BIT(1) (new software) to old
> > firmware it'd try and attempt a partial reconfiguration with the data I
> > send? Is that safe?
> > 
> 
> Yes, it is possible and it is not safe. But we will inform our customers
> they should update to the latest firmware (SDM firmware and ATF) if they
> want to have authentication feature.
> 
> We are migrating boot loader boot flow to the new ATF boot flow, which is
> SDM firmware -> SPL -> ATF -> U-boot proper -> Linux. The new authentication
> feature is supported only in the new ATF boot flow. ATF communicates with
> SDM firmware via mailbox, and SDM firmware performs the actual full/partial
> reconfiguration and bitstream authentication. ATF sets up EL3 environment
> and initializes PSCI services.

Can U-Boot determine whether it's the new or old flow? Can you set a
different compatible value in your device-tree, to disambiguate
behaviors?

> The old boot flow is SDM firmware -> SPL -> U-boot proper -> Linux, which
> SPL/U-boot handles PSCI services and communicates with SDM firmware via
> mailbox. SDM firmware performs the actual full/partial reconfiguration.
> 
> ATF = Arm Trust Firmware, SDM = Secure Device Manager
> 
> > Is there a way for software to figure out the firmware version and do
> > the right thing?
> 
> It is not feasible for kernel driver to get the firmware version per current
> designs and implementations. I don't think there is other way around this.
> 
> > 
> > > Therefore I have updated the command flag setting at Intel service layer
> > > driver to align with firmware.
> > > 
> > > Regards,
> > > Richard
> > > 
> > > > >    /**
> > > > >     * Timeout settings for service clients:
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > 
> > > > Cheers,
> > > > Moritz
> > > > 
> > 
> > Thanks,
> > Moritz
> > 
> Regards,
> Richard

Thanks,
Moritz