[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v10 6/7] vmx: VT-d posted-interrupt core logic handling
> -----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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |