* [PATCH v2] KVM: x86: Revise guest_fpu xcomp_bv field @ 2021-02-25 10:49 Jing Liu 2021-03-01 23:59 ` Sean Christopherson 0 siblings, 1 reply; 3+ messages in thread From: Jing Liu @ 2021-02-25 10:49 UTC (permalink / raw) To: pbonzini, seanjc Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm, linux-kernel 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 enabled by the host kernel. Use xfeatures_mask_all for calculating xcomp_bv and reuse XCOMP_BV_COMPACTED_FORMAT defined by kernel. 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] 3+ messages in thread
* Re: [PATCH v2] KVM: x86: Revise guest_fpu xcomp_bv field 2021-02-25 10:49 [PATCH v2] KVM: x86: Revise guest_fpu xcomp_bv field Jing Liu @ 2021-03-01 23:59 ` Sean Christopherson 2021-03-09 5:31 ` Liu, Jing2 0 siblings, 1 reply; 3+ messages in thread From: Sean Christopherson @ 2021-03-01 23:59 UTC (permalink / raw) To: Jing Liu Cc: pbonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm, linux-kernel On Thu, Feb 25, 2021, Jing Liu 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 enabled by the host > kernel. Use xfeatures_mask_all for calculating xcomp_bv and reuse > XCOMP_BV_COMPACTED_FORMAT defined by kernel. > > 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; Doesn't fill_xsave also need to be updated? Not with xfeatures_mask_all, but to account for arch.ia32_xss? I believe it's a nop with the current code, since supported_xss is zero, but it should be fixed, no? > > /* > * 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; Ugh, this _really_ needs a comment in the changelog. It took me a while to realize fpstate_init() does exactly what the new fill_xave() is doing. And isn't the code in load_xsave() redundant and can be removed? Any code that uses get_xsave_addr() would be have a dependency on load_xsave() if it's not redundant, and I can't see how that would work. > > /* > * Ensure guest xcr0 is valid for loading > -- > 2.18.4 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] KVM: x86: Revise guest_fpu xcomp_bv field 2021-03-01 23:59 ` Sean Christopherson @ 2021-03-09 5:31 ` Liu, Jing2 0 siblings, 0 replies; 3+ messages in thread From: Liu, Jing2 @ 2021-03-09 5:31 UTC (permalink / raw) To: Sean Christopherson Cc: pbonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, kvm, linux-kernel On 3/2/2021 7:59 AM, Sean Christopherson wrote: > On Thu, Feb 25, 2021, Jing Liu 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 enabled by the host >> kernel. Use xfeatures_mask_all for calculating xcomp_bv and reuse >> XCOMP_BV_COMPACTED_FORMAT defined by kernel. >> >> 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; > Doesn't fill_xsave also need to be updated? Not with xfeatures_mask_all, but > to account for arch.ia32_xss? I believe it's a nop with the current code, since > supported_xss is zero, but it should be fixed, no? Yes. For the arch.ia32_xss, I noticed CET (https://lkml.org/lkml/2020/7/15/1412) has posted related change so I didn't touch xstate_bv for fill_xsave for now. Finally, fill_xsave() need e.g. arch.guest_supported_xss for xstate_bv, for xcomp_bv, xfeatures_mask_all is ok. > >> >> /* >> * 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; > Ugh, this _really_ needs a comment in the changelog. It took me a while to > realize fpstate_init() does exactly what the new fill_xave() is doing. How about introducing that "fx_init()->fpstate_init() initializes xcomp_bv of guest_fpu so no need to set again in later fill_xsave() and load_xsave()" in commit message? > > And isn't the code in load_xsave() redundant and can be removed? Oh, yes. Keep fx_init() initializing xcomp_bv for guest_fpu is enough. Let me remove it in load_xsave later. And for fill_xsave(), I think no need to set xcomp_bv there. > Any code that > uses get_xsave_addr() would be have a dependency on load_xsave() if it's not > redundant, and I can't see how that would work. Sorry I didn't quite understand why get_xsave_addr() has dependency on load_xsave(), do you mean the xstate_bv instead of xcomp_bv, that load_xsave() uses it to get the addr? Thanks, Jing > >> >> /* >> * Ensure guest xcr0 is valid for loading >> -- >> 2.18.4 >> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-09 5:32 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-25 10:49 [PATCH v2] KVM: x86: Revise guest_fpu xcomp_bv field Jing Liu 2021-03-01 23:59 ` Sean Christopherson 2021-03-09 5:31 ` 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.