diff mbox series

[04/14] platform/x86/intel/ifs: Remove image loading during init

Message ID 20221021203413.1220137-5-jithu.joseph@intel.com (mailing list archive)
State Changes Requested, archived
Headers show
Series IFS multi test image support and misc changes | expand

Commit Message

Joseph, Jithu Oct. 21, 2022, 8:34 p.m. UTC 
Existing implementation loads IFS test image during the driver
init flow.

Dropping test image loading from the init path improves
module load time.

Prior to starting IFS tests, the user has to load one of
the IFS test images by writing to the current_batch sysfs file.

Removing IFS test image  loading during init also allows us to
make ifs_sem static as it is used only within sysfs.c.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Suggested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
---
 drivers/platform/x86/intel/ifs/ifs.h   | 2 --
 drivers/platform/x86/intel/ifs/core.c  | 6 +-----
 drivers/platform/x86/intel/ifs/sysfs.c | 2 +-
 3 files changed, 2 insertions(+), 8 deletions(-)

Comments

Sohil Mehta Oct. 24, 2022, 11:50 p.m. UTC | #1 
On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> Existing implementation loads IFS test image during the driver
> init flow.
> 
> Dropping test image loading from the init path improves
> module load time.
> 
> Prior to starting IFS tests, the user has to load one of
> the IFS test images by writing to the current_batch sysfs file.
> 

The kernel is still unaware of the 'current_batch' sysfs file. It will 
be introduced in patch 12. You can avoid the reference here.

> Removing IFS test image  loading during init also allows us to
> make ifs_sem static as it is used only within sysfs.c.
> 

Does something like this sound better?

IFS test image is unnecessarily loaded during driver initialization. The 
user has to load one when starting the tests.

Drop image loading during ifs_init() and improve module load time. As a 
consequence, make ifs_sem static as it is only used within sysfs.c

> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 27204e3d674d..5fb7f655c291 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -51,12 +51,8 @@ static int __init ifs_init(void)
>   	ifs_device.misc.groups = ifs_get_groups();
>   
>   	if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&

Is there a reason to store the integrity cap constant in the device.data 
global structure?

.data = {
	.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
},

IIUC, you are basically reading an MSR and testing a bit within? You 
already have an BIT macro (unused) for that.

#define MSR_INTEGRITY_CAPS_PERIODIC_BIST 
BIT(MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT)



> -	    !misc_register(&ifs_device.misc)) {
> -		down(&ifs_sem);
> -		ifs_load_firmware(ifs_device.misc.this_device);
> -		up(&ifs_sem);
> +	    !misc_register(&ifs_device.misc))
>   		return 0;
> -	}
>   
>   	return -ENODEV;
>   }

Sohil
Joseph, Jithu Oct. 25, 2022, 12:41 a.m. UTC | #2 
On 10/24/2022 4:50 PM, Sohil Mehta wrote:
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
>> Existing implementation loads IFS test image during the driver
>> init flow.
>>
>> Dropping test image loading from the init path improves
>> module load time.
>>
>> Prior to starting IFS tests, the user has to load one of
>> the IFS test images by writing to the current_batch sysfs file.
>>
> 
> The kernel is still unaware of the 'current_batch' sysfs file. It will be introduced in patch 12. You can avoid the reference here.

ok will remove the current_batch reference and mention that user has to explicitly load test images

> 
>> Removing IFS test image  loading during init also allows us to
>> make ifs_sem static as it is used only within sysfs.c.
>>
> 
> Does something like this sound better?
> 
> IFS test image is unnecessarily loaded during driver initialization. The user has to load one when starting the tests.
> 
> Drop image loading during ifs_init() and improve module load time. As a consequence, make ifs_sem static as it is only used within sysfs.c

Will adopt this, Thanks

> 
>> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
>> index 27204e3d674d..5fb7f655c291 100644
>> --- a/drivers/platform/x86/intel/ifs/core.c
>> +++ b/drivers/platform/x86/intel/ifs/core.c
>> @@ -51,12 +51,8 @@ static int __init ifs_init(void)
>>       ifs_device.misc.groups = ifs_get_groups();
>>         if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&
> 
> Is there a reason to store the integrity cap constant in the device.data global structure?
> 
> .data = {
>     .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
> },

This was originally added so that, when future additional tests are supported by the driver, support can be checked using  the same if ((msrval & BIT(ifs_device.data.integrity_cap_bit)
Different tests would have different integrity_cap_bit assigned in the ifs_device[] array  (today it is just a single element and not an array

Note that the current series doesn't introduce this



Jithu
Sohil Mehta Oct. 25, 2022, 6:06 a.m. UTC | #3 
On 10/24/2022 5:41 PM, Joseph, Jithu wrote:
>>> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
>>> index 27204e3d674d..5fb7f655c291 100644
>>> --- a/drivers/platform/x86/intel/ifs/core.c
>>> +++ b/drivers/platform/x86/intel/ifs/core.c
>>> @@ -51,12 +51,8 @@ static int __init ifs_init(void)
>>>        ifs_device.misc.groups = ifs_get_groups();
>>>          if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&
>>
>> Is there a reason to store the integrity cap constant in the device.data global structure?
>>
>> .data = {
>>      .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
>> },
> 
> This was originally added so that, when future additional tests are supported by the driver, support can be checked using  the same if ((msrval & BIT(ifs_device.data.integrity_cap_bit)
> Different tests would have different integrity_cap_bit assigned in the ifs_device[] array  (today it is just a single element and not an array
> 
> Note that the current series doesn't introduce this
> 
> 

Sorry, I am still not able to follow.

Let's say you have an ifs_device[] array which has different integrity 
capabilities, there would still need to be some logic in the init code 
to differentiate between the resulting action that needs to be taken? 
Currently, the init function only registers the device. Maybe some 
example/code might be helpful to drive the point.

Also, are the future additional tests expected to be added soon? If not, 
maybe we can defer this optimization until the need arrives.

Sohil



> 
> Jithu
Joseph, Jithu Oct. 26, 2022, 11:53 p.m. UTC | #4 
On 10/24/2022 11:06 PM, Sohil Mehta wrote:
> On 10/24/2022 5:41 PM, Joseph, Jithu wrote:
>>>> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
>>>> index 27204e3d674d..5fb7f655c291 100644
>>>> --- a/drivers/platform/x86/intel/ifs/core.c
>>>> +++ b/drivers/platform/x86/intel/ifs/core.c
>>>> @@ -51,12 +51,8 @@ static int __init ifs_init(void)
>>>>        ifs_device.misc.groups = ifs_get_groups();
>>>>          if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&
>>>
>>> Is there a reason to store the integrity cap constant in the device.data global structure?
>>>
>>> .data = {
>>>      .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
>>> },
>>
>> This was originally added so that, when future additional tests are supported by the driver, support can be checked using  the same if ((msrval & BIT(ifs_device.data.integrity_cap_bit)
>> Different tests would have different integrity_cap_bit assigned in the ifs_device[] array  (today it is just a single element and not an array
>>
>> Note that the current series doesn't introduce this
>>
>>
> 
> Sorry, I am still not able to follow.
> 
> Let's say you have an ifs_device[] array which has different integrity capabilities, there would still need to be some logic in the init code to differentiate between the resulting action that needs to be taken? Currently, the init function only registers the device. Maybe some example/code might be helpful to drive the point.

multiple devices will be created if support for more than one is advertised by MSR_INTEGRITY_CAPS as shown below

static int __init ifs_init(void)
{

 ....
	if (rdmsrl_safe(MSR_INTEGRITY_CAPS, &msrval))
		return -ENODEV;

	for (i = 0; i < IFS_NUMTESTS; i++) {
		if (!(msrval & BIT(ifs_devices[i].data.integrity_cap_bit)))
			continue;

		ifs_devices[i].misc.groups = ifs_get_groups();
		if (!misc_register(&ifs_devices[i].misc)) {
			ndevices++;
		}
	}

  return ndevices ? 0 : -ENODEV;
}

At the handler side we can branch to the corresponding handler by looking at this bit
Say for e.g the following function in runtest.c could be extended to something like

int do_core_test(int cpu, struct device *dev)
{
	struct ifs_data *ifsd = ifs_get_data(dev);

  .....
	
	switch (ifsd->integrity_cap_bit) {
	case MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT:
		ifs_test_core(cpu, dev);
		break;
	case MSR_INTEGRITY_CAPS_TEST2
		ifs_do_test2(cpu, dev);
		break;
	case MSR_INTEGRITY_CAPS_TEST3
		ifs_do_test3(cpu, dev);
		break;
	default:
		return -EINVAL;
	}

....
}


Jithu
Sohil Mehta Nov. 1, 2022, 7 a.m. UTC | #5 
Thanks for the psuedo code. I think I understand the reasoning now.

There would be an IFS device array created for each type of test that 
exists. Based on the capabilities supported in MSR_INTEGRITY_CAPS the 
specific IFS devices would be created to run the tests.

> multiple devices will be created if support for more than one is advertised by MSR_INTEGRITY_CAPS as shown below
> 

Well, it would also depend on whether the currently running kernel has 
enumerated that test. IIUC, older kernels running on newer hardware 
would only create ifs test devices they are aware of.

It would have been great if the above statement would be true as is :)

> static int __init ifs_init(void)
> {
> 
>   ....
> 	if (rdmsrl_safe(MSR_INTEGRITY_CAPS, &msrval))
> 		return -ENODEV;
> 
> 	for (i = 0; i < IFS_NUMTESTS; i++) {
> 		if (!(msrval & BIT(ifs_devices[i].data.integrity_cap_bit)))
> 			continue;
> 
> 		ifs_devices[i].misc.groups = ifs_get_groups();
> 		if (!misc_register(&ifs_devices[i].misc)) {
> 			ndevices++;
> 		}
> 	}
> 
>    return ndevices ? 0 : -ENODEV;
> }
> 

Nit:

The _BIT extension is probably unnecessary. How about?

.data = {
      .integrity_cap = MSR_INTEGRITY_CAPS_PERIODIC_BIST,
},
diff mbox series

Patch

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 782bcc039ddb..be37512535f2 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -229,6 +229,4 @@  int ifs_load_firmware(struct device *dev);
 int do_core_test(int cpu, struct device *dev);
 const struct attribute_group **ifs_get_groups(void);
 
-extern struct semaphore ifs_sem;
-
 #endif
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 27204e3d674d..5fb7f655c291 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -51,12 +51,8 @@  static int __init ifs_init(void)
 	ifs_device.misc.groups = ifs_get_groups();
 
 	if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&
-	    !misc_register(&ifs_device.misc)) {
-		down(&ifs_sem);
-		ifs_load_firmware(ifs_device.misc.this_device);
-		up(&ifs_sem);
+	    !misc_register(&ifs_device.misc))
 		return 0;
-	}
 
 	return -ENODEV;
 }
diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
index 4af4e1bea98d..766cee651bd6 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -13,7 +13,7 @@ 
  * Protects against simultaneous tests on multiple cores, or
  * reloading can file while a test is in progress
  */
-DEFINE_SEMAPHORE(ifs_sem);
+static DEFINE_SEMAPHORE(ifs_sem);
 
 /*
  * The sysfs interface to check additional details of last test