Re: [patch 09/41] x86/kvm: Avoid looking up PKRU in XSAVE buffer
From: Dave Hansen
Date: Mon Jun 14 2021 - 15:34:44 EST
On 6/14/21 3:26 AM, Borislav Petkov wrote:
> On Fri, Jun 11, 2021 at 06:15:32PM +0200, Thomas Gleixner wrote:
>> - if (src) {
>> - u32 size, offset, ecx, edx;
>> - cpuid_count(XSTATE_CPUID, xfeature_nr,
>> - &size, &offset, &ecx, &edx);
>> - if (xfeature_nr == XFEATURE_PKRU)
>> - memcpy(dest + offset, &vcpu->arch.pkru,
>> - sizeof(vcpu->arch.pkru));
>> - else
>> - memcpy(dest + offset, src, size);
>> + cpuid_count(XSTATE_CPUID, xfeature_nr,
>> + &size, &offset, &ecx, &edx);
>>
>> + if (xfeature_nr == XFEATURE_PKRU) {
>> + memcpy(dest + offset, &vcpu->arch.pkru,
>> + sizeof(vcpu->arch.pkru));
>> + } else {
>> + src = get_xsave_addr(xsave, xfeature_nr);
>> + if (src)
>> + memcpy(dest + offset, src, size);
>> }
>>
>> valid -= xfeature_mask;
>
> How about pulling up that PKRU case even further (pasting the whole
> changed loop instead of an unreadable diff) and keeping the CPUID access
> and the xsave address handling separate?
>
> valid = xstate_bv & ~XFEATURE_MASK_FPSSE;
> while (valid) {
> u32 size, offset, ecx, edx;
> u64 xfeature_mask = valid & -valid;
> int xfeature_nr = fls64(xfeature_mask) - 1;
> void *src;
>
> if (xfeature_nr == XFEATURE_PKRU) {
> memcpy(dest + offset, &vcpu->arch.pkru, sizeof(vcpu->arch.pkru));
> continue;
> }
>
> cpuid_count(XSTATE_CPUID, xfeature_nr, &size, &offset, &ecx, &edx);
>
> src = get_xsave_addr(xsave, xfeature_nr);
> if (src)
> memcpy(dest + offset, src, size);
>
> valid -= xfeature_mask;
> }
I gave that a shot. Two wrinkles: The PKRU memcpy() needs 'offset' from
cpuid_count() and the PKRU case also needs the 'valid -=' manipulation.
The result is attached, and while it makes the diff look better, I
don't think the resulting code is an improvement.
> Btw, I'm wondering if that CPUID read in a loop can be replaced with
> adding accessors for xstate_{offsets,sizes,..} etc and providing them to
> kvm...
I *think* these are already stored in xfeature_uncompacted_offset[]. It
would be a pretty simple matter to export it. I just assumed that this
is a slow enough path that the KVM folks don't care.
>> @@ -4632,18 +4633,20 @@ static void load_xsave(struct kvm_vcpu *
>> */
>> valid = xstate_bv & ~XFEATURE_MASK_FPSSE;
>> while (valid) {
>> + u32 size, offset, ecx, edx;
>> u64 xfeature_mask = valid & -valid;
>> int xfeature_nr = fls64(xfeature_mask) - 1;
>> - void *dest = get_xsave_addr(xsave, xfeature_nr);
>>
>> - if (dest) {
>> - u32 size, offset, ecx, edx;
>> - cpuid_count(XSTATE_CPUID, xfeature_nr,
>> - &size, &offset, &ecx, &edx);
>> - if (xfeature_nr == XFEATURE_PKRU)
>> - memcpy(&vcpu->arch.pkru, src + offset,
>> - sizeof(vcpu->arch.pkru));
>> - else
>> + cpuid_count(XSTATE_CPUID, xfeature_nr,
>> + &size, &offset, &ecx, &edx);
>> +
>> + if (xfeature_nr == XFEATURE_PKRU) {
>> + memcpy(&vcpu->arch.pkru, src + offset,
>> + sizeof(vcpu->arch.pkru));
>> + } else {
>> + void *dest = get_xsave_addr(xsave, xfeature_nr);
>> +
>
>
> ^ Superfluous newline.
I'm happy to change it, but I usually like to separate declarations from
pure code. Although, I guess that's a bit inconsistent in that file.
commit a761b0a48fb62429bd98c9a1275ef3ce33f9925a
Author: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
Date: Thu Jun 3 16:08:12 2021 -0700
x86/kvm: Avoid looking up PKRU in XSAVE buffer
PKRU is being removed from the kernel XSAVE/FPU buffers. This removal
will probably include warnings for code that look up PKRU in those
buffers.
KVM currently looks up the location of PKRU but doesn't even use the
pointer that it gets back. Rework the code to avoid calling
get_xsave_addr() except in cases where its result is actually used.
This makes the code more clear and also avoids the inevitable PKRU
warnings.
This is probably a good cleanup and could go upstream idependently
of any PKRU rework.
Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
--
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b594275d49b5..ed4c3d90a18b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4589,20 +4589,21 @@ static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu)
*/
valid = xstate_bv & ~XFEATURE_MASK_FPSSE;
while (valid) {
+ u32 size, offset, ecx, edx;
u64 xfeature_mask = valid & -valid;
int xfeature_nr = fls64(xfeature_mask) - 1;
- void *src = get_xsave_addr(xsave, xfeature_nr);
-
- if (src) {
- u32 size, offset, ecx, edx;
- cpuid_count(XSTATE_CPUID, xfeature_nr,
- &size, &offset, &ecx, &edx);
- if (xfeature_nr == XFEATURE_PKRU)
- memcpy(dest + offset, &vcpu->arch.pkru,
- sizeof(vcpu->arch.pkru));
- else
- memcpy(dest + offset, src, size);
+ void *src;
+
+ cpuid_count(XSTATE_CPUID, xfeature_nr,
+ &size, &offset, &ecx, &edx);
+ if (xfeature_nr == XFEATURE_PKRU) {
+ memcpy(dest + offset, &vcpu->arch.pkru,
+ sizeof(vcpu->arch.pkru));
+ } else {
+ src = get_xsave_addr(xsave, xfeature_nr);
+ if (src)
+ memcpy(dest + offset, src, size);
}
valid -= xfeature_mask;
@@ -4632,18 +4633,20 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src)
*/
valid = xstate_bv & ~XFEATURE_MASK_FPSSE;
while (valid) {
+ u32 size, offset, ecx, edx;
u64 xfeature_mask = valid & -valid;
int xfeature_nr = fls64(xfeature_mask) - 1;
- void *dest = get_xsave_addr(xsave, xfeature_nr);
-
- if (dest) {
- u32 size, offset, ecx, edx;
- cpuid_count(XSTATE_CPUID, xfeature_nr,
- &size, &offset, &ecx, &edx);
- if (xfeature_nr == XFEATURE_PKRU)
- memcpy(&vcpu->arch.pkru, src + offset,
- sizeof(vcpu->arch.pkru));
- else
+
+ cpuid_count(XSTATE_CPUID, xfeature_nr,
+ &size, &offset, &ecx, &edx);
+
+ if (xfeature_nr == XFEATURE_PKRU) {
+ memcpy(&vcpu->arch.pkru, src + offset,
+ sizeof(vcpu->arch.pkru));
+ } else {
+ void *dest = get_xsave_addr(xsave, xfeature_nr);
+
+ if (dest)
memcpy(dest, src + offset, size);
}