Moritz, On 4/28/21 9:10 PM, Moritz Fischer wrote: > 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. Thanks for the feedback Mortiz. Have you had a chance to look at the follow-up patch that I sent out? Between the cover letter and the RFC patch itself it gives a better idea of what a merged version of the driver would look like and raises some questions about how tightly the new functionality should be integrated with the fpga manager. https://marc.info/?l=linux-fpga&m=161841884401312&w=2 https://marc.info/?l=linux-fpga&m=161884993123556&w=2 Thanks, - Russ > > - Moritz
