From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: Fwd: [v3 14/15] Update Posted-Interrupts Descriptor during vCPU scheduling Date: Tue, 14 Jul 2015 18:02:37 +0200 Message-ID: <1436889757.13522.185.camel@citrix.com> References: <1435123109-10481-15-git-send-email-feng.wu@intel.com> <55918214.4030102@citrix.com> <1435633087.25170.274.camel@citrix.com> <1435825253.25170.406.camel@citrix.com> <559E6EB7.3050609@eu.citrix.com> <559E96E0020000780008ED84@mail.emea.novell.com> <1436451512.22672.333.camel@citrix.com> <559E84EF.7060507@eu.citrix.com> <559F80C6020000780008F348@mail.emea.novell.com> <1436526348.22672.387.camel@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3416689402330721971==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Wu, Feng" Cc: "Tian, Kevin" , "keir@xen.org" , George Dunlap , "andrew.cooper3@citrix.com" , xen-devel , Jan Beulich , "Zhang, Yang Z" List-Id: xen-devel@lists.xenproject.org --===============3416689402330721971== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-AOziVInDOYxyY3dI4F6u" --=-AOziVInDOYxyY3dI4F6u Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2015-07-14 at 14:08 +0000, Wu, Feng wrote: >=20 > > -----Original Message----- > > From: Dario Faggioli [mailto:dario.faggioli@citrix.com] > > - do you need to perform an action upon context switch (on prev and/or > > next vcpu)? If yes, there's an arch specific path in there already; > > - do you need to perform an action when a vcpu wakes-up? If yes, we > > need an arch hook in vcpu_wake(); > > - do you need to perform an action when a vcpu goes to sleep? If yes, > > we need an arch hook in vcpu_sleep_nosync(); > >=20 > > I think this makes a more than fair solution. I happen to like it even > > better than the centralized approach, actually! That is for personal > > taste, but also because I think it may be useful for others too, in > > future, to be able to execute arch specific code, e.g., upon wakes-up, > > in which case we will be able to use the hook that we're introducing > > here for PI. > >=20 > > Thanks and Regards, > > Dario >=20 > Hi Dario, >=20 Hi, > Thanks for the suggestion! I made a draft patch for this idea,=20 > Great! > It may have > some issues since It is just a draft version, kind of like prototype, I p= ost > it here just like to know whether it is meet your expectation, if it is I > can continue with this direction and this may speed up the upstreaming > process. > Yes, I think this is a good approach, and the proper way for this feature to interact with the scheduler. I appreciate it is a draft, so I'm not performing a thorough review, but I'll try to at least give some comments, in the hope that it helps. > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 6eebc1a..7e678c8 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -740,6 +740,81 @@ static void vmx_ctxt_switch_from(struct vcpu *v) > vmx_save_guest_msrs(v); > vmx_restore_host_msrs(); > vmx_save_dr(v); > + > + if ( iommu_intpost ) > + { > I'd put an helper together ( vmx__pi() ) and put the body of this if in it. Then, either just call it unconditionally from here and have, in the helper, something like this: if ( !iommu_intpost ) return; Or just have this in here: if ( iommu_intpost ) vmx__pi(); > + struct pi_desc *pi_desc =3D &v->arch.hvm_vmx.pi_desc; > + struct pi_desc old, new; > + unsigned long flags; > + > + if ( vcpu_runnable(v) || !test_bit(_VPF_blocked, &v->pause_flags= ) ) > + { > Aha! So, AFAICT, this means we can deal with preemptions, sleeps and blockings (as can be seen below) here in _ctxt_switch_from, i.e., we don't have to call in this code from vcpu_sleep_nosync(), like we were, when tying this to vcpu_runstate_change())... nice! :-D > + /* > + * The vCPU is preempted or sleeped.=20 > "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); > + > + } > + else if ( test_bit(_VPF_blocked, &v->pause_flags) ) > + { > + /* The vCPU is blocked */ > This comment does not add much, I'd kill it. > + ASSERT(v->arch.hvm_vmx.pi_block_cpu =3D=3D -1); > + > + /* > + * The vCPU is blocked on the block list.=20 > What about "The vCPU is blocking, we need to add it to one of the per pCPU lists." > Add the blocked > + * vCPU on the list of the v->arch.hvm_vmx.pi_block_cpu, > What you're doing seems more "Add the vCPU to the blocked list of v->processor, which will be the target of the wake-up notification". > + * which is the destination of the wake-up notification even= t. > + */ > + v->arch.hvm_vmx.pi_block_cpu =3D 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_b= lock_cpu)); > + spin_unlock_irqrestore(&per_cpu(pi_blocked_vcpu_lock, > + v->arch.hvm_vmx.pi_block_cpu), flags); > + > + do { > + old.control =3D new.control =3D pi_desc->control; > + > + /* > + * We should not block the vCPU if > + * an interrupt was posted for it. > + */ > + > + if ( old.on ) > + { > + /* > + * The vCPU will be removed from the block list > + * during its state transferring from RUNSTATE_block= ed > + * to RUNSTATE_runnable after the following tasklet > + * is executed. > We can avoid referencing RUNSTATEs at all, can't we? Just say something about the vCPU leaving the blocked vCPUs list on the wake-up path. > + */ > + tasklet_schedule(&v->arch.hvm_vmx.pi_vcpu_wakeup_tas= klet); > + return; > + } > + > + /* > + * Change the 'NDST' field to v->arch.hvm_vmx.pi_block_c= pu, > + * so when external interrupts from assigned deivces hap= pen, > + * wakeup notifiction event will go to > + * v->arch.hvm_vmx.pi_block_cpu, then in pi_wakeup_inter= rupt() > + * we can find the vCPU in the right list to wake up. > + */ > + if ( x2apic_enabled ) > + new.ndst =3D cpu_physical_id(v->arch.hvm_vmx.pi_bloc= k_cpu); > + else > + new.ndst =3D MASK_INSR(cpu_physical_id( > + v->arch.hvm_vmx.pi_block_cpu), > + PI_xAPIC_NDST_MASK); > + new.sn =3D 0; > + new.nv =3D pi_wakeup_vector; > + } while ( cmpxchg(&pi_desc->control, old.control, new.contro= l) > + !=3D old.control ); > + } > + } ISTR, Jan had some comments on this code (variable names, etc.). It's probably goes without saying that those still applies. > static void vmx_ctxt_switch_to(struct vcpu *v) > @@ -764,6 +839,22 @@ static void vmx_ctxt_switch_to(struct vcpu *v) >=20 > vmx_restore_guest_msrs(v); > vmx_restore_dr(v); > + > + if ( iommu_intpost ) > + { > You may consider having an helper for this too, for symmetry with the above case, but this is less of an issue, IMO. > + struct pi_desc *pi_desc =3D &v->arch.hvm_vmx.pi_desc; > + > + ASSERT( pi_desc->sn =3D=3D 1 ); ^space Above you wrote: ASSERT(v->arch.hvm_vmx.pi_block_cpu =3D=3D -1); ^no space Please, pick up one format (ideally, following suit from other occurrences in the file, if any), and be consistent. > + > + 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); > + } > } > +void arch_vcpu_wake(struct vcpu *v) > +{ > + if ( !iommu_intpost || (v->runstate.state !=3D RUNSTATE_blocked) ) > + return; > + > + if ( likely(vcpu_runnable(v)) || > + !test_bit(_VPF_blocked, &v->pause_flags) ) > + { Invert this and bail if true? Well, a matter of taste, I guess... but it will save one level of indentation. > + struct pi_desc *pi_desc =3D &v->arch.hvm_vmx.pi_desc; > + unsigned long flags; > + > + /* > + * blocked -> runnable/offline > + * If the state is transferred from RUNSTATE_blocked, > + * we should set 'NV' feild back to posted_intr_vector, > + * so the Posted-Interrupts can be delivered to the vCPU > + * by VT-d HW after it is scheduled to run. > + */ > Again, make the comment describe things in a RUNSTATE independent way (e.g., in terms of 'generic states', like "it's preempted", "it's blocked", "it's runnable"; or in terms of flags; or both). Thanks and Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-AOziVInDOYxyY3dI4F6u 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 v2 iEYEABECAAYFAlWlMqwACgkQk4XaBE3IOsRACQCdEoXnFuLw+u/q4vgdDGprOHXC pywAn2/ds0B8N+mGWAxjApGonrT9qrCY =izhk -----END PGP SIGNATURE----- --=-AOziVInDOYxyY3dI4F6u-- --===============3416689402330721971== 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 --===============3416689402330721971==--