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

From: "Jan Beulich" <JBeulich@suse.com>
To: Feng Wu <feng.wu@intel.com>
Cc: "george.dunlap@eu.citrix.com" <george.dunlap@eu.citrix.com>,
	"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
	"dario.faggioli@citrix.com" <dario.faggioli@citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Subject: Re: [PATCH v4 2/6] VMX: Properly handle pi when all the assigned devices are removed
Date: Wed, 28 Sep 2016 03:52:09 -0600	[thread overview]
Message-ID: <57EBAEE90200007800113370@prv-mh.provo.novell.com> (raw)
In-Reply-To: <E959C4978C3B6342920538CF579893F01988049B@SHSMSX104.ccr.corp.intel.com>

>>> 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

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