| Message ID | 1483533584-8015-9-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> wrote: > One check against EFER_SVME is replaced with the more appropriate cpu_has_svm, > when determining whether MSR bitmaps are available. I don't think this is correct - start_svm() may fail, in which case the CPUID flag doesn't get cleared, yet EFER.SVME also doesn't get set. How about comparing hvm_funcs (if not NULL) ->name against "SVM"? > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> Cc: Paul Durrant <paul.durrant@citrix.com> > --- a/xen/arch/x86/cpuid.c > +++ b/xen/arch/x86/cpuid.c > @@ -319,8 +319,21 @@ int init_domain_cpuid_policy(struct domain *d) > void guest_cpuid(const struct vcpu *v, unsigned int leaf, > unsigned int subleaf, struct cpuid_leaf *res) > { > + const struct domain *d = v->domain; > + > *res = EMPTY_LEAF; > > + /* > + * First pass: > + * - Dispatch the virtualised leaves to their respective handlers. > + */ > + switch ( leaf ) > + { > + case 0x40000000 ... 0x400000ff: > + if ( is_viridian_domain(d) ) > + return cpuid_viridian_leaves(v, leaf, subleaf, res); > + } Can we please have a break statement above here? > +void cpuid_viridian_leaves(const struct vcpu *v, unsigned int leaf, > + unsigned int subleaf, struct cpuid_leaf *res) > { > - struct domain *d = current->domain; > + const struct domain *d = v->domain; > > - if ( !is_viridian_domain(d) ) > - return 0; > + ASSERT(is_viridian_domain(d)); > + ASSERT(leaf >= 0x40000000 && leaf < 0x40000100); > > leaf -= 0x40000000; > - if ( leaf > 6 ) > - return 0; > > - *eax = *ebx = *ecx = *edx = 0; > switch ( leaf ) > { > case 0: > - *eax = 0x40000006; /* Maximum leaf */ > - *ebx = 0x7263694d; /* Magic numbers */ > - *ecx = 0x666F736F; > - *edx = 0x76482074; > + res->a = 0x40000006; /* Maximum leaf */ > + res->b = 0x7263694d; /* Magic numbers */ > + res->c = 0x666F736F; > + res->d = 0x76482074; > break; > + > case 1: > - *eax = 0x31237648; /* Version number */ > + res->a = 0x31237648; /* Version number */ > break; > + > case 2: > /* Hypervisor information, but only if the guest has set its > own version number. */ > if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 ) > break; > - *eax = 1; /* Build number */ > - *ebx = (xen_major_version() << 16) | xen_minor_version(); > - *ecx = 0; /* SP */ > - *edx = 0; /* Service branch and number */ > + res->a = 1; /* Build number */ > + res->b = (xen_major_version() << 16) | xen_minor_version(); I think the comments warrant the zeroing of ECX and EDX to be retained. Jan
On 04/01/17 15:24, Jan Beulich wrote: >>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> wrote: >> One check against EFER_SVME is replaced with the more appropriate cpu_has_svm, >> when determining whether MSR bitmaps are available. > I don't think this is correct - start_svm() may fail, in which case > the CPUID flag doesn't get cleared, yet EFER.SVME also doesn't > get set. How about comparing hvm_funcs (if not NULL) ->name > against "SVM"? Hmm. This shows that the same logical bug is present in the vmx side. Let me see about finding a better way of doing this. > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> > Cc: Paul Durrant <paul.durrant@citrix.com> Oops yes sorry. > >> --- a/xen/arch/x86/cpuid.c >> +++ b/xen/arch/x86/cpuid.c >> @@ -319,8 +319,21 @@ int init_domain_cpuid_policy(struct domain *d) >> void guest_cpuid(const struct vcpu *v, unsigned int leaf, >> unsigned int subleaf, struct cpuid_leaf *res) >> { >> + const struct domain *d = v->domain; >> + >> *res = EMPTY_LEAF; >> >> + /* >> + * First pass: >> + * - Dispatch the virtualised leaves to their respective handlers. >> + */ >> + switch ( leaf ) >> + { >> + case 0x40000000 ... 0x400000ff: >> + if ( is_viridian_domain(d) ) >> + return cpuid_viridian_leaves(v, leaf, subleaf, res); >> + } > Can we please have a break statement above here? I think this got lost in a rebase. The following patch makes it all sensible. I will adjust. > >> +void cpuid_viridian_leaves(const struct vcpu *v, unsigned int leaf, >> + unsigned int subleaf, struct cpuid_leaf *res) >> { >> - struct domain *d = current->domain; >> + const struct domain *d = v->domain; >> >> - if ( !is_viridian_domain(d) ) >> - return 0; >> + ASSERT(is_viridian_domain(d)); >> + ASSERT(leaf >= 0x40000000 && leaf < 0x40000100); >> >> leaf -= 0x40000000; >> - if ( leaf > 6 ) >> - return 0; >> >> - *eax = *ebx = *ecx = *edx = 0; >> switch ( leaf ) >> { >> case 0: >> - *eax = 0x40000006; /* Maximum leaf */ >> - *ebx = 0x7263694d; /* Magic numbers */ >> - *ecx = 0x666F736F; >> - *edx = 0x76482074; >> + res->a = 0x40000006; /* Maximum leaf */ >> + res->b = 0x7263694d; /* Magic numbers */ >> + res->c = 0x666F736F; >> + res->d = 0x76482074; >> break; >> + >> case 1: >> - *eax = 0x31237648; /* Version number */ >> + res->a = 0x31237648; /* Version number */ >> break; >> + >> case 2: >> /* Hypervisor information, but only if the guest has set its >> own version number. */ >> if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 ) >> break; >> - *eax = 1; /* Build number */ >> - *ebx = (xen_major_version() << 16) | xen_minor_version(); >> - *ecx = 0; /* SP */ >> - *edx = 0; /* Service branch and number */ >> + res->a = 1; /* Build number */ >> + res->b = (xen_major_version() << 16) | xen_minor_version(); > I think the comments warrant the zeroing of ECX and EDX to be > retained. Ok. ~Andrew
>>> On 04.01.17 at 16:36, <andrew.cooper3@citrix.com> wrote: > On 04/01/17 15:24, Jan Beulich wrote: >>>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> wrote: >>> One check against EFER_SVME is replaced with the more appropriate cpu_has_svm, >>> when determining whether MSR bitmaps are available. >> I don't think this is correct - start_svm() may fail, in which case >> the CPUID flag doesn't get cleared, yet EFER.SVME also doesn't >> get set. How about comparing hvm_funcs (if not NULL) ->name >> against "SVM"? > > Hmm. This shows that the same logical bug is present in the vmx side. Oh, indeed. I had assumed vmx_secondary_exec_control & Co would remain zero until after the last failure point, or get zeroed in case of failure. I think we need to make that happen, or else I think we have the same problem elsewhere. For the actual CPUID feature flags, otoh, I don't think we should clear them in these failure cases though. Jan
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index 36d11c0..c38e477 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -319,8 +319,21 @@ int init_domain_cpuid_policy(struct domain *d) void guest_cpuid(const struct vcpu *v, unsigned int leaf, unsigned int subleaf, struct cpuid_leaf *res) { + const struct domain *d = v->domain; + *res = EMPTY_LEAF; + /* + * First pass: + * - Dispatch the virtualised leaves to their respective handlers. + */ + switch ( leaf ) + { + case 0x40000000 ... 0x400000ff: + if ( is_viridian_domain(d) ) + return cpuid_viridian_leaves(v, leaf, subleaf, res); + } + /* {pv,hvm}_cpuid() have this expectation. */ ASSERT(v == current); diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 70afcc6..ce2785f 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3353,9 +3353,6 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, if ( !edx ) edx = &dummy; - if ( cpuid_viridian_leaves(input, eax, ebx, ecx, edx) ) - return; - if ( cpuid_hypervisor_leaves(input, count, eax, ebx, ecx, edx) ) return; diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c index f6abdd2..f2ac0ab 100644 --- a/xen/arch/x86/hvm/viridian.c +++ b/xen/arch/x86/hvm/viridian.c @@ -66,77 +66,74 @@ #define CPUID6A_MSR_BITMAPS (1 << 1) #define CPUID6A_NESTED_PAGING (1 << 3) -int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax, - unsigned int *ebx, unsigned int *ecx, - unsigned int *edx) +void cpuid_viridian_leaves(const struct vcpu *v, unsigned int leaf, + unsigned int subleaf, struct cpuid_leaf *res) { - struct domain *d = current->domain; + const struct domain *d = v->domain; - if ( !is_viridian_domain(d) ) - return 0; + ASSERT(is_viridian_domain(d)); + ASSERT(leaf >= 0x40000000 && leaf < 0x40000100); leaf -= 0x40000000; - if ( leaf > 6 ) - return 0; - *eax = *ebx = *ecx = *edx = 0; switch ( leaf ) { case 0: - *eax = 0x40000006; /* Maximum leaf */ - *ebx = 0x7263694d; /* Magic numbers */ - *ecx = 0x666F736F; - *edx = 0x76482074; + res->a = 0x40000006; /* Maximum leaf */ + res->b = 0x7263694d; /* Magic numbers */ + res->c = 0x666F736F; + res->d = 0x76482074; break; + case 1: - *eax = 0x31237648; /* Version number */ + res->a = 0x31237648; /* Version number */ break; + case 2: /* Hypervisor information, but only if the guest has set its own version number. */ if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 ) break; - *eax = 1; /* Build number */ - *ebx = (xen_major_version() << 16) | xen_minor_version(); - *ecx = 0; /* SP */ - *edx = 0; /* Service branch and number */ + res->a = 1; /* Build number */ + res->b = (xen_major_version() << 16) | xen_minor_version(); break; + case 3: /* Which hypervisor MSRs are available to the guest */ - *eax = (CPUID3A_MSR_APIC_ACCESS | - CPUID3A_MSR_HYPERCALL | - CPUID3A_MSR_VP_INDEX); + res->a = (CPUID3A_MSR_APIC_ACCESS | + CPUID3A_MSR_HYPERCALL | + CPUID3A_MSR_VP_INDEX); if ( !(viridian_feature_mask(d) & HVMPV_no_freq) ) - *eax |= CPUID3A_MSR_FREQ; + res->a |= CPUID3A_MSR_FREQ; if ( viridian_feature_mask(d) & HVMPV_time_ref_count ) - *eax |= CPUID3A_MSR_TIME_REF_COUNT; + res->a |= CPUID3A_MSR_TIME_REF_COUNT; if ( viridian_feature_mask(d) & HVMPV_reference_tsc ) - *eax |= CPUID3A_MSR_REFERENCE_TSC; + res->a |= CPUID3A_MSR_REFERENCE_TSC; break; + case 4: /* Recommended hypercall usage. */ if ( (d->arch.hvm_domain.viridian.guest_os_id.raw == 0) || (d->arch.hvm_domain.viridian.guest_os_id.fields.os < 4) ) break; - *eax = CPUID4A_RELAX_TIMER_INT; + res->a = CPUID4A_RELAX_TIMER_INT; if ( viridian_feature_mask(d) & HVMPV_hcall_remote_tlb_flush ) - *eax |= CPUID4A_HCALL_REMOTE_TLB_FLUSH; + res->a |= CPUID4A_HCALL_REMOTE_TLB_FLUSH; if ( !cpu_has_vmx_apic_reg_virt ) - *eax |= CPUID4A_MSR_BASED_APIC; - *ebx = 2047; /* long spin count */ + res->a |= CPUID4A_MSR_BASED_APIC; + res->b = 2047; /* long spin count */ break; + case 6: /* Detected and in use hardware features. */ if ( cpu_has_vmx_virtualize_apic_accesses ) - *eax |= CPUID6A_APIC_OVERLAY; - if ( cpu_has_vmx_msr_bitmap || (read_efer() & EFER_SVME) ) - *eax |= CPUID6A_MSR_BITMAPS; + res->a |= CPUID6A_APIC_OVERLAY; + if ( cpu_has_vmx_msr_bitmap || cpu_has_svm ) + res->a |= CPUID6A_MSR_BITMAPS; if ( hap_enabled(d) ) - *eax |= CPUID6A_NESTED_PAGING; + res->a |= CPUID6A_NESTED_PAGING; break; } - - return 1; } static void dump_guest_os_id(const struct domain *d) diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h index bdbccd5..6e062df 100644 --- a/xen/include/asm-x86/hvm/viridian.h +++ b/xen/include/asm-x86/hvm/viridian.h @@ -97,13 +97,8 @@ struct viridian_domain union viridian_reference_tsc reference_tsc; }; -int -cpuid_viridian_leaves( - unsigned int leaf, - unsigned int *eax, - unsigned int *ebx, - unsigned int *ecx, - unsigned int *edx); +void cpuid_viridian_leaves(const struct vcpu *v, unsigned int leaf, + unsigned int subleaf, struct cpuid_leaf *res); int wrmsr_viridian_regs(
... rather than from the legacy path. Update the API to match guest_cpuid(), and remove its dependence on current. One check against EFER_SVME is replaced with the more appropriate cpu_has_svm, when determining whether MSR bitmaps are available. Make use of guest_cpuid() unconditionally zeroing res to avoid repeated re-zeroing. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/cpuid.c | 13 ++++++++ xen/arch/x86/hvm/hvm.c | 3 -- xen/arch/x86/hvm/viridian.c | 65 ++++++++++++++++++-------------------- xen/include/asm-x86/hvm/viridian.h | 9 ++---- 4 files changed, 46 insertions(+), 44 deletions(-)