| Message ID | 1472615791-8664-3-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 31.08.16 at 05:56, <feng.wu@intel.com> wrote: > 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> Looks plausible, but a) as already for patch 1 I'm missing information on what changed since v2 and b) doesn't this make unnecessary patch 1? Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Thursday, September 1, 2016 4:21 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 v3 2/6] VMX: Properly handle pi when all the assigned > devices are removed > > >>> On 31.08.16 at 05:56, <feng.wu@intel.com> wrote: > > 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> > > Looks plausible, but > a) as already for patch 1 I'm missing information on what changed > since v2 and The biggest changes since v2 is that we use domain pause/unpause (suggested by George) to handle the concern case, while v2 was using some ugly and tricky method to do it, which was considered as hard to maintain. > b) doesn't this make unnecessary patch 1? The purpose of patch 1 is to make sure the two hooks are installed while CPU side PI is available event VT-d PI is not supported, I cannot see why this patch will make it unnecessary. Thanks, Feng > > Jan
>>> On 01.09.16 at 11:22, <feng.wu@intel.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Thursday, September 1, 2016 4:21 PM >> >>> On 31.08.16 at 05:56, <feng.wu@intel.com> wrote: >> > 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> >> >> Looks plausible, but >> a) as already for patch 1 I'm missing information on what changed >> since v2 and > > The biggest changes since v2 is that we use domain pause/unpause > (suggested by George) to handle the concern case, while v2 was using > some ugly and tricky method to do it, which was considered as hard > to maintain. > >> b) doesn't this make unnecessary patch 1? > > The purpose of patch 1 is to make sure the two hooks are installed > while CPU side PI is available event VT-d PI is not supported, I cannot > see why this patch will make it unnecessary. So I guess this doesn't hold anymore with your subsequent reply to my comments on patch 1? Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Thursday, September 1, 2016 6:24 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 v3 2/6] VMX: Properly handle pi when all the assigned > devices are removed > > >>> On 01.09.16 at 11:22, <feng.wu@intel.com> wrote: > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Thursday, September 1, 2016 4:21 PM > >> >>> On 31.08.16 at 05:56, <feng.wu@intel.com> wrote: > >> > 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> > >> > >> Looks plausible, but > >> a) as already for patch 1 I'm missing information on what changed > >> since v2 and > > > > The biggest changes since v2 is that we use domain pause/unpause > > (suggested by George) to handle the concern case, while v2 was using > > some ugly and tricky method to do it, which was considered as hard > > to maintain. > > > >> b) doesn't this make unnecessary patch 1? > > > > The purpose of patch 1 is to make sure the two hooks are installed > > while CPU side PI is available event VT-d PI is not supported, I cannot > > see why this patch will make it unnecessary. > > So I guess this doesn't hold anymore with your subsequent reply > to my comments on patch 1? I think we still need patch 1, since we may pause a runnable vcpu which is in the run queue ('SN' bit is set), and 'SN' continues to be set when the vCPU is paused. If we zap these two hooks, the 'SN' can still be set after the vCPU is unpaused Thanks, Feng > > Jan
On Wed, 2016-08-31 at 11:56 +0800, Feng Wu wrote: > diff --git 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); > } > Not terribly important, but what about calling this: > -static void vmx_pi_do_resume(struct vcpu *v) > +static void vmx_pi_remove_vcpu_from_blocking_list(struct vcpu *v) vmx_pi_list_del() or vmx_pi_list_remove() > @@ -198,6 +196,21 @@ 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_remove_vcpu_from_blocking_list(v); > +} > + And this: > +static void vmx_pi_blocking_cleanup(struct vcpu *v) > vmx_pi_list_cleanup() etc.? I.e., using shorter and more consistent names. > +{ > + if ( !iommu_intpost ) > + return; > + At least as far as this patch is concerned, you are only calling this function from vmx_pi_hooks_deassing() which already checks at the very beginning iommu_intpost to be true, or it returns, and you won't get here. So you don't need to re-check the same thing here. Regards, Dario
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index f5d2d3c..b869728 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_remove_vcpu_from_blocking_list(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 @@ -198,6 +196,21 @@ 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_remove_vcpu_from_blocking_list(v); +} + +static void vmx_pi_blocking_cleanup(struct vcpu *v) +{ + if ( !iommu_intpost ) + return; + + vmx_pi_remove_vcpu_from_blocking_list(v); +} + /* This function is called when pcidevs_lock is held */ void vmx_pi_hooks_assign(struct domain *d) { @@ -213,13 +226,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_blocking_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> --- xen/arch/x86/hvm/vmx/vmx.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)