[PATCH 1/2] x86/bugs: Check VERW mitigations for consistency

Daniel Sneddon posted 2 patches 3 weeks, 6 days ago
[PATCH 1/2] x86/bugs: Check VERW mitigations for consistency
Posted by Daniel Sneddon 3 weeks, 6 days ago
There are currently 4 mitigations that use VERW: MDS, TAA,
MMIO Stale Data, and Register File Data Sampling. Because
all 4 use the same mitigation path, if any one of them is
enabled, they're all enabled. Normally, this is what is
wanted. However, if a user wants to disable the mitigation,
this can cause problems. If the user misses disabling even
one of these mitigations, then none of them will be
disabled. This can cause confusion as the user expects to
regain the performance lost to the mitigation but isn't
seeing any improvement. Since there are already 4 knobs for
controlling it, adding a 5th knob that controls all 4
mitigations together would just overcomplicate things.
Instead, let the user know their mitigations are out of sync
when at least one of these mitigations is disabled but not
all 4.

Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
---
 arch/x86/kernel/cpu/bugs.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index d1915427b4ff..b26b3b554330 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -582,8 +582,26 @@ static void __init md_clear_update_mitigation(void)
 		pr_info("Register File Data Sampling: %s\n", rfds_strings[rfds_mitigation]);
 }
 
