>>> On 02.09.16 at 10:40, <feng...@intel.com> wrote: > >> -----Original Message----- >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: Friday, September 2, 2016 4:16 PM >> To: Wu, Feng <feng...@intel.com> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> george.dun...@eu.citrix.com; Tian, Kevin <kevin.t...@intel.com>; xen- >> de...@lists.xen.org >> Subject: RE: [PATCH v3 4/6] Pause/Unpause the domain before/after assigning >> PI hooks >> >> >>> On 02.09.16 at 09:31, <feng...@intel.com> wrote: >> >> > >> >> -----Original Message----- >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> Sent: Friday, September 2, 2016 3:04 PM >> >> To: Wu, Feng <feng...@intel.com> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> george.dun...@eu.citrix.com; Tian, Kevin <kevin.t...@intel.com>; xen- >> >> de...@lists.xen.org >> >> Subject: RE: [PATCH v3 4/6] Pause/Unpause the domain before/after >> assigning >> >> PI hooks >> >> >> >> >>> On 02.09.16 at 03:46, <feng...@intel.com> wrote: >> >> >> >> > >> >> >> -----Original Message----- >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> >> Sent: Thursday, September 1, 2016 4:30 PM >> >> >> To: Wu, Feng <feng...@intel.com> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> >> george.dun...@eu.citrix.com; Tian, Kevin <kevin.t...@intel.com>; xen- >> >> >> de...@lists.xen.org >> >> >> Subject: Re: [PATCH v3 4/6] Pause/Unpause the domain before/after >> >> assigning >> >> >> PI hooks >> >> >> >> >> >> >>> On 31.08.16 at 05:56, <feng...@intel.com> 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. > > Actually here is the scenario I am concerned about: > 1. ' vmx_vcpu_block ' is installed while vCPU is running vcpu_block() > and then vmx_vcpu_block(). > 2. There is a ASSERT() about 'NDST' field in vmx_vcpu_block(), I think > we may hit the ASSERT() since 'NDST' may not have been set to the > current processor yet. > > My previous solution in v2 is to delete that ASSERT(), but seems you > guys don't like it. So here I use this new method in v3 to make sure > the vCPU is running while we are installing the hooks.
Indeed, deleting the assertion doesn't seem right. But then why can't vmx_vcpu_block() bail early when the domain has no devices assigned? That would allow for 1) set blocking hook 2) set up PI state 3) actually assign device Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
