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

[Wu, Feng]

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

[Jan Beulich]

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

[Wu, Feng]

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

[Dario Faggioli]

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

[Wu, Feng]

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

[Jan Beulich]

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

[Wu, Feng]

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

[Dario Faggioli]

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

[Wu, Feng]

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 = >arch.hvm_vmx.pi_desc;
> >
> > /* spot 1 */
> >
> > write_atomic(_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 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

[Dario Faggioli]

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 = >arch.hvm_vmx.pi_desc;
> 
> /* spot 1 */
> 
> write_atomic(_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 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

[Jan Beulich]

>>> 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 = >arch.hvm_vmx.pi_desc;
> 
> /* spot 1 */
> 
> write_atomic(_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

[Wu, Feng]

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

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

[Jan Beulich]

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

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

[Wu, Feng]

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

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

[Jan Beulich]

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

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

[Wu, Feng]

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

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

[Jan Beulich]

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

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

[Wu, Feng]

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

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

[Jan Beulich]

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

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

[Wu, Feng]

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

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

[Jan Beulich]

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

[Wu, Feng]

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

[Jan Beulich]

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

[Wu, Feng]

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

[Jan Beulich]

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

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

[Feng Wu]

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