* [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field @ 2021-02-08 16:16 Jing Liu 2021-02-08 15:23 ` Dave Hansen 0 siblings, 1 reply; 8+ messages in thread From: Jing Liu @ 2021-02-08 16:16 UTC (permalink / raw) To: pbonzini Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm, linux-kernel Bit 63 of the XCOMP_BV field indicates that the save area is in the compacted format and the remaining bits indicate the states that have space allocated in the save area, not only user states. Since fpstate_init() has initialized xcomp_bv, let's just use that. Signed-off-by: Jing Liu <jing2.liu@linux.intel.com> --- arch/x86/kvm/x86.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 1b404e4d7dd8..f115493f577d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4435,8 +4435,6 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu, return 0; } -#define XSTATE_COMPACTION_ENABLED (1ULL << 63) - static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu) { struct xregs_state *xsave = &vcpu->arch.guest_fpu->state.xsave; @@ -4494,7 +4492,8 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src) /* Set XSTATE_BV and possibly XCOMP_BV. */ xsave->header.xfeatures = xstate_bv; if (boot_cpu_has(X86_FEATURE_XSAVES)) - xsave->header.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED; + xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | + xfeatures_mask_all; /* * Copy each region from the non-compacted offset to the @@ -9912,9 +9911,6 @@ static void fx_init(struct kvm_vcpu *vcpu) return; fpstate_init(&vcpu->arch.guest_fpu->state); - if (boot_cpu_has(X86_FEATURE_XSAVES)) - vcpu->arch.guest_fpu->state.xsave.header.xcomp_bv = - host_xcr0 | XSTATE_COMPACTION_ENABLED; /* * Ensure guest xcr0 is valid for loading -- 2.18.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field 2021-02-08 16:16 [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field Jing Liu @ 2021-02-08 15:23 ` Dave Hansen 2021-02-08 17:24 ` Sean Christopherson 0 siblings, 1 reply; 8+ messages in thread From: Dave Hansen @ 2021-02-08 15:23 UTC (permalink / raw) To: Jing Liu, pbonzini Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm, linux-kernel On 2/8/21 8:16 AM, Jing Liu wrote: > -#define XSTATE_COMPACTION_ENABLED (1ULL << 63) > - > static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu) > { > struct xregs_state *xsave = &vcpu->arch.guest_fpu->state.xsave; > @@ -4494,7 +4492,8 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src) > /* Set XSTATE_BV and possibly XCOMP_BV. */ > xsave->header.xfeatures = xstate_bv; > if (boot_cpu_has(X86_FEATURE_XSAVES)) > - xsave->header.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED; > + xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | > + xfeatures_mask_all; Are 'host_xcr0' and 'xfeatures_mask_all' really interchangeable? If so, shouldn't we just remove 'host_xcr0' everywhere? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field 2021-02-08 15:23 ` Dave Hansen @ 2021-02-08 17:24 ` Sean Christopherson 2021-02-22 3:21 ` Liu, Jing2 0 siblings, 1 reply; 8+ messages in thread From: Sean Christopherson @ 2021-02-08 17:24 UTC (permalink / raw) To: Dave Hansen Cc: Jing Liu, pbonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm, linux-kernel On Mon, Feb 08, 2021, Dave Hansen wrote: > On 2/8/21 8:16 AM, Jing Liu wrote: > > -#define XSTATE_COMPACTION_ENABLED (1ULL << 63) > > - > > static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu) > > { > > struct xregs_state *xsave = &vcpu->arch.guest_fpu->state.xsave; > > @@ -4494,7 +4492,8 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src) > > /* Set XSTATE_BV and possibly XCOMP_BV. */ > > xsave->header.xfeatures = xstate_bv; > > if (boot_cpu_has(X86_FEATURE_XSAVES)) > > - xsave->header.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED; > > + xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | > > + xfeatures_mask_all; This is wrong, xfeatures_mask_all also tracks supervisor states. > Are 'host_xcr0' and 'xfeatures_mask_all' really interchangeable? If so, > shouldn't we just remove 'host_xcr0' everywhere? I think so? But use xfeatures_mask_user(). In theory, host_xss can also be replaced with the _supervisor() and _dynamic() variants. That code needs a good hard look at the _dynamic() features, which is currently just architectural LBRs. E.g. I wouldn't be surprised if KVM currently fails to save/restore arch LBRs due to the bit not being set in host_xss. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field 2021-02-08 17:24 ` Sean Christopherson @ 2021-02-22 3:21 ` Liu, Jing2 2021-02-22 16:06 ` Sean Christopherson 0 siblings, 1 reply; 8+ messages in thread From: Liu, Jing2 @ 2021-02-22 3:21 UTC (permalink / raw) To: Sean Christopherson, Dave Hansen Cc: pbonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm, linux-kernel On 2/9/2021 1:24 AM, Sean Christopherson wrote: > On Mon, Feb 08, 2021, Dave Hansen wrote: >> On 2/8/21 8:16 AM, Jing Liu wrote: >>> -#define XSTATE_COMPACTION_ENABLED (1ULL << 63) >>> - >>> static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu) >>> { >>> struct xregs_state *xsave = &vcpu->arch.guest_fpu->state.xsave; >>> @@ -4494,7 +4492,8 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src) >>> /* Set XSTATE_BV and possibly XCOMP_BV. */ >>> xsave->header.xfeatures = xstate_bv; >>> if (boot_cpu_has(X86_FEATURE_XSAVES)) >>> - xsave->header.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED; >>> + xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | >>> + xfeatures_mask_all; > This is wrong, xfeatures_mask_all also tracks supervisor states. When looking at SDM Vol2 XSAVES instruction Operation part, it says as follows, RFBM ← (XCR0 OR IA32_XSS) AND EDX:EAX; COMPMASK ← RFBM OR 80000000_00000000H; ... XCOMP_BV field in XSAVE header ← COMPMASK; So it seems xcomp_bv also tracks supervisor states? BRs, Jing > >> Are 'host_xcr0' and 'xfeatures_mask_all' really interchangeable? If so, >> shouldn't we just remove 'host_xcr0' everywhere? > I think so? But use xfeatures_mask_user(). > > In theory, host_xss can also be replaced with the _supervisor() and _dynamic() > variants. That code needs a good hard look at the _dynamic() features, which is > currently just architectural LBRs. E.g. I wouldn't be surprised if KVM currently > fails to save/restore arch LBRs due to the bit not being set in host_xss. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field 2021-02-22 3:21 ` Liu, Jing2 @ 2021-02-22 16:06 ` Sean Christopherson 2021-02-23 3:06 ` Liu, Jing2 0 siblings, 1 reply; 8+ messages in thread From: Sean Christopherson @ 2021-02-22 16:06 UTC (permalink / raw) To: Liu, Jing2 Cc: Dave Hansen, pbonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm, linux-kernel On Mon, Feb 22, 2021, Liu, Jing2 wrote: > > On 2/9/2021 1:24 AM, Sean Christopherson wrote: > > On Mon, Feb 08, 2021, Dave Hansen wrote: > > > On 2/8/21 8:16 AM, Jing Liu wrote: > > > > -#define XSTATE_COMPACTION_ENABLED (1ULL << 63) > > > > - > > > > static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu) > > > > { > > > > struct xregs_state *xsave = &vcpu->arch.guest_fpu->state.xsave; > > > > @@ -4494,7 +4492,8 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src) > > > > /* Set XSTATE_BV and possibly XCOMP_BV. */ > > > > xsave->header.xfeatures = xstate_bv; > > > > if (boot_cpu_has(X86_FEATURE_XSAVES)) > > > > - xsave->header.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED; > > > > + xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | > > > > + xfeatures_mask_all; > > This is wrong, xfeatures_mask_all also tracks supervisor states. > When looking at SDM Vol2 XSAVES instruction Operation part, it says as > follows, > > RFBM ← (XCR0 OR IA32_XSS) AND EDX:EAX; > COMPMASK ← RFBM OR 80000000_00000000H; > ... > > XCOMP_BV field in XSAVE header ← COMPMASK; > > > So it seems xcomp_bv also tracks supervisor states? Yes, sorry, I got distracted by Dave's question and didn't read the changelog closely. Now that I have, I find "Since fpstate_init() has initialized xcomp_bv, let's just use that." confusing. I think what you intend to say is that we can use the same _logic_ as fpstate_init_xstate() for calculating xcomp_bv. That said, it would be helpful for the changelog to explain why it's correct to use xfeatures_mask_all, e.g. just a short comment stating that the variable holds all XCR0 and XSS bits enabled by the host kernel. Justifying a change with "because other code does it" is sketchy, becuse there's no guarantee that what something else does is also correct for KVM, or that the existing code itself is even correct. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field 2021-02-22 16:06 ` Sean Christopherson @ 2021-02-23 3:06 ` Liu, Jing2 2021-02-24 20:40 ` Sean Christopherson 0 siblings, 1 reply; 8+ messages in thread From: Liu, Jing2 @ 2021-02-23 3:06 UTC (permalink / raw) To: Sean Christopherson Cc: Dave Hansen, pbonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm, linux-kernel On 2/23/2021 12:06 AM, Sean Christopherson wrote: > On Mon, Feb 22, 2021, Liu, Jing2 wrote: >> On 2/9/2021 1:24 AM, Sean Christopherson wrote: >>> On Mon, Feb 08, 2021, Dave Hansen wrote: >>>> On 2/8/21 8:16 AM, Jing Liu wrote: >>>>> -#define XSTATE_COMPACTION_ENABLED (1ULL << 63) >>>>> - >>>>> static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu) >>>>> { >>>>> struct xregs_state *xsave = &vcpu->arch.guest_fpu->state.xsave; >>>>> @@ -4494,7 +4492,8 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src) >>>>> /* Set XSTATE_BV and possibly XCOMP_BV. */ >>>>> xsave->header.xfeatures = xstate_bv; >>>>> if (boot_cpu_has(X86_FEATURE_XSAVES)) >>>>> - xsave->header.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED; >>>>> + xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | >>>>> + xfeatures_mask_all; >>> This is wrong, xfeatures_mask_all also tracks supervisor states. >> When looking at SDM Vol2 XSAVES instruction Operation part, it says as >> follows, >> >> RFBM ← (XCR0 OR IA32_XSS) AND EDX:EAX; >> COMPMASK ← RFBM OR 80000000_00000000H; >> ... >> >> XCOMP_BV field in XSAVE header ← COMPMASK; >> >> >> So it seems xcomp_bv also tracks supervisor states? > Yes, sorry, I got distracted by Dave's question and didn't read the changelog > closely. > > Now that I have, I find "Since fpstate_init() has initialized xcomp_bv, let's > just use that." confusing. I think what you intend to say is that we can use > the same _logic_ as fpstate_init_xstate() for calculating xcomp_bv. Yes, that's the idea. > > That said, it would be helpful for the changelog to explain why it's correct to > use xfeatures_mask_all, e.g. just a short comment stating that the variable holds > all XCR0 and XSS bits enabled by the host kernel. Justifying a change with > "because other code does it" is sketchy, becuse there's no guarantee that what > something else does is also correct for KVM, or that the existing code itself is > even correct. Got it, thanks for the details on this. Then how about making the commit message like, XCOMP_BV[63] field indicates that the save area is in the compacted format and XCOMP_BV[62:0] indicates the states that have space allocated in the save area, including both XCR0 and XSS bits enable by the host kernel. Use xfeatures_mask_all for calculating xcomp_bv and reuse XCOMP_BV_COMPACTED_FORMAT defined by kernel. Thanks, Jing ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field 2021-02-23 3:06 ` Liu, Jing2 @ 2021-02-24 20:40 ` Sean Christopherson 2021-02-25 2:02 ` Liu, Jing2 0 siblings, 1 reply; 8+ messages in thread From: Sean Christopherson @ 2021-02-24 20:40 UTC (permalink / raw) To: Liu, Jing2 Cc: Dave Hansen, pbonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm, linux-kernel On Tue, Feb 23, 2021, Liu, Jing2 wrote: > XCOMP_BV[63] field indicates that the save area is in the > compacted format and XCOMP_BV[62:0] indicates the states that > have space allocated in the save area, including both XCR0 > and XSS bits enable by the host kernel. Use xfeatures_mask_all > for calculating xcomp_bv and reuse XCOMP_BV_COMPACTED_FORMAT > defined by kernel. Works for me, just please wrap at ~73-75 chars, not ~64. Thanks! ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field 2021-02-24 20:40 ` Sean Christopherson @ 2021-02-25 2:02 ` Liu, Jing2 0 siblings, 0 replies; 8+ messages in thread From: Liu, Jing2 @ 2021-02-25 2:02 UTC (permalink / raw) To: Sean Christopherson Cc: Dave Hansen, pbonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm, linux-kernel On 2/25/2021 4:40 AM, Sean Christopherson wrote: > On Tue, Feb 23, 2021, Liu, Jing2 wrote: >> XCOMP_BV[63] field indicates that the save area is in the >> compacted format and XCOMP_BV[62:0] indicates the states that >> have space allocated in the save area, including both XCR0 >> and XSS bits enable by the host kernel. Use xfeatures_mask_all >> for calculating xcomp_bv and reuse XCOMP_BV_COMPACTED_FORMAT >> defined by kernel. > Works for me, just please wrap at ~73-75 chars, not ~64. > > Thanks! Sure, let me update v2. BRs, Jing ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-02-25 2:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-08 16:16 [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field Jing Liu 2021-02-08 15:23 ` Dave Hansen 2021-02-08 17:24 ` Sean Christopherson 2021-02-22 3:21 ` Liu, Jing2 2021-02-22 16:06 ` Sean Christopherson 2021-02-23 3:06 ` Liu, Jing2 2021-02-24 20:40 ` Sean Christopherson 2021-02-25 2:02 ` Liu, Jing2
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.