From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45574) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6fOW-0006xW-5a for qemu-devel@nongnu.org; Wed, 09 Dec 2015 09:07:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a6fOR-0005QM-5g for qemu-devel@nongnu.org; Wed, 09 Dec 2015 09:07:00 -0500 Received: from smtp.citrix.com ([66.165.176.89]:9459) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6fOR-0005QE-1d for qemu-devel@nongnu.org; Wed, 09 Dec 2015 09:06:55 -0500 Message-ID: <1449669949.16124.252.camel@citrix.com> From: Ian Campbell Date: Wed, 9 Dec 2015 14:05:49 +0000 In-Reply-To: <5668331C.3040200@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> <1449668506.16124.239.camel@citrix.com> <5668331C.3040200@citrix.com> Content-Type: text/plain; charset="UTF-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Xen-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: Andrew Cooper , stefano.stabellini@eu.citrix.com Cc: ian.jackson@eu.citrix.com, wei.liu2@citrix.com, qemu-devel@nongnu.org, xen-devel@lists.xen.org On Wed, 2015-12-09 at 13:56 +0000, Andrew Cooper wrote: > On 09/12/15 13:41, 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); > > 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=C2=A0=C2=A0uint64_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 > It is never ok to convert a pointer like this.=C2=A0=C2=A0In 32bit (littl= e endian) > userspace, it will leave the upper half of mfn uninitialised on the > stack. mfn is a 32-bit value on such systems, so there is no upper half any way. NB I was talking about passing to xc_map_..., not the call to xenstore_read_fe... In any case my preference is the more long winded way I had further down. Ian.