Re: [Xen-devel] [PATCH v10 6/7] vmx: VT-d posted-interrupt core logic handling

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, January 20, 2016 7:36 PM
> To: Wu, Feng <feng.wu@xxxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Dario Faggioli
> <dario.faggioli@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxxxxx>;
> Tian, Kevin <kevin.tian@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxx; Keir Fraser
> <keir@xxxxxxx>
> Subject: RE: [PATCH v10 6/7] vmx: VT-d posted-interrupt core logic handling
> 
> >>> On 20.01.16 at 12:20, <feng.wu@xxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Wednesday, January 20, 2016 4:35 PM
> >> >>> On 20.01.16 at 08:49, <feng.wu@xxxxxxxxx> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> Sent: Monday, January 18, 2016 11:14 PM
> >> >> >>> On 03.12.15 at 09:35, <feng.wu@xxxxxxxxx> wrote:
> >> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> >> > @@ -83,7 +83,131 @@ static int vmx_msr_write_intercept(unsigned
> int
> >> >> msr, uint64_t msr_content);
> >> >> > +    ASSERT(v->arch.hvm_vmx.pi_block_cpu == NR_CPUS);
> >> >> > +
> >> >> > +    /*
> >> >> > +     * The vCPU is blocking, we need to add it to one of the per pCPU
> >> lists.
> >> >> > +     * We save v->processor to v->arch.hvm_vmx.pi_block_cpu and
> use
> >> it
> >> >> for
> >> >> > +     * the per-CPU list, we also save it to posted-interrupt 
> >> >> > descriptor
> >> and
> >> >> > +     * make it as the destination of the wake-up notification event.
> >> >> > +     */
> >> >> > +    v->arch.hvm_vmx.pi_block_cpu = v->processor;
> >> >> > +
> >> >> > +    spin_lock_irqsave(&per_cpu(pi_blocked_vcpu_lock,
> >> >> > +                      v->arch.hvm_vmx.pi_block_cpu), flags);
> >> >> > +    list_add_tail(&v->arch.hvm_vmx.pi_blocked_vcpu_list,
> >> >> > +                  &per_cpu(pi_blocked_vcpu, v-
> >> >arch.hvm_vmx.pi_block_cpu));
> >> >> > +    spin_unlock_irqrestore(&per_cpu(pi_blocked_vcpu_lock,
> >> >> > +                           v->arch.hvm_vmx.pi_block_cpu), flags);
> >> >> > +
> >> >> > +    ASSERT(!pi_test_sn(pi_desc));
> >> >> > +
> >> >> > +    /*
> >> >> > +     * We don't need to set the 'NDST' field, since it should point 
> >> >> > to
> >> >> > +     * the same pCPU as v->processor.
> >> >> > +     */
> >> >> > +
> >> >> > +    write_atomic(&pi_desc->nv, pi_wakeup_vector);
> >> >>
> >> >> Stray blank line between comment and corresponding code.
> >> >>
> >> >> Also the ASSERT() is rather more disconnected from the write
> >> >> than seems desirable: Wouldn't it be better to cmpxchg() the
> >> >> whole 32-bit value, validating that SN is clear in the result?
> >> >
> >> > Not quite understand this. The control field is 64 bits, do you
> >> > mean cmpxchg() the whole 64-bit value like follows:
> >> >
> >> > +        do {
> >> > +            old.control = new.control = pi_desc->control;
> >> > +            new.nv = pi_wakeup_vector;
> >> > +        } while ( cmpxchg(&pi_desc->control, old.control, new.control)
> >> > +                  != old.control );
> >> >
> >> > This a somewhat like the implementation in the earlier versions,
> >> > why do you want to change it back?
> >>
> >> Did you read what I've said? I'm worried about the disconnect of
> >> assertion and update: You really care about SN being clear
> >> _after_ the update aiui.
> >
> > No, the ASSERT has no connection with the update. the SN bit should
> > be clear before updating pi_desc->nv, adding ASSERT here is just kind
> > of protection code.
> 
> And SN can't get set behind your back between the ASSERT() and
> the update?

Yes.

> If the answer is "yes", then the code is indeed fine as is.
> 
> >> >> > +    pi_block_cpu = v->arch.hvm_vmx.pi_block_cpu;
> >> >> > +    if ( pi_block_cpu == NR_CPUS )
> >> >> > +        return;
> >> >>
> >> >> Are this condition and the one in the immediately preceding if()
> >> >> connected in any way?
> >> >
> >> > I am not sure I understand this correctly. Which one is
> >> > " the one in the immediately preceding if()"?
> >>
> >> If you hadn't ripped out too much of the context, it would be a
> >> matter of pointing you back up. Now I have to re-quote the full
> >> code:
> >>
> >> +    if ( pi_desc->nv != posted_intr_vector )
> >> +        write_atomic(&pi_desc->nv, posted_intr_vector);
> >> +
> >> +    /* the vCPU is not on any blocking list. */
> >> +    pi_block_cpu = v->arch.hvm_vmx.pi_block_cpu;
> >> +    if ( pi_block_cpu == NR_CPUS )
> >> +        return;
> >>
> >> These are the two if()-s the question is about.
> >>
> >> >> I.e. could the latter perhaps become an
> >> >> ASSERT() by connecting the two blocks suitably? I'm simply
> >> >> trying to limit the number of different cases to consider mentally
> >> >> when looking at this code ...
> >> >
> >> > If we get a true from above check, it means the vcpu was removed by
> >> > pi_wakeup_interrupt(), then we just need to return. If we get a false,
> >> > we will acquire the spinlock as the following code does, there are two
> >> > scenarios:
> >> > - pi_wakeup_interrupt() acquires the spinlock before us, then when
> >> > we get the lock, 'v->arch.hvm_vmx.pi_block_cpu' has already been
> >> > set to NR_CPUS by pi_wakeup_interrupt(), we need do nothing.
> >> > - We get the spinlock before pi_wakeup_interrupt(), this time we need
> >> > to remove the vCPU from the list and set 'v-
> >arch.hvm_vmx.pi_block_cpu'
> >> > to NR_CPUS.
> >>
> >> This is all only about the second condition, but the question really is
> >> whether one being true or false may imply the result of other. In
> >> which case this would better be ASSERT()ed than handled by two
> >> conditionals.
> >
> > Thanks for the explanation. I don't think there are any connections
> > Between " if ( pi_desc->nv != posted_intr_vector )" and
> > " if ( pi_block_cpu == NR_CPUS )".
> 
> Well, it would have seemed to me that some of the four possible
> combinations can't validly occur, e.g. due to NV always having a
> particular value when pi_block_cpu != NR_CPUS.

If pi_desc->nv != posted_intr_vector, we can have pi_block_cpu == NR_CPUS
or pi_block_cpu != NR_CPUS

If pi_desc->nv == posted_intr_vector, I think pi_block_cpu == NR_CPUS

Base on the above fact, how do you think I should add the ASSERT() thing?

Thanks,
Feng

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