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

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, July 15, 2015 5:28 PM
> To: Wu, Feng
> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin;
> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: RE: [v3 06/15] vmx: Extend struct pi_desc to support VT-d
> Posted-Interrupts
> 
> >>> On 15.07.15 at 10:43, <feng.wu@xxxxxxxxx> wrote:
> 
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Wednesday, July 15, 2015 4:36 PM
> >> To: Wu, Feng
> >> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin;
> >> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> >> Subject: RE: [v3 06/15] vmx: Extend struct pi_desc to support VT-d
> >> Posted-Interrupts
> >>
> >> >>> On 15.07.15 at 10:26, <feng.wu@xxxxxxxxx> wrote:
> >>
> >> >
> >> >> -----Original Message-----
> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> Sent: Wednesday, July 15, 2015 4:20 PM
> >> >> To: Wu, Feng
> >> >> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian,
> Kevin;
> >> >> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> >> >> Subject: RE: [v3 06/15] vmx: Extend struct pi_desc to support VT-d
> >> >> Posted-Interrupts
> >> >>
> >> >> >>> On 15.07.15 at 04:40, <feng.wu@xxxxxxxxx> wrote:
> >> >>
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> >> Sent: Friday, July 10, 2015 9:08 PM
> >> >> >> To: Wu, Feng
> >> >> >> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian,
> >> Kevin;
> >> >> >> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> >> >> >> 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@xxxxxxxxx> 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)?
> >> >
> >> > It is because, we can only enter running state from runnable, in which,
> >> > the NV field has been already changed back to ' posted_intr_vector ',
> >> > we don't need to do it here again.
> >>
> >> Without sitting in the runstate update path anymore, I can't see how
> >> you would get to see all transitions to runnable.
> >
> > Sorry, I cannot understanding the above comments well. Do you mean
> > after using the new method (arch hooks ) to update posted-interrupt
> > descriptor, I cannot track all the state transitions to runnable?
> 
> Not sure if "track" is the right word here, but yes.
> 

The new method is still in development, let's see how it will be then. :)

Thanks,
Feng

> Jan


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