From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v4 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation Date: Wed, 14 Sep 2016 18:52:17 +0200 Message-ID: <1473871937.6339.228.camel@citrix.com> References: <1473668175-3088-1-git-send-email-dongli.zhang@oracle.com> <1473668175-3088-2-git-send-email-dongli.zhang@oracle.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4556190160048276355==" Return-path: In-Reply-To: <1473668175-3088-2-git-send-email-dongli.zhang@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Dongli Zhang , xen-devel@lists.xen.org Cc: sstabellini@kernel.org, wei.liu2@citrix.com, George.Dunlap@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, tim@xen.org, david.vrabel@citrix.com, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org --===============4556190160048276355== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-4XKlm2k/f6z+CBuKImYx" --=-4XKlm2k/f6z+CBuKImYx Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2016-09-12 at 16:16 +0800, Dongli Zhang wrote: > This patch implemented parts of TODO left in commit id > a902c12ee45fc9389eb8fe54eeddaf267a555c58.=20 > We usually put both the (not necessarily full)=C2=A0hash and the subject line of the commit in here. > Signed-off-by: Dongli Zhang >=C2=A0 > diff --git a/xen/common/domain.c b/xen/common/domain.c > index a8804e4..7be1bee 100644 > @@ -303,6 +303,8 @@ struct domain *domain_create(domid_t domid, > unsigned int domcr_flags, > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( !zalloc_cpumask_var(&d->domain_dirty_c= pumask) ) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0goto fail; > =C2=A0 > +=C2=A0=C2=A0=C2=A0=C2=A0d->is_ever_unpaused =3D false; > + > I'd go for something like "first_unpaused" or "creation_finished", but if maintainers are happy with this one already, I'm fine too. > @@ -1004,6 +1006,15 @@ int domain_unpause_by_systemcontroller(struct > domain *d) > =C2=A0{ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int old, new, prev =3D d->controller_pause_= count; > =C2=A0 > +=C2=A0=C2=A0=C2=A0=C2=A0/* > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Set is_ever_unpaused to true when this d= omain gets unpaused > for the > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* first time. We record this information h= ere to help > populate_physmap > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* verify whether the domain has ever been = unpaused. > MEMF_no_tlbflush > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* is allowed to be set by populate_physmap= only during vm > creation. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ "We record this information here for populate_physmap to figure out =C2=A0that the domain has already been unpaused, after finishing being =C2=A0created. That's because we're allowed to set MEMF_no_tlbflush only =C2=A0during VM creation." Or, de-focusing the unpausing even more: "We record this information here for populate_physmap to figure out =C2=A0tha t the domain has finished being created. In fact, we're only =C2=A0allowed to set the MEMF_no_tlbflush flag during VM creation." I.e., the important thing is not really the unpausing (that's where we found it handy to put the check), it's the fact that something should only happen at creation time and why (see below). > +=C2=A0=C2=A0=C2=A0=C2=A0if ( unlikely(!d->is_ever_unpaused) ) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0d->is_ever_unpaused =3D = true; > + > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0do > =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=A0old =3D prev; > diff --git a/xen/common/memory.c b/xen/common/memory.c > index cc0f69e..f3a733b 100644 > @@ -150,6 +152,14 @@ static void populate_physmap(struct memop_args > *a) > =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=A0=C2=A0=C2=A0max_order(curr_d)) ) > =C2=A0=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=C2=A0* MEMF_no_tlbflush can be set only during = vm creation phase > when > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* is_ever_unpaused is still false before t= his domain gets > unpaused for > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* the first time. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > What about, 'citing' from the changelog: "With MEMF_no_tlbflush set, alloc_heap_pages() will ignore TLB- =C2=A0flushes. After VM creation, this is a security issue (it can make =C2=A0pages accessible to guest B, when guest A may still have a cached =C2=A0mapping to them). So we only do this only during domain creation, =C2=A0when the domain itself has not yet been unpaused for the first =C2=A0time." > +=C2=A0=C2=A0=C2=A0=C2=A0if ( unlikely(!d->is_ever_unpaused) ) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0a->memflags |=3D MEMF_no= _tlbflush; > + > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0for ( i =3D a->nr_done; i < a->nr_extents; = i++ ) > =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=A0if ( i !=3D a->nr_d= one && hypercall_preempt_check() ) > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h > index 2f9c15f..7fe8841 100644 > @@ -474,6 +474,9 @@ struct domain > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int guest_= request_enabled=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0: 1; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int guest_= request_sync=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0: 1= ; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} monitor; > + > +=C2=A0=C2=A0=C2=A0=C2=A0/* set to true the first time this domain gets u= npaused. */ > I think it's relevant to say _when_ that is. What about: /* =C2=A0* Set to true at the very end of domain creation, when the domain is= =C2=A0 =C2=A0* unpaused for the first time by the systemcontroller. =C2=A0*/ (not 100% happy about the "by the systemcontroller" part... but that's the idea.) > +=C2=A0=C2=A0=C2=A0=C2=A0bool_t is_ever_unpaused; > As said by Jan already --here and elsewhere-- new code should use 'bool'. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-4XKlm2k/f6z+CBuKImYx 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 iQIcBAABCAAGBQJX2YBBAAoJEBZCeImluHPupGoP/2eK16c2rJdwWXl8A1VCYL/G Oi2dQBKBBKOXfyMsbEp8FtS4v5c5dMjwroZWf4kt0mxQZg4D42/nUlb3Xy94yRw+ 4erf3huNhjPGByol4IlERizE2HX1iTkqD8rmuxHiS4TkYK0iD6hSfD2dAUkfUA0S t7G3XksnGtQlGM8SrmBuGny74UKZROPqt1x7jBXlyLHm4+IRJZeMB8QuRA8B+AWU In50Zt+bPOqKX00RP6cZff7beydkzW7eaP1KMMC+E4ojsd55JaccZ3JCRpXfhAWB E6iTC2rNsQKUzNzOdnlsfUwtzV8V5nCt3Flp+nrP1yRvRn3TM7isXp4xCBVPzQFW EsGN+E3r1htO0352mtty9mCUVnqhZVuo0m5yQkYGic5hSl5VaB894En2JKq2fy2R PpgJVN6kqrolwTHC30FFvbGJQmAZcgXcpyyWqZuusxG5hgy3wVQA9cNc4FZ4mVuZ kK0A0gVkMma127apCDq9lHaa1I1hQocEGOfnzz7V22NAuPBSrNgy00bcPA2H50q6 LcV06wAAQxXjo5UdcjEPqtAg/3wEvqDOBXfXDk0gtxiWMYvU4CnHNCbazH1Q/VZ9 kdkodvlOkvNoKnYkWcco+s4QvtrypQfyBAGm6EIlly2DVnHcM6MeIERa99aBfm5N pPM2Ge6g7diGRylbBNCp =cZ5s -----END PGP SIGNATURE----- --=-4XKlm2k/f6z+CBuKImYx-- --===============4556190160048276355== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============4556190160048276355==--