Re: [PATCH v2 3/4] VMX: Assign the right value to 'NDST' field in a concern case

["Jan Beulich" <JBeulich@suse.com>]

>>> On 03.06.16 at 07:23, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Tuesday, May 31, 2016 7:58 PM
>> To: Wu, Feng <feng.wu@intel.com>
>> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
>> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
>> devel@lists.xen.org; konrad.wilk@oracle.com; keir@xen.org 
>> Subject: RE: [PATCH v2 3/4] VMX: Assign the right value to 'NDST' field in a
>> concern case
>> 
>> >>> On 31.05.16 at 12:27, <feng.wu@intel.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: Friday, May 27, 2016 10:00 PM
>> >> >>> On 26.05.16 at 15:39, <feng.wu@intel.com> wrote:
>> >> > Normally, in vmx_cpu_block() 'NDST' filed should have the same
>> >> > value with 'dest' or 'MASK_INSR(dest, PI_xAPIC_NDST_MASK)' depending
>> >> > on whether x2apic is enabled. However, in the following scenario,
>> >> > 'NDST' has different value:
>> >> >
>> >> > 'vcpu_block' hook gets assigned in vmx_pi_hooks_assign(), but all
>> >> > other three PI hooks have not been assigned or not been excuted yet.
>> >> > And during this interval, we are running in vmx_vcpu_block(), then
>> >> > 'NDST' may have different value.
>> >>
>> >> How about simply assigning vcpu_block last? Or alternatively
>> >> holding pi_blocking_list_lock around all four assignments? Or
>> >> (maybe in addition to one of these) writing something sensible in
>> >> vmx_pi_hooks_assign() before doing the hook assignments?
>> >
>> > The problem is vmx_vcpu_block() can still get called first, since
>> > the other ones might not get called yet.
>> 
>> That refers to the first two questions, yet the third (unanswered
>> one) was added by me because I anticipated that response. So
>> what's wrong with that last option?
> 
> Do you mean " Or (maybe in addition to one of these) writing
> something sensible in vmx_pi_hooks_assign() before doing the
> hook assignments?"?

Yes.

> The problem is you cannot know whether vmx_vcpu_block() is
> called before or after the other hooks.

Well - it for sure can't get called before getting installed as a hook.
I.e. if you set up proper state before installing it, you should be
fine.

> And I am wondering
> why do you think the current solution is not good.

See the other thread (on patch 1). When dealing with corner
cases, as a first option (leading to less complication, or even
simplification) it should be understood why they are corner
cases in the first place, i.e. why the normal code flow doesn't
take care of them. It's only the second best option to add
extra logic / conditionals in order to deal with such.

For the case at hand that first question translates to "Why
does the ASSERT() not hold, despite us having thought it
always would?" And yes, the change here isn't complicated,
so may well be fine to go in, but together with the other
patches I can't help getting the feeling that the overall
situation wasn't really fully understood (and thus explained in
the patch overview and descriptions). In particular, this part
of the source comment

+     * 'vcpu_block' hook gets assigned in vmx_pi_hooks_assign(), but all
+     * other three PI hooks have not been assigned or not been excuted yet.
+     * And during this interval, we get here in this function, then 'NDST'
+     * may have different value.

suggests to me that my consideration above (about setting up
state correctly _before_ assigning the hooks) provides a viable
alternative solution.

Jan


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