Re: [Xen-devel] [RFC v2 13/15] Update Posted-Interrupts Descriptor during vCPU scheduling

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, June 17, 2015 3:56 PM
> To: Wu, Feng
> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin;
> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: RE: [RFC v2 13/15] Update Posted-Interrupts Descriptor during vCPU
> scheduling
> 
> >>> On 17.06.15 at 08:54, <feng.wu@xxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Wednesday, June 10, 2015 2:44 PM
> >> >>> On 08.05.15 at 11:07, <feng.wu@xxxxxxxxx> wrote:
> >> > +    switch ( v->runstate.state )
> >> > +    {
> >> > +    case RUNSTATE_runnable:
> >> > +    case RUNSTATE_offline:
> >> > +        /*
> >> > +         * We don't need to send notification event to a non-running
> >> > +         * vcpu, the interrupt information will be delivered to it 
> >> > before
> >> > +         * VM-ENTRY when the vcpu is scheduled to run next time.
> >> > +         */
> >> > +        pi_desc->sn = 1;
> >> > +
> >> > +        /*
> >> > +         * If the state is transferred from RUNSTATE_blocked,
> >> > +         * we should set 'NV' feild back to posted_intr_vector,
> >> > +         * so the Posted-Interrupts can be delivered to the vCPU
> >> > +         * by VT-d HW after it is scheduled to run.
> >> > +         */
> >> > +        if ( old_state == RUNSTATE_blocked )
> >> > +        {
> >> > +            do
> >> > +            {
> >> > +                old.control = new.control = pi_desc->control;
> >> > +                new.nv = posted_intr_vector;
> >> > +            }
> >> > +            while ( cmpxchg(&pi_desc->control, old.control,
> new.control)
> >> > +                    != old.control );
> >>
> >> So why is it okay to do the SN update non-atomically when the
> >> vector update is done atomically?
> >>
> >> Also the curly braces do _not_ go on separate lines for do/while
> >> constructs.
> >>
> >> And then do you really need to atomically update the whole 64 bits
> >> here, rather than just the nv field? By not making it a bit field
> >> you could perhaps just write_atomic() it?
> >>
> >
> > Thinking more about this, maybe write_atomic() is not good for this, we need
> > use
> > the LOCK prefix when updating the posted-interrupt descriptor.
> 
> The LOCK prefix can't be applied to simple loads and stores; they're
> implicitly atomic.
> 

Yes, I know LOCK prefix cannot be used together with 'mov', but
my concern is without LOCK, does it work well when this
write-atomic() operation and hardware setting 'ON' occur at the
same time?

Hardware updates the 'ON' filed like this:
(which is listed in the Spec.)

"Hardware performs a coherent atomic read-modify-write
operation of the posted-interrupt descriptor as follows:
- Read contents of the Posted Interrupt Descriptor, claiming
exclusive ownership of its hosting cache-line. If invalid
programming of Posted Interrupt Descriptor is detected,
release ownership of the cache-line, and block the interrupt
request.
- If above checks succeed, retrieve current values of Posted
Interrupt Requests (PIR bits 255:0), Outstanding Notification (ON),
Suppress Notification (SN), Notification Vector (NV), and
Notification Destination (NDST) fields in the Posted Interrupt
Descriptor.
- Modify the following descriptor field values atomically:
* Set bit in PIR corresponding to the Vector field value from
the IRTE
* Compute X = ((ON == 0) & (URG | (SN == 0)))
* If (X == 1), Set ON field.
- Promote the cache-line to be globally observable, so that
the modifications are visible to other caching agents. Hardware
may write-back the cache-line any time after this step."

If we don't use the LOCK prefix, 'NV' filed may be changed between
reading these filed and writing back by hardware, right? But for
'NV', maybe this is not an issue, since hardware doesn't change it,
It only check 'ON', 'SN' and updates 'ON' if needed. And that's
why we need use LOCK prefix to update 'SN', right? What is you
thoughts about this?

Sorry for the long description above, since the hardware guys
advise me to use LOCK prefix when updating the posted-interrupt
descriptor, I just don't want to make any mistake here.

Thanks,
Feng

> Also - please remember to trim your replies.
> 
> Jan


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