From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Lengyel, Tamas" Subject: Re: [PATCH v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access Date: Fri, 29 Jan 2016 09:12:41 -0700 Message-ID: References: <1454014688-25060-1-git-send-email-tlengyel@novetta.com> <56AB552202000078000CC5E5@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5641235498281547647==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aPBfU-00078N-LA for xen-devel@lists.xenproject.org; Fri, 29 Jan 2016 16:13:04 +0000 Received: by mail-vk0-f51.google.com with SMTP id e64so44757334vkg.0 for ; Fri, 29 Jan 2016 08:13:02 -0800 (PST) In-Reply-To: <56AB552202000078000CC5E5@prv-mh.provo.novell.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: Jan Beulich Cc: Wei Liu , Ian Campbell , Razvan Cojocaru , Stefano Stabellini , George Dunlap , Andrew Cooper , Ian Jackson , Stefano Stabellini , xen-devel@lists.xenproject.org, Keir Fraser List-Id: xen-devel@lists.xenproject.org --===============5641235498281547647== Content-Type: multipart/alternative; boundary=001a1143f24a2d095e052a7b4f24 --001a1143f24a2d095e052a7b4f24 Content-Type: text/plain; charset=UTF-8 On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich wrote: > >>> On 28.01.16 at 21:58, wrote: > > --- a/xen/arch/x86/mm/p2m.c > > +++ b/xen/arch/x86/mm/p2m.c > > @@ -1777,14 +1777,57 @@ bool_t p2m_mem_access_check(paddr_t gpa, > unsigned long gla, > > return (p2ma == p2m_access_n2rwx); > > } > > > > +static int p2m_set_altp2m_mem_access(struct domain *d, struct > p2m_domain *hp2m, > > + struct p2m_domain *ap2m, > p2m_access_t a, > > + unsigned long gfn) > > I think new functions would better not use "unsigned long" for frame > numbers. > The only place this is called from the gfn is already converted to unsigned long. I don't see much point in converting it back to gfn_t and then back to unsigned long again.. I was thinking this function may even be declared as inline? > > > +{ > > + mfn_t mfn; > > + p2m_type_t t; > > + p2m_access_t old_a; > > + unsigned int page_order; > > + int rc; > > + > > + mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); > > + > > + /* Check host p2m if no valid entry in alternate */ > > + if ( !mfn_valid(mfn) ) > > + { > > + mfn = hp2m->get_entry(hp2m, gfn, &t, &old_a, > > + P2M_ALLOC | P2M_UNSHARE, &page_order, > NULL); > > + > > + rc = -ESRCH; > > + if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > > + goto out; > > + > > + /* If this is a superpage, copy that first */ > > + if ( page_order != PAGE_ORDER_4K ) > > + { > > + unsigned long mask = ~((1UL << page_order) - 1); > > + gfn_t gfn2 = _gfn(gfn & mask); > > + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask); > > + > > + rc = ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, > t, old_a, 1); > > + if ( rc ) > > + goto out; > > + } > > + } > > + > > + rc = ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, > > + (current->domain != d)); > > + > > + out: > > + return rc; > > +} > > With there not being any involved error handling here, I don't think > using a label and goto is warranted here. But I'll leave the ultimate > decision to George, of course. > RIght, this is a remnant from the previous version of this function where out also had the p2m_unlock. Now that it is just a return I could do the return in place of the gotos. Let me know which one is preferred. > > > --- a/xen/include/public/memory.h > > +++ b/xen/include/public/memory.h > > @@ -423,11 +423,14 @@ struct xen_mem_access_op { > > /* xenmem_access_t */ > > uint8_t access; > > domid_t domid; > > + uint16_t altp2m_idx; > > + uint16_t _pad; > > /* > > * Number of pages for set op > > * Ignored on setting default access and other ops > > */ > > uint32_t nr; > > + uint32_t _pad2; > > Repeating what I had said on v1: So this is a tools only interface, > yes. But it's not versioned (other than e.g. domctl and sysctl), so > altering the interface structure is at least fragile. > Not sure what I can do to address this. Tamas --001a1143f24a2d095e052a7b4f24 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <JBeulich@suse.com>= ; wrote:
>>= > On 28.01.16 at 21:58, <tlen= gyel@novetta.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1777,14 +1777,57 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsig= ned long gla,
>=C2=A0 =C2=A0 =C2=A0 return (p2ma =3D=3D p2m_access_n2rwx);
>=C2=A0 }
>
> +static int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_dom= ain *hp2m,
> +=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=A0struct p2m_d= omain *ap2m, p2m_access_t 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=A0unsigned lon= g gfn)

