[Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
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. Signed-off-by: Feng Wu --- xen/arch/x86/hvm/vmx/vmx.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 37fa2f1..071c063 100644 --- 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); } /* This function is called when pcidevs_lock is held */ -- 2.1.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
> -Original Message- > From: Wu, Feng > Sent: Monday, September 5, 2016 11:12 AM > To: Jan Beulich > Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > george.dun...@eu.citrix.com; Tian, Kevin ; xen- > de...@lists.xen.org; Wu, Feng > Subject: RE: [PATCH v3 4/6] Pause/Unpause the domain before/after assigning > PI hooks > > > > > -Original Message- > > From: Jan Beulich [mailto:jbeul...@suse.com] > > Sent: Friday, September 2, 2016 9:55 PM > > To: Wu, Feng > > Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > > george.dun...@eu.citrix.com; Tian, Kevin ; 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 15:15, wrote: > > > > > > > >> -Original Message- > > >> From: Jan Beulich [mailto:jbeul...@suse.com] > > >> Sent: Friday, September 2, 2016 6:46 PM > > >> To: Wu, Feng > > >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > > >> george.dun...@eu.citrix.com; Tian, Kevin ; 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 12:30, wrote: > > >> > > >> > > > >> >> -Original Message- > > >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > > >> >> Sent: Friday, September 2, 2016 5:26 PM > > >> >> To: Wu, Feng > > >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > > >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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 10:40, wrote: > > >> >> > > >> >> > > > >> >> >> -Original Message- > > >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > > >> >> >> Sent: Friday, September 2, 2016 4:16 PM > > >> >> >> To: Wu, Feng > > >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > > >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; > > 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, wrote: > > >> >> >> > > >> >> >> > > > >> >> >> >> -Original Message- > > >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > > >> >> >> >> Sent: Friday, September 2, 2016 3:04 PM > > >> >> >> >> To: Wu, Feng > > >> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > > >> >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; > > >> 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, wrote: > > >> >> >> >> > > >> >> >> >> > > > >> >> >> >> >> -Original Message- > > >> >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > > >> >> >> >> >> Sent: Thursday, September 1, 2016 4:30 PM > > >> >> >> >> >> To: Wu, Feng > > >> >> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > > >> >> >> >> >> george.dun...@eu.citrix.com; Tian, Kevin > > ; > > >> >> 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, 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 pcidev
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
>>> On 14.09.16 at 04:23, wrote: >> From: Wu, Feng >> Sent: Monday, September 5, 2016 11:12 AM >> I still have trouble to fully understand this, please see the following >> scenario: >> >> 1) Set 'NDST' to the pCPU0 (which vCPU is currently running on, but >> it may changes since vCPU scheduling happens behind us, so how to >> decide which pCPU for 'NDST'?) >> 2) Install all four hooks and vCPU is re-scheduled before >> 'vmx_pi_switch_to()' gets installed, so the pCPU of the vCPU might >> be changed to pCPU1 without 'NDST' gets updated. >> 4) vmx_vcpu_block() gets called and we hit the ASSERT() >> >> Maybe I missed something, It would be appreciated if you can >> correct me if my understanding is wrong. > > My email system had some problems, hence some emails didn't go > to my index, but I found your replay from the Internet as below: > > " Well, the problem is that you keep trying to find issues with the > (simplified) sequence of things I'm proposing to investigate as > an alternative. The above scenario _again_ can be dealt with > without having to pause the guest: Install the context switch > hook(s) first, then set NDST, then set the block hook. So what > I'd really like to ask you to do is first try to find a model without > pausing, and only if you figure that's impossible, then explain > why and we'll go with the pausing approach. > > Jan" > > Then I tried to implement the function like the following: > > /* This function is called when pcidevs_lock is held */ > void vmx_pi_hooks_assign(struct domain *d) > { > if ( !iommu_intpost || !has_hvm_container_domain(d) ) > return; > > ASSERT(!d->arch.hvm_domain.vmx.vcpu_block); > > /* > * We carefully handle the timing here: > * - Install the context switch first > * - Then set the NDST field > * - Install the block and resume hooks in the end > * > * This can make sure the PI (especially the NDST feild) is > * in proper state when we call vmx_vcpu_block(). > */ > d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; > d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; > > for_each_vcpu ( d, v ) > { > unsigned int dest = cpu_physical_id(v->processor); > struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc; > > /* spot 1 */ > > write_atomic(&pi_desc->ndst, > x2apic_enabled ? dest : MASK_INSR(dest, > PI_xAPIC_NDST_MASK)); > } > > d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block; > d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume; > } > > However, I still think it is problematic. Consider the _spot 1_ above, we get > the pCPU of the vCPU and update the NDST, but the vCPU can be happily > rescheduled to another pCPU before updating the NDST. The problem is > that it is not safe to update NDST here, since vCPU can be scheduled > behind us at any time. And if this is the case, it is hard to safely do this > without guarantee the vCPU is in a known state. Any further advice > on this? Thanks a lot! Well, yes, there still is a problem, but (sadly) also yes, you continue to follow the pattern I mentioned in my earlier reply: Instead of trying to find a suitable ordering of things, you simply complain about the (necessarily simplified) suggestion I gave. For example, is v->arch.hvm_vmx.pi_desc perhaps in a known never-touched state upon entering vmx_pi_hooks_assign() the very first time? In that case instead of the write_atomic() you use in the code above you could cmpxchg() against that known initial value, avoiding the write here if one has already happened in vmx_pi_switch_to(). (And before you ask, please also consider writing a known-invalid NDST before assigning the context switch hooks, and cmpxchg() against that one, in case the initial state isn't sufficiently distinguishable. Using an invalid value here shouldn't matter as no notifications should arrive prior to any device getting assigned to the guest.) Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
On Wed, 2016-09-14 at 02:23 +, Wu, Feng wrote: > Then I tried to implement the function like the following: > > /* This function is called when pcidevs_lock is held */ > void vmx_pi_hooks_assign(struct domain *d) > { > if ( !iommu_intpost || !has_hvm_container_domain(d) ) > return; > > ASSERT(!d->arch.hvm_domain.vmx.vcpu_block); > > /* > * We carefully handle the timing here: > * - Install the context switch first > * - Then set the NDST field > * - Install the block and resume hooks in the end > * > * This can make sure the PI (especially the NDST feild) is > * in proper state when we call vmx_vcpu_block(). > */ > d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; > d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; > > for_each_vcpu ( d, v ) > { > unsigned int dest = cpu_physical_id(v->processor); > struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc; > > /* spot 1 */ > > write_atomic(&pi_desc->ndst, > x2apic_enabled ? dest : MASK_INSR(dest, > PI_xAPIC_NDST_MASK)); > } > > d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block; > d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume; > } > > However, I still think it is problematic. Consider the _spot 1_ > above, we get > the pCPU of the vCPU and update the NDST, but the vCPU can be happily > rescheduled to another pCPU before updating the NDST. The problem is > that it is not safe to update NDST here, since vCPU can be scheduled > behind us at any time. And if this is the case, it is hard to safely > do this > without guarantee the vCPU is in a known state. Any further advice > on this? Thanks a lot! > So, I'm sorry if this is me missing or not remembering something... but I do remember that, at some point, in the early days of this series, there were concerns about the use of v->processor behind the back of the scheduler, i.e., without holding the proper scheduler related locks. Pausing the domain was not even being remotely considered at the time, it (again, at least AFAICR) came up later for trying to address other issues. No, the whole point of why that was not a problem in the first place is that what counts here is on the wait list of what pcpu the vcpu is put, not really where the vcpu is being or has been scheduled last. Of course it'd be better --and it would also be true most of the times-- if there where a match, but that was not a correctness concern. So why this is all of the sudden becoming one? Am I completely off with my recollection (or in general :-P)? Or what am I missing about the issue we are trying to address with this new bits of the work? Thanks and Regards, Dario -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
Hi Dario, > -Original Message- > From: Dario Faggioli [mailto:dario.faggi...@citrix.com] > Sent: Wednesday, September 14, 2016 10:52 PM > To: Wu, Feng ; Jan Beulich > Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Tian, Kevin > ; xen-devel@lists.xen.org > Subject: Re: [PATCH v3 4/6] Pause/Unpause the domain before/after assigning > PI hooks > > On Wed, 2016-09-14 at 02:23 +, Wu, Feng wrote: > > Then I tried to implement the function like the following: > > > > /* This function is called when pcidevs_lock is held */ > > void vmx_pi_hooks_assign(struct domain *d) > > { > > if ( !iommu_intpost || !has_hvm_container_domain(d) ) > > return; > > > > ASSERT(!d->arch.hvm_domain.vmx.vcpu_block); > > > > /* > > * We carefully handle the timing here: > > * - Install the context switch first > > * - Then set the NDST field > > * - Install the block and resume hooks in the end > > * > > * This can make sure the PI (especially the NDST feild) is > > * in proper state when we call vmx_vcpu_block(). > > */ > > d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; > > d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; > > > > for_each_vcpu ( d, v ) > > { > > unsigned int dest = cpu_physical_id(v->processor); > > struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc; > > > > /* spot 1 */ > > > > write_atomic(&pi_desc->ndst, > > x2apic_enabled ? dest : MASK_INSR(dest, > > PI_xAPIC_NDST_MASK)); > > } > > > > d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block; > > d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume; > > } > > > > However, I still think it is problematic. Consider the _spot 1_ > > above, we get > > the pCPU of the vCPU and update the NDST, but the vCPU can be happily > > rescheduled to another pCPU before updating the NDST. The problem is > > that it is not safe to update NDST here, since vCPU can be scheduled > > behind us at any time. And if this is the case, it is hard to safely > > do this > > without guarantee the vCPU is in a known state. Any further advice > > on this? Thanks a lot! > > > So, I'm sorry if this is me missing or not remembering something... but > I do remember that, at some point, in the early days of this series, > there were concerns about the use of v->processor behind the back of > the scheduler, i.e., without holding the proper scheduler related > locks. > > Pausing the domain was not even being remotely considered at the time, > it (again, at least AFAICR) came up later for trying to address other > issues. > > No, the whole point of why that was not a problem in the first place is > that what counts here is on the wait list of what pcpu the vcpu is put, > not really where the vcpu is being or has been scheduled last. Of > course it'd be better --and it would also be true most of the times-- > if there where a match, but that was not a correctness concern. > > So why this is all of the sudden becoming one? Am I completely off with > my recollection (or in general :-P)? Or what am I missing about the > issue we are trying to address with this new bits of the work? The issue discussed between Jan and me is that now we have four PI hooks: vmx_pi_switch_from, vmx_pi_switch_to, vmx_vcpu_block, and vmx_pi_do_resume. The previous assumption in vmx_vcpu_block() is that when we are running this function, the NDST field should have the same meaning with the current pCPU the vCPU is running on. However, I found this is not true in some scenarios, such as, vmx_pi_switch_to() hasn't been installed or executed before vmx_vcpu_block() gets called, in which case, we may hit the ASSERT in it. In previous version, I suggested we remove the ASSERT in vmx_vcpu_block() and set NDST explicitly in it. But seems maintainer doesn't like this idea. So currently we need to make sure the PI is in proper state before the hooks get called, that is why I used pause/ unpause mechanism. Thanks, Feng > > Thanks and Regards, > Dario > -- > <> (Raistlin Majere) > - > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
On Sun, 2016-09-18 at 08:37 +, Wu, Feng wrote: > > From: Dario Faggioli [mailto:dario.faggi...@citrix.com] > > So why this is all of the sudden becoming one? Am I completely off > > with > > my recollection (or in general :-P)? Or what am I missing about the > > issue we are trying to address with this new bits of the work? > > The issue discussed between Jan and me is that now we have four > PI hooks: vmx_pi_switch_from, vmx_pi_switch_to, vmx_vcpu_block, > and vmx_pi_do_resume. The previous assumption in vmx_vcpu_block() > is that when we are running this function, the NDST field should have > the same meaning with the current pCPU the vCPU is running on. > I'm sorry, but I'm still not getting it. Feel free to drop me, if I'm doing more harm than good, but really, I can't parse this sentence into something that makes me better understand the problem at hand. "The previous assumption": "previous" with respect to what? "the field should have the same meaning with the current pCPU the vCPU is running on", what's this meaning you're mentioning, and how would it be the same? > However, I found this is not true in some scenarios, such as, > vmx_pi_switch_to() hasn't been installed or executed before > vmx_vcpu_block() gets called, in which case, we may hit the ASSERT > in it. In previous version, I suggested we remove the ASSERT in > vmx_vcpu_block() and set NDST explicitly in it. But seems maintainer > doesn't like this idea. > And this is not helping either. An ASSERT() being hit means something wrong happened. Whether or not to remove (or change) an ASSERT() is not a matter of "like". If we hit the ASSERT() but nothing is wrong with the code, then it is the ASSERT() itself that is wrong (buggy), and we must remove it, like it or not. OTOH, if we hit a non buggy ASSERT(), then it means that the ASSERT() has done its job in finding something wrong in the code, and we should leave it alone and fix the problem. How's possible for the solution here to be "either remove the ASSERT() _OR_ change the code"? That really makes few sense to me... :-O Thanks and Regards, Dario -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
> -Original Message- > From: Dario Faggioli [mailto:dario.faggi...@citrix.com] > Sent: Tuesday, September 20, 2016 7:12 AM > To: Wu, Feng ; Jan Beulich > Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Tian, Kevin > ; xen-devel@lists.xen.org > Subject: Re: [PATCH v3 4/6] Pause/Unpause the domain before/after assigning > PI hooks > > On Sun, 2016-09-18 at 08:37 +, Wu, Feng wrote: > > > From: Dario Faggioli [mailto:dario.faggi...@citrix.com] > > > So why this is all of the sudden becoming one? Am I completely off > > > with > > > my recollection (or in general :-P)? Or what am I missing about the > > > issue we are trying to address with this new bits of the work? > > > > The issue discussed between Jan and me is that now we have four > > PI hooks: vmx_pi_switch_from, vmx_pi_switch_to, vmx_vcpu_block, > > and vmx_pi_do_resume. The previous assumption in vmx_vcpu_block() > > is that when we are running this function, the NDST field should have > > the same meaning with the current pCPU the vCPU is running on. > > > I'm sorry, but I'm still not getting it. Feel free to drop me, if I'm > doing more harm than good, but really, I can't parse this sentence into > something that makes me better understand the problem at hand. > > "The previous assumption": "previous" with respect to what? > > "the field should have the same meaning with the current pCPU the vCPU > is running on", what's this meaning you're mentioning, and how would it > be the same? Sorry for my bad description. I mean in the current code there an ASSERT in vmx_vcpu_block(): ASSERT(pi_desc->ndst == (x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK))) So we assume that the value of NDST field should get from the pCPU the vCPU is currently running on. > > > However, I found this is not true in some scenarios, such as, > > vmx_pi_switch_to() hasn't been installed or executed before > > vmx_vcpu_block() gets called, in which case, we may hit the ASSERT > > in it. In previous version, I suggested we remove the ASSERT in > > vmx_vcpu_block() and set NDST explicitly in it. But seems maintainer > > doesn't like this idea. > > > And this is not helping either. An ASSERT() being hit means something > wrong happened. Whether or not to remove (or change) an ASSERT() is not > a matter of "like". > > If we hit the ASSERT() but nothing is wrong with the code, then it is > the ASSERT() itself that is wrong (buggy), and we must remove it, like > it or not. > > OTOH, if we hit a non buggy ASSERT(), then it means that the ASSERT() > has done its job in finding something wrong in the code, and we should > leave it alone and fix the problem. Yes, this is what we are doing now. We think the ASSERT() should be still there, and try to fix the issue in the other place to make sure we will not hit the ASSERT() here. > > How's possible for the solution here to be "either remove the ASSERT() > _OR_ change the code"? That really makes few sense to me... :-O As mentioned above, I will remain the ASSERT and fix the issue. Thanks for the comments! Thanks, Feng > > Thanks and Regards, > Dario > -- > <> (Raistlin Majere) > - > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
>>> On 20.09.16 at 01:12, wrote: > On Sun, 2016-09-18 at 08:37 +, Wu, Feng wrote: >> > From: Dario Faggioli [mailto:dario.faggi...@citrix.com] >> However, I found this is not true in some scenarios, such as, >> vmx_pi_switch_to() hasn't been installed or executed before >> vmx_vcpu_block() gets called, in which case, we may hit the ASSERT >> in it. In previous version, I suggested we remove the ASSERT in >> vmx_vcpu_block() and set NDST explicitly in it. But seems maintainer >> doesn't like this idea. >> > And this is not helping either. An ASSERT() being hit means something > wrong happened. Whether or not to remove (or change) an ASSERT() is not > a matter of "like". > > If we hit the ASSERT() but nothing is wrong with the code, then it is > the ASSERT() itself that is wrong (buggy), and we must remove it, like > it or not. > > OTOH, if we hit a non buggy ASSERT(), then it means that the ASSERT() > has done its job in finding something wrong in the code, and we should > leave it alone and fix the problem. > > How's possible for the solution here to be "either remove the ASSERT() > _OR_ change the code"? That really makes few sense to me... :-O I disagree: Whether fixing/removing an ASSERT() that triggered or adjusting other code to make the ASSERT() not trigger can indeed both be an option, and may to some degree be a matter of taste. And that's where I think Feng and I disagree - I'd prefer the ASSERT() to stay, and code elsewhere to make sure it won't trigger, while he would prefer to get rid of the ASSERT() (aiui on the basis that the code following the ASSERT() establishes the intended state - Feng, please correct me if I'm wrong). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
> -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Tuesday, September 20, 2016 3:32 PM > To: Dario Faggioli > Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Wu, Feng > ; Tian, Kevin ; xen- > de...@lists.xen.org > Subject: Re: [PATCH v3 4/6] Pause/Unpause the domain before/after assigning > PI hooks > > >>> On 20.09.16 at 01:12, wrote: > > On Sun, 2016-09-18 at 08:37 +, Wu, Feng wrote: > >> > From: Dario Faggioli [mailto:dario.faggi...@citrix.com] > >> However, I found this is not true in some scenarios, such as, > >> vmx_pi_switch_to() hasn't been installed or executed before > >> vmx_vcpu_block() gets called, in which case, we may hit the ASSERT > >> in it. In previous version, I suggested we remove the ASSERT in > >> vmx_vcpu_block() and set NDST explicitly in it. But seems maintainer > >> doesn't like this idea. > >> > > And this is not helping either. An ASSERT() being hit means something > > wrong happened. Whether or not to remove (or change) an ASSERT() is not > > a matter of "like". > > > > If we hit the ASSERT() but nothing is wrong with the code, then it is > > the ASSERT() itself that is wrong (buggy), and we must remove it, like > > it or not. > > > > OTOH, if we hit a non buggy ASSERT(), then it means that the ASSERT() > > has done its job in finding something wrong in the code, and we should > > leave it alone and fix the problem. > > > > How's possible for the solution here to be "either remove the ASSERT() > > _OR_ change the code"? That really makes few sense to me... :-O > > I disagree: Whether fixing/removing an ASSERT() that triggered or > adjusting other code to make the ASSERT() not trigger can indeed > both be an option, and may to some degree be a matter of taste. > And that's where I think Feng and I disagree - I'd prefer the ASSERT() > to stay, and code elsewhere to make sure it won't trigger, while he > would prefer to get rid of the ASSERT() (aiui on the basis that the > code following the ASSERT() establishes the intended state - Feng, > please correct me if I'm wrong). Yes, you are correct. Actually both solutions can resolve the issue. And after some discussion, we are agree on fixing thing in the other place to make we won't tirgger the ASSERT(). Thanks, Feng > > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
On Tue, 2016-09-20 at 01:31 -0600, Jan Beulich wrote: > > > > On 20.09.16 at 01:12, wrote: > > How's possible for the solution here to be "either remove the > > ASSERT() > > _OR_ change the code"? That really makes few sense to me... :-O > > I disagree: Whether fixing/removing an ASSERT() that triggered or > adjusting other code to make the ASSERT() not trigger can indeed > both be an option, and may to some degree be a matter of taste. > Yes, of course this is a possibility... I thought it was clear that I was simplifying things, but maybe I was simplifying too much, or "just" expressed myself bad. What I wasn't clear about was whether it is _correctness_ that is at risk or not. And that's right because I thought we established already that this wasn't a correctness issue, while it looked to me, from reading the discussion, that it actually may be. My bad again, for sure, sorry. Now I'll go back to the code to fix my misconceptions and (hopefully) be able to make myself more useful. :-) Thanks and Regards, Dario -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
> -Original Message- > From: Dario Faggioli [mailto:dario.faggi...@citrix.com] > Sent: Tuesday, September 20, 2016 4:14 PM > To: Jan Beulich > Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Wu, Feng > ; Tian, Kevin ; xen- > de...@lists.xen.org > Subject: Re: [PATCH v3 4/6] Pause/Unpause the domain before/after assigning > PI hooks > > On Tue, 2016-09-20 at 01:31 -0600, Jan Beulich wrote: > > > > > On 20.09.16 at 01:12, wrote: > > > How's possible for the solution here to be "either remove the > > > ASSERT() > > > _OR_ change the code"? That really makes few sense to me... :-O > > > > I disagree: Whether fixing/removing an ASSERT() that triggered or > > adjusting other code to make the ASSERT() not trigger can indeed > > both be an option, and may to some degree be a matter of taste. > > > Yes, of course this is a possibility... I thought it was clear that I > was simplifying things, but maybe I was simplifying too much, or "just" > expressed myself bad. > > What I wasn't clear about was whether it is _correctness_ that is at > risk or not. And that's right because I thought we established already > that this wasn't a correctness issue, while it looked to me, from > reading the discussion, that it actually may be. > > My bad again, for sure, sorry. Now I'll go back to the code to fix my > misconceptions and (hopefully) be able to make myself more useful. :-) You are always useful, your advices are always valuable! :) Thanks, Feng > > Thanks and Regards, > Dario > -- > <> (Raistlin Majere) > - > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
>>> On 02.09.16 at 15:15, wrote: > And that is another reason I use pause/unpause here, it can address > all the races. And this is an one-time action (Only occurs at the first > device gets assigned), do you think it is that unacceptable? Btw. please see George's very nice description - much better than I would ever have been able to give - for why we will always want alternatives to pausing considered first: https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg02587.html Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
> -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Friday, September 23, 2016 10:19 PM > To: Wu, Feng > Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > george.dun...@eu.citrix.com; Tian, Kevin ; 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 15:15, wrote: > > And that is another reason I use pause/unpause here, it can address > > all the races. And this is an one-time action (Only occurs at the first > > device gets assigned), do you think it is that unacceptable? > > Btw. please see George's very nice description - much better than > I would ever have been able to give - for why we will always want > alternatives to pausing considered first: > https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg02587.html Yeah, thanks for sharing this and thanks to the nice description from George! Thanks, Feng > > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
>>> On 31.08.16 at 05:56, 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). And then - isn't this overkill? Wouldn't a simple spin lock, taken here and in the deassign counterpart, do? Or wait - is the comment perhaps wrongly talking about deassign? If so the change is still questionable, as the hooks get set before the first device gets actually assigned to a guest (I remember that I insisted on things getting done that way when those original patches had been under review). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
> -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Thursday, September 1, 2016 4:30 PM > To: Wu, Feng > Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > george.dun...@eu.citrix.com; Tian, Kevin ; 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, 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, then the race condition comes again, so we still need to handle it. > > And then - isn't this overkill? Wouldn't a simple spin lock, taken > here and in the deassign counterpart, do? > > Or wait - is the comment perhaps wrongly talking about deassign? Oh, yes, there are something wrong in the comments, this patch has nothing to do with the _deassign_ stuff. The comments should be like below: +/* + * Pausing the domain can make sure the vCPU is not + * running and hence calling the hooks simultaneously + * when _assigning_ 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. + */ Sorry for that. > > If so the change is still questionable, as the hooks get set before > the first device gets actually assigned to a guest (I remember > that I insisted on things getting done that way when those > original patches had been under review). Yes, the hooks were installed before the first device gets assigned. Then could you please elaborate what is the question here? Thanks, Feng > > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
>>> On 02.09.16 at 03:46, wrote: > >> -Original Message- >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: Thursday, September 1, 2016 4:30 PM >> To: Wu, Feng >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, 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. > then the race > condition comes again, so we still need to handle it. > >> >> And then - isn't this overkill? Wouldn't a simple spin lock, taken >> here and in the deassign counterpart, do? >> >> Or wait - is the comment perhaps wrongly talking about deassign? > > Oh, yes, there are something wrong in the comments, this patch > has nothing to do with the _deassign_ stuff. The comments should > be like below: > > +/* > + * Pausing the domain can make sure the vCPU is not > + * running and hence calling the hooks simultaneously > + * when _assigning_ 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. > + */ > > Sorry for that. > >> >> If so the change is still questionable, as the hooks get set before >> the first device gets actually assigned to a guest (I remember >> that I insisted on things getting done that way when those >> original patches had been under review). > > Yes, the hooks were installed before the first device gets assigned. > Then could you please elaborate what is the question here? The question here is whether this patch (taking into consideration comments on patches earlier in the series) is (a) needed and if so (b) reasonable in how it achieves the goal. Pausing a domain shouldn't really become a thing we routinely do. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
> -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Friday, September 2, 2016 3:04 PM > To: Wu, Feng > Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: > > > > >> -Original Message- > >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> Sent: Thursday, September 1, 2016 4:30 PM > >> To: Wu, Feng > >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, 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. Let's make the above clear before going forward on this patch. Thanks, Feng > > > then the race > > condition comes again, so we still need to handle it. > > > >> > >> And then - isn't this overkill? Wouldn't a simple spin lock, taken > >> here and in the deassign counterpart, do? > >> > >> Or wait - is the comment perhaps wrongly talking about deassign? > > > > Oh, yes, there are something wrong in the comments, this patch > > has nothing to do with the _deassign_ stuff. The comments should > > be like below: > > > > +/* > > + * Pausing the domain can make sure the vCPU is not > > + * running and hence calling the hooks simultaneously > > + * when _assigning_ 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. > > + */ > > > > Sorry for that. > > > >> > >> If so the change is still questionable, as the hooks get set before > >> the first device gets actually assigned to a guest (I remember > >> that I insisted on things getting done that way when those > >> original patches had been under review). > > > > Yes, the hooks were installed before the first device gets assigned. > > Then could you please elaborate what is the question here? > > The question here is whether this patch (taking into consideration > comments on patches earlier in the series) is (a) needed and if so > (b) reasonable in how it achieves the goal. Pausing a domain > shouldn't really become a thing we routinely do. > > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
>>> On 02.09.16 at 09:31, wrote: > >> -Original Message- >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: Friday, September 2, 2016 3:04 PM >> To: Wu, Feng >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: >> >> > >> >> -Original Message- >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> Sent: Thursday, September 1, 2016 4:30 PM >> >> To: Wu, Feng >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, 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@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
> -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Friday, September 2, 2016 4:16 PM > To: Wu, Feng > Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: > > > > >> -Original Message- > >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> Sent: Friday, September 2, 2016 3:04 PM > >> To: Wu, Feng > >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: > >> > >> > > >> >> -Original Message- > >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> Sent: Thursday, September 1, 2016 4:30 PM > >> >> To: Wu, Feng > >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, 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. Thanks, Feng > > Jan ___ Xen-devel mail
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
>>> On 02.09.16 at 10:40, wrote: > >> -Original Message- >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: Friday, September 2, 2016 4:16 PM >> To: Wu, Feng >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: >> >> > >> >> -Original Message- >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> Sent: Friday, September 2, 2016 3:04 PM >> >> To: Wu, Feng >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: >> >> >> >> > >> >> >> -Original Message- >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> >> Sent: Thursday, September 1, 2016 4:30 PM >> >> >> To: Wu, Feng >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, 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 meth
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
> -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Friday, September 2, 2016 5:26 PM > To: Wu, Feng > Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > george.dun...@eu.citrix.com; Tian, Kevin ; 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 10:40, wrote: > > > > >> -Original Message- > >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> Sent: Friday, September 2, 2016 4:16 PM > >> To: Wu, Feng > >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: > >> > >> > > >> >> -Original Message- > >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> Sent: Friday, September 2, 2016 3:04 PM > >> >> To: Wu, Feng > >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: > >> >> > >> >> > > >> >> >> -Original Message- > >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> >> Sent: Thursday, September 1, 2016 4:30 PM > >> >> >> To: Wu, Feng > >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; > 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, 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 cor
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
>>> On 02.09.16 at 12:30, wrote: > >> -Original Message- >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: Friday, September 2, 2016 5:26 PM >> To: Wu, Feng >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> george.dun...@eu.citrix.com; Tian, Kevin ; 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 10:40, wrote: >> >> > >> >> -Original Message- >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> Sent: Friday, September 2, 2016 4:16 PM >> >> To: Wu, Feng >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: >> >> >> >> > >> >> >> -Original Message- >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> >> Sent: Friday, September 2, 2016 3:04 PM >> >> >> To: Wu, Feng >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: >> >> >> >> >> >> > >> >> >> >> -Original Message- >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> >> >> Sent: Thursday, September 1, 2016 4:30 PM >> >> >> >> To: Wu, Feng >> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; >> 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, 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 >> >
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
> -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Friday, September 2, 2016 6:46 PM > To: Wu, Feng > Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > george.dun...@eu.citrix.com; Tian, Kevin ; 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 12:30, wrote: > > > > >> -Original Message- > >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> Sent: Friday, September 2, 2016 5:26 PM > >> To: Wu, Feng > >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> george.dun...@eu.citrix.com; Tian, Kevin ; 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 10:40, wrote: > >> > >> > > >> >> -Original Message- > >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> Sent: Friday, September 2, 2016 4:16 PM > >> >> To: Wu, Feng > >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: > >> >> > >> >> > > >> >> >> -Original Message- > >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> >> Sent: Friday, September 2, 2016 3:04 PM > >> >> >> To: Wu, Feng > >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; > 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, wrote: > >> >> >> > >> >> >> > > >> >> >> >> -Original Message- > >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> >> >> Sent: Thursday, September 1, 2016 4:30 PM > >> >> >> >> To: Wu, Feng > >> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; > >> 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, 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/list
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
>>> On 02.09.16 at 15:15, wrote: > >> -Original Message- >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: Friday, September 2, 2016 6:46 PM >> To: Wu, Feng >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> george.dun...@eu.citrix.com; Tian, Kevin ; 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 12:30, wrote: >> >> > >> >> -Original Message- >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> Sent: Friday, September 2, 2016 5:26 PM >> >> To: Wu, Feng >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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 10:40, wrote: >> >> >> >> > >> >> >> -Original Message- >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> >> Sent: Friday, September 2, 2016 4:16 PM >> >> >> To: Wu, Feng >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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, wrote: >> >> >> >> >> >> > >> >> >> >> -Original Message- >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> >> >> Sent: Friday, September 2, 2016 3:04 PM >> >> >> >> To: Wu, Feng >> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; >> 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, wrote: >> >> >> >> >> >> >> >> > >> >> >> >> >> -Original Message- >> >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> >> >> >> Sent: Thursday, September 1, 2016 4:30 PM >> >> >> >> >> To: Wu, Feng >> >> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; >> >> 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, 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
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
> -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Friday, September 2, 2016 9:55 PM > To: Wu, Feng > Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > george.dun...@eu.citrix.com; Tian, Kevin ; 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 15:15, wrote: > > > > >> -Original Message- > >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> Sent: Friday, September 2, 2016 6:46 PM > >> To: Wu, Feng > >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> george.dun...@eu.citrix.com; Tian, Kevin ; 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 12:30, wrote: > >> > >> > > >> >> -Original Message- > >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> Sent: Friday, September 2, 2016 5:26 PM > >> >> To: Wu, Feng > >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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 10:40, wrote: > >> >> > >> >> > > >> >> >> -Original Message- > >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> >> Sent: Friday, September 2, 2016 4:16 PM > >> >> >> To: Wu, Feng > >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; > 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, wrote: > >> >> >> > >> >> >> > > >> >> >> >> -Original Message- > >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> >> >> Sent: Friday, September 2, 2016 3:04 PM > >> >> >> >> To: Wu, Feng > >> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; > >> 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, wrote: > >> >> >> >> > >> >> >> >> > > >> >> >> >> >> -Original Message- > >> >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> >> >> >> Sent: Thursday, September 1, 2016 4:30 PM > >> >> >> >> >> To: Wu, Feng > >> >> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> >> >> >> george.dun...@eu.citrix.com; Tian, Kevin > ; > >> >> 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, 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 pa
Re: [Xen-devel] [PATCH v3 4/6] Pause/Unpause the domain before/after assigning PI hooks
>>> On 05.09.16 at 05:11, wrote: > >> -Original Message- >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: Friday, September 2, 2016 9:55 PM >> To: Wu, Feng >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> george.dun...@eu.citrix.com; Tian, Kevin ; 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 15:15, wrote: >> >> > >> >> -Original Message- >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> Sent: Friday, September 2, 2016 6:46 PM >> >> To: Wu, Feng >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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 12:30, wrote: >> >> >> >> > >> >> >> -Original Message- >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> >> Sent: Friday, September 2, 2016 5:26 PM >> >> >> To: Wu, Feng >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; 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 10:40, wrote: >> >> >> >> >> >> > >> >> >> >> -Original Message- >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> >> >> Sent: Friday, September 2, 2016 4:16 PM >> >> >> >> To: Wu, Feng >> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; >> 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, wrote: >> >> >> >> >> >> >> >> > >> >> >> >> >> -Original Message- >> >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> >> >> >> Sent: Friday, September 2, 2016 3:04 PM >> >> >> >> >> To: Wu, Feng >> >> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> >> >> >> george.dun...@eu.citrix.com; Tian, Kevin ; >> >> 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, wrote: >> >> >> >> >> >> >> >> >> >> > >> >> >> >> >> >> -Original Message- >> >> >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> >> >> >> >> Sent: Thursday, September 1, 2016 4:30 PM >> >> >> >> >> >> To: Wu, Feng >> >> >> >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; >> >> >> >> >> >> george.dun...@eu.citrix.com; Tian, Kevin >> ; >> >> >> 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, 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 >> >> >> >>