Re: [PATCH v2 6/7] fpga: add PolarFire SoC Auto Update support

From: Conor Dooley <conor.dooley@microchip.com>
To: Xu Yilun <yilun.xu@intel.com>
Cc: <linux-fpga@vger.kernel.org>, <conor@kernel.org>,
	Daire McNamara <daire.mcnamara@microchip.com>,
	Rob Herring <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	Moritz Fischer <mdf@kernel.org>, Wu Hao <hao.wu@intel.com>,
	Tom Rix <trix@redhat.com>, <linux-riscv@lists.infradead.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 6/7] fpga: add PolarFire SoC Auto Update support
Date: Tue, 11 Apr 2023 12:51:34 +0100	[thread overview]
Message-ID: <20230411-tinsmith-matchless-af5c7c41d23b@wendy> (raw)
In-Reply-To: <ZClfjuLa/pRATwCp@yilunxu-OptiPlex-7050>

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

On Sun, Apr 02, 2023 at 06:57:18PM +0800, Xu Yilun wrote:
> On 2023-03-31 at 08:18:22 +0100, Conor Dooley wrote:
> > Add support for Auto Update reprogramming of the FPGA fabric on
> > PolarFire SoC, using the fw_upload mechanism a la the
> > intel-m10-bmc-sec-update driver.
> 
> I'm not an expert of MTD and have several concerns about the mtd
> writing part:
> 
> - This seems a generic case that writing a file content into the
>   driver restricted offset on mtd.

Right, writing at an offset is pretty generic.

> - Seems the firmware update interface could be created for mtd device,
>   like the nvmem interface?

I'm not entirely sure what you mean by this one.
Are you saying that there should be a generic way to do "firmware"
uploads to mtd devices, based on some sort of structure defined in
devicetree etc?
And perhaps then, this driver would register as a consumer of that
interface, pushing all of the direct mtd stuff out to that interface?

> - I assume the purpose of the fw upload interface is to protect
>   unexpected writing from user. So how to protect the mtd be
>   written by other user interfaces like mtdchar/block or nvmem?

I'm not very good with the security knobs so I don't have an answer for
you here (yet). But your assumption about the rationale here is correct
and I would like to block access.

> For other parts:
> 
> - directory block check, fw upload don't touch this block, so why check
>   it in fw upload process.

I don't know what you mean. The mtd device could be empty and the
directory would need to be populated with the image address etc.
Or, it could previously have been used for something else, and then it
would be required to reconfigure it correctly for Auto Upgrade.

Maybe I am misunderstanding you?

> 
> - image verification, no matter pass or fail, fw upload is actually done,
>   the flash is written and no way to rollback. So may remove from fw
>   upload process.

The FPGA is flash based, so writing to the mtd device is not going to
actually program the FPGA with this content. The original image/bitstream
in the mtd device is either going to be a) equivalent to what is already
programmed in the FPGA, b) not an upgrade over what is current in the
FPGA, c) something that was uploaded since the last time the device was
restarted.
I don't think that overwriting any of those is problematic, as if the
verification fails, the original bitstream will continue being used
until an image that does actually pass is provided.

Without doing the verification though, there's no way to tell for certain
if the FPGA is actually going to perform an upgrade to this new bitstream
once it is restarted, so I think there is value in keeping this.

If you're still very sceptical about whether this belongs in
drivers/fpga, I can always dump it in drivers/soc instead I guess.

Cheers,
Conor.

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