I think new functions would better not use "unsigned long"= for frame
numbers.

The only place this is called = from the gfn is already converted to unsigned long. I don't see much po= int in converting it back to gfn_t and then back to unsigned long again.. I= was thinking this function may even be declared as inline?
=C2=A0

> +{
> +=C2=A0 =C2=A0 mfn_t mfn;
> +=C2=A0 =C2=A0 p2m_type_t t;
> +=C2=A0 =C2=A0 p2m_access_t old_a;
> +=C2=A0 =C2=A0 unsigned int page_order;
> +=C2=A0 =C2=A0 int rc;
> +
> +=C2=A0 =C2=A0 mfn =3D ap2m->get_entry(ap2m, gfn, &t, &old_= a, 0, NULL, NULL);
> +
> +=C2=A0 =C2=A0 /* Check host p2m if no valid entry in alternate */
> +=C2=A0 =C2=A0 if ( !mfn_valid(mfn) )
> +=C2=A0 =C2=A0 {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 mfn =3D hp2m->get_entry(hp2m, gfn, &am= p;t, &old_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 P2M_ALLOC | P2M_UNSHARE, &page_orde= r, NULL);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D -ESRCH;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( !mfn_valid(mfn) || t !=3D p2m_ram_rw= )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* If this is a superpage, copy that firs= t */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( page_order !=3D PAGE_ORDER_4K )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 unsigned long mask =3D ~((1= UL << page_order) - 1);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 gfn_t gfn2 =3D _gfn(gfn &am= p; mask);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mfn_t mfn2 =3D _mfn(mfn_x(m= fn) & mask);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D ap2m->set_entry(a= p2m, gfn_x(gfn2), mfn2, page_order, t, old_a, 1);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( rc )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 }
> +
> +=C2=A0 =C2=A0 rc =3D ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K= , t, 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(current->domain !=3D d));
> +
> + out:
> +=C2=A0 =C2=A0 return rc;
> +}

With there not being any involved error handling here, I don= 9;t think
using a label and goto is warranted here. But I'll leave the ultimate decision to George, of course.

RIght, t= his is a remnant from the previous version of this function where out also = had the p2m_unlock. Now that it is just a return I could do the return in p= lace of the gotos. Let me know which one is preferred.
=C2=A0=

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -423,11 +423,14 @@ struct xen_mem_access_op {
>=C2=A0 =C2=A0 =C2=A0 /* xenmem_access_t */
>=C2=A0 =C2=A0 =C2=A0 uint8_t access;
>=C2=A0 =C2=A0 =C2=A0 domid_t domid;
> +=C2=A0 =C2=A0 uint16_t altp2m_idx;
> +=C2=A0 =C2=A0 uint16_t _pad;
>=C2=A0 =C2=A0 =C2=A0 /*
>=C2=A0 =C2=A0 =C2=A0 =C2=A0* Number of pages for set op
>=C2=A0 =C2=A0 =C2=A0 =C2=A0* Ignored on setting default access and othe= r ops
>=C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>=C2=A0 =C2=A0 =C2=A0 uint32_t nr;
> +=C2=A0 =C2=A0 uint32_t _pad2;

Repeating what I had said on v1: So this is a tools only interface,<= br> yes. But it's not versioned (other than e.g. domctl and sysctl), so
altering the interface structure is at least fragile.

Not sure what I can do to address this.

Tamas
=

--001a1143f24a2d095e052a7b4f24-- --===============5641235498281547647== 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 --===============5641235498281547647==--