x86: Drop opt_pku entirely

[PATCH] x86: Drop opt_pku entirely

Posted by Andrew Cooper 10 months, 1 week ago

This option is particularly dubious as Xen does not use Protection Keys, owing
to the sharing of pagetables with PV guests.  All this option does is hide PKU
by default from HVM guests, and is therefore redundant with the more generic
cpuid=no-pku.

The variable ought to be in __initdata given it's single user, but deleting it
entirely looks to be a better course of action.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Henry Wang <Henry.Wang@arm.com>
---
 CHANGELOG.md                      |  3 +++
 docs/misc/xen-command-line.pandoc | 10 ----------
 xen/arch/x86/cpu/common.c         |  7 -------
 3 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7d7e0590f8c6..43f15dc34cbf 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -25,6 +25,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
  - Add support for AVX512-FP16 on x86.
  - On Arm, Xen supports guests running SVE/SVE2 instructions. (Tech Preview)
 
+### Removed
+ - On x86, the "pku" command line option has been removed.  It has never
+   behaved precisely as described, and redundant with "cpuid=no-pku".
 
 ## [4.17.0](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.17.0) - 2022-12-12
 
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 4060ebdc5d76..9d66688bd1ff 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1950,16 +1950,6 @@ for all of them (`true`), only for those subject to XPTI (`xpti`) or for
 those not subject to XPTI (`no-xpti`). The feature is used only in case
 INVPCID is supported and not disabled via `invpcid=false`.
 
-### pku (x86)
-> `= <boolean>`
-
-> Default: `true`
-
-Flag to enable Memory Protection Keys.
-
-The protection-key feature provides an additional mechanism by which IA-32e
-paging controls access to usermode addresses.
-
 ### ple_gap
 > `= <integer>`
 
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index cfcdaace125b..14021ffc66d8 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -29,10 +29,6 @@ bool __read_mostly opt_dom0_cpuid_faulting = true;
 bool_t opt_arat = 1;
 boolean_param("arat", opt_arat);
 
-/* pku: Flag to enable Memory Protection Keys (default on). */
-static bool_t opt_pku = 1;
-boolean_param("pku", opt_pku);
-
 unsigned int opt_cpuid_mask_ecx = ~0u;
 integer_param("cpuid_mask_ecx", opt_cpuid_mask_ecx);
 unsigned int opt_cpuid_mask_edx = ~0u;
@@ -522,9 +518,6 @@ void identify_cpu(struct cpuinfo_x86 *c)
 		this_cpu->c_init(c);
 
 
