| Message ID | 1483163161-2402-11-git-send-email-suravee.suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> > +static void avic_dump(unsigned char ch) > +{ > + struct domain *d; > + struct vcpu *v; > + > + printk("*********** SVM AVIC Statistics **************\n"); > + > + rcu_read_lock(&domlist_read_lock); > + > + for_each_domain ( d ) > + { > + if ( !is_hvm_domain(d) ) || !svm_avic_vcpu_enabled(d->v[0]) (or something along these lines)? > + continue; > + printk(">>> Domain %d <<<\n", d->domain_id); > + for_each_vcpu ( d, v ) > + { > + printk("\tVCPU %d\n", v->vcpu_id); > + printk("\t* incomp_ipi = %u\n", > + v->arch.hvm_svm.cnt_avic_incomp_ipi); > + printk("\t* noaccel = %u\n", > + v->arch.hvm_svm.cnt_avic_noaccel); > + printk("\t* post_intr = %u\n", > + v->arch.hvm_svm.cnt_avic_post_intr); > + printk("\t* doorbell = %u\n", > + v->arch.hvm_svm.cnt_avic_doorbell); > + } > + } > + > + rcu_read_unlock(&domlist_read_lock); > + > + printk("**************************************\n"); > + > +} > + > +void __init setup_avic_dump(void) > +{ if ( !svm_avic ) return; ? -boris > + register_keyhandler('j', avic_dump, "dump SVM AVIC", 1); > +} > +
On 03/01/17 16:01, Boris Ostrovsky wrote: >> >> +static void avic_dump(unsigned char ch) >> +{ >> + struct domain *d; >> + struct vcpu *v; >> + >> + printk("*********** SVM AVIC Statistics **************\n"); >> + >> + rcu_read_lock(&domlist_read_lock); >> + >> + for_each_domain ( d ) >> + { >> + if ( !is_hvm_domain(d) ) > || !svm_avic_vcpu_enabled(d->v[0]) (or something along these lines)? It isn't safe to deference the vcpu array like this, which in turn highlights that the avic predicate should be per-domain, not per-cpu. Under no circumstances should we have AVIC on some vcpus but not others of the same domain. ~Andrew
Hi Boris/Andrew, On 1/3/17 23:04, Andrew Cooper wrote: > On 03/01/17 16:01, Boris Ostrovsky wrote: >>> >>> +static void avic_dump(unsigned char ch) >>> +{ >>> + struct domain *d; >>> + struct vcpu *v; >>> + >>> + printk("*********** SVM AVIC Statistics **************\n"); >>> + >>> + rcu_read_lock(&domlist_read_lock); >>> + >>> + for_each_domain ( d ) >>> + { >>> + if ( !is_hvm_domain(d) ) >> || !svm_avic_vcpu_enabled(d->v[0]) (or something along these lines)? > > It isn't safe to deference the vcpu array like this, which in turn > highlights that the avic predicate should be per-domain, not per-cpu. > Under no circumstances should we have AVIC on some vcpus but not others > of the same domain. > > ~Andrew > Let me add something like: static inline bool svm_is_avic_domain(struct domain *d) { return ( d->arch.hvm_domain.svm.avic_physical_id_table != 0 ); } This should allow us to check whether the svm_avic_dom_init() is enabled successfully. Thanks, Suravee
>>> On 31.12.16 at 06:46, <suravee.suthikulpanit@amd.com> wrote: > +void __init setup_avic_dump(void) > +{ > + register_keyhandler('j', avic_dump, "dump SVM AVIC", 1); > +} For one the description should include the word "stats". And then I'm rather uncertain this is work burning one of the few remaining available keys. Could this be made a domctl instead? Jan
Jan, On 01/05/2017 11:07 PM, Jan Beulich wrote: >>>> On 31.12.16 at 06:46, <suravee.suthikulpanit@amd.com> wrote: >> +void __init setup_avic_dump(void) >> +{ >> + register_keyhandler('j', avic_dump, "dump SVM AVIC", 1); >> +} > > For one the description should include the word "stats". And then > I'm rather uncertain this is work burning one of the few remaining > available keys. Could this be made a domctl instead? > > Jan > Not sure how you are thinking about using domctl to output statistics. Are there examples? Could you please describe? Thanks, Suravee
>>> On 10.01.17 at 12:14, <Suravee.Suthikulpanit@amd.com> wrote: > On 01/05/2017 11:07 PM, Jan Beulich wrote: >>>>> On 31.12.16 at 06:46, <suravee.suthikulpanit@amd.com> wrote: >>> +void __init setup_avic_dump(void) >>> +{ >>> + register_keyhandler('j', avic_dump, "dump SVM AVIC", 1); >>> +} >> >> For one the description should include the word "stats". And then >> I'm rather uncertain this is work burning one of the few remaining >> available keys. Could this be made a domctl instead? > > Not sure how you are thinking about using domctl to output statistics. > Are there examples? Could you please describe? Provide a domctl to obtain the values, and a new xl command to wrap that domctl. Jan
diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c index faa5e45..1aea724 100644 --- a/xen/arch/x86/hvm/svm/avic.c +++ b/xen/arch/x86/hvm/svm/avic.c @@ -27,6 +27,7 @@ #include <asm/hvm/support.h> #include <asm/hvm/svm/avic.h> #include <asm/hvm/vlapic.h> +#include <xen/keyhandler.h> #include <asm/p2m.h> #include <asm/page.h> @@ -320,6 +321,8 @@ void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs) u32 id = vmcb->exitinfo2 >> 32; u32 index = vmcb->exitinfo2 && 0xFF; + curr->arch.hvm_svm.cnt_avic_incomp_ipi++; + switch ( id ) { case AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE: @@ -580,6 +583,8 @@ void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs) u32 offset = vmcb->exitinfo1 & 0xFF0; u32 rw = (vmcb->exitinfo1 >> 32) & 0x1; + curr->arch.hvm_svm.cnt_avic_noaccel++; + switch ( offset ) { case APIC_ID: @@ -654,16 +659,59 @@ void svm_avic_deliver_posted_intr(struct vcpu *v, u8 vec) if ( vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) ) return; + v->arch.hvm_svm.cnt_avic_post_intr++; /* * If vcpu is running on another cpu, hit the doorbell to signal * it to process interrupt. Otherwise, kick it. */ if ( v->is_running && (v != current) ) + { wrmsrl(AVIC_DOORBELL, cpu_data[v->processor].apicid); + v->arch.hvm_svm.cnt_avic_doorbell++; + } else vcpu_kick(v); } +static void avic_dump(unsigned char ch) +{ + struct domain *d; + struct vcpu *v; + + printk("*********** SVM AVIC Statistics **************\n"); + + rcu_read_lock(&domlist_read_lock); + + for_each_domain ( d ) + { + if ( !is_hvm_domain(d) ) + continue; + printk(">>> Domain %d <<<\n", d->domain_id); + for_each_vcpu ( d, v ) + { + printk("\tVCPU %d\n", v->vcpu_id); + printk("\t* incomp_ipi = %u\n", + v->arch.hvm_svm.cnt_avic_incomp_ipi); + printk("\t* noaccel = %u\n", + v->arch.hvm_svm.cnt_avic_noaccel); + printk("\t* post_intr = %u\n", + v->arch.hvm_svm.cnt_avic_post_intr); + printk("\t* doorbell = %u\n", + v->arch.hvm_svm.cnt_avic_doorbell); + } + } + + rcu_read_unlock(&domlist_read_lock); + + printk("**************************************\n"); + +} + +void __init setup_avic_dump(void) +{ + register_keyhandler('j', avic_dump, "dump SVM AVIC", 1); +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index 7c0cda0..b8861d8 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1456,6 +1456,7 @@ const struct hvm_function_table * __init start_svm(void) } setup_vmcb_dump(); + setup_avic_dump(); svm_feature_flags = (current_cpu_data.extended_cpuid_level >= 0x8000000A ? cpuid_edx(0x8000000A) : 0); diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h index 9abf077..e2b810e 100644 --- a/xen/include/asm-x86/hvm/svm/vmcb.h +++ b/xen/include/asm-x86/hvm/svm/vmcb.h @@ -552,6 +552,12 @@ struct arch_svm_struct { struct avic_phy_apic_id_ent *avic_last_phy_id; u32 avic_last_ldr; + + /* AVIC Statistics */ + u32 cnt_avic_incomp_ipi; + u32 cnt_avic_noaccel; + u32 cnt_avic_post_intr; + u32 cnt_avic_doorbell; }; struct vmcb_struct *alloc_vmcb(void);