> -----Original Message-----
> From: Dario Faggioli [mailto:dario.faggi...@citrix.com]
> Sent: Wednesday, December 23, 2015 10:21 AM
> To: Wu, Feng <feng...@intel.com>; xen-devel@lists.xen.org
> Cc: Tian, Kevin <kevin.t...@intel.com>; Keir Fraser <k...@xen.org>; George
> Dunlap <george.dun...@eu.citrix.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Jan Beulich <jbeul...@suse.com>
> 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@lists.xen.org
http://lists.xen.org/xen-devel