+static void __init verw_mitigations_check(void)
+{
+	if (mds_mitigation == MDS_MITIGATION_OFF ||
+	    taa_mitigation == TAA_MITIGATION_OFF ||
+	    mmio_mitigation == MMIO_MITIGATION_OFF ||
+	    rfds_mitigation == RFDS_MITIGATION_OFF) {
+		if (mds_mitigation == MDS_MITIGATION_OFF &&
+		    taa_mitigation == TAA_MITIGATION_OFF &&
+		    mmio_mitigation == MMIO_MITIGATION_OFF &&
+		    rfds_mitigation == RFDS_MITIGATION_OFF)
+			return;
+
+		pr_info("MDS, TAA, MMIO Stale Data, and Register File Data Sampling all depend on VERW\n");
+		pr_info("In order to disable any one of them please ensure all 4 are disabled.\n");
+	}
+}
+
 static void __init md_clear_select_mitigation(void)
 {
+	verw_mitigations_check();
 	mds_select_mitigation();
 	taa_select_mitigation();
 	mmio_select_mitigation();
-- 
2.25.1
Re: [PATCH 1/2] x86/bugs: Check VERW mitigations for consistency
Posted by Nikolay Borisov 3 weeks, 5 days ago

On 29.10.24 г. 1:50 ч., Daniel Sneddon wrote:
> There are currently 4 mitigations that use VERW: MDS, TAA,
> MMIO Stale Data, and Register File Data Sampling. Because
> all 4 use the same mitigation path, if any one of them is
> enabled, they're all enabled. Normally, this is what is
> wanted. However, if a user wants to disable the mitigation,
> this can cause problems. If the user misses disabling even
> one of these mitigations, then none of them will be
> disabled. This can cause confusion as the user expects to
> regain the performance lost to the mitigation but isn't
> seeing any improvement. Since there are already 4 knobs for
> controlling it, adding a 5th knob that controls all 4
> mitigations together would just overcomplicate things.
> Instead, let the user know their mitigations are out of sync
> when at least one of these mitigations is disabled but not
> all 4.
> 
> Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
> ---
>   arch/x86/kernel/cpu/bugs.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index d1915427b4ff..b26b3b554330 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -582,8 +582,26 @@ static void __init md_clear_update_mitigation(void)
>   		pr_info("Register File Data Sampling: %s\n", rfds_strings[rfds_mitigation]);
>   }
>   
> +static void __init verw_mitigations_check(void)
> +{
> +	if (mds_mitigation == MDS_MITIGATION_OFF ||
> +	    taa_mitigation == TAA_MITIGATION_OFF ||
> +	    mmio_mitigation == MMIO_MITIGATION_OFF ||
> +	    rfds_mitigation == RFDS_MITIGATION_OFF) {
> +		if (mds_mitigation == MDS_MITIGATION_OFF &&
> +		    taa_mitigation == TAA_MITIGATION_OFF &&
> +		    mmio_mitigation == MMIO_MITIGATION_OFF &&
> +		    rfds_mitigation == RFDS_MITIGATION_OFF)
> +			return;

Ugh, why don't you simply XOR the 4 values and if it's 1 it means the 
inputs differe => there is inconsistency.

> +
> +		pr_info("MDS, TAA, MMIO Stale Data, and Register File Data Sampling all depend on VERW\n");
> +		pr_info("In order to disable any one of them please ensure all 4 are disabled.\n");
> +	}
> +}
> +
>   static void __init md_clear_select_mitigation(void)
>   {
> +	verw_mitigations_check();
>   	mds_select_mitigation();
>   	taa_select_mitigation();
>   	mmio_select_mitigation();
Re: [PATCH 1/2] x86/bugs: Check VERW mitigations for consistency
Posted by Daniel Sneddon 3 weeks, 5 days ago
On 10/29/24 09:32, Nikolay Borisov wrote:
> 
> 
> On 29.10.24 г. 1:50 ч., Daniel Sneddon wrote:
>> There are currently 4 mitigations that use VERW: MDS, TAA,
>> MMIO Stale Data, and Register File Data Sampling. Because
>> all 4 use the same mitigation path, if any one of them is
>> enabled, they're all enabled. Normally, this is what is
>> wanted. However, if a user wants to disable the mitigation,
>> this can cause problems. If the user misses disabling even
>> one of these mitigations, then none of them will be
>> disabled. This can cause confusion as the user expects to
>> regain the performance lost to the mitigation but isn't
>> seeing any improvement. Since there are already 4 knobs for
>> controlling it, adding a 5th knob that controls all 4
>> mitigations together would just overcomplicate things.
>> Instead, let the user know their mitigations are out of sync
>> when at least one of these mitigations is disabled but not
>> all 4.
>>
>> Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
>> ---
>>   arch/x86/kernel/cpu/bugs.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
>> index d1915427b4ff..b26b3b554330 100644
>> --- a/arch/x86/kernel/cpu/bugs.c
>> +++ b/arch/x86/kernel/cpu/bugs.c
>> @@ -582,8 +582,26 @@ static void __init md_clear_update_mitigation(void)
>>   		pr_info("Register File Data Sampling: %s\n", rfds_strings[rfds_mitigation]);
>>   }
>>   
>> +static void __init verw_mitigations_check(void)
>> +{
>> +	if (mds_mitigation == MDS_MITIGATION_OFF ||
>> +	    taa_mitigation == TAA_MITIGATION_OFF ||
>> +	    mmio_mitigation == MMIO_MITIGATION_OFF ||
>> +	    rfds_mitigation == RFDS_MITIGATION_OFF) {
>> +		if (mds_mitigation == MDS_MITIGATION_OFF &&
>> +		    taa_mitigation == TAA_MITIGATION_OFF &&
>> +		    mmio_mitigation == MMIO_MITIGATION_OFF &&
>> +		    rfds_mitigation == RFDS_MITIGATION_OFF)
>> +			return;
> 
> Ugh, why don't you simply XOR the 4 values and if it's 1 it means the 
> inputs differe => there is inconsistency.
> 
That's certainly cleaner. Will update.

Thx
>> +
>> +		pr_info("MDS, TAA, MMIO Stale Data, and Register File Data Sampling all depend on VERW\n");
>> +		pr_info("In order to disable any one of them please ensure all 4 are disabled.\n");
>> +	}
>> +}
>> +
>>   static void __init md_clear_select_mitigation(void)
>>   {
>> +	verw_mitigations_check();
>>   	mds_select_mitigation();
>>   	taa_select_mitigation();
>>   	mmio_select_mitigation();

Re: [PATCH 1/2] x86/bugs: Check VERW mitigations for consistency
Posted by Borislav Petkov 3 weeks, 5 days ago
On Mon, Oct 28, 2024 at 04:50:34PM -0700, Daniel Sneddon wrote:
> There are currently 4 mitigations that use VERW: MDS, TAA,
> MMIO Stale Data, and Register File Data Sampling. Because
> all 4 use the same mitigation path, if any one of them is
> enabled, they're all enabled. Normally, this is what is
> wanted. However, if a user wants to disable the mitigation,
> this can cause problems. If the user misses disabling even
> one of these mitigations, then none of them will be
> disabled. This can cause confusion as the user expects to
> regain the performance lost to the mitigation but isn't
> seeing any improvement. Since there are already 4 knobs for
> controlling it, adding a 5th knob that controls all 4
> mitigations together would just overcomplicate things.
> Instead, let the user know their mitigations are out of sync
> when at least one of these mitigations is disabled but not
> all 4.

Please split this commit message into smaller chunks for better readability.
For example:

    There are currently 4 mitigations that use VERW: MDS, TAA, MMIO Stale Data,
    and Register File Data Sampling. Because all 4 use the same mitigation path,
    if any one of them is enabled, they're all enabled.
    
    Normally, this is what is wanted. However, if a user wants to disable the
    mitigation, this can cause problems. If the user misses disabling even one of
    these mitigations, then none of them will be disabled.
    
    This can cause confusion as the user expects to regain the performance lost to
    the mitigation but isn't seeing any improvement. Since there are already
    4 knobs for controlling it, adding a 5th knob that controls all 4 mitigations
    together would just overcomplicate things.
    
    Instead, let the user know their mitigations are out of sync when at least one
    of these mitigations is disabled but not all 4.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH 1/2] x86/bugs: Check VERW mitigations for consistency
