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:
signature.asc
Description: PGP signature
