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 <[email protected]>
---
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
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?
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.
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.
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.
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
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!
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