On Tue, Feb 18, 2020 at 01:44:17PM +0800, Xiaoyao Li wrote:
> On 2/17/2020 9:20 PM, Xiaoyao Li wrote:
> > On 2/17/2020 9:03 PM, Yang Weijiang wrote:
> > > On Mon, Feb 17, 2020 at 12:26:51PM +0800, Xiaoyao Li wrote:
> > > > On 2/11/2020 2:57 PM, Yang Weijiang wrote:
> > > > > CPUID.(EAX=DH, ECX={i}H i>=0) enumerates XSAVE related
> > > > > leaves/sub-leaves,
> > > > > +extern int host_xss;
> > > > > +u64 kvm_supported_xss(void)
> > > > > +{
> > > > > + return KVM_SUPPORTED_XSS & host_xss;
> > > > > +}
> > > > > +
> > > >
> > > > How about using a global variable, supported_xss, instead of
> > > > calculating the
> > > > mask on every call. Just like what Sean posted on
> > > > https://lore.kernel.org/kvm/20200201185218.24473-21-sean.j.christopherson@xxxxxxxxx/
> > > >
> > > >
> > > Thanks Xiaoyao for the comments!
> > > Good suggestion, I'll change it in next version.
> > >
> > > > > #define F(x) bit(X86_FEATURE_##x)
> > > > > int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> > > > > @@ -112,10 +118,17 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> > > > > vcpu->arch.guest_xstate_size = best->ebx =
> > > > > xstate_required_size(vcpu->arch.xcr0, false);
> > > > > }
> > > > > -
> > > > > best = kvm_find_cpuid_entry(vcpu, 0xD, 1);
> > > > > - if (best && (best->eax & (F(XSAVES) | F(XSAVEC))))
> > > > > - best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
> > > > > + if (best && (best->eax & (F(XSAVES) | F(XSAVEC)))) {
> > > > > + u64 xstate = vcpu->arch.xcr0 | vcpu->arch.ia32_xss;
> > > > > +
> > > > > + best->ebx = xstate_required_size(xstate, true);
> > > > > + vcpu->arch.guest_supported_xss =
> > > > > + (best->ecx | ((u64)best->edx << 32)) &
> > > > > + kvm_supported_xss();
> > > > > + } else {
> > > > > + vcpu->arch.guest_supported_xss = 0;
> > > > > + }
>
> also here should be something like below:
>
> if (best) {
> if (best->eax & (F(XSAVES) | F(XSAVEC)))) {
> u64 xstate = vcpu->arch.xcr0 | vcpu->arch.ia32_xss;
> best->ebx = xstate_required_size(xstate, true);
> }
>
> if (best->eax & F(XSAVES) {
> vcpu->arch.guest_supported_xss =
> (best->ecx | ((u64)best->edx << 32)) &
> kvm_supported_xss();
> } else {
> best->ecx = 0;
> best->edx = 0;
> vcpu->arch.guest_supported_xss = 0;
> }
> }
Right, to keep it consistent with CPUID enumeration, thanks for review!
>
> > > > > /*
> > > > > * The existing code assumes virtual address is
> > > > > 48-bit or 57-bit in the
> > > > > @@ -426,6 +439,56 @@ static inline void
> > > > > do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index)
> > > > > }
> > > > > }
> > > > > +static inline bool do_cpuid_0xd_mask(struct
> > > > > kvm_cpuid_entry2 *entry, int index)
> > > > > +{
> > > > > + unsigned int f_xsaves = kvm_x86_ops->xsaves_supported()
> > > > > ? F(XSAVES) : 0;
> > > > > + /* cpuid 0xD.1.eax */
> > > > > + const u32 kvm_cpuid_D_1_eax_x86_features =
> > > > > + F(XSAVEOPT) | F(XSAVEC) | F(XGETBV1) | f_xsaves;
> > > > > + u64 u_supported = kvm_supported_xcr0();
> > > > > + u64 s_supported = kvm_supported_xss();
> > > > > + u64 supported;
> > > > > +
> > > > > + switch (index) {
> > > > > + case 0:
> > > > > + if (!u_supported) {
> > > > > + entry->eax = 0;
> > > > > + entry->ebx = 0;
> > > > > + entry->ecx = 0;
> > > > > + entry->edx = 0;
> > > > > + return false;
> > > > > + }
> > > > > + entry->eax &= u_supported;
> > > > > + entry->ebx = xstate_required_size(u_supported, false);
> > > > > + entry->ecx = entry->ebx;
> > > > > + entry->edx &= u_supported >> 32;
> > > > > + break;
> > > > > + case 1:
> > > > > + supported = u_supported | s_supported;
> > > > > + entry->eax &= kvm_cpuid_D_1_eax_x86_features;
> > > > > + cpuid_mask(&entry->eax, CPUID_D_1_EAX);
> > > > > + entry->ebx = 0;
> > > > > + entry->edx &= s_supported >> 32;
> > > > > + entry->ecx &= s_supported;
> > > >
> > > > We'd better initialize msr_ia32_xss bitmap (entry->ecx &
> > > > entry-edx) as zeros
> > > > here.
> > > Hmm, explicit setting the MSR to 0 is good in this case, but there's
> > > implied
> > > flow to ensure guest MSR_IA32_XSS will be 0 if entry->ecx and
> > > entry->edx are 0s.
> > > In above kvm_update_cpuid(), vcpu->arch.guest_supported_xss is set to 0
> > > when they're 0s. this masks guest cannot set non-zero value to this
> > > MSR. And in kvm_vcpu_reset(), vcpu->arch.ia32_xss is initialized to 0,
> > > in kvm_load_guest_xsave_state() MSR_IA32_XSS is set to ia32_xss,
> > > therefore the MSR is kept to 0.
> >
> > Sorry, I think what I said "msr_ia32_xss bitmap" misled you.
> >
> > "msr_ia32_xss bitmap" is not MSR_IA32_XSS,
> > but the (entry->ecx | entry->edx >> 32) of cpuid.D_1
> >
> > I meant we'd better set entry->ecx and entry->edx to 0 here.
> >
> > > >
> > > > > + if (entry->eax & (F(XSAVES) | F(XSAVEC)))
> > > > > + entry->ebx = xstate_required_size(supported, true);
> > > >
> > > > And setup msr_ia32_xss bitmap based on the s_supported within
> > > > this condition
> > > > when F(XSAVES) is supported.
> > >
> >
> > And set entry->ecx & entry->edx only when F(XSAVES) is supported.
> >
> > > IIUC, both XSAVEC and XSAVES use compacted format of the extended
> > > region, so if XSAVEC is supported while XSAVES is not, guest still can
> > > get correct size, so in existing code the two bits are ORed.
> >
> > Yeah, but entry->ecx and entry->edx should be non-zero only when
> > F(XSAVES) is set.
> >
> > > >
> > > > > + break;
> > > > > + default:
> > > > > + supported = (entry->ecx & 0x1) ? s_supported : u_supported;
> > > > > + if (!(supported & (BIT_ULL(index)))) {
> > > > > + entry->eax = 0;
> > > > > + entry->ebx = 0;
> > > > > + entry->ecx = 0;
> > > > > + entry->edx = 0;
> > > > > + return false;
> > > > > + }
> > > > > + if (entry->ecx & 0x1)
> > > > > + entry->ebx = 0;
> > > > > + break;
> > > > > + }
> > > > > + return true;
> > > > > +}
> > > > > +
> > > > > static inline int __do_cpuid_func(struct kvm_cpuid_entry2
> > > > > *entry, u32 function,
> > > > > int *nent, int maxnent)
> > > > > {
> > > > > @@ -440,7 +503,6 @@ static inline int __do_cpuid_func(struct
> > > > > kvm_cpuid_entry2 *entry, u32 function,
> > > > > unsigned f_lm = 0;
> > > > > #endif
> > > > > unsigned f_rdtscp = kvm_x86_ops->rdtscp_supported() ?
> > > > > F(RDTSCP) : 0;
> > > > > - unsigned f_xsaves = kvm_x86_ops->xsaves_supported() ?
> > > > > F(XSAVES) : 0;
> > > > > unsigned f_intel_pt = kvm_x86_ops->pt_supported() ?
> > > > > F(INTEL_PT) : 0;
> > > > > /* cpuid 1.edx */
> > > > > @@ -495,10 +557,6 @@ static inline int
> > > > > __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32
> > > > > function,
> > > > > F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
> > > > > F(PMM) | F(PMM_EN);
> > > > > - /* cpuid 0xD.1.eax */
> > > > > - const u32 kvm_cpuid_D_1_eax_x86_features =
> > > > > - F(XSAVEOPT) | F(XSAVEC) | F(XGETBV1) | f_xsaves;
> > > > > -
> > > > > /* all calls to cpuid_count() should be made on the same cpu */
> > > > > get_cpu();
> > > > > @@ -639,38 +697,21 @@ static inline int
> > > > > __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32
> > > > > function,
> > > > > break;
> > > > > }
> > > > > case 0xd: {
> > > > > - int idx, i;
> > > > > - u64 supported = kvm_supported_xcr0();
> > > > > + int i, idx;
> > > > > - entry->eax &= supported;
> > > > > - entry->ebx = xstate_required_size(supported, false);
> > > > > - entry->ecx = entry->ebx;
> > > > > - entry->edx &= supported >> 32;
> > > > > - if (!supported)
> > > > > + if (!do_cpuid_0xd_mask(&entry[0], 0))
> > > > > break;
> > > > > -
> > > > > - for (idx = 1, i = 1; idx < 64; ++idx) {
> > > > > - u64 mask = ((u64)1 << idx);
> > > > > + for (i = 1, idx = 1; idx < 64; ++idx) {
> > > > > if (*nent >= maxnent)
> > > > > goto out;
> > > > > -
> > > > > do_host_cpuid(&entry[i], function, idx);
> > > > > - if (idx == 1) {
> > > > > - entry[i].eax &= kvm_cpuid_D_1_eax_x86_features;
> > > > > - cpuid_mask(&entry[i].eax, CPUID_D_1_EAX);
> > > > > - entry[i].ebx = 0;
> > > > > - if (entry[i].eax & (F(XSAVES)|F(XSAVEC)))
> > > > > - entry[i].ebx =
> > > > > - xstate_required_size(supported,
> > > > > - true);
> > > > > - } else {
> > > > > - if (entry[i].eax == 0 || !(supported & mask))
> > > > > - continue;
> > > > > - if (WARN_ON_ONCE(entry[i].ecx & 1))
> > > > > - continue;
> > > > > - }
> > > > > - entry[i].ecx = 0;
> > > > > - entry[i].edx = 0;
> > > > > +
> > > > > + if (entry[i].eax == 0 && entry[i].ebx == 0 &&
> > > > > + entry[i].ecx == 0 && entry[i].edx == 0)
> > > > > + continue;
> > > > > +
> > > > > + if (!do_cpuid_0xd_mask(&entry[i], idx))
> > > > > + continue;
> > > > > ++*nent;
> > > > > ++i;
> > > > > }
> > > >
> > >
> >
