Re: [PATCH v11 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Dec 03, 2021 at 08:37:44PM +0100, Rafael J. Wysocki wrote:
> On Sun, Nov 21, 2021 at 4:17 PM Chen Yu <yu.c.chen@xxxxxxxxx> wrote:
> >
> > Introduce the pfru_update driver which can be used for Platform Firmware
> > Runtime code injection and driver update [1].
> 
> The PFRU TLA already covers the "update" part. so the naming is a bit unclean.
> 
> IMO any of pfrut_update, or pfr_update, or even pfru alone would be better.
> 
> My favorite is pfr_update, and the telemetry part can be called
> pfr_telementry for symmetry.
>
Ok, will change it to this name. 
> > The user is expected to
> > provide the update firmware in the form of capsule file, and pass it to
> 
> This is an EFI capsule and not necessarily in a separate file, so why
> don't you just say "EFI capsule"?
>
ok, will do. 
> > the driver via ioctl.
> 
> It is actually passed by writing the capsule to a device special file, isn't it?
> 
Yes, it is, I'll revise it.
> The ioctl() interface is necessary for setup/cleanup.
> 
> > Then the driver would hand this capsule file to the
> > Platform Firmware Runtime Update via the ACPI device _DSM method. At last
> 
> "The capsule is transferred by the driver to the platform firmware
> with the help of an ACPI _DSM method under the special ACPI Platform
> Firmware Runtime Update device (INTC1080)"
>
Ok. 
> > the low level Management Mode would do the firmware update.
> 
> "and the actual firmware update is carried out by the low-level
> Management Mode code in the platform firmware."
>
Ok. 
> > The corresponding userspace tool and man page will be introduced at
> > tools/power/acpi/tools/pfru.
> 
> The above is irrelevant for this particular patch IMO.
> 
> What is relevant, and missing, is a bit of justification, like why it
> is a good idea to add this driver to the kernel.  There are some
> pieces of it in the Kconfig help text and elsewhere, but it should be
> there in the patch changelog too.
>
Ok, will add it in the changelog. 
> > Link: https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf # [1]
> > Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> > ---
> 
> [cut the history]
> 
> > ---
> >  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
> >  drivers/acpi/Kconfig                          |   1 +
> >  drivers/acpi/Makefile                         |   1 +
> >  drivers/acpi/pfru/Kconfig                     |  13 +
> >  drivers/acpi/pfru/Makefile                    |   2 +
> >  drivers/acpi/pfru/pfru_update.c               | 601 ++++++++++++++++++
> >  include/uapi/linux/pfru.h                     | 174 +++++
> >  7 files changed, 793 insertions(+)
> >  create mode 100644 drivers/acpi/pfru/Kconfig
> >  create mode 100644 drivers/acpi/pfru/Makefile
> >  create mode 100644 drivers/acpi/pfru/pfru_update.c
> >  create mode 100644 include/uapi/linux/pfru.h
> >
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index cfe6cccf0f44..adf830410e0d 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -367,6 +367,7 @@ Code  Seq#    Include File                                           Comments
> >                                                                       <mailto:aherrman@xxxxxxxxxx>
> >  0xE5  00-3F  linux/fuse.h
> >  0xEC  00-01  drivers/platform/chrome/cros_ec_dev.h                   ChromeOS EC driver
> > +0xEE  00-09  uapi/linux/pfru.h                                       Platform Firmware Runtime Update and Telemetry
> 
> If this is called pfrut.h, it would be more consistent with the description.
>
Ok, will change it. 
> >  0xF3  00-3F  drivers/usb/misc/sisusbvga/sisusb.h                     sisfb (in development)
> >                                                                       <mailto:thomas@xxxxxxxxxxxxxxxx>
> >  0xF6  all                                                            LTTng Linux Trace Toolkit Next Generation
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index cdbdf68bd98f..1cf26e404b63 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -482,6 +482,7 @@ source "drivers/acpi/nfit/Kconfig"
> >  source "drivers/acpi/numa/Kconfig"
> >  source "drivers/acpi/apei/Kconfig"
> >  source "drivers/acpi/dptf/Kconfig"
> > +source "drivers/acpi/pfru/Kconfig"
> >
> >  config ACPI_WATCHDOG
> >         bool
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index 3018714e87d9..9c2c5ddff6ec 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -102,6 +102,7 @@ obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o
> >  obj-$(CONFIG_ACPI_SPCR_TABLE)  += spcr.o
> >  obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
> >  obj-$(CONFIG_ACPI_PPTT)        += pptt.o
> > +obj-$(CONFIG_ACPI_PFRU)                += pfru/
> >
> >  # processor has its own "processor." module_param namespace
> >  processor-y                    := processor_driver.o
> > diff --git a/drivers/acpi/pfru/Kconfig b/drivers/acpi/pfru/Kconfig
> > new file mode 100644
> > index 000000000000..fd0a41b88fac
> > --- /dev/null
> > +++ b/drivers/acpi/pfru/Kconfig
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +config ACPI_PFRU
> 
> The telemetry will depend on the same config option, so I would call
> it ACPI_PFRUT.
>
Ok. 
> > +       tristate "ACPI Platform Firmware Runtime Update (PFRU)"
> 
> And I would simply say "ACPI Platform Firmware Runtime Update and
> Telemetry" here.
>
Ok. 
> > +       depends on 64BIT
> > +       help
> > +         In order to reduce the system reboot times and update the platform firmware
> > +         in time, Platform Firmware Runtime Update is leveraged to patch the system
> > +         without reboot. This driver supports Platform Firmware Runtime Update,
> > +         which is composed of two parts: code injection and driver update. It also
> > +         allows telemetry data to be retrieved from the platform firmware.
> 
> The above needs to be edited.  What about using something like this instead:
> 
> "This mechanism allows certain pieces of the platform firmware to be
> updated on the fly while the system is running (runtime) without the
> need to restart it, which is key in the cases when the system needs to
> be available 100% of the time and it cannot afford the downtime
> related to restarting it, or when the work carried out by the system
> is particularly important, so it cannot be interrupted, and it is not
> practical to wait until it is complete.
> 
> The existing firmware code can be modified (driver update) or extended
> by adding new code to the firmware (code injection)."
>
Ok, will change it. 
> The telemetry part will be added separately later, so it need not be
> mentioned here just yet.
>
Ok. 
> > +
> > +         To compile this driver as module, choose M here:
> > +         the modules will be called pfru_update and pfru_telemetry.
> > diff --git a/drivers/acpi/pfru/Makefile b/drivers/acpi/pfru/Makefile
> > new file mode 100644
> > index 000000000000..098cbe80cf3d
> > --- /dev/null
> > +++ b/drivers/acpi/pfru/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +obj-$(CONFIG_ACPI_PFRU) += pfru_update.o
> > diff --git a/drivers/acpi/pfru/pfru_update.c b/drivers/acpi/pfru/pfru_update.c
> > new file mode 100644
> > index 000000000000..cf670d052cc9
> > --- /dev/null
> > +++ b/drivers/acpi/pfru/pfru_update.c
> 
> So why don't you call this file pfr_update.c?
>
Ok, will change it. 
> > @@ -0,0 +1,601 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ACPI Platform Firmware Runtime Update Device Driver
> > + *
> > + * Copyright (C) 2021 Intel Corporation
> > + * Author: Chen Yu <yu.c.chen@xxxxxxxxx>
> 
> Please describe what the driver is for here at least briefly.
>
Ok, will do. 
> > + */
> > +#include <linux/acpi.h>
> > +#include <linux/device.h>
> > +#include <linux/efi.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/file.h>
> > +#include <linux/fs.h>
> > +#include <linux/idr.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/string.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/uio.h>
> > +#include <linux/uuid.h>
> > +
> > +#include <uapi/linux/pfru.h>
> > +
> > +#define PFRU_FUNC_STANDARD_QUERY       0
> > +#define PFRU_FUNC_QUERY_UPDATE_CAP     1
> > +#define PFRU_FUNC_QUERY_BUF            2
> > +#define PFRU_FUNC_START                3
> > +
> > +#define PFRU_CODE_INJECT_TYPE  1
> > +#define PFRU_DRIVER_UPDATE_TYPE        2
> > +
> > +#define PFRU_REVID_1           1
> > +#define PFRU_REVID_2           2
> > +#define PFRU_DEFAULT_REV_ID    PFRU_REVID_1
> > +
> > +enum cap_index {
> > +       CAP_STATUS_IDX = 0,
> > +       CAP_UPDATE_IDX = 1,
> > +       CAP_CODE_TYPE_IDX = 2,
> > +       CAP_FW_VER_IDX = 3,
> > +       CAP_CODE_RT_VER_IDX = 4,
> > +       CAP_DRV_TYPE_IDX = 5,
> > +       CAP_DRV_RT_VER_IDX = 6,
> > +       CAP_DRV_SVN_IDX = 7,
> > +       CAP_PLAT_ID_IDX = 8,
> > +       CAP_OEM_ID_IDX = 9,
> > +       CAP_OEM_INFO_IDX = 10,
> > +       CAP_NR_IDX
> > +};
> > +
> > +enum buf_index {
> > +       BUF_STATUS_IDX = 0,
> > +       BUF_EXT_STATUS_IDX = 1,
> > +       BUF_ADDR_LOW_IDX = 2,
> > +       BUF_ADDR_HI_IDX = 3,
> > +       BUF_SIZE_IDX = 4,
> > +       BUF_NR_IDX
> > +};
> > +
> > +enum update_index {
> > +       UPDATE_STATUS_IDX = 0,
> > +       UPDATE_EXT_STATUS_IDX = 1,
> > +       UPDATE_AUTH_TIME_LOW_IDX = 2,
> > +       UPDATE_AUTH_TIME_HI_IDX = 3,
> > +       UPDATE_EXEC_TIME_LOW_IDX = 4,
> > +       UPDATE_EXEC_TIME_HI_IDX = 5,
> > +       UPDATE_NR_IDX
> > +};
> > +
> > +enum pfru_start_action {
> 
> The pfru prefix here (and below) is OK.
> 
> > +       START_STAGE = 0,
> > +       START_ACTIVATE = 1,
> > +       START_STAGE_ACTIVATE = 2,
> > +};
> > +
> > +struct pfru_device {
> > +       u32 rev_id, index;
> > +       struct device *parent_dev;
> > +       struct miscdevice miscdev;
> > +};
> > +
> > +static DEFINE_IDA(pfru_ida);
> > +
> > +static const guid_t pfru_guid =
> > +       GUID_INIT(0xECF9533B, 0x4A3C, 0x4E89, 0x93, 0x9E, 0xC7, 0x71,
> > +                 0x12, 0x60, 0x1C, 0x6D);
> > +
> > +static const guid_t pfru_code_inj_guid =
> > +       GUID_INIT(0xB2F84B79, 0x7B6E, 0x4E45, 0x88, 0x5F, 0x3F, 0xB9,
> > +                 0xBB, 0x18, 0x54, 0x02);
> > +
> > +static const guid_t pfru_drv_update_guid =
> > +       GUID_INIT(0x4569DD8C, 0x75F1, 0x429A, 0xA3, 0xD6, 0x24, 0xDE,
> > +                 0x80, 0x97, 0xA0, 0xDF);
> 
> The above are _DSM UUIDs, right?
>
Yes. 
> If so, please add a comment pointing to their definitions and
> explaining briefly what they are for.
> 
Ok, will do.
> > +
> > +static inline int pfru_valid_revid(u32 id)
> > +{
> > +       return id == PFRU_REVID_1 || id == PFRU_REVID_2;
> > +}
> > +
> > +static inline struct pfru_device *to_pfru_dev(struct file *file)
> > +{
> > +       return container_of(file->private_data, struct pfru_device, miscdev);
> > +}
> > +
> > +static int query_capability(struct pfru_update_cap_info *cap_hdr,
> > +                           struct pfru_device *pfru_dev)
> > +{
> > +       acpi_handle handle = ACPI_HANDLE(pfru_dev->parent_dev);
> > +       union acpi_object *out_obj;
> > +       int ret = -EINVAL;
> > +
> > +       out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
> > +                                         pfru_dev->rev_id,
> > +                                         PFRU_FUNC_QUERY_UPDATE_CAP,
> > +                                         NULL, ACPI_TYPE_PACKAGE);
> > +       if (!out_obj)
> > +               return ret;
> > +
> > +       if (out_obj->package.count < CAP_NR_IDX)
> > +               goto free_acpi_buffer;
> > +
> > +       if (out_obj->package.elements[CAP_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       cap_hdr->status = out_obj->package.elements[CAP_STATUS_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[CAP_UPDATE_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       cap_hdr->update_cap = out_obj->package.elements[CAP_UPDATE_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[CAP_CODE_TYPE_IDX].type != ACPI_TYPE_BUFFER)
> > +               goto free_acpi_buffer;
> > +
> > +       memcpy(&cap_hdr->code_type,
> > +              out_obj->package.elements[CAP_CODE_TYPE_IDX].buffer.pointer,
> > +              out_obj->package.elements[CAP_CODE_TYPE_IDX].buffer.length);
> > +
> > +       if (out_obj->package.elements[CAP_FW_VER_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       cap_hdr->fw_version =
> > +               out_obj->package.elements[CAP_FW_VER_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[CAP_CODE_RT_VER_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       cap_hdr->code_rt_version =
> > +               out_obj->package.elements[CAP_CODE_RT_VER_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[CAP_DRV_TYPE_IDX].type != ACPI_TYPE_BUFFER)
> > +               goto free_acpi_buffer;
> > +
> > +       memcpy(&cap_hdr->drv_type,
> > +              out_obj->package.elements[CAP_DRV_TYPE_IDX].buffer.pointer,
> > +              out_obj->package.elements[CAP_DRV_TYPE_IDX].buffer.length);
> > +
> > +       if (out_obj->package.elements[CAP_DRV_RT_VER_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       cap_hdr->drv_rt_version =
> > +               out_obj->package.elements[CAP_DRV_RT_VER_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[CAP_DRV_SVN_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       cap_hdr->drv_svn =
> > +               out_obj->package.elements[CAP_DRV_SVN_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[CAP_PLAT_ID_IDX].type != ACPI_TYPE_BUFFER)
> > +               goto free_acpi_buffer;
> > +
> > +       memcpy(&cap_hdr->platform_id,
> > +              out_obj->package.elements[CAP_PLAT_ID_IDX].buffer.pointer,
> > +              out_obj->package.elements[CAP_PLAT_ID_IDX].buffer.length);
> > +
> > +       if (out_obj->package.elements[CAP_OEM_ID_IDX].type != ACPI_TYPE_BUFFER)
> > +               goto free_acpi_buffer;
> > +
> > +       memcpy(&cap_hdr->oem_id,
> > +              out_obj->package.elements[CAP_OEM_ID_IDX].buffer.pointer,
> > +              out_obj->package.elements[CAP_OEM_ID_IDX].buffer.length);
> > +
> > +       if (out_obj->package.elements[CAP_OEM_INFO_IDX].type != ACPI_TYPE_BUFFER)
> > +               goto free_acpi_buffer;
> > +
> > +       cap_hdr->oem_info_len =
> > +               out_obj->package.elements[CAP_OEM_INFO_IDX].buffer.length;
> > +
> > +       ret = 0;
> 
> The number of goto statements and possibly redundant memory updates
> can be reduced by doing all of the checks upfront (possibly in one big
> if () statement even).  And there would be fewer LOC then.
>
Ok, I'll compact this. 
> > +
> > +free_acpi_buffer:
> > +       kfree(out_obj);
> > +
> > +       return ret;
> > +}
> > +
> > +static int query_buffer(struct pfru_com_buf_info *info,
> > +                       struct pfru_device *pfru_dev)
> > +{
> > +       acpi_handle handle = ACPI_HANDLE(pfru_dev->parent_dev);
> > +       union acpi_object *out_obj;
> > +       int ret = -EINVAL;
> > +
> > +       out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
> > +                                         pfru_dev->rev_id, PFRU_FUNC_QUERY_BUF,
> > +                                         NULL, ACPI_TYPE_PACKAGE);
> > +       if (!out_obj)
> > +               return ret;
> > +
> > +       if (out_obj->package.count < BUF_NR_IDX)
> > +               goto free_acpi_buffer;
> > +
> > +       if (out_obj->package.elements[BUF_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       info->status = out_obj->package.elements[BUF_STATUS_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[BUF_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       info->ext_status =
> > +               out_obj->package.elements[BUF_EXT_STATUS_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[BUF_ADDR_LOW_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       info->addr_lo =
> > +               out_obj->package.elements[BUF_ADDR_LOW_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[BUF_ADDR_HI_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       info->addr_hi =
> > +               out_obj->package.elements[BUF_ADDR_HI_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[BUF_SIZE_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       info->buf_size = out_obj->package.elements[BUF_SIZE_IDX].integer.value;
> 
> The same comment applies to this function.
> 
> > +
> > +       ret = 0;
> > +
> > +free_acpi_buffer:
> > +       kfree(out_obj);
> > +
> > +       return ret;
> > +}
> > +
> > +static int get_image_type(const struct efi_manage_capsule_image_header *img_hdr,
> > +                         struct pfru_device *pfru_dev)
> > +{
> > +       const efi_guid_t *image_type_id = &img_hdr->image_type_id;
> > +
> > +       /* check whether this is a code injection or driver update */
> > +       if (guid_equal(image_type_id, &pfru_code_inj_guid))
> > +               return PFRU_CODE_INJECT_TYPE;
> > +
> > +       if (guid_equal(image_type_id, &pfru_drv_update_guid))
> > +               return PFRU_DRIVER_UPDATE_TYPE;
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +static int adjust_efi_size(const struct efi_manage_capsule_image_header *img_hdr,
> > +                          int size)
> > +{
> > +       /*
> > +        * The (u64 hw_ins) was introduced in UEFI spec version 2,
> > +        * and (u64 capsule_support) was introduced in version 3.
> > +        * The size needs to be adjusted accordingly. That is to
> > +        * say, version 1 should subtract the size of hw_ins+capsule_support,
> > +        * and version 2 should sbstract the size of capsule_support.
> > +        */
> > +       size += sizeof(struct efi_manage_capsule_image_header);
> > +       switch (img_hdr->ver) {
> > +       case 1:
> > +               return size - 2 * sizeof(u64);
> > +       case 2:
> > +               return size - sizeof(u64);
> > +       default:
> > +               /* only support version 1 and 2 */
> > +               return -EINVAL;
> > +       }
> > +}
> > +
> > +static bool valid_version(const void *data, struct pfru_update_cap_info *cap,
> > +                         struct pfru_device *pfru_dev)
> > +{
> > +       struct pfru_payload_hdr *payload_hdr;
> > +       const efi_capsule_header_t *cap_hdr = data;
> > +       const struct efi_manage_capsule_header *m_hdr;
> > +       const struct efi_manage_capsule_image_header *m_img_hdr;
> > +       const struct efi_image_auth *auth;
> > +       int type, size;
> > +
> > +       /*
> > +        * Sanity check if the capsule image has a newer version
> > +        * than current one. In this way, if the user provides an
> > +        * invalid capsule image, the kernel could be used as a
> > +        * guard to reject it, without switching to the MM update mode.
> > +        * (which might be costly)
> 
> So this really isn't "valid", but "applicable", so I would call the
> function applicable_image().
>
Ok. 
> And I would use a different comment text, like the following:
> 
> "If the code in the capsule is older than the current firmware code,
> the update will be rejected by the firmware, so check the version of
> it upfront without engaging the Management Mode update mechanism which
> may be costly."
> 
Ok, will do.
> > +        */
> > +       size = cap_hdr->headersize;
> > +       m_hdr = data + size;
> > +       /*
> > +        * Current data structure size plus variable array indicated
> > +        * by number of (emb_drv_cnt + payload_cnt)
> > +        */
> > +       size += offsetof(struct efi_manage_capsule_header, offset_list) +
> > +               (m_hdr->emb_drv_cnt + m_hdr->payload_cnt) * sizeof(u64);
> > +       m_img_hdr = data + size;
> > +
> > +       type = get_image_type(m_img_hdr, pfru_dev);
> > +       if (type < 0)
> > +               return false;
> > +
> > +       size = adjust_efi_size(m_img_hdr, size);
> > +       if (size < 0)
> > +               return false;
> > +
> > +       auth = data + size;
> > +       size += sizeof(u64) + auth->auth_info.hdr.len;
> > +       payload_hdr = (struct pfru_payload_hdr *)(data + size);
> > +
> > +       /* finally compare the version */
> > +       if (type == PFRU_CODE_INJECT_TYPE)
> > +               return payload_hdr->rt_ver >= cap->code_rt_version;
> > +       else
> 
> Redundant else.
> 
Will remove it.
> > +               return payload_hdr->rt_ver >= cap->drv_rt_version;
> > +}
> > +
> > +static void dump_update_result(struct pfru_updated_result *result,
> > +                              struct pfru_device *pfru_dev)
> 
> What about calling this print_ipdate_debug_info()?
> 
Ok, will do.
> > +{
> > +       dev_dbg(pfru_dev->parent_dev, "Update result:\n");
> > +       dev_dbg(pfru_dev->parent_dev, "Status:%d\n", result->status);
> > +       dev_dbg(pfru_dev->parent_dev, "Extended Status:%d\n", result->ext_status);
> > +       dev_dbg(pfru_dev->parent_dev, "Authentication Time Low:%lld\n",
> > +               result->low_auth_time);
> > +       dev_dbg(pfru_dev->parent_dev, "Authentication Time High:%lld\n",
> > +               result->high_auth_time);
> > +       dev_dbg(pfru_dev->parent_dev, "Execution Time Low:%lld\n",
> > +               result->low_exec_time);
> > +       dev_dbg(pfru_dev->parent_dev, "Execution Time High:%lld\n",
> > +               result->high_exec_time);
> > +}
> > +
> > +static int start_acpi_update(int action, struct pfru_device *pfru_dev)
> 
> Why not just start_update()?
> 
Ok, will change it.
> > +{
> > +       union acpi_object *out_obj, in_obj, in_buf;
> > +       struct pfru_updated_result update_result;
> > +       acpi_handle handle;
> > +       int ret = -EINVAL;
> > +
> > +       memset(&in_obj, 0, sizeof(in_obj));
> > +       memset(&in_buf, 0, sizeof(in_buf));
> > +       in_obj.type = ACPI_TYPE_PACKAGE;
> > +       in_obj.package.count = 1;
> > +       in_obj.package.elements = &in_buf;
> > +       in_buf.type = ACPI_TYPE_INTEGER;
> > +       in_buf.integer.value = action;
> > +
> > +       handle = ACPI_HANDLE(pfru_dev->parent_dev);
> > +       out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
> > +                                         pfru_dev->rev_id, PFRU_FUNC_START,
> > +                                         &in_obj, ACPI_TYPE_PACKAGE);
> > +       if (!out_obj)
> > +               return ret;
> > +
> > +       if (out_obj->package.count < UPDATE_NR_IDX)
> > +               goto free_acpi_buffer;
> > +
> > +       if (out_obj->package.elements[UPDATE_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       update_result.status =
> > +               out_obj->package.elements[UPDATE_STATUS_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[UPDATE_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       update_result.ext_status =
> > +               out_obj->package.elements[UPDATE_EXT_STATUS_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       update_result.low_auth_time =
> > +               out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       update_result.high_auth_time =
> > +               out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       update_result.low_exec_time =
> > +               out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].integer.value;
> > +
> > +       if (out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
> > +               goto free_acpi_buffer;
> > +
> > +       update_result.high_exec_time =
> > +               out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].integer.value;
> 
> Again, checks can be combined to reduce the number of goto statements etc.
> 
> > +
> > +       dump_update_result(&update_result, pfru_dev);
> > +       ret = 0;
> > +
> > +free_acpi_buffer:
> > +       kfree(out_obj);
> > +
> > +       return ret;
> > +}
> > +
> > +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > +       struct pfru_update_cap_info cap_hdr;
> > +       struct pfru_device *pfru_dev = to_pfru_dev(file);
> > +       void __user *p = (void __user *)arg;
> > +       u32 rev;
> > +       int ret;
> > +
> > +       switch (cmd) {
> > +       case PFRU_IOC_QUERY_CAP:
> > +               ret = query_capability(&cap_hdr, pfru_dev);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               if (copy_to_user(p, &cap_hdr, sizeof(cap_hdr)))
> > +                       return -EFAULT;
> > +
> > +               return 0;
> 
> I would add a blank line before each "case xxx:" line to visually
> separate the cases from each other.
> 
Ok, will do.
> > +       case PFRU_IOC_SET_REV:
> > +               if (copy_from_user(&rev, p, sizeof(rev)))
> > +                       return -EFAULT;
> > +
> > +               if (!pfru_valid_revid(rev))
> > +                       return -EINVAL;
> > +
> > +               pfru_dev->rev_id = rev;
> > +
> > +               return 0;
> > +       case PFRU_IOC_STAGE:
> > +               return start_acpi_update(START_STAGE, pfru_dev);
> > +       case PFRU_IOC_ACTIVATE:
> > +               return start_acpi_update(START_ACTIVATE, pfru_dev);
> > +       case PFRU_IOC_STAGE_ACTIVATE:
> > +               return start_acpi_update(START_STAGE_ACTIVATE, pfru_dev);
> > +       default:
> > +               return -ENOTTY;
> > +       }
> > +}
> > +
> > +static ssize_t pfru_write(struct file *file, const char __user *buf,
> > +                         size_t len, loff_t *ppos)
> > +{
> > +       struct pfru_device *pfru_dev = to_pfru_dev(file);
> > +       struct pfru_update_cap_info cap;
> > +       struct pfru_com_buf_info buf_info;
> > +       phys_addr_t phy_addr;
> > +       struct iov_iter iter;
> > +       struct iovec iov;
> > +       char *buf_ptr;
> > +       int ret;
> > +
> > +       ret = query_buffer(&buf_info, pfru_dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (len > buf_info.buf_size)
> > +               return -EINVAL;
> > +
> > +       iov.iov_base = (void __user *)buf;
> > +       iov.iov_len = len;
> > +       iov_iter_init(&iter, WRITE, &iov, 1, len);
> > +
> > +       /* map the communication buffer */
> > +       phy_addr = (phys_addr_t)((buf_info.addr_hi << 32) | buf_info.addr_lo);
> > +       buf_ptr = memremap(phy_addr, buf_info.buf_size, MEMREMAP_WB);
> > +       if (IS_ERR(buf_ptr))
> > +               return PTR_ERR(buf_ptr);
> > +
> > +       if (!copy_from_iter_full(buf_ptr, len, &iter)) {
> > +               ret = -EINVAL;
> > +               goto unmap;
> > +       }
> > +
> > +       /* check if the capsule header has a valid version number */
> > +       ret = query_capability(&cap, pfru_dev);
> > +       if (ret)
> > +               goto unmap;
> > +
> > +       if (cap.status != DSM_SUCCEED)
> > +               ret = -EBUSY;
> 
> Can this check be made by query_capability() and -EBUSY returned by it
> if the check triggers?
> 
Yes, I'll change it in next version.
> > +       else if (!valid_version(buf_ptr, &cap, pfru_dev))
> > +               ret = -EINVAL;
> > +
> > +unmap:
> > +       memunmap(buf_ptr);
> > +
> > +       return ret ?: len;
> > +}
> > +
> > +static const struct file_operations acpi_pfru_fops = {
> > +       .owner          = THIS_MODULE,
> > +       .write          = pfru_write,
> > +       .unlocked_ioctl = pfru_ioctl,
> > +       .llseek         = noop_llseek,
> > +};
> > +
> > +static int acpi_pfru_remove(struct platform_device *pdev)
> > +{
> > +       struct pfru_device *pfru_dev = platform_get_drvdata(pdev);
> > +
> > +       misc_deregister(&pfru_dev->miscdev);
> > +
> > +       return 0;
> > +}
> > +
> > +static void pfru_put_idx(void *data)
> > +{
> > +       struct pfru_device *pfru_dev = data;
> > +
> > +       ida_free(&pfru_ida, pfru_dev->index);
> > +}
> > +
> > +static int acpi_pfru_probe(struct platform_device *pdev)
> > +{
> > +       acpi_handle handle = ACPI_HANDLE(&pdev->dev);
> > +       struct pfru_device *pfru_dev;
> > +       int ret;
> > +
> > +       if (!acpi_has_method(handle, "_DSM")) {
> > +               dev_dbg(&pdev->dev, "Missing _DSM\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       pfru_dev = devm_kzalloc(&pdev->dev, sizeof(*pfru_dev), GFP_KERNEL);
> > +       if (!pfru_dev)
> > +               return -ENOMEM;
> > +
> > +       ret = ida_alloc(&pfru_ida, GFP_KERNEL);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       pfru_dev->index = ret;
> > +       ret = devm_add_action_or_reset(&pdev->dev, pfru_put_idx, pfru_dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       pfru_dev->rev_id = PFRU_DEFAULT_REV_ID;
> > +       pfru_dev->parent_dev = &pdev->dev;
> > +
> > +       pfru_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +       pfru_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > +                                               "pfru%d", pfru_dev->index);
> > +       if (!pfru_dev->miscdev.name)
> > +               return -ENOMEM;
> > +
> > +       pfru_dev->miscdev.nodename = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > +                                                   "acpi_pfru%d", pfru_dev->index);
> 
> So in accordance with the naming considerations above, I would call
> this devis "acpi_pfr_update".
> 
Ok, will do.
> > +       if (!pfru_dev->miscdev.nodename)
> > +               return -ENOMEM;
> > +
> > +       pfru_dev->miscdev.fops = &acpi_pfru_fops;
> > +       pfru_dev->miscdev.parent = &pdev->dev;
> > +
> > +       ret = misc_register(&pfru_dev->miscdev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       platform_set_drvdata(pdev, pfru_dev);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct acpi_device_id acpi_pfru_ids[] = {
> > +       {"INTC1080"},
> > +       {}
> > +};
> > +MODULE_DEVICE_TABLE(acpi, acpi_pfru_ids);
> > +
> > +static struct platform_driver acpi_pfru_driver = {
> > +       .driver = {
> > +               .name = "pfru_update",
> 
> "pfr_update" ?
> 
Ok.
> > +               .acpi_match_table = acpi_pfru_ids,
> > +       },
> > +       .probe = acpi_pfru_probe,
> > +       .remove = acpi_pfru_remove,
> > +};
> > +module_platform_driver(acpi_pfru_driver);
> > +
> > +MODULE_DESCRIPTION("Platform Firmware Runtime Update device driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/uapi/linux/pfru.h b/include/uapi/linux/pfru.h
> > new file mode 100644
> > index 000000000000..fed50cb01309
> > --- /dev/null
> > +++ b/include/uapi/linux/pfru.h
> > @@ -0,0 +1,174 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Platform Firmware Runtime Update header
> > + *
> > + * Copyright(c) 2021 Intel Corporation. All rights reserved.
> > + */
> > +#ifndef __PFRU_H__
> > +#define __PFRU_H__
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +#define PFRU_MAGIC_FOR_IOCTL 0xEE
> 
> If you follow my comments regarding the naming, this should be
> PFRUT_MAGIC_FOR_IOCTL, or even PFRUT_IOCTL_MAGIC for brevity.
> 
> 
Ok, will change it to PFRUT_IOCTL_MAGIC.
> > +
> > +/**
> > + * PFRU_IOC_SET_REV - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x01, unsigned int)
> > + *
> > + * Return:
> > + * * 0                 - success
> > + * * -EFAULT           - fail to read the revision id
> > + * * -EINVAL           - user provides an invalid revision id
> > + *
> > + * Set the Revision ID for PFRU Runtime Update.
> > + */
> > +#define PFRU_IOC_SET_REV _IOW(PFRU_MAGIC_FOR_IOCTL, 0x01, unsigned int)
> > +
> > +/**
> > + * PFRU_IOC_STAGE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x02, unsigned int)
> > + *
> > + * Return:
> > + * * 0                 - success
> > + * * -EINVAL           - stage phase returns invalid result
> > + *
> > + * Stage a capsule image from communication buffer and perform authentication.
> > + */
> > +#define PFRU_IOC_STAGE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x02, unsigned int)
> > +
> > +/**
> > + * PFRU_IOC_ACTIVATE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x03, unsigned int)
> > + *
> > + * Return:
> > + * * 0                 - success
> > + * * -EINVAL           - activate phase returns invalid result
> > + *
> > + * Activate a previous staged capsule image.
> 
> "previously"
> 
Ok.
> > + */
> > +#define PFRU_IOC_ACTIVATE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x03, unsigned int)
> > +
> > +/**
> > + * PFRU_IOC_STAGE_ACTIVATE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x04, unsigned int)
> > + *
> > + * Return:
> > + * * 0                 - success
> > + * * -EINVAL           - stage/activate phase returns invalid result.
> > + *
> > + * Perform both stage and activation action.
> > + */
> > +#define PFRU_IOC_STAGE_ACTIVATE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x04, unsigned int)
> > +
> > +/**
> > + * PFRU_IOC_QUERY_CAP - _IOR(PFRU_MAGIC_FOR_IOCTL, 0x05,
> > + *                          struct pfru_update_cap_info)
> > + *
> > + * Return:
> > + * * 0                 - success
> > + * * -EINVAL           - query phase returns invalid result
> > + * * -EFAULT           - the result fails to be copied to userspace
> > + *
> > + * Retrieve information about the PFRU Runtime Update capability.
> 
> "information on"
> 
Ok.
> > + * The information is a struct pfru_update_cap_info.
> > + */
> > +#define PFRU_IOC_QUERY_CAP _IOR(PFRU_MAGIC_FOR_IOCTL, 0x05, struct pfru_update_cap_info)
> > +
> > +/**
> > + * struct pfru_payload_hdr - Capsule file payload header.
> > + *
> > + * @sig: Signature of this capsule file.
> > + * @hdr_version: Revision of this header structure.
> > + * @hdr_size: Size of this header, including the OemHeader bytes.
> > + * @hw_ver: The supported firmware version.
> > + * @rt_ver: Version of the code injection image.
> > + * @platform_id: A platform specific GUID to specify the platform what
> > + *               this capsule image support.
> > + */
> > +struct pfru_payload_hdr {
> > +       __u32 sig;
> > +       __u32 hdr_version;
> > +       __u32 hdr_size;
> > +       __u32 hw_ver;
> > +       __u32 rt_ver;
> > +       __u8 platform_id[16];
> > +};
> > +
> > +enum pfru_dsm_status {
> > +       DSM_SUCCEED = 0,
> > +       DSM_FUNC_NOT_SUPPORT = 1,
> > +       DSM_INVAL_INPUT = 2,
> > +       DSM_HARDWARE_ERR = 3,
> > +       DSM_RETRY_SUGGESTED = 4,
> > +       DSM_UNKNOWN = 5,
> > +       DSM_FUNC_SPEC_ERR = 6,
> > +};
> > +
> > +/**
> > + * struct pfru_update_cap_info - Runtime update capability information.
> > + *
> > + * @status: Indicator of whether this query succeed.
> > + * @update_cap: Bitmap to indicate whether the feature is supported.
> > + * @code_type: A buffer containing an image type GUID.
> > + * @fw_version: Platform firmware version.
> > + * @code_rt_version: Code injection runtime version for anti-rollback.
> > + * @drv_type: A buffer containing an image type GUID.
> > + * @drv_rt_version: The version of the driver update runtime code.
> > + * @drv_svn: The secure version number(SVN) of the driver update runtime code.
> > + * @platform_id: A buffer containing a platform ID GUID.
> > + * @oem_id: A buffer containing an OEM ID GUID.
> > + * @oem_info_len: Length of the buffer containing the vendor specific information.
> > + */
> > +struct pfru_update_cap_info {
> > +       __u32 status;
> > +       __u32 update_cap;
> > +
> > +       __u8 code_type[16];
> > +       __u32 fw_version;
> > +       __u32 code_rt_version;
> > +
> > +       __u8 drv_type[16];
> > +       __u32 drv_rt_version;
> > +       __u32 drv_svn;
> > +
> > +       __u8 platform_id[16];
> > +       __u8 oem_id[16];
> > +
> > +       __u32 oem_info_len;
> > +};
> > +
> > +/**
> > + * struct pfru_com_buf_info - Communication buffer information.
> > + *
> > + * @status: Indicator of whether this query succeed.
> > + * @ext_status: Implementation specific query result.
> > + * @addr_lo: Low 32bit physical address of the communication buffer to hold
> > + *           a runtime update package.
> > + * @addr_hi: High 32bit physical address of the communication buffer to hold
> > + *           a runtime update package.
> > + * @buf_size: Maximum size in bytes of the communication buffer.
> > + */
> > +struct pfru_com_buf_info {
> > +       __u32 status;
> > +       __u32 ext_status;
> > +       __u64 addr_lo;
> > +       __u64 addr_hi;
> > +       __u32 buf_size;
> > +};
> > +
> > +/**
> > + * struct pfru_updated_result - Platform firmware runtime update result information.
> > + * @status: Indicator of whether this update succeed.
> > + * @ext_status: Implementation specific update result.
> > + * @low_auth_time: Low 32bit value of image authentication time in nanosecond.
> > + * @high_auth_time: High 32bit value of image authentication time in nanosecond.
> > + * @low_exec_time: Low 32bit value of image execution time in nanosecond.
> > + * @high_exec_time: High 32bit value of image execution time in nanosecond.
> > + */
> > +struct pfru_updated_result {
> > +       __u32 status;
> > +       __u32 ext_status;
> > +       __u64 low_auth_time;
> > +       __u64 high_auth_time;
> > +       __u64 low_exec_time;
> > +       __u64 high_exec_time;
> > +};
> > +
> > +#endif /* __PFRU_H__ */
> > --
> 
> Overall, I think that the design is fine now, it just needs to be made nicer.
> 
> [Please wait for my comments on the next patch before sending a new version.]

Ok, will wait for it. Thanks!

Best,
Chenyu



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux