Re: [PATCH v1 0/6] PolarFire SoC Auto Update Support

On 2/27/23 14:16, Conor Dooley wrote:

Hey Russ,

On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:

quoted

On 2/24/23 00:28, Conor Dooley wrote:

quoted

On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:

quoted

On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:

quoted

This patchset adds support for the "Auto Update" feature on PolarFire
SoC that allows for writing an FPGA bistream to the SPI flash connected
to the system controller.

I haven't fully checked the patches yet, just some quick comments:

Since this feature is just to R/W the flash, and would not affect the
runtime FPGA region, I don't think an FPGA manager is actually needed.
Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
MTD tool if I remember correctly.

A lack of interest in opening up the system controller to userspace!
You're right in that the writing of the image can be done that way, and
while I was testing I used the userspace bits of mtd along the way - but
for validating that the image we are writing we rely on the system
controller.
I'm really not interested in exposing the system controller's
functionality, especially the bitstream manipulation parts, to userspace
due to the risk of input validation bugs, so at least that side of
things should remain in the kernel.
I suppose I could implement something custom in drivers/soc that does
the validation only, and push the rest out to userspace. Just seemed
fitting to do the whole lot in drivers/fpga.

In case you haven't already looked at the new firmware-upload
support in the firmware-loader, I'll give you some references
here to see if it fit you needs. This would only support the
write (not the read) but it would allow the controller to do
validation on the write.

The is the cover letter which shows a usage example:
https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/ (local)

And this is a pointer to the latest documentation for it:
https://docs.kernel.org/driver-api/firmware/fw_upload.html

The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c

Sounds interesting, I shall go and take a look. Just quickly, when you
say it wouldn't support the read, what exactly do you mean?
The only read that I am really interested in doing is reading the first
1K of flash as I need to RMW it, but I don't think that that's what you
mean.
Do you mean that I would not be able to dump the firmware using your
fw_upload interface? If so, that's an acceptable constraint IMO.

Yes - I mean that you couldn't dump the firmware image from userspace
using the fw_upload interface. The sysfs interface allows you to read
and write a temporary buffer, but once you "echo 0 > loading", there
is no sysfs interface associated with the firmware-loader that would
allow you to read the image from flash. Your controller actually does
the writes, so RMW in that context is fine.

- Russ

Your interface also circumvents us (Microchip) defining yet another
method for interacting with the FPGA, since from my nosing around,
everyone seems to do something different.

Cheers,
Conor.