Hi Russ, On Wed, Apr 07, 2021 at 04:56:29PM -0700, Russ Weight wrote: > Hi Moritz, > > Please see below my analysis of the effort to integrate secure functions > into the FPGA Manager. I have recommendations interspersed below. The > bottom line is that it seems like there is very little opportunity to > share code. I could try to rewrite the fpga_mgr_load() related functions > to accommodate worker threads, cancellation, and progress updates, but I'm > not sure there is enough value-add to justify the the additional complexity, > especially for the scatter-gather versions. The other options is to just > port the secure-manager functions into the FPGA Manager. > > - Russ > > ======================================================================= > > This is a comparison of the FPGA Security Manager patch-set with the > current FPGA Manager. Recommendations are provided on how to the extend > the FPGA Manager to support the Security Manager functions. > > Purpose of the FPGA Security Manager Class Driver > ================================================= > > The security manager provides a common user API (via sysfs) for transferring > an opaque image to the card BMC for validation and disposition with a > completion time of up to 40 minutes. A separate trigger function (image_load) > can be used to activate a previously transferred image (e.g. FPGA Static > Region or BMC image). > > Note that secure updates have no notion of Regions, Bridges or FPGA > running state. Not needed. > > A successful merge of the Security Manager with the FPGA Manager would include > an extension of the FPGA Manager API to provide sysfs nodes to support secure > updates. Yes. > > The FPGA Security Manager API > ============================= > > Image Update (transfer data to Card BMC for validation & storage) > WO: filename, cancel > RO: name, status, error, hw_errinfo, remaining_size > > Image Load (trigger BMC, FPGA, or FW load from FLASH) > RO: available_images # Images that can be loaded/activated > WO: image_load: # Trigger an image load > > > Merging Security Manager and FPGA Manager functions (organized by sysfs-node) > ============================================================================= > > The "name" sysfs node has essentially the same purpose, so no change is > required. > > The sec-mgr "status" is similar to fpga-mgr "state" > The sec-mgr "error" is simliar to fpga-mgr "status" > > All others are unique to the security manager. > > * RECOMMENDATION: The secure-update sysfs files should be grouped together > * in "secure-update" sysfs directory to clearly associate them with the > * secure update process and separate them from the other driver functions. SGTM. > > The sec-mgr sysfs status file has some similarity to the fpga-mgr sysfs > state file: > > Sec-Mgr status FPGA-Mgr state > -------------- -------------- > FPGA_MGR_STATE_UNKNOWN > FPGA_MGR_STATE_POWER_OFF > FPGA_MGR_STATE_POWER_UP > FPGA_MGR_STATE_RESET > FPGA_SEC_PROG_IDLE > FPGA_SEC_PROG_READING FPGA_MGR_STATE_FIRMWARE_REQ > FPGA_MGR_STATE_FIRMWARE_REQ_ERR > FPGA_SEC_PROG_PREPARING FPGA_MGR_STATE_WRITE_INIT > FPGA_MGR_STATE_WRITE_INIT_ERR > FPGA_SEC_PROG_WRITING FPGA_MGR_STATE_WRITE > FPGA_MGR_STATE_WRITE_ERR > FPGA_SEC_PROG_PROGRAMMING FPGA_MGR_STATE_WRITE_COMPLETE > FPGA_MGR_STATE_WRITE_COMPLETE_ERR > FPGA_MGR_STATE_OPERATING > FPGA_SEC_PROG_IDLE > > The sec-mgr sysfs error file seems to be similar in purpose to the > fpga-mgr sysfs status file, but there is no overlap in the actual > error codes. > > * RECOMMENDATION: there is not enough similarity between the status/state and > * error/status nodes to try to share them. If all of the other secure-update > * related nodes are grouped together in a secure-update directory, it would > * probably create confusion to try to share the state and status files that > * are in a different location. Yes. > > sysfs: filename > =============== > > In the current security manager patches, writing the name of an image file > to "filename" is roughly equivalent to creating a worker thread and having it > call fpga_mgr_load() with info->firmware_name set to the image file name. > > The update sequences are similar: > > Sec-Mgr ops flow FPGA-Mgr ops flow > ================ ================= > prepare() write_init() > # Loop on 16KB blocks write() > write_blk() > poll_complete() write_complete() > > In the worst case, we have seen n3000 FPGA image updates take up to 40 > minutes. The remaining_size is updated between each 16KB block to > allow users to monitor the progress of the data transfer. The sec-mgr > also checks for a cancel flag between each 16KB write and between > each stage of the update. > > The fpga_mgr has a "buffer" flow and a "scatter-gather" flow. The sec-mgr > flow is always started via sysfs and uses request_firmware(), so it only > needs one (buffer) flow. > > * RECOMMENDATION: keep the update functions separate. Although they are > * similar, the fpga-mgr updates are simpler and more readable as they > * are. The sec-mgr update is done via kernel worker thread and is a little > * more complicated with support for maintaining the remaining size and > * with support for cancellation. There currently no need for a scatter-gather > * secure update implementation since secure updates use request_firmware. > > Note that the sec-mgr supports additional ops: > cancel() : send cancel-request to lower-level driver > cleanup() : optional: called if and only if the prepare op succeeds > get_hw_errinfo() : optional: provides device-specific 64-bit error info > > sysfs: cancel, status, error, hw_errinfo, remaining_size > ======================================================== > > * RECOMMENDATION: There are no equivalent features in fpga-mgr. Port > * these each from the security manager to the FPGA Manager. SGTM. > > sysfs: available_images, load_image > =================================== > > available_images: RO: list device-specific keywords that can be used > to trigger an image to be loaded/activated. eg. > fpga_factory, fpga_user1, fpga_user2, bmc_factory. > > load_image: WO: trigger the load/activation of an image from FLASH > > * RECOMMENDATION: Nothing similar in fpga-mgr. Port functionality to the > * FPGA Manager > > Conclusion > ========== > > The purpose of the secure-update support is to provide a common user API > for loading an opaque image to a device with completion times of up to 40 > minutes. The FPGA Manager can be extended to include the sysfs nodes > required for a secure update. There is very little opportunity for shared > code between the current FPGA Manager and the secure update support. For > the most part, this would be an addition of new functionality. Sounds good to me overall, sorry for the late response. - Moritz
