Re: [RFC 04/10] platform/x86/intel/ifs: Load IFS Image

["Williams, Dan J" <dan.j.williams@intel.com>]

On Tue, 2022-03-01 at 11:54 -0800, Jithu Joseph wrote:
> IFS uses a test image that can be regarded as firmware. The image is
> specific to a processor family, model and stepping. IFS requires that a
> test image be loaded before any ifs test is initiated. Load the image
> that matches processor signature. The IFS image is signed by Intel.
> 
> The IFS image file follows a similar naming convention as used for
> Intel CPU microcode files. The file must be located in the firmware
> directory where the microcode files are placed and named as {family/model
> /stepping}.scan as below:
> 
> /lib/firmware/intel/ifs/{ff-mm-ss}.scan
> 
> Originally-by: Kyung Min Park <kyung.min.park@intel.com>
> Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>

Sender signoff last, please.

> Reviewed-by: Ashok Raj <ashok.raj@intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/platform/x86/intel/ifs/Makefile |  2 +-
>  drivers/platform/x86/intel/ifs/core.c   |  8 +++
>  drivers/platform/x86/intel/ifs/ifs.h    | 13 ++++
>  drivers/platform/x86/intel/ifs/load.c   | 95 +++++++++++++++++++++++++
>  4 files changed, 117 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/platform/x86/intel/ifs/load.c
> 
> diff --git a/drivers/platform/x86/intel/ifs/Makefile b/drivers/platform/x86/intel/ifs/Makefile
> index 05b4925402b4..105b377de410 100644
> --- a/drivers/platform/x86/intel/ifs/Makefile
> +++ b/drivers/platform/x86/intel/ifs/Makefile
> @@ -4,4 +4,4 @@
>  
>  obj-$(CONFIG_INTEL_IFS)                        += intel_ifs.o
>  
> -intel_ifs-objs                         := core.o
> +intel_ifs-objs                         := core.o load.o
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index fb3c864d3085..765d9a2c4683 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -8,6 +8,7 @@
>  #include <asm/cpu_device_id.h>
>  
>  #include "ifs.h"
> +struct ifs_params ifs_params;
>  
>  #define X86_MATCH(model)                                       \
>         X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6,            \
> @@ -24,6 +25,7 @@ static int __init ifs_init(void)
>  {
>         const struct x86_cpu_id *m;
>         u64 ia32_core_caps;
> +       int ret;
>  
>         /* ifs capability check */
>         m = x86_match_cpu(ifs_cpu_ids);
> @@ -34,6 +36,12 @@ static int __init ifs_init(void)
>         if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_INTEGRITY))
>                 return -ENODEV;
>  
> +       ret = load_ifs_binary();
> +       if (ret) {
> +               pr_err("loading ifs binaries failed\n");

What's wrong the error code returned to echo, why spam the log?

Similar comment I forgot to add on the pr_info() upon unloading the
module in the previous patch. What's wrong with the error code returned
to "modprobe -r"?

> +               return ret;
> +       }
> +
>         return 0;
>  }
>  
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index f3f924fced06..f2daf2cfd3e6 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -7,8 +7,21 @@
>  #ifndef _IFS_H_
>  #define _IFS_H_
>  
> +#undef pr_fmt
> +#define pr_fmt(fmt) "ifs: " fmt

It's unfortunate that this is needed when dev_{err,info,dbg} family of
functions would scope the messages appropriately automatically. If only
there was a 'struct device' this driver could reference. More on this
below...

> +
>  /* These bits are in the IA32_CORE_CAPABILITIES MSR */
>  #define MSR_IA32_CORE_CAPS_INTEGRITY_BIT       2
>  #define MSR_IA32_CORE_CAPS_INTEGRITY           BIT(MSR_IA32_CORE_CAPS_INTEGRITY_BIT)
>  
> +/**
> + * struct ifs_params - global ifs parameter for all cpus.
> + * @loaded_version: stores the currently loaded ifs image version.
> + */
> +struct ifs_params {
> +       int loaded_version;
> +};
> +
> +int load_ifs_binary(void);
> +extern struct ifs_params ifs_params;
>  #endif
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> new file mode 100644
> index 000000000000..1a5e906c51af
> --- /dev/null
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2021 Intel Corporation.
> + *
> + * Author: Jithu Joseph <jithu.joseph@intel.com>
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/platform_device.h>
> +
> +#include "ifs.h"
> +static const char *ifs_path = "intel/ifs/";
> +
> +struct ifs_header {
> +       u32 header_ver;
> +       u32 blob_revision;
> +       u32 date;
> +       u32 processor_sig;
> +       u32 check_sum;
> +       u32 loader_rev;
> +       u32 processor_flags;
> +       u32 metadata_size;
> +       u32 total_size;
> +       u32 fusa_info;
> +       u64 reserved;
> +};
> +
> +#define IFS_HEADER_SIZE        (sizeof(struct ifs_header))
> +static struct ifs_header *ifs_header_ptr;      /* pointer to the ifs image header */
> +static u64 ifs_hash_ptr;                       /* Address of ifs metadata (hash) */
> +
> +static const struct firmware *load_binary(const char *path)
> +{
> +       struct platform_device *ifs_pdev;
> +       const struct firmware *fw;
> +       int err;
> +
> +       ifs_pdev = platform_device_register_simple("ifs", -1, NULL, 0);

This looks like an abuse of the platform_device_register_simple() API
to me, i.e. to register and tear down a device every time someone
echoes a value to a sysfs file. This registration process fires off a
KOBJ_ADD event to tell udev a new device has appeared, and before udev
scripts have a chance to do anything useful this device is gone again.
If I were someone that wanted to automate testing my CPUs as uevent
notifying me when the "ifs" device has arrived would be useful.

If ifs_init() registered a platform device to represent the ifs
interface it would also provide a private place to hang the ifs sysfs
interface rather than glomming onto the /sys/devices/system/cpu core
ABI. I personally don't think ifs functionality belongs under
/sys/devices/system/cpu. Also, with statically defined sysfs attributes
automation scripts would be able to safely assume that the sysfs
interface is ready coincident with the KOBJ_ADD event.