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

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

> -----Original Message-----
> From: Dario Faggioli [mailto:dario.faggioli@xxxxxxxxxx]
> Sent: Wednesday, December 23, 2015 10:21 AM
> To: Wu, Feng <feng.wu@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Cc: Tian, Kevin <kevin.tian@xxxxxxxxx>; Keir Fraser <keir@xxxxxxx>; George
> Dunlap <george.dunlap@xxxxxxxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v10 6/7] vmx: VT-d posted-interrupt core logic
> handling
> 
> Here I am,

Nice to having you reviewing the code :)

> 
> On Thu, 2015-12-03 at 16:35 +0800, Feng Wu wrote:
> > This is the core logic handling for VT-d posted-interrupts. Basically
> > it
> > deals with how and when to update posted-interrupts during the
> > following
> > scenarios:
> > - vCPU is preempted
> > - vCPU is slept
> > - vCPU is blocked
> >
> > [..]
> >
> Thanks for changing the changelog, making it look like much more an
> high level description of what happens and why.
> 
> > v10:
> > - Check iommu_intpost first
> > - Remove pointless checking of has_hvm_container_vcpu(v)
> > - Rename 'vmx_pi_state_change' to 'vmx_pi_state_to_normal'
> > - Since vcpu_unblock() doesn't acquire 'pi_blocked_vcpu_lock', we
> > Â don't need use another list to save the vCPUs with 'ON' set, just
> > Â directly call vcpu_unblock(v).
> >
> This were all my comments to v9 and, I've verified in the patch, they
> actually have been all addressed... Thanks for this.
> 
> This patch looks fine to me now. There are a few minor issues that I'll
> raise inline, but in general, I think this is a good design, and the
> patch does it job fine at implementing it.
> 
> Here they are the detailed comments.
> 
> First of all, trying to apply it, I got:
> 
> <stdin>:105: trailing whitespace.
> void arch_vcpu_block(struct vcpu *v)
> <stdin>:106: trailing whitespace.
> {
> <stdin>:107: trailing whitespace.
> ÂÂÂÂif ( v->arch.vcpu_block )
> <stdin>:108: trailing whitespace.
> ÂÂÂÂÂÂÂÂv->arch.vcpu_block(v);
> <stdin>:109: trailing whitespace.
> }
> 
> It may not be accurate, though (i.e., it may be due to what I used for
> applying the patches), so, please, double check.

Sure, will double check it.

> 
> And there are also a couple of long lines, which you should split.
> 
> > +void vmx_vcpu_block(struct vcpu *v)
> > +{
> > +ÂÂÂÂunsigned long flags;
> > +ÂÂÂÂstruct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> > +
> > +ÂÂÂÂif ( !has_arch_pdevs(v->domain) )
> > +ÂÂÂÂÂÂÂÂreturn;
> > +
> > +ÂÂÂÂ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.
> > +ÂÂÂÂÂ*/
> > +
> So, maybe we can ASSERT() that?

Yes, I think ASSERT() is preferred here.

> 
> > +ÂÂÂÂwrite_atomic(&pi_desc->nv, pi_wakeup_vector);
> > +}
> 
> > +static void vmx_pi_switch_from(struct vcpu *v)
> > +{
> > +ÂÂÂÂstruct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> > +
> > +ÂÂÂÂif ( !iommu_intpost || !has_arch_pdevs(v->domain) ||
> > +ÂÂÂÂÂÂÂÂÂtest_bit(_VPF_blocked, &v->pause_flags) )
> > +ÂÂÂÂÂÂÂÂreturn;
> > +
> > +ÂÂÂÂ/*
> > +ÂÂÂÂÂ* The vCPU has been preempted or went to sleep. 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_set_sn(pi_desc);
> > +}
> > +
> > +static void vmx_pi_switch_to(struct vcpu *v)
> > +{
> > +ÂÂÂÂstruct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> > +
> > +ÂÂÂÂif ( !iommu_intpost || !has_arch_pdevs(v->domain) )
> > +ÂÂÂÂÂÂÂÂreturn;
> > +
> > +ÂÂÂÂif ( x2apic_enabled )
> > +ÂÂÂÂÂÂÂÂwrite_atomic(&pi_desc->ndst, cpu_physical_id(v->processor));
> > +ÂÂÂÂelse
> > +ÂÂÂÂÂÂÂÂwrite_atomic(&pi_desc->ndst,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂMASK_INSR(cpu_physical_id(v->processor),
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂPI_xAPIC_NDST_MASK));
> > +
> > +ÂÂÂÂpi_clear_sn(pi_desc);
> >
> No comment matching the one above (for pi_set_sn(), in
> vmx_pi_switch_from())? I don't feel too strong about this, but it would
> be nice to have both (or none, but I'd prefer both! :-D).

I will add some comments here.

> 
> > +}
> > +
> > +static void vmx_pi_state_to_normal(struct vcpu *v)
> > +{
> >
> I'm still a bit puzzled about the name... But it's better than in the
> previous round, and I can't suggest a solution that I would like myself
> better... so I'm fine with this one.

Yeah, I also didn't find a better name, I will continue to think about it:)

In the end, thanks for the review, Dario! Merry Christmas and Happy New Year!!

Thanks,
Feng

> 
> Regards,
> Dario
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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