| Message ID | 20210909021846.681121-1-russell.h.weight@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | FPGA Image Load (previously Security Manager) | expand |
On 9/11/21 7:37 PM, Hillf Danton wrote: > On Wed, 8 Sep 2021 19:18:42 -0700 Russ Weight wrote: >> Extend the FPGA Image Load class driver to include IOCTL support >> (FPGA_IMAGE_LOAD_WRITE) for initiating an upload of an image to a device. >> The IOCTL will return immediately, and the update will begin in the >> context of a kernel worker thread. >> >> Signed-off-by: Russ Weight <russell.h.weight@intel.com> >> --- > Good work overall. Nits below. > [...] > >> +++ b/drivers/fpga/fpga-image-load.c >> @@ -5,18 +5,181 @@ >> * Copyright (C) 2019-2021 Intel Corporation, Inc. >> */ >> >> +#include <linux/delay.h> >> #include <linux/fpga/fpga-image-load.h> >> +#include <linux/fs.h> >> +#include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/slab.h> >> +#include <linux/uaccess.h> >> #include <linux/vmalloc.h> >> >> #define IMAGE_LOAD_XA_LIMIT XA_LIMIT(0, INT_MAX) >> static DEFINE_XARRAY_ALLOC(fpga_image_load_xa); >> >> static struct class *fpga_image_load_class; >> +static dev_t fpga_image_devt; >> >> #define to_image_load(d) container_of(d, struct fpga_image_load, dev) >> >> +static void fpga_image_dev_error(struct fpga_image_load *imgld, >> + enum fpga_image_err err_code) >> +{ >> + imgld->err_code = err_code; >> + imgld->lops->cancel(imgld); >> +} >> + >> +static void fpga_image_prog_complete(struct fpga_image_load *imgld) >> +{ >> + mutex_lock(&imgld->lock); >> + imgld->progress = FPGA_IMAGE_PROG_IDLE; >> + complete_all(&imgld->update_done); >> + mutex_unlock(&imgld->lock); >> +} >> + >> +static void fpga_image_do_load(struct work_struct *work) >> +{ >> + struct fpga_image_load *imgld; >> + enum fpga_image_err ret; >> + u32 size, offset = 0; >> + >> + imgld = container_of(work, struct fpga_image_load, work); >> + size = imgld->remaining_size; > Bail out if imgld->driver_unload is set. I'll add this. > >> + >> + get_device(&imgld->dev); >> + if (!try_module_get(imgld->dev.parent->driver->owner)) { >> + imgld->err_code = FPGA_IMAGE_ERR_BUSY; >> + goto idle_exit; >> + } >> + >> + imgld->progress = FPGA_IMAGE_PROG_PREPARING; >> + ret = imgld->lops->prepare(imgld); >> + if (ret != FPGA_IMAGE_ERR_NONE) { >> + fpga_image_dev_error(imgld, ret); >> + goto modput_exit; >> + } >> + >> + imgld->progress = FPGA_IMAGE_PROG_WRITING; >> + while (imgld->remaining_size) { >> + ret = imgld->lops->write_blk(imgld, offset); >> + if (ret != FPGA_IMAGE_ERR_NONE) { >> + fpga_image_dev_error(imgld, ret); >> + goto done; >> + } >> + >> + offset = size - imgld->remaining_size; > If cond_resched() is needed here then make imgld->work unbound. > > queue_work(system_unbound_wq, &imgld->work); Given that class driver does not have control over the size of the data or the actual implementation of the writes, it is probably best to add these changes. I'll do this for the next submission of theses patches. > >> + } >> + >> + imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING; >> + ret = imgld->lops->poll_complete(imgld); >> + if (ret != FPGA_IMAGE_ERR_NONE) >> + fpga_image_dev_error(imgld, ret); >> + >> +done: >> + if (imgld->lops->cleanup) >> + imgld->lops->cleanup(imgld); >> + >> +modput_exit: >> + module_put(imgld->dev.parent->driver->owner); >> + >> +idle_exit: >> + /* >> + * Note: imgld->remaining_size is left unmodified here to provide >> + * additional information on errors. It will be reinitialized when >> + * the next image load begins. >> + */ >> + vfree(imgld->data); >> + imgld->data = NULL; >> + put_device(&imgld->dev); >> + fpga_image_prog_complete(imgld); >> +} >> + >> +static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld, >> + unsigned long arg) >> +{ >> + struct fpga_image_write wb; >> + unsigned long minsz; >> + u8 *buf; >> + >> + if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE) >> + return -EBUSY; >> + >> + minsz = offsetofend(struct fpga_image_write, buf); >> + if (copy_from_user(&wb, (void __user *)arg, minsz)) >> + return -EFAULT; >> + >> + if (wb.flags) >> + return -EINVAL; >> + >> + /* Enforce 32-bit alignment on the write data */ >> + if (wb.size & 0x3) >> + return -EINVAL; >> + >> + buf = vzalloc(wb.size); >> + if (!buf) >> + return -ENOMEM; >> + >> + if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) { >> + vfree(buf); >> + return -EFAULT; >> + } >> + >> + imgld->data = buf; >> + imgld->remaining_size = wb.size; >> + imgld->err_code = FPGA_IMAGE_ERR_NONE; >> + imgld->progress = FPGA_IMAGE_PROG_STARTING; >> + reinit_completion(&imgld->update_done); >> + schedule_work(&imgld->work); >> + >> + return 0; >> +} >> + >> +static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd, >> + unsigned long arg) >> +{ >> + struct fpga_image_load *imgld = filp->private_data; >> + int ret = -ENOTTY; >> + >> + switch (cmd) { >> + case FPGA_IMAGE_LOAD_WRITE: >> + mutex_lock(&imgld->lock); >> + ret = fpga_image_load_ioctl_write(imgld, arg); >> + mutex_unlock(&imgld->lock); >> + break; >> + } >> + >> + return ret; >> +} >> + >> +static int fpga_image_load_open(struct inode *inode, struct file *filp) >> +{ >> + struct fpga_image_load *imgld = container_of(inode->i_cdev, >> + struct fpga_image_load, cdev); >> + >> + if (test_and_set_bit(0, &imgld->opened)) >> + return -EBUSY; >> + >> + filp->private_data = imgld; >> + >> + return 0; >> +} >> + >> +static int fpga_image_load_release(struct inode *inode, struct file *filp) >> +{ >> + struct fpga_image_load *imgld = filp->private_data; >> + >> + clear_bit(0, &imgld->opened); >> + >> + return 0; >> +} >> + >> +static const struct file_operations fpga_image_load_fops = { >> + .owner = THIS_MODULE, >> + .open = fpga_image_load_open, >> + .release = fpga_image_load_release, >> + .unlocked_ioctl = fpga_image_load_ioctl, >> +}; >> + >> /** >> * fpga_image_load_register - create and register an FPGA Image Load Device >> * >> @@ -33,11 +196,17 @@ fpga_image_load_register(struct device *parent, >> const struct fpga_image_load_ops *lops, void *priv) >> { >> struct fpga_image_load *imgld; >> - int id, ret; >> + int ret; >> + >> + if (!lops || !lops->cancel || !lops->prepare || >> + !lops->write_blk || !lops->poll_complete) { >> + dev_err(parent, "Attempt to register without all required ops\n"); >> + return ERR_PTR(-ENOMEM); >> + } >> >> imgld = kzalloc(sizeof(*imgld), GFP_KERNEL); >> if (!imgld) >> - return NULL; >> + return ERR_PTR(-ENOMEM); >> >> ret = xa_alloc(&fpga_image_load_xa, &imgld->dev.id, imgld, IMAGE_LOAD_XA_LIMIT, >> GFP_KERNEL); >> @@ -48,13 +217,19 @@ fpga_image_load_register(struct device *parent, >> >> imgld->priv = priv; >> imgld->lops = lops; >> + imgld->err_code = FPGA_IMAGE_ERR_NONE; >> + imgld->progress = FPGA_IMAGE_PROG_IDLE; >> + init_completion(&imgld->update_done); >> + INIT_WORK(&imgld->work, fpga_image_do_load); >> >> imgld->dev.class = fpga_image_load_class; >> imgld->dev.parent = parent; >> + imgld->dev.devt = MKDEV(MAJOR(fpga_image_devt), imgld->dev.id); >> >> - ret = dev_set_name(&imgld->dev, "fpga_image%d", id); >> + ret = dev_set_name(&imgld->dev, "fpga_image%d", imgld->dev.id); >> if (ret) { >> - dev_err(parent, "Failed to set device name: fpga_image%d\n", id); >> + dev_err(parent, "Failed to set device name: fpga_image%d\n", >> + imgld->dev.id); >> goto error_device; >> } >> >> @@ -64,6 +239,16 @@ fpga_image_load_register(struct device *parent, >> return ERR_PTR(ret); >> } >> >> + cdev_init(&imgld->cdev, &fpga_image_load_fops); >> + imgld->cdev.owner = parent->driver->owner; >> + imgld->cdev.kobj.parent = &imgld->dev.kobj; >> + >> + ret = cdev_add(&imgld->cdev, imgld->dev.devt, 1); >> + if (ret) { >> + put_device(&imgld->dev); >> + return ERR_PTR(ret); >> + } >> + >> return imgld; >> >> error_device: >> @@ -83,9 +268,29 @@ EXPORT_SYMBOL_GPL(fpga_image_load_register); >> * >> * This function is intended for use in an FPGA Image Load driver's >> * remove() function. >> + * >> + * For some devices, once authentication of the uploaded image has begun, >> + * the hardware cannot be signaled to stop, and the driver will not exit >> + * until the hardware signals completion. This could be 30+ minutes of >> + * waiting. The driver_unload flag enables a force-unload of the driver >> + * (e.g. modprobe -r) by signaling the parent driver to exit even if the >> + * hardware update is incomplete. The driver_unload flag also prevents >> + * new updates from starting once the unregister process has begun. >> */ >> void fpga_image_load_unregister(struct fpga_image_load *imgld) >> { >> + mutex_lock(&imgld->lock); >> + imgld->driver_unload = true; >> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) { >> + mutex_unlock(&imgld->lock); >> + goto unregister; >> + } >> + >> + mutex_unlock(&imgld->lock); >> + wait_for_completion(&imgld->update_done); > You can cut update_done if wait_for_completion() is replaced with > flush_work() or cancel_work_sync(&imgld->work). Thanks! I'll use flush_work() and eliminate the update_done completion instance. Thanks for the suggestions! - Russ > >> + >> +unregister: >> + cdev_del(&imgld->cdev); >> device_unregister(&imgld->dev); >> } >> EXPORT_SYMBOL_GPL(fpga_image_load_unregister);
The FPGA Image Load class driver provides an API to upload image files to an FPGA device. Image files are self-describing. They could contain FPGA images, BMC images, Root Entry Hashes, or other device specific files. It is up to the lower-level device driver and the target device to authenticate and disposition the file data. The n3000bmc-secure driver is the first driver to use the FPGA Security Manager. This driver was previously submitted in the same patch set, but was split into a separate patch set starting with v2. Changelog v14 -> v15: - Changed the driver name from FPGA Security Manager to FPGA Image Load - Changed file, symbol, and config names to reflect the new driver name - Rewrote documentation. - Removed all sysfs files except for "status", which has moved to the parent directory. - Implemented FPGA_IMAGE_LOAD_WRITE, FPGA_IMAGE_LOAD_STATUS, and FPGA_IMAGE_LOAD_CANCEL IOCTLs to initiate, monitor, and cancel image uploads. - Fixed some error return values in fpga_image_load_register() - Added an eventfd which is signalled upon completion of an image load. - Minor changes to locking to accommodate the FPGA_IMAGE_LOAD_STATUS IOCTL - Removed signed-off/reviewed-by tags. Please resend as appropriate. Changelog v13 -> v14: - Dropped the patch: fpga: sec-mgr: expose hardware error info - Updated copyrights to 2021 - Updated ABI documentation date and kernel version - Removed the name sysfs entry from the class driver - Use xa_alloc() instead of ida_simple_get() - Rename dev to parent for parent devices - Remove fpga_sec_mgr_create(), devm_fpga_sec_mgr_create(), and fpga_sec_mgr_free() functions and update the fpga_sec_mgr_register() function to both create and register a new security manager. - Populate the fpga_sec_mgr_dev_release() function. - Added MAINTAINERS reference for Documentation/ABI/testing/sysfs-class-fpga-sec-mgr Changelog v12 -> v13: - Change "if (count == 0 || " to "if (!count || " - Improve error message: "Attempt to register without all required ops\n" - Change set_error() to fpga_sec_set_error() - Change set_hw_errinfo() to fpga_sec_set_hw_errinfo() Changelog v11 -> v12: - Updated Date and KernelVersion fields in ABI documentation - Removed size parameter from write_blk() op - it is now up to the lower-level driver to determine the appropriate size and to update smgr->remaining_size accordingly. - Changed syntax of sec_mgr_prog_str[] and sec_mgr_err_str array definitions from: "idle", /* FPGA_SEC_PROG_IDLE */ to: [FPGA_SEC_PROG_IDLE] = "idle", Changelog v10 -> v11: - Fixed a spelling error in a comment - Initialize smgr->err_code and smgr->progress explicitly in fpga_sec_mgr_create() instead of accepting the default 0 value. Changelog v9 -> v10: - Rebased to 5.12-rc2 next - Updated Date and KernelVersion in ABI documentation Changelog v8 -> v9: - Rebased patches for 5.11-rc2 - Updated Date and KernelVersion in ABI documentation Changelog v7 -> v8: - Fixed grammatical error in Documentation/fpga/fpga-sec-mgr.rst Changelog v6 -> v7: - Changed dates in documentation file to December 2020 - Changed filename_store() to use kmemdup_nul() instead of kstrndup() and changed the count to not assume a line-return. Changelog v5 -> v6: - Removed sysfs support and documentation for the display of the flash count, root entry hashes, and code-signing-key cancellation vectors from the class driver. This information can vary by device and will instead be displayed by the device-specific parent driver. Changelog v4 -> v5: - Added the devm_fpga_sec_mgr_unregister() function, following recent changes to the fpga_manager() implementation. - Changed most of the *_show() functions to use sysfs_emit() instead of sprintf( - When checking the return values for functions of type enum fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0 Changelog v3 -> v4: - This driver is generic enough that it could be used for non Intel FPGA devices. Changed from "Intel FPGA Security Manager" to FPGA Security Manager" and removed unnecessary references to "Intel". - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_ Note that this also affects some filenames. Changelog v2 -> v3: - Use dev_err() to report invalid progress in sec_progress() - Use dev_err() to report invalid error code in sec_error() - Modified sysfs handler check in check_sysfs_handler() to make it more readable. - Removed unnecessary "goto done" - Added a comment to explain imgr->driver_unload in ifpga_sec_mgr_unregister() Changelog v1 -> v2: - Separated out the MAX10 BMC Security Engine to be submitted in a separate patch-set. - Bumped documentation dates and versions - Split ifpga_sec_mgr_register() into create() and register() functions - Added devm_ifpga_sec_mgr_create() - Added Documentation/fpga/ifpga-sec-mgr.rst - Changed progress state "read_file" to "reading" - Added sec_error() function (similar to sec_progress()) - Removed references to bmc_flash_count & smbus_flash_count (not supported) - Removed typedefs for imgr ops - Removed explicit value assignments in enums - Other minor code cleanup per review comments Russ Weight (6): fpga: image-load: fpga image load class driver fpga: image-load: enable image loads fpga: image-load: signal eventfd when complete fpga: image-load: add status ioctl fpga: image-load: create status sysfs node fpga: image-load: enable cancel of image upload .../ABI/testing/sysfs-class-fpga-image-load | 7 + Documentation/fpga/fpga-image-load.rst | 45 ++ Documentation/fpga/index.rst | 1 + MAINTAINERS | 10 + drivers/fpga/Kconfig | 10 + drivers/fpga/Makefile | 3 + drivers/fpga/fpga-image-load.c | 466 ++++++++++++++++++ include/linux/fpga/fpga-image-load.h | 67 +++ include/uapi/linux/fpga-image-load.h | 78 +++ 9 files changed, 687 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-image-load create mode 100644 Documentation/fpga/fpga-image-load.rst create mode 100644 drivers/fpga/fpga-image-load.c create mode 100644 include/linux/fpga/fpga-image-load.h create mode 100644 include/uapi/linux/fpga-image-load.h