Re: [PATCH 6/6] x86/hvm: Support PKS
On 21/12/2021 12:18 pm, Jan Beulich wrote:
> On 16.12.2021 10:54, Andrew Cooper wrote:
>> With all infrastructure in place, advertise the PKS CPUID bit to guests, and
>> let them set CR4.PKS.
>>
>> Experiment with a tweak to the layout of hvm_cr4_guest_valid_bits() so future
>> additions will be just a single added line.
>>
>> The current context switching behaviour is tied to how VT-x works, so leave a
>> safety check in the short term.
>>
>> Signed-off-by: Andrew Cooper
> Reviewed-by: Jan Beulich
Thanks.
>
> I would like to ask though that you ...
>
>> --- a/xen/include/public/arch-x86/cpufeatureset.h
>> +++ b/xen/include/public/arch-x86/cpufeatureset.h
>> @@ -244,7 +244,7 @@ XEN_CPUFEATURE(CLDEMOTE, 6*32+25) /*A CLDEMOTE
>> instruction */
>> XEN_CPUFEATURE(MOVDIRI, 6*32+27) /*a MOVDIRI instruction */
>> XEN_CPUFEATURE(MOVDIR64B, 6*32+28) /*a MOVDIR64B instruction */
>> XEN_CPUFEATURE(ENQCMD,6*32+29) /* ENQCMD{,S} instructions */
>> -XEN_CPUFEATURE(PKS, 6*32+31) /* Protection Key for Supervisor */
>> +XEN_CPUFEATURE(PKS, 6*32+31) /*H Protection Key for Supervisor */
> ... clarify this restriction of not covering shadow mode guests by
> an adjustment to title or description. Aiui the sole reason for
> the restriction is that shadow code doesn't propagate the key bits
> from guest to shadow PTEs?
PKU is only exposed on HAP, so PKS really ought to match.
We indeed don't copy the leaf PKEY into the shadows. While that ought
to be relatively to adjust, we would then have to make sh_page_fault()
cope with seeing PFEC_prot_key.
But honestly, there are far far more important things to spend time on.
~Andrew
Re: [PATCH 6/6] x86/hvm: Support PKS
On 16.12.2021 10:54, Andrew Cooper wrote:
> With all infrastructure in place, advertise the PKS CPUID bit to guests, and
> let them set CR4.PKS.
>
> Experiment with a tweak to the layout of hvm_cr4_guest_valid_bits() so future
> additions will be just a single added line.
>
> The current context switching behaviour is tied to how VT-x works, so leave a
> safety check in the short term.
>
> Signed-off-by: Andrew Cooper
Reviewed-by: Jan Beulich
I would like to ask though that you ...
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -244,7 +244,7 @@ XEN_CPUFEATURE(CLDEMOTE, 6*32+25) /*A CLDEMOTE
> instruction */
> XEN_CPUFEATURE(MOVDIRI, 6*32+27) /*a MOVDIRI instruction */
> XEN_CPUFEATURE(MOVDIR64B, 6*32+28) /*a MOVDIR64B instruction */
> XEN_CPUFEATURE(ENQCMD,6*32+29) /* ENQCMD{,S} instructions */
> -XEN_CPUFEATURE(PKS, 6*32+31) /* Protection Key for Supervisor */
> +XEN_CPUFEATURE(PKS, 6*32+31) /*H Protection Key for Supervisor */
... clarify this restriction of not covering shadow mode guests by
an adjustment to title or description. Aiui the sole reason for
the restriction is that shadow code doesn't propagate the key bits
from guest to shadow PTEs?
Jan
[PATCH 6/6] x86/hvm: Support PKS
With all infrastructure in place, advertise the PKS CPUID bit to guests, and
let them set CR4.PKS.
Experiment with a tweak to the layout of hvm_cr4_guest_valid_bits() so future
additions will be just a single added line.
The current context switching behaviour is tied to how VT-x works, so leave a
safety check in the short term.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
---
xen/arch/x86/cpuid.c| 9 +
xen/arch/x86/hvm/hvm.c | 4 +++-
xen/include/public/arch-x86/cpufeatureset.h | 2 +-
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 151944f65702..03653d3766f4 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -512,6 +512,15 @@ static void __init calculate_hvm_max_policy(void)
__clear_bit(X86_FEATURE_XSAVES, hvm_featureset);
}
+/*
+ * Xen doesn't use PKS, so the guest support for it has opted to not use
+ * the VMCS load/save controls for efficiency reasons. This depends on
+ * the exact vmentry/exit behaviour, so don't expose PKS in other
+ * situations until someone has cross-checked the behaviour for safety.
+ */
+if ( !cpu_has_vmx )
+__clear_bit(X86_FEATURE_PKS, hvm_featureset);
+
guest_common_feature_adjustments(hvm_featureset);
sanitise_featureset(hvm_featureset);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e75245f36dce..2552e7f45499 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1010,7 +1010,9 @@ unsigned long hvm_cr4_guest_valid_bits(const struct
domain *d)
(p->feat.smep ? X86_CR4_SMEP : 0) |
(p->feat.smap ? X86_CR4_SMAP : 0) |
(p->feat.pku ? X86_CR4_PKE : 0) |
-(cet ? X86_CR4_CET : 0));
+(cet ? X86_CR4_CET : 0) |
+(p->feat.pks ? X86_CR4_PKS : 0) |
+0);
}
static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
diff --git a/xen/include/public/arch-x86/cpufeatureset.h
b/xen/include/public/arch-x86/cpufeatureset.h
index 79a8f244d88a..92ec9eed3fd1 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -244,7 +244,7 @@ XEN_CPUFEATURE(CLDEMOTE, 6*32+25) /*A CLDEMOTE
instruction */
XEN_CPUFEATURE(MOVDIRI, 6*32+27) /*a MOVDIRI instruction */
XEN_CPUFEATURE(MOVDIR64B, 6*32+28) /*a MOVDIR64B instruction */
XEN_CPUFEATURE(ENQCMD,6*32+29) /* ENQCMD{,S} instructions */
-XEN_CPUFEATURE(PKS, 6*32+31) /* Protection Key for Supervisor */
+XEN_CPUFEATURE(PKS, 6*32+31) /*H Protection Key for Supervisor */
/* AMD-defined CPU features, CPUID level 0x8007.edx, word 7 */
XEN_CPUFEATURE(HW_PSTATE, 7*32+ 7) /* Hardware Pstates */
--
2.11.0