Posted by Daniel Sneddon 3 weeks, 5 days ago
On 10/29/24 04:39, Borislav Petkov wrote:
> On Mon, Oct 28, 2024 at 04:50:34PM -0700, Daniel Sneddon wrote:
>> There are currently 4 mitigations that use VERW: MDS, TAA,
>> MMIO Stale Data, and Register File Data Sampling. Because
>> all 4 use the same mitigation path, if any one of them is
>> enabled, they're all enabled. Normally, this is what is
>> wanted. However, if a user wants to disable the mitigation,
>> this can cause problems. If the user misses disabling even
>> one of these mitigations, then none of them will be
>> disabled. This can cause confusion as the user expects to
>> regain the performance lost to the mitigation but isn't
>> seeing any improvement. Since there are already 4 knobs for
>> controlling it, adding a 5th knob that controls all 4
>> mitigations together would just overcomplicate things.
>> Instead, let the user know their mitigations are out of sync
>> when at least one of these mitigations is disabled but not
>> all 4.
> 
> Please split this commit message into smaller chunks for better readability.
> For example:
> 
>     There are currently 4 mitigations that use VERW: MDS, TAA, MMIO Stale Data,
>     and Register File Data Sampling. Because all 4 use the same mitigation path,
>     if any one of them is enabled, they're all enabled.
>     
>     Normally, this is what is wanted. However, if a user wants to disable the
>     mitigation, this can cause problems. If the user misses disabling even one of
>     these mitigations, then none of them will be disabled.
>     
>     This can cause confusion as the user expects to regain the performance lost to
>     the mitigation but isn't seeing any improvement. Since there are already
>     4 knobs for controlling it, adding a 5th knob that controls all 4 mitigations
>     together would just overcomplicate things.
>     
>     Instead, let the user know their mitigations are out of sync when at least one
>     of these mitigations is disabled but not all 4.
> 
> Thx.
> 

Will do.