| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bbcb902c-49e4-75e1-6e8e-4e791d8c6537@xilinx.com>
Date: Mon, 25 Oct 2021 17:07:02 -0700
From: Lizhi Hou <lizhi.hou@...inx.com>
To: Russ Weight <russell.h.weight@...el.com>,
Lizhi Hou <lizhi.hou@...inx.com>, <mdf@...nel.org>,
<linux-fpga@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC: <trix@...hat.com>, <lgoncalv@...hat.com>, <yilun.xu@...el.com>,
<hao.wu@...el.com>, <matthew.gerlach@...el.com>
Subject: Re: [PATCH v17 4/5] fpga: image-load: add status ioctl
On 10/20/21 2:42 PM, Russ Weight wrote:
> On 10/15/21 1:22 PM, Lizhi Hou wrote:
>> On 9/29/21 4:00 PM, Russ Weight wrote:
>>> Extend the FPGA Image Load framework to include an FPGA_IMAGE_LOAD_STATUS
>>> IOCTL that can be used to monitor the progress of an ongoing image upload.
>>> The status returned includes how much data remains to be transferred, the
>>> progress of the image upload, and error information in the case of a
>>> failure.
>>>
>>> Signed-off-by: Russ Weight <russell.h.weight@...el.com>
>>> ---
>>> v17:
>>> - Rebased for changes to earlier patches.
>>> v16:
>>> - Minor changes to adapt in changes in prevoius patches.
>>> v15:
>>> - This patch is new to the patchset and provides an FPGA_IMAGE_LOAD_STATUS
>>> IOCTL to return the current values for: remaining_size, progress,
>>> err_progress, and err_code.
>>> - This patch has elements of the following three patches from the previous
>>> patch-set:
>>> [PATCH v14 3/6] fpga: sec-mgr: expose sec-mgr update status
>>> [PATCH v14 4/6] fpga: sec-mgr: expose sec-mgr update errors
>>> [PATCH v14 5/6] fpga: sec-mgr: expose sec-mgr update size
>>> - Changed file, symbol, and config names to reflect the new driver name
>>> - There are some minor changes to locking to enable this ioctl to return
>>> coherent data.
>>> ---
>>> Documentation/fpga/fpga-image-load.rst | 6 +++
>>> drivers/fpga/fpga-image-load.c | 58 +++++++++++++++++++++-----
>>> include/linux/fpga/fpga-image-load.h | 1 +
>>> include/uapi/linux/fpga-image-load.h | 18 ++++++++
>>> 4 files changed, 73 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
>>> index 487b5466f67c..f64f5ee473b8 100644
>>> --- a/Documentation/fpga/fpga-image-load.rst
>>> +++ b/Documentation/fpga/fpga-image-load.rst
>>> @@ -33,3 +33,9 @@ being updated. This is an exclusive operation; an attempt to start
>>> concurrent image uploads for the same device will fail with EBUSY. An
>>> eventfd file descriptor parameter is provided to this IOCTL. It will be
>>> signalled at the completion of the image upload.
>>> +
>>> +FPGA_IMAGE_LOAD_STATUS:
>>> +
>>> +Collect status for an on-going image upload. The status returned includes
>>> +how much data remains to be transferred, the progress of the image upload,
>>> +and error information in the case of a failure.
>>> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
>>> index f04dfc71c190..58373b9e8c02 100644
>>> --- a/drivers/fpga/fpga-image-load.c
>>> +++ b/drivers/fpga/fpga-image-load.c
>>> @@ -22,6 +22,22 @@ static dev_t fpga_image_devt;
>>>
>>> #define to_image_load(d) container_of(d, struct fpga_image_load, dev)
>>>
>>> +static void fpga_image_update_progress(struct fpga_image_load *imgld,
>>> + u32 new_progress)
>>> +{
>>> + mutex_lock(&imgld->lock);
>>> + imgld->progress = new_progress;
>>> + mutex_unlock(&imgld->lock);
>>> +}
>>> +
>>> +static void fpga_image_set_error(struct fpga_image_load *imgld, u32 err_code)
>>> +{
>>> + mutex_lock(&imgld->lock);
>>> + imgld->err_progress = imgld->progress;
>>> + imgld->err_code = err_code;
>>> + mutex_unlock(&imgld->lock);
>>> +}
>>> +
>>> static void fpga_image_prog_complete(struct fpga_image_load *imgld)
>>> {
>>> mutex_lock(&imgld->lock);
>>> @@ -38,24 +54,24 @@ static void fpga_image_do_load(struct work_struct *work)
>>> imgld = container_of(work, struct fpga_image_load, work);
>>>
>>> if (imgld->driver_unload) {
>>> - imgld->err_code = FPGA_IMAGE_ERR_CANCELED;
>>> + fpga_image_set_error(imgld, FPGA_IMAGE_ERR_CANCELED);
>>> goto idle_exit;
>>> }
>>>
>>> get_device(&imgld->dev);
>>> if (!try_module_get(imgld->dev.parent->driver->owner)) {
>>> - imgld->err_code = FPGA_IMAGE_ERR_BUSY;
>>> + fpga_image_set_error(imgld, FPGA_IMAGE_ERR_BUSY);
>>> goto putdev_exit;
>>> }
>>>
>>> - imgld->progress = FPGA_IMAGE_PROG_PREPARING;
>>> + fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PREPARING);
>>> ret = imgld->ops->prepare(imgld, imgld->data, imgld->remaining_size);
>>> if (ret) {
>>> - imgld->err_code = ret;
>>> + fpga_image_set_error(imgld, ret);
>>> goto modput_exit;
>>> }
>>>
>>> - imgld->progress = FPGA_IMAGE_PROG_WRITING;
>>> + fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_WRITING);
>>> while (imgld->remaining_size) {
>>> ret = imgld->ops->write(imgld, imgld->data, offset,
>>> imgld->remaining_size);
>>> @@ -65,7 +81,7 @@ static void fpga_image_do_load(struct work_struct *work)
>>> "write-op wrote zero data\n");
>>> ret = -FPGA_IMAGE_ERR_RW_ERROR;
>>> }
>>> - imgld->err_code = -ret;
>>> + fpga_image_set_error(imgld, -ret);
>>> goto done;
>>> }
>>>
>>> @@ -73,10 +89,10 @@ static void fpga_image_do_load(struct work_struct *work)
>>> offset += ret;
>>> }
>>>
>>> - imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
>>> + fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PROGRAMMING);
>>> ret = imgld->ops->poll_complete(imgld);
>>> if (ret)
>>> - imgld->err_code = ret;
>>> + fpga_image_set_error(imgld, ret);
>>>
>>> done:
>>> if (imgld->ops->cleanup)
>>> @@ -151,20 +167,42 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>>> return ret;
>>> }
>>>
>>> +static int fpga_image_load_ioctl_status(struct fpga_image_load *imgld,
>>> + unsigned long arg)
>>> +{
>>> + struct fpga_image_status status;
>>> +
>>> + memset(&status, 0, sizeof(status));
>>> + status.progress = imgld->progress;
>>> + status.remaining_size = imgld->remaining_size;
>>> + status.err_progress = imgld->err_progress;
>>> + status.err_code = imgld->err_code;
>>> +
>>> + if (copy_to_user((void __user *)arg, &status, sizeof(status)))
>>> + return -EFAULT;
>>> +
>>> + 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;
>>>
>>> + mutex_lock(&imgld->lock);
>>> +
>>> switch (cmd) {
>>> case FPGA_IMAGE_LOAD_WRITE:
>>> - mutex_lock(&imgld->lock);
>>> ret = fpga_image_load_ioctl_write(imgld, arg);
>>> - mutex_unlock(&imgld->lock);
>>> + break;
>>> + case FPGA_IMAGE_LOAD_STATUS:
>>> + ret = fpga_image_load_ioctl_status(imgld, arg);
>>> break;
>>> }
>>>
>>> + mutex_unlock(&imgld->lock);
>>> +
>>> return ret;
>>> }
>>>
>>> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
>>> index 77b3c91ce073..366111d090fb 100644
>>> --- a/include/linux/fpga/fpga-image-load.h
>>> +++ b/include/linux/fpga/fpga-image-load.h
>>> @@ -49,6 +49,7 @@ struct fpga_image_load {
>>> const u8 *data; /* pointer to update data */
>>> u32 remaining_size; /* size remaining to transfer */
>>> u32 progress;
>>> + u32 err_progress; /* progress at time of error */
>>> u32 err_code; /* image load error code */
>>> bool driver_unload;
>>> struct eventfd_ctx *finished;
>>> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
>>> index 8d2d3db92e87..1b91343961df 100644
>>> --- a/include/uapi/linux/fpga-image-load.h
>>> +++ b/include/uapi/linux/fpga-image-load.h
>>> @@ -51,4 +51,22 @@ struct fpga_image_write {
>>>
>>> #define FPGA_IMAGE_LOAD_WRITE _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
>>>
>>> +/**
>>> + * FPGA_IMAGE_LOAD_STATUS - _IOR(FPGA_IMAGE_LOAD_MAGIC, 1,
>>> + * struct fpga_image_status)
>>> + *
>>> + * Request status information for an ongoing update.
>>> + *
>>> + * Return: 0 on success, -errno on failure.
>>> + */
>>> +struct fpga_image_status {
>>> + /* Output */
>>> + __u32 remaining_size; /* size remaining to transfer */
>>> + __u32 progress; /* current progress of image load */
>>> + __u32 err_progress; /* progress at time of error */
>>> + __u32 err_code; /* error code */
>>> +};
>> Could this be extended to also collect the image detail?
>>
>> Image version, name, etc been successfully written to device (flash).
>>
>> Image version, name, etc is currently running on the device.
>>
>> Or maybe add another query command to do these?
>>
>> So the userland utility will be able to show what image is running and what image is going to run with next cold reboot.
> This status call is intended specifically to show the progress and
> status of the image load. Yilun's comments on patch 5 suggest that
> some of these types of operations could be handled as part of the
> fpga-region class driver
>
> I have patches that have not been submitted yet that contain some
> of these types of features, although not in the class driver (yet).
> Here is a link to documentation for what I have now. Take a look
> at the sysfs node descriptions that are in the control subdirectory.
> These are listed at the bottom of the file.
>
> https://github.com/OPAE/linux-dfl/blob/fpga-ofs-dev/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
Thanks a lot. This is very helpful.
Lizhi
>
> - Russ
>
>
>>
>> Thanks,
>>
>> Lizhi
>>
>>> +
>>> +#define FPGA_IMAGE_LOAD_STATUS _IOR(FPGA_IMAGE_LOAD_MAGIC, 1, struct fpga_image_status)
>>> +
>>> #endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
>>> --
>>> 2.25.1
>>>
Powered by blists - more mailing lists