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
