On Tue, Apr 19, 2022 at 09:38:51AM -0700, Tony Luck wrote:
> The initial implementation of IFS is model specific. Enumeration is
> via a combination of family-model-stepping and a check for a bit in the
> CORE_CAPABILITIES MSR.
>
> Linux has handled this lack of enumeration before with a code stub to
> create a device. See arch/x86/kernel/pmem.c. Use the same approach
> here.
Ick, why? Why not just create a simple virtual device and use that? Do
you really want to bind a driver to this? Or do you already "know" the
only driver that you have will bind to this?
pmem.c should not be used as a good example of anything, sorry.
greg k-h
>
> Reviewed-by: Dan Williams <[email protected]>
> Signed-off-by: Tony Luck <[email protected]>
> ---
> MAINTAINERS | 7 +++
> drivers/platform/x86/intel/Kconfig | 1 +
> drivers/platform/x86/intel/Makefile | 1 +
> drivers/platform/x86/intel/ifs/Kconfig | 2 +
> drivers/platform/x86/intel/ifs/Makefile | 1 +
> .../platform/x86/intel/ifs/intel_ifs_device.c | 50 +++++++++++++++++++
> 6 files changed, 62 insertions(+)
> create mode 100644 drivers/platform/x86/intel/ifs/Kconfig
> create mode 100644 drivers/platform/x86/intel/ifs/Makefile
> create mode 100644 drivers/platform/x86/intel/ifs/intel_ifs_device.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 40fa1955ca3f..9e372a960fa5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9861,6 +9861,13 @@ B: https://bugzilla.kernel.org
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git
> F: drivers/idle/intel_idle.c
>
> +INTEL IN FIELD SCAN (IFS) DRIVER
> +M: Jithu Joseph <[email protected]>
> +R: Ashok Raj <[email protected]>
> +R: Tony Luck <[email protected]>
> +S: Maintained
> +F: drivers/platform/x86/intel/ifs
> +
> INTEL INTEGRATED SENSOR HUB DRIVER
> M: Srinivas Pandruvada <[email protected]>
> M: Jiri Kosina <[email protected]>
> diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
> index 1f01a8a23c57..794968bda115 100644
> --- a/drivers/platform/x86/intel/Kconfig
> +++ b/drivers/platform/x86/intel/Kconfig
> @@ -4,6 +4,7 @@
> #
>
> source "drivers/platform/x86/intel/atomisp2/Kconfig"
> +source "drivers/platform/x86/intel/ifs/Kconfig"
> source "drivers/platform/x86/intel/int1092/Kconfig"
> source "drivers/platform/x86/intel/int3472/Kconfig"
> source "drivers/platform/x86/intel/pmc/Kconfig"
> diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
> index c61bc3e97121..10285d0fd16a 100644
> --- a/drivers/platform/x86/intel/Makefile
> +++ b/drivers/platform/x86/intel/Makefile
> @@ -5,6 +5,7 @@
> #
>
> obj-$(CONFIG_INTEL_ATOMISP2_PDX86) += atomisp2/
> +obj-y += ifs/
> obj-$(CONFIG_INTEL_SAR_INT1092) += int1092/
> obj-$(CONFIG_INTEL_SKL_INT3472) += int3472/
> obj-$(CONFIG_INTEL_PMC_CORE) += pmc/
> diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig
> new file mode 100644
> index 000000000000..51325b699563
> --- /dev/null
> +++ b/drivers/platform/x86/intel/ifs/Kconfig
> @@ -0,0 +1,2 @@
> +config INTEL_IFS_DEVICE
> + bool
> diff --git a/drivers/platform/x86/intel/ifs/Makefile b/drivers/platform/x86/intel/ifs/Makefile
> new file mode 100644
> index 000000000000..12c2f5ce9925
> --- /dev/null
> +++ b/drivers/platform/x86/intel/ifs/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_INTEL_IFS_DEVICE) += intel_ifs_device.o
> diff --git a/drivers/platform/x86/intel/ifs/intel_ifs_device.c b/drivers/platform/x86/intel/ifs/intel_ifs_device.c
> new file mode 100644
> index 000000000000..64a143871d72
> --- /dev/null
> +++ b/drivers/platform/x86/intel/ifs/intel_ifs_device.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2022 Intel Corporation. */
> +
> +#include <linux/platform_device.h>
> +#include <linux/init.h>
> +#include <asm/cpu_device_id.h>
> +
> +#define MSR_IA32_CORE_CAPS_INTEGRITY_BIT 2
> +#define MSR_IA32_CORE_CAPS_INTEGRITY BIT(MSR_IA32_CORE_CAPS_INTEGRITY_BIT)
> +
> +#define X86_MATCH(model) \
> + X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6, \
> + INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL)
> +
> +static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
> + X86_MATCH(SAPPHIRERAPIDS_X),
> + {}
> +};
> +
> +static __init int register_ifs_device(void)
> +{
> + struct platform_device *pdev;
> + const struct x86_cpu_id *m;
> + u64 ia32_core_caps;
> +
> + m = x86_match_cpu(ifs_cpu_ids);
> + if (!m)
> + return -ENODEV;
> +
> + if (rdmsrl_safe(MSR_IA32_CORE_CAPS, &ia32_core_caps))
> + return -ENODEV;
> +
> + if (ia32_core_caps & MSR_IA32_CORE_CAPS_INTEGRITY) {
> + pdev = platform_device_alloc("intel_ifs", 0);
> + if (pdev) {
> + if (platform_device_add(pdev))
> + platform_device_put(pdev);
> + }
> + }
> +
> + /*
> + * Failure here will be visible by a missing device
> + * in sysfs. Returning an error code would not make
> + * that any easier to diagnose. Would also complicate
> + * future implementations that may support a subset of
> + * the types of tests.
> + */
> + return 0;
So even if everything fails, you succeed? But you are failing above for
some cases, so why is creating the device somehow special here that you
should succeed no matter what?
thanks,
greg k-h
On Tue, Apr 19, 2022 at 9:48 AM Greg KH <[email protected]> wrote:
>
> On Tue, Apr 19, 2022 at 09:38:51AM -0700, Tony Luck wrote:
> > The initial implementation of IFS is model specific. Enumeration is
> > via a combination of family-model-stepping and a check for a bit in the
> > CORE_CAPABILITIES MSR.
> >
> > Linux has handled this lack of enumeration before with a code stub to
> > create a device. See arch/x86/kernel/pmem.c. Use the same approach
> > here.
>
> Ick, why? Why not just create a simple virtual device and use that? Do
> you really want to bind a driver to this? Or do you already "know" the
> only driver that you have will bind to this?
With the realization that there may be multiple instances of an
IFS-like capability going forward, and that ideally those capabilities
would move away from a CPU capability bit to an ACPI description, then
it seemed to me that a simulated platform_device for this is a
reasonable fit. I.e. when / if an ACPI _HID is assigned for this
capability the same platform_driver can be reused for those instances.
> pmem.c should not be used as a good example of anything, sorry.
Yes, the arch/x86/kernel/pmem.c hack was supplanted by an ACPI device
description. There is no ACPI device description for the IFS
capability, yet.
So I saw these two cases as similar, that capabilities like this need
enumeration besides a CPU-id bit or an E820 table entry, and when they
move to an enumerable bus like ACPI a platform_driver is expected.
>
> greg k-h
>
>
> >
> > Reviewed-by: Dan Williams <[email protected]>
> > Signed-off-by: Tony Luck <[email protected]>
> > ---
> > MAINTAINERS | 7 +++
> > drivers/platform/x86/intel/Kconfig | 1 +
> > drivers/platform/x86/intel/Makefile | 1 +
> > drivers/platform/x86/intel/ifs/Kconfig | 2 +
> > drivers/platform/x86/intel/ifs/Makefile | 1 +
> > .../platform/x86/intel/ifs/intel_ifs_device.c | 50 +++++++++++++++++++
> > 6 files changed, 62 insertions(+)
> > create mode 100644 drivers/platform/x86/intel/ifs/Kconfig
> > create mode 100644 drivers/platform/x86/intel/ifs/Makefile
> > create mode 100644 drivers/platform/x86/intel/ifs/intel_ifs_device.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 40fa1955ca3f..9e372a960fa5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9861,6 +9861,13 @@ B: https://bugzilla.kernel.org
> > T: git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git
> > F: drivers/idle/intel_idle.c
> >
> > +INTEL IN FIELD SCAN (IFS) DRIVER
> > +M: Jithu Joseph <[email protected]>
> > +R: Ashok Raj <[email protected]>
> > +R: Tony Luck <[email protected]>
> > +S: Maintained
> > +F: drivers/platform/x86/intel/ifs
> > +
> > INTEL INTEGRATED SENSOR HUB DRIVER
> > M: Srinivas Pandruvada <[email protected]>
> > M: Jiri Kosina <[email protected]>
> > diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
> > index 1f01a8a23c57..794968bda115 100644
> > --- a/drivers/platform/x86/intel/Kconfig
> > +++ b/drivers/platform/x86/intel/Kconfig
> > @@ -4,6 +4,7 @@
> > #
> >
> > source "drivers/platform/x86/intel/atomisp2/Kconfig"
> > +source "drivers/platform/x86/intel/ifs/Kconfig"
> > source "drivers/platform/x86/intel/int1092/Kconfig"
> > source "drivers/platform/x86/intel/int3472/Kconfig"
> > source "drivers/platform/x86/intel/pmc/Kconfig"
> > diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
> > index c61bc3e97121..10285d0fd16a 100644
> > --- a/drivers/platform/x86/intel/Makefile
> > +++ b/drivers/platform/x86/intel/Makefile
> > @@ -5,6 +5,7 @@
> > #
> >
> > obj-$(CONFIG_INTEL_ATOMISP2_PDX86) += atomisp2/
> > +obj-y += ifs/
> > obj-$(CONFIG_INTEL_SAR_INT1092) += int1092/
> > obj-$(CONFIG_INTEL_SKL_INT3472) += int3472/
> > obj-$(CONFIG_INTEL_PMC_CORE) += pmc/
> > diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig
> > new file mode 100644
> > index 000000000000..51325b699563
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/ifs/Kconfig
> > @@ -0,0 +1,2 @@
> > +config INTEL_IFS_DEVICE
> > + bool
> > diff --git a/drivers/platform/x86/intel/ifs/Makefile b/drivers/platform/x86/intel/ifs/Makefile
> > new file mode 100644
> > index 000000000000..12c2f5ce9925
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/ifs/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_INTEL_IFS_DEVICE) += intel_ifs_device.o
> > diff --git a/drivers/platform/x86/intel/ifs/intel_ifs_device.c b/drivers/platform/x86/intel/ifs/intel_ifs_device.c
> > new file mode 100644
> > index 000000000000..64a143871d72
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/ifs/intel_ifs_device.c
> > @@ -0,0 +1,50 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright(c) 2022 Intel Corporation. */
> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/init.h>
> > +#include <asm/cpu_device_id.h>
> > +
> > +#define MSR_IA32_CORE_CAPS_INTEGRITY_BIT 2
> > +#define MSR_IA32_CORE_CAPS_INTEGRITY BIT(MSR_IA32_CORE_CAPS_INTEGRITY_BIT)
> > +
> > +#define X86_MATCH(model) \
> > + X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6, \
> > + INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL)
> > +
> > +static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
> > + X86_MATCH(SAPPHIRERAPIDS_X),
> > + {}
> > +};
> > +
> > +static __init int register_ifs_device(void)
> > +{
> > + struct platform_device *pdev;
> > + const struct x86_cpu_id *m;
> > + u64 ia32_core_caps;
> > +
> > + m = x86_match_cpu(ifs_cpu_ids);
> > + if (!m)
> > + return -ENODEV;
> > +
> > + if (rdmsrl_safe(MSR_IA32_CORE_CAPS, &ia32_core_caps))
> > + return -ENODEV;
> > +
> > + if (ia32_core_caps & MSR_IA32_CORE_CAPS_INTEGRITY) {
> > + pdev = platform_device_alloc("intel_ifs", 0);
> > + if (pdev) {
> > + if (platform_device_add(pdev))
> > + platform_device_put(pdev);
> > + }
> > + }
> > +
> > + /*
> > + * Failure here will be visible by a missing device
> > + * in sysfs. Returning an error code would not make
> > + * that any easier to diagnose. Would also complicate
> > + * future implementations that may support a subset of
> > + * the types of tests.
> > + */
> > + return 0;
>
> So even if everything fails, you succeed? But you are failing above for
> some cases, so why is creating the device somehow special here that you
> should succeed no matter what?
My bad, this failure is not fatal to init and test execution tooling
will notice the missing device, but yes this can just return the
initcall error to be logged.