Re: [Xen-devel] [PATCH v4 2/6] VMX: Properly handle pi when all the assigned devices are removed

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, September 26, 2016 7:47 PM
> To: Wu, Feng <feng.wu@xxxxxxxxx>
> Cc: andrew.cooper3@xxxxxxxxxx; dario.faggioli@xxxxxxxxxx;
> george.dunlap@xxxxxxxxxxxxx; Tian, Kevin <kevin.tian@xxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxx
> 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@xxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel