From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a7PYd-0005dh-Q4 for qemu-devel@nongnu.org; Fri, 11 Dec 2015 10:24:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a7PYZ-0002LO-O3 for qemu-devel@nongnu.org; Fri, 11 Dec 2015 10:24:31 -0500 Received: from smtp02.citrix.com ([66.165.176.63]:30265) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a7PYZ-0002LF-Jl for qemu-devel@nongnu.org; Fri, 11 Dec 2015 10:24:27 -0500 Message-ID: <1449847396.30975.49.camel@citrix.com> From: Ian Campbell Date: Fri, 11 Dec 2015 15:23:16 +0000 In-Reply-To: References: <1449141675.4424.125.camel@citrix.com> <1449141788-15084-1-git-send-email-ian.campbell@citrix.com> <1449141788-15084-5-git-send-email-ian.campbell@citrix.com> <1449668506.16124.239.camel@citrix.com> Content-Type: text/plain; charset="UTF-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH QEMU-XEN v6 4/8] xen: Switch uses of xc_map_foreign_range into xc_map_foreign_pages List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: wei.liu2@citrix.com, ian.jackson@eu.citrix.com, qemu-devel@nongnu.org, xen-devel@lists.xen.org On Fri, 2015-12-11 at 14:26 +0000, Stefano Stabellini wrote: > On Wed, 9 Dec 2015, Ian Campbell wrote: > > On Thu, 2015-12-03 at 11:23 +0000, Ian Campbell wrote: > > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c > > > index 5e324ef..c96d974 100644 > > > --- a/hw/display/xenfb.c > > > +++ b/hw/display/xenfb.c > > > @@ -104,9 +104,8 @@ static int common_bind(struct common *c) > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (xenstore_read_fe_int(&c->xendev, "e= vent-channel", &c- > > > >xendev.remote_port) =3D=3D -1) > > > =C2=A0 return -1; > > > =C2=A0 > > > -=C2=A0=C2=A0=C2=A0=C2=A0c->page =3D xc_map_foreign_range(xen_xc, c->= xendev.dom, > > > - =C2=A0=C2=A0=C2=A0XC_PAGE_SIZE, > > > - =C2=A0=C2=A0=C2=A0PROT_READ | PROT_WRITE, mfn); > > > +=C2=A0=C2=A0=C2=A0=C2=A0c->page =3D xc_map_foreign_pages(xen_xc, c->= xendev.dom, > > > +=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=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0PROT_R= EAD | PROT_WRITE, &mfn, 1); > >=20 > > This doesn't build for i386 userspace, since mfn is a uint64_t but > > xc_map_foreign_pages() wants a xen_pfn_t * (where xen_pfn_t =3D=3D unsi= gned > > long on x86). > >=20 > > Until now that was just a truncation which was already checked for > > with: > >=20 > > =C2=A0 =C2=A0 uint64_t mfn; > >=20 > > =C2=A0=C2=A0=C2=A0=C2=A0if (xenstore_read_fe_uint64(&c->xendev, "page-r= ef", &mfn) =3D=3D -1) > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return -1; > > =C2=A0=C2=A0=C2=A0=C2=A0assert(mfn =3D=3D (xen_pfn_t)mfn); > >=20 > > I think in principal passing "(xen_pfn_t *)&mfn" would ok (since it is > > a > > singleton array in this case), but I was thinking of going a bit > > further > > and: > >=20 > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c > > index 8b86b4a..54585fa 100644 > > --- a/hw/display/xenfb.c > > +++ b/hw/display/xenfb.c > > @@ -95,11 +95,13 @@ struct XenFB { > > =C2=A0 > > =C2=A0static int common_bind(struct common *c) > > =C2=A0{ > > -=C2=A0=C2=A0=C2=A0=C2=A0uint64_t mfn; > > +=C2=A0=C2=A0=C2=A0=C2=A0uint64_t val; > > +=C2=A0=C2=A0=C2=A0=C2=A0xen_pfn_t mfn; > > =C2=A0 > > -=C2=A0=C2=A0=C2=A0=C2=A0if (xenstore_read_fe_uint64(&c->xendev, "page-= ref", &mfn) =3D=3D -1) > > +=C2=A0=C2=A0=C2=A0=C2=A0if (xenstore_read_fe_uint64(&c->xendev, "page-= ref", &val) =3D=3D -1) > > =C2=A0 return -1; > > -=C2=A0=C2=A0=C2=A0=C2=A0assert(mfn =3D=3D (xen_pfn_t)mfn); > > +=C2=A0=C2=A0=C2=A0=C2=A0mfn =3D (xen_pfn_t)val; > > +=C2=A0=C2=A0=C2=A0=C2=A0assert(val =3D=3D mfn); > > =C2=A0 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (xenstore_read_fe_int(&c->xendev, "eve= nt-channel", &c- > > >xendev.remote_port) =3D=3D -1) > > =C2=A0 return -1; > >=20 > > Stefano, what do you think/prefer? An alternative to the above >=20 > I like this change because it makes the code more obvious Thanks, with that change may I keep your Reviewed-by?