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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Dario Faggioli [mailto:dario.faggioli@xxxxxxxxxx]
> Sent: Tuesday, September 20, 2016 7:12 AM
> To: Wu, Feng <feng.wu@xxxxxxxxx>; Jan Beulich <JBeulich@xxxxxxxx>
> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin
> <kevin.tian@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v3 4/6] Pause/Unpause the domain before/after assigning
> PI hooks
> 
> On Sun, 2016-09-18 at 08:37 +0000, Wu, Feng wrote:
> > > From: Dario Faggioli [mailto:dario.faggioli@xxxxxxxxxx]
> > > 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
> --
> <<This happens because I choose it to happen!>> (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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel