| Message ID | 1474425470-3629-3-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote: > +static void vmx_pi_list_cleanup(struct vcpu *v) > +{ > + vmx_pi_list_remove(v); > +} Please avoid such a no-op wrapper - the caller can easily call vmx_pi_list_remove() directly. > @@ -215,13 +225,28 @@ void vmx_pi_hooks_assign(struct domain *d) > /* This function is called when pcidevs_lock is held */ > void vmx_pi_hooks_deassign(struct domain *d) > { > + struct vcpu *v; > + > if ( !iommu_intpost || !has_hvm_container_domain(d) ) > return; > > ASSERT(d->arch.hvm_domain.vmx.vcpu_block); > > + /* > + * Pausing the domain can make sure the vCPU is not > + * running and hence calling the hooks simultaneously > + * when deassigning the PI hooks and removing the vCPU > + * from the blocking list. > + */ > + domain_pause(d); > + > d->arch.hvm_domain.vmx.vcpu_block = NULL; > d->arch.hvm_domain.vmx.pi_do_resume = NULL; > + > + for_each_vcpu ( d, v ) > + vmx_pi_list_cleanup(v); > + > + domain_unpause(d); > } So you continue using pausing, and I continue to miss the argumentation of why you can't do without (even if previously the discussion was for patch 4, but it obviously applies here as well). Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Monday, September 26, 2016 7:47 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > devel@lists.xen.org > Subject: Re: [PATCH v4 2/6] VMX: Properly handle pi when all the assigned > devices are removed > > >>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote: > > +static void vmx_pi_list_cleanup(struct vcpu *v) > > +{ > > + vmx_pi_list_remove(v); > > +} > > Please avoid such a no-op wrapper - the caller can easily call > vmx_pi_list_remove() directly. > > > @@ -215,13 +225,28 @@ void vmx_pi_hooks_assign(struct domain *d) > > /* This function is called when pcidevs_lock is held */ > > void vmx_pi_hooks_deassign(struct domain *d) > > { > > + struct vcpu *v; > > + > > if ( !iommu_intpost || !has_hvm_container_domain(d) ) > > return; > > > > ASSERT(d->arch.hvm_domain.vmx.vcpu_block); > > > > + /* > > + * Pausing the domain can make sure the vCPU is not > > + * running and hence calling the hooks simultaneously > > + * when deassigning the PI hooks and removing the vCPU > > + * from the blocking list. > > + */ > > + domain_pause(d); > > + > > d->arch.hvm_domain.vmx.vcpu_block = NULL; > > d->arch.hvm_domain.vmx.pi_do_resume = NULL; > > + > > + for_each_vcpu ( d, v ) > > + vmx_pi_list_cleanup(v); > > + > > + domain_unpause(d); > > } > > So you continue using pausing, and I continue to miss the argumentation > of why you can't do without (even if previously the discussion was for > patch 4, but it obviously applies here as well). I think this case is slightly different. Here we need to call vmx_pi_list_cleanup() to remove the vCPU from the blocking list if it is on the list. However, this can be happened when vmx_vcpu_block() is called, hence we might incorrectly add the vcpu to the blocking list while the last device is detached from the domain. In fact, v2 gave some trick methods to handle this, and that was considered as hard to maintain, so George suggested to use pause/unpause for this case, and I also think it is easy and acceptable consider that devices detaching is not a frequent action. Thanks, Feng > > Jan
>>> On 28.09.16 at 08:50, <feng.wu@intel.com> wrote: > >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Monday, September 26, 2016 7:47 PM >> To: Wu, Feng <feng.wu@intel.com> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- >> devel@lists.xen.org >> Subject: Re: [PATCH v4 2/6] VMX: Properly handle pi when all the assigned >> devices are removed >> >> >>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote: >> > +static void vmx_pi_list_cleanup(struct vcpu *v) >> > +{ >> > + vmx_pi_list_remove(v); >> > +} >> >> Please avoid such a no-op wrapper - the caller can easily call >> vmx_pi_list_remove() directly. >> >> > @@ -215,13 +225,28 @@ void vmx_pi_hooks_assign(struct domain *d) >> > /* This function is called when pcidevs_lock is held */ >> > void vmx_pi_hooks_deassign(struct domain *d) >> > { >> > + struct vcpu *v; >> > + >> > if ( !iommu_intpost || !has_hvm_container_domain(d) ) >> > return; >> > >> > ASSERT(d->arch.hvm_domain.vmx.vcpu_block); >> > >> > + /* >> > + * Pausing the domain can make sure the vCPU is not >> > + * running and hence calling the hooks simultaneously >> > + * when deassigning the PI hooks and removing the vCPU >> > + * from the blocking list. >> > + */ >> > + domain_pause(d); >> > + >> > d->arch.hvm_domain.vmx.vcpu_block = NULL; >> > d->arch.hvm_domain.vmx.pi_do_resume = NULL; >> > + >> > + for_each_vcpu ( d, v ) >> > + vmx_pi_list_cleanup(v); >> > + >> > + domain_unpause(d); >> > } >> >> So you continue using pausing, and I continue to miss the argumentation >> of why you can't do without (even if previously the discussion was for >> patch 4, but it obviously applies here as well). > > I think this case is slightly different. Here we need to call > vmx_pi_list_cleanup() > to remove the vCPU from the blocking list if it is on the list. However, this > can be happened when vmx_vcpu_block() is called, hence we might incorrectly > add the vcpu to the blocking list while the last device is detached from the domain. > In fact, v2 gave some trick methods to handle this, and that was considered as > hard to maintain, so George suggested to use pause/unpause for this case, and I > also think it is easy and acceptable consider that devices detaching is not a > frequent action. Note how I said "I continue to miss the argumentation of why you can't do without" - I'm not opposed to pausing getting used here, but it needs to be at least briefly explained in the commit message. That's among other things so that (see that other thread) people can't later come and say "Hey, pausing is done in all sorts of situations, why won't you let me add some more pausing?" Jan
> >> > >> So you continue using pausing, and I continue to miss the argumentation > >> of why you can't do without (even if previously the discussion was for > >> patch 4, but it obviously applies here as well). > > > > I think this case is slightly different. Here we need to call > > vmx_pi_list_cleanup() > > to remove the vCPU from the blocking list if it is on the list. However, this > > can be happened when vmx_vcpu_block() is called, hence we might incorrectly > > add the vcpu to the blocking list while the last device is detached from the > domain. > > In fact, v2 gave some trick methods to handle this, and that was considered as > > hard to maintain, so George suggested to use pause/unpause for this case, > and I > > also think it is easy and acceptable consider that devices detaching is not a > > frequent action. > > Note how I said "I continue to miss the argumentation of why you > can't do without" - I'm not opposed to pausing getting used here, but > it needs to be at least briefly explained in the commit message. That's > among other things so that (see that other thread) people can't later > come and say "Hey, pausing is done in all sorts of situations, why > won't you let me add some more pausing?" Fair enough, I will elaborate a bit more on it. Thanks, Feng > > Jan
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 355936a..7305f40 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -158,14 +158,12 @@ static void vmx_pi_switch_to(struct vcpu *v) pi_clear_sn(pi_desc); } -static void vmx_pi_do_resume(struct vcpu *v) +static void vmx_pi_list_remove(struct vcpu *v) { unsigned long flags; spinlock_t *pi_blocking_list_lock; struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc; - ASSERT(!test_bit(_VPF_blocked, &v->pause_flags)); - /* * Set 'NV' field back to posted_intr_vector, so the * Posted-Interrupts can be delivered to the vCPU when @@ -173,12 +171,12 @@ static void vmx_pi_do_resume(struct vcpu *v) */ write_atomic(&pi_desc->nv, posted_intr_vector); - /* The vCPU is not on any blocking list. */ pi_blocking_list_lock = v->arch.hvm_vmx.pi_blocking.lock; /* Prevent the compiler from eliminating the local variable.*/ smp_rmb(); + /* The vCPU is not on any blocking list. */ if ( pi_blocking_list_lock == NULL ) return; @@ -198,6 +196,18 @@ static void vmx_pi_do_resume(struct vcpu *v) spin_unlock_irqrestore(pi_blocking_list_lock, flags); } +static void vmx_pi_do_resume(struct vcpu *v) +{ + ASSERT(!test_bit(_VPF_blocked, &v->pause_flags)); + + vmx_pi_list_remove(v); +} + +static void vmx_pi_list_cleanup(struct vcpu *v) +{ + vmx_pi_list_remove(v); +} + /* This function is called when pcidevs_lock is held */ void vmx_pi_hooks_assign(struct domain *d) { @@ -215,13 +225,28 @@ void vmx_pi_hooks_assign(struct domain *d) /* This function is called when pcidevs_lock is held */ void vmx_pi_hooks_deassign(struct domain *d) { + struct vcpu *v; + if ( !iommu_intpost || !has_hvm_container_domain(d) ) return; ASSERT(d->arch.hvm_domain.vmx.vcpu_block); + /* + * Pausing the domain can make sure the vCPU is not + * running and hence calling the hooks simultaneously + * when deassigning the PI hooks and removing the vCPU + * from the blocking list. + */ + domain_pause(d); + d->arch.hvm_domain.vmx.vcpu_block = NULL; d->arch.hvm_domain.vmx.pi_do_resume = NULL; + + for_each_vcpu ( d, v ) + vmx_pi_list_cleanup(v); + + domain_unpause(d); } static int vmx_domain_initialise(struct domain *d)
This patch handles some concern cases when the last assigned device is removed from the domain. In this case we should carefully handle pi descriptor and the per-cpu blocking list, to make sure: - all the PI descriptor are in the right state when next time a devices is assigned to the domain again. - No remaining vcpus of the domain in the per-cpu blocking list. Basically, we pause the domain before zapping the PI hooks and removing the vCPU from the blocking list, then unpause it after that. Signed-off-by: Feng Wu <feng.wu@intel.com> --- v4: - Rename some functions: vmx_pi_remove_vcpu_from_blocking_list() -> vmx_pi_list_remove() vmx_pi_blocking_cleanup() -> vmx_pi_list_cleanup() - Remove the check in vmx_pi_list_cleanup() - Comments adjustment xen/arch/x86/hvm/vmx/vmx.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)