-   	if (c == &boot_cpu_data && !opt_pku)
-		setup_clear_cpu_cap(X86_FEATURE_PKU);
-
 	/*
 	 * The vendor-specific functions might have changed features.  Now
 	 * we do "generic changes."
-- 
2.30.2

Re: [PATCH] x86: Drop opt_pku entirely

Posted by Jan Beulich 10 months, 1 week ago

On 20.06.2023 19:47, Andrew Cooper wrote:
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -25,6 +25,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>   - Add support for AVX512-FP16 on x86.
>   - On Arm, Xen supports guests running SVE/SVE2 instructions. (Tech Preview)
>  
> +### Removed
> + - On x86, the "pku" command line option has been removed.  It has never
> +   behaved precisely as described, and redundant with "cpuid=no-pku".

Nit: Missing "was"?

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1950,16 +1950,6 @@ for all of them (`true`), only for those subject to XPTI (`xpti`) or for
>  those not subject to XPTI (`no-xpti`). The feature is used only in case
>  INVPCID is supported and not disabled via `invpcid=false`.
>  
> -### pku (x86)
> -> `= <boolean>`
> -
> -> Default: `true`
> -
> -Flag to enable Memory Protection Keys.
> -
> -The protection-key feature provides an additional mechanism by which IA-32e
> -paging controls access to usermode addresses.
> -
>  ### ple_gap
>  > `= <integer>`

Elsewhere you said that we kind of imply that only the explicitly named
sub-options of cpuid= are supported. If that's the case (which could do
with saying more explicitly), you will want to add "pku" there in order
to not regress what is (deemed) supported.

It also looks as if the speculation control related enumeration there has
gone stale / is now incomplete.

Jan

Re: [PATCH] x86: Drop opt_pku entirely

Posted by Andrew Cooper 8 months, 3 weeks ago

On 21/06/2023 8:37 am, Jan Beulich wrote:
> On 20.06.2023 19:47, Andrew Cooper wrote:
>> --- a/CHANGELOG.md
>> +++ b/CHANGELOG.md
>> @@ -25,6 +25,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>   - Add support for AVX512-FP16 on x86.
>>   - On Arm, Xen supports guests running SVE/SVE2 instructions. (Tech Preview)
>>  
>> +### Removed
>> + - On x86, the "pku" command line option has been removed.  It has never
>> +   behaved precisely as described, and redundant with "cpuid=no-pku".
> Nit: Missing "was"?

Fixed

>
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1950,16 +1950,6 @@ for all of them (`true`), only for those subject to XPTI (`xpti`) or for
>>  those not subject to XPTI (`no-xpti`). The feature is used only in case
>>  INVPCID is supported and not disabled via `invpcid=false`.
>>  
>> -### pku (x86)
>> -> `= <boolean>`
>> -
>> -> Default: `true`
>> -
>> -Flag to enable Memory Protection Keys.
>> -
>> -The protection-key feature provides an additional mechanism by which IA-32e
>> -paging controls access to usermode addresses.
>> -
>>  ### ple_gap
>>  > `= <integer>`
> Elsewhere you said that we kind of imply that only the explicitly named
> sub-options of cpuid= are supported. If that's the case (which could do
> with saying more explicitly), you will want to add "pku" there in order
> to not regress what is (deemed) supported.

I disagree.  I can say it was equivalent to X without X being an
explicitly supported option.

PKU shouldn't be adjusted by either of these options; it should be
controlled in the VM config file (if at all).  I'm unwilling to make any
suggestion that this is supported.

~Andrew

Re: [PATCH] x86: Drop opt_pku entirely

Posted by Jan Beulich 8 months, 3 weeks ago

On 07.08.2023 15:21, Andrew Cooper wrote:
> On 21/06/2023 8:37 am, Jan Beulich wrote:
>> On 20.06.2023 19:47, Andrew Cooper wrote:
>>> --- a/CHANGELOG.md
>>> +++ b/CHANGELOG.md
>>> @@ -25,6 +25,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>>   - Add support for AVX512-FP16 on x86.
>>>   - On Arm, Xen supports guests running SVE/SVE2 instructions. (Tech Preview)
>>>  
>>> +### Removed
>>> + - On x86, the "pku" command line option has been removed.  It has never
>>> +   behaved precisely as described, and redundant with "cpuid=no-pku".
>> Nit: Missing "was"?
> 
> Fixed
> 
>>
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -1950,16 +1950,6 @@ for all of them (`true`), only for those subject to XPTI (`xpti`) or for
>>>  those not subject to XPTI (`no-xpti`). The feature is used only in case
>>>  INVPCID is supported and not disabled via `invpcid=false`.
>>>  
>>> -### pku (x86)
>>> -> `= <boolean>`
>>> -
>>> -> Default: `true`
>>> -
>>> -Flag to enable Memory Protection Keys.
>>> -
>>> -The protection-key feature provides an additional mechanism by which IA-32e
>>> -paging controls access to usermode addresses.
>>> -
>>>  ### ple_gap
>>>  > `= <integer>`
>> Elsewhere you said that we kind of imply that only the explicitly named
>> sub-options of cpuid= are supported. If that's the case (which could do
>> with saying more explicitly), you will want to add "pku" there in order
>> to not regress what is (deemed) supported.
> 
> I disagree.  I can say it was equivalent to X without X being an
> explicitly supported option.
> 
> PKU shouldn't be adjusted by either of these options; it should be
> controlled in the VM config file (if at all).  I'm unwilling to make any
> suggestion that this is supported.

Hmm, so you're suggesting "pku=" use was unsupported, too? If so, perhaps
worth adding the word "unsupported" to the CHANGELOG.md entry. At which
point I'm fine with no adjustment to cpuid= doc. And then also
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH] x86: Drop opt_pku entirely

Posted by Andrew Cooper 8 months, 3 weeks ago

On 07/08/2023 2:34 pm, Jan Beulich wrote:
> On 07.08.2023 15:21, Andrew Cooper wrote:
>> On 21/06/2023 8:37 am, Jan Beulich wrote:
>>> On 20.06.2023 19:47, Andrew Cooper wrote:
>>>> --- a/docs/misc/xen-command-line.pandoc
>>>> +++ b/docs/misc/xen-command-line.pandoc
>>>> @@ -1950,16 +1950,6 @@ for all of them (`true`), only for those subject to XPTI (`xpti`) or for
>>>>  those not subject to XPTI (`no-xpti`). The feature is used only in case
>>>>  INVPCID is supported and not disabled via `invpcid=false`.
>>>>  
>>>> -### pku (x86)
>>>> -> `= <boolean>`
>>>> -
>>>> -> Default: `true`
>>>> -
>>>> -Flag to enable Memory Protection Keys.
>>>> -
>>>> -The protection-key feature provides an additional mechanism by which IA-32e
>>>> -paging controls access to usermode addresses.
>>>> -
>>>>  ### ple_gap
>>>>  > `= <integer>`
>>> Elsewhere you said that we kind of imply that only the explicitly named
>>> sub-options of cpuid= are supported. If that's the case (which could do
>>> with saying more explicitly), you will want to add "pku" there in order
>>> to not regress what is (deemed) supported.
>> I disagree.  I can say it was equivalent to X without X being an
>> explicitly supported option.
>>
>> PKU shouldn't be adjusted by either of these options; it should be
>> controlled in the VM config file (if at all).  I'm unwilling to make any
>> suggestion that this is supported.
> Hmm, so you're suggesting "pku=" use was unsupported, too? If so, perhaps
> worth adding the word "unsupported" to the CHANGELOG.md entry.

Well - it's just hiding a feature so nothing (interesting) is going to
break.  But it also isn't an option that anyone ought to have been using.

>  At which
> point I'm fine with no adjustment to cpuid= doc. And then also
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks.  I'll tweak.

~Andrew

Re: [PATCH] x86: Drop opt_pku entirely

Posted by Henry Wang 8 months, 3 weeks ago

> Re: [PATCH] x86: Drop opt_pku entirely

Hi Andrew and Jan,

Sorry for the possible format issue.

>>  At which
>> point I'm fine with no adjustment to cpuid= doc. And then also
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> Thanks.  I'll tweak.

Acked-by: Henry Wang <Henry.Wang@arm.com> #CHANGELOG

Kind regards,
Henry

> ~Andrew