From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v10 6/7] vmx: VT-d posted-interrupt core logic handling Date: Wed, 23 Dec 2015 03:21:08 +0100 Message-ID: <1450837268.19320.192.camel@citrix.com> References: <1449131734-3578-1-git-send-email-feng.wu@intel.com> <1449131734-3578-7-git-send-email-feng.wu@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5788234543489815651==" Return-path: In-Reply-To: <1449131734-3578-7-git-send-email-feng.wu@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Feng Wu , xen-devel@lists.xen.org Cc: George Dunlap , Andrew Cooper , Kevin Tian , Keir Fraser , Jan Beulich List-Id: xen-devel@lists.xenproject.org --===============5788234543489815651== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-hWQ1J+Kk0eX5nvD1GRsL" --=-hWQ1J+Kk0eX5nvD1GRsL Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Here I am, 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 >=20 > [..] > 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 > =C2=A0 don't need use another list to save the vCPUs with 'ON' set, just > =C2=A0 directly call vcpu_unblock(v). >=20 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: :105: trailing whitespace. void arch_vcpu_block(struct vcpu *v) :106: trailing whitespace. { :107: trailing whitespace. =C2=A0=C2=A0=C2=A0=C2=A0if ( v->arch.vcpu_block ) :108: trailing whitespace. =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0v->arch.vcpu_block(v); :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. And there are also a couple of long lines, which you should split. > +void vmx_vcpu_block(struct vcpu *v) > +{ > +=C2=A0=C2=A0=C2=A0=C2=A0unsigned long flags; > +=C2=A0=C2=A0=C2=A0=C2=A0struct pi_desc *pi_desc =3D &v->arch.hvm_vmx.pi_= desc; > + > +=C2=A0=C2=A0=C2=A0=C2=A0if ( !has_arch_pdevs(v->domain) ) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return; > + > +=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(v->arch.hvm_vmx.pi_block_cpu =3D=3D NR_CP= US); > + > +=C2=A0=C2=A0=C2=A0=C2=A0/* > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* The vCPU is blocking, we need to add it = to one of the per > pCPU lists. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* We save v->processor to v->arch.hvm_vmx.= pi_block_cpu and use > it for > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* the per-CPU list, we also save it to pos= ted-interrupt > descriptor and > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* make it as the destination of the wake-u= p notification event. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > +=C2=A0=C2=A0=C2=A0=C2=A0v->arch.hvm_vmx.pi_block_cpu =3D v->processor; > + > +=C2=A0=C2=A0=C2=A0=C2=A0spin_lock_irqsave(&per_cpu(pi_blocked_vcpu_lock, > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0v->arch.hvm_vmx= .pi_block_cpu), flags); > +=C2=A0=C2=A0=C2=A0=C2=A0list_add_tail(&v->arch.hvm_vmx.pi_blocked_vcpu_l= ist, > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0&per_cpu(pi_blocked_vcpu, v- > >arch.hvm_vmx.pi_block_cpu)); > +=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock_irqrestore(&per_cpu(pi_blocked_vcpu_= lock, > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0v->arch.hvm_vmx.pi_block_cpu), flags); > + > +=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(!pi_test_sn(pi_desc)); > + > +=C2=A0=C2=A0=C2=A0=C2=A0/* > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* We don't need to set the 'NDST' field, s= ince it should point > to > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* the same pCPU as v->processor. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > + So, maybe we can ASSERT() that? > +=C2=A0=C2=A0=C2=A0=C2=A0write_atomic(&pi_desc->nv, pi_wakeup_vector); > +} > +static void vmx_pi_switch_from(struct vcpu *v) > +{ > +=C2=A0=C2=A0=C2=A0=C2=A0struct pi_desc *pi_desc =3D &v->arch.hvm_vmx.pi_= desc; > + > +=C2=A0=C2=A0=C2=A0=C2=A0if ( !iommu_intpost || !has_arch_pdevs(v->domain= ) || > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0test_bit(_VPF_bloc= ked, &v->pause_flags) ) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return; > + > +=C2=A0=C2=A0=C2=A0=C2=A0/* > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* The vCPU has been preempted or went to s= leep. We don't need > to send > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* notification event to a non-running vcpu= , the interrupt > information > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* will be delivered to it before VM-ENTRY = when the vcpu is > scheduled > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* to run next time. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > +=C2=A0=C2=A0=C2=A0=C2=A0pi_set_sn(pi_desc); > +} > + > +static void vmx_pi_switch_to(struct vcpu *v) > +{ > +=C2=A0=C2=A0=C2=A0=C2=A0struct pi_desc *pi_desc =3D &v->arch.hvm_vmx.pi_= desc; > + > +=C2=A0=C2=A0=C2=A0=C2=A0if ( !iommu_intpost || !has_arch_pdevs(v->domain= ) ) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return; > + > +=C2=A0=C2=A0=C2=A0=C2=A0if ( x2apic_enabled ) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0write_atomic(&pi_desc->n= dst, cpu_physical_id(v->processor)); > +=C2=A0=C2=A0=C2=A0=C2=A0else > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0write_atomic(&pi_desc->n= dst, > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0MASK_INSR(cpu_physica= l_id(v->processor), > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0PI_xAPIC_NDST_MASK)); > + > +=C2=A0=C2=A0=C2=A0=C2=A0pi_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). > +} > + > +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. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-hWQ1J+Kk0eX5nvD1GRsL Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEABECAAYFAlZ6BRUACgkQk4XaBE3IOsRprgCfe5XgPOAoyhbpmSku6228N5VB Gh0AninWwsSZ+JHK1405jV27gYKMZQ1V =PJMi -----END PGP SIGNATURE----- --=-hWQ1J+Kk0eX5nvD1GRsL-- --===============5788234543489815651== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============5788234543489815651==--