Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.09.16 at 09:31, <feng.wu@xxxxxxxxx> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Friday, September 2, 2016 3:04 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 v3 4/6] Pause/Unpause the domain before/after assigning
>> PI hooks
>> 
>> >>> On 02.09.16 at 03:46, <feng.wu@xxxxxxxxx> wrote:
>> 
>> >
>> >> -----Original Message-----
>> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> >> Sent: Thursday, September 1, 2016 4:30 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 v3 4/6] Pause/Unpause the domain before/after
>> assigning
>> >> PI hooks
>> >>
>> >> >>> On 31.08.16 at 05:56, <feng.wu@xxxxxxxxx> wrote:
>> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> >> > @@ -219,8 +219,19 @@ void vmx_pi_hooks_assign(struct domain *d)
>> >> >
>> >> >      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. This makes sure that
>> >> > +     * all the appropriate state of PI descriptor is actually
>> >> > +     * set up for all vCPus before leaving this function.
>> >> > +     */
>> >> > +    domain_pause(d);
>> >> > +
>> >> >      d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block;
>> >> >      d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume;
>> >> > +
>> >> > +    domain_unpause(d);
>> >> >  }
>> >>
>> >> First of all I'm missing a word on whether the race mentioned in
>> >> the description and comment can actually happen. Device
>> >> (de)assignment should already be pretty much serialized (via
>> >> the domctl lock, and maybe also via the pcidevs one).
>> >
>> > The purpose of this patch is to address the race condition that
>> > the _vCPU_ is running while we are installing these hooks. Do you
>> > think this cannot happen?  This patch is trying to fix the issue
>> > described at:
>> > http://www.gossamer-threads.com/lists/xen/devel/433229 
>> > Consider that the other two hooks were installed when the VM
>> > is created, seems no such race condition. However, according
>> > to the discussion about patch 1 and patch 2 of series, we need
>> > to install the other two hooks here as well,
>> 
>> I don't think we've agreed that the creation time installation of
>> those hooks is actually necessary. In fact your most recent
>> response to patch 1 makes me think you now agree we don't
>> need to do so. And hence with that precondition not holding
>> anymore, I don't think the conclusion does.
> 
> I think there might be some confusion. Let me explain what I
> am think of to make sure we are on the same page:
> 1. We need install all the four hooks when the first device is
> assigned.
> 2. If _1_ is true, the issue described in
> http://www.gossamer-threads.com/lists/xen/devel/433229 
> exists.

If you mean this

* vcpu 0 starts running on a pcpu
* a device is assigned, causing the hooks to be set
* an interrupt from the device is routed to vcpu 0, but it is not
actually delivered properly, since ndst is not pointing to the right
processor.

raised by George, then I'm not convinced it can happen (after all, the
hooks get set _before_ the device gets assigned, and hence before
the device can raise an interrupt destined at the guest). And if it can
happen, then rather than pausing the guest I don't see why, along
with setting the hooks, any possibly affected NDST field can't be
programmed correctly. ISTR having recommended something like
this already during review of the series originally introducing PI.

Jan

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