From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Tian, Kevin" Subject: Re: [RFC v1 11/15] vmx: Add a global wake-up vector for VT-d Posted-Interrupts Date: Wed, 8 Apr 2015 09:02:10 +0000 Message-ID: References: <1427286717-4093-1-git-send-email-feng.wu@intel.com> <1427286717-4093-12-git-send-email-feng.wu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Wu, Feng" , "xen-devel@lists.xen.org" Cc: "Zhang, Yang Z" , "keir@xen.org" , "JBeulich@suse.com" List-Id: xen-devel@lists.xenproject.org > From: Wu, Feng > Sent: Thursday, April 02, 2015 3:18 PM > > > -----Original Message----- > > From: Tian, Kevin > > Sent: Thursday, April 02, 2015 2:01 PM > > To: Wu, Feng; xen-devel@lists.xen.org > > Cc: JBeulich@suse.com; keir@xen.org; Zhang, Yang Z > > Subject: RE: [RFC v1 11/15] vmx: Add a global wake-up vector for VT-d > > Posted-Interrupts > > > > > From: Wu, Feng > > > Sent: Wednesday, March 25, 2015 8:32 PM > > > > > > This patch adds a global vector which is used to wake up > > > the blocked vCPU when an interrupt is being posted to it. > > > > > > Signed-off-by: Feng Wu > > > Suggested-by: Yang Zhang > > > --- > > > xen/arch/x86/hvm/vmx/vmx.c | 33 > > > +++++++++++++++++++++++++++++++++ > > > xen/include/asm-x86/hvm/hvm.h | 1 + > > > xen/include/asm-x86/hvm/vmx/vmx.h | 3 +++ > > > xen/include/xen/sched.h | 2 ++ > > > 4 files changed, 39 insertions(+) > > > > > > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > > > index ff5544d..b2b4c26 100644 > > > --- a/xen/arch/x86/hvm/vmx/vmx.c > > > +++ b/xen/arch/x86/hvm/vmx/vmx.c > > > @@ -89,6 +89,7 @@ DEFINE_PER_CPU(struct list_head, > > > blocked_vcpu_on_cpu); > > > DEFINE_PER_CPU(spinlock_t, blocked_vcpu_on_cpu_lock); > > > > > > uint8_t __read_mostly posted_intr_vector; > > > +uint8_t __read_mostly pi_wakeup_vector; > > > > > > static int vmx_domain_initialise(struct domain *d) > > > { > > > @@ -131,6 +132,8 @@ static int vmx_vcpu_initialise(struct vcpu *v) > > > if ( v->vcpu_id == 0 ) > > > v->arch.user_regs.eax = 1; > > > > > > + INIT_LIST_HEAD(&v->blocked_vcpu_list); > > > + > > > return 0; > > > } > > > > > > @@ -1834,11 +1837,19 @@ const struct hvm_function_table * __init > > > start_vmx(void) > > > } > > > > > > if ( cpu_has_vmx_posted_intr_processing ) > > > + { > > > alloc_direct_apic_vector(&posted_intr_vector, > > > event_check_interrupt); > > > + > > > + if ( iommu_intpost ) > > > + alloc_direct_apic_vector(&pi_wakeup_vector, > > > pi_wakeup_interrupt); > > > + else > > > + vmx_function_table.pi_desc_update = NULL; > > > + } > > > > just style issue. Above conditional logic looks not intuitive to me. > > usually we have: > > if ( iommu_intpost ) > > vmx_function_table.pi_desc_update = func; > > else > > vmx_function_table.pi_desc_update = NULL; > > > > suppose you will register callback in later patch. then better to > > move the NULL one there too. Putting it here doesn't meet the > > normal if...else implications. :-) > > You suggestion is good. Here is my idea about this code fragment: > > Here is the place to register notification event handle, so it is better > to register the wakeup event handle for VT-d PI here as well. Just like other > members in vmx_function_table, such as, deliver_posted_intr, sync_pir_to_irr, > pi_desc_update is initialed to 'vmx_pi_desc_update' in the definition of > vmx_function_table statically. So do you have any ideas to make this > gracefully? > I didn't see the problem exactly. If the point is to register callback here, then you can register it here w/ intpost and NULL w/o intpost. or if there is some dependency so you must do registration later, then do it later. I just don't understand why you register a NULL for no-intpost only here :-) > > > > > > else > > > { > > > vmx_function_table.deliver_posted_intr = NULL; > > > vmx_function_table.sync_pir_to_irr = NULL; > > > + vmx_function_table.pi_desc_update = NULL; > > > } > > > > > > if ( cpu_has_vmx_ept > > > @@ -3255,6 +3266,28 @@ void vmx_vmenter_helper(const struct > > > cpu_user_regs *regs) > > > } > > > > > > /* > > > + * Handle VT-d posted-interrupt when VCPU is blocked. > > > + */ > > > +void pi_wakeup_interrupt(struct cpu_user_regs *regs) > > > +{ > > > + struct vcpu *v; > > > + int cpu = smp_processor_id(); > > > + > > > + spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, cpu)); > > > + list_for_each_entry(v, &per_cpu(blocked_vcpu_on_cpu, cpu), > > > + blocked_vcpu_list) { > > > + struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc; > > > + > > > + if ( pi_test_on(pi_desc) == 1 ) > > > + tasklet_schedule(&v->vcpu_wakeup_tasklet); > > > > why can't we directly call vcpu_unblock here? > > Please see the following scenario if we use vcpu_unblock directly here: > > pi_wakeup_interrupt() (blocked_vcpu_on_cpu_lock is required) --> > vcpu_unblock() --> > vcpu_wake() --> vcpu_runstate_change() --> vmx_ pi_desc_update() (In this > function we > may need to require blocked_vcpu_on_cpu_lock, this will cause dead lock.) > yes, it makes sense. Thanks Kevin