From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40506) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6f1a-00019o-Au for qemu-devel@nongnu.org; Wed, 09 Dec 2015 08:43:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a6f1X-0007mc-58 for qemu-devel@nongnu.org; Wed, 09 Dec 2015 08:43:18 -0500 Received: from smtp02.citrix.com ([66.165.176.63]:31700) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6f1W-0007mO-Uz for qemu-devel@nongnu.org; Wed, 09 Dec 2015 08:43:15 -0500 Message-ID: <1449668506.16124.239.camel@citrix.com> From: Ian Campbell Date: Wed, 9 Dec 2015 13:41:46 +0000 In-Reply-To: <1449141788-15084-5-git-send-email-ian.campbell@citrix.com> 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> 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@eu.citrix.com Cc: wei.liu2@citrix.com, ian.jackson@eu.citrix.com, qemu-devel@nongnu.org, xen-devel@lists.xen.org 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, "event= -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->xend= ev.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->xend= ev.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_READ | = PROT_WRITE, &mfn, 1); 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 unsigned long on x86). Until now that was just a truncation which was already checked for with: =C2=A0 =C2=A0 uint64_t mfn; =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=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); 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: 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, "event-c= hannel", &c->xendev.remote_port) =3D=3D -1) =C2=A0 return -1; Stefano, what do you think/prefer? An alternative to the above I've not spotted any other similar constructs, xenfb is a bit unusual here in that it apparently uses foreign mappings rather than grants like most drivers do. Ian.