Re: [v3 06/15] vmx: Extend struct pi_desc to support VT-d Posted-Interrupts

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

>>> On 15.07.15 at 04:40, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, July 10, 2015 9:08 PM
>> To: Wu, Feng
>> Cc: andrew.cooper3@citrix.com; george.dunlap@eu.citrix.com; Tian, Kevin;
>> Zhang, Yang Z; xen-devel@lists.xen.org; keir@xen.org 
>> Subject: Re: [v3 06/15] vmx: Extend struct pi_desc to support VT-d
>> Posted-Interrupts
>> 
>> >>> On 24.06.15 at 07:18, <feng.wu@intel.com> wrote:
>> > @@ -81,8 +81,19 @@ struct vmx_domain {
>> >
>> >  struct pi_desc {
>> >      DECLARE_BITMAP(pir, NR_VECTORS);
>> > -    u32 control;
>> > -    u32 rsvd[7];
>> > +    union {
>> > +        struct
>> > +        {
>> > +        u16 on     : 1,  /* bit 256 - Outstanding Notification */
>> > +            sn     : 1,  /* bit 257 - Suppress Notification */
>> > +            rsvd_1 : 14; /* bit 271:258 - Reserved */
>> > +        u8  nv;          /* bit 279:272 - Notification Vector */
>> > +        u8  rsvd_2;      /* bit 287:280 - Reserved */
>> > +        u32 ndst;        /* bit 319:288 - Notification Destination */
>> > +        };
>> > +        u64 control;
>> > +    };
>> 
>> So current code, afaics, uses e.g. test_and_set_bit() to set ON.
>> By also declaring this as a bitfield you're opening the structure for
>> non-atomic accesses. If that's correct, why is other code not
>> being changed to _only_ use the bitfield mechanism (likely also
>> eliminating the need for it being a union with the now 64-bit
>> "control"? If atomic accesses are required, then I'd strongly
>> suggest against making this a bit field.
>> 
>> And in no event can I see why "ndst" needs to be union-ized
>> with "control" if it doesn't need to be updated atomically with
>> e.g. "nv".
>> 
> 
> When the vCPU is to be blocked, we need to atomically update
> the "nv" and "ndst", then the wakeup notification event can be
> delivered to the right destination.

Okay. Your reply made me go through the patches again to check
where updates to nv/ndst happen - what's the reason they aren't
being updated as a pair in patch 14's RUNSTATE_running handling
(or in the replacement draft's vmx_ctxt_switch_to() adjustment)?

Jan