Re: [RFC 04/10] platform/x86/intel/ifs: Load IFS Image
From: Williams, Dan J
Date: Wed Mar 02 2022 - 21:58:30 EST
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@xxxxxxxxx>
> Signed-off-by: Jithu Joseph <jithu.joseph@xxxxxxxxx>
Sender signoff last, please.
> Reviewed-by: Ashok Raj <ashok.raj@xxxxxxxxx>
> Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> + */
> +
> +#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.