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? From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH QEMU-XEN v6 4/8] xen: Switch uses of xc_map_foreign_range into xc_map_foreign_pages Date: Fri, 11 Dec 2015 15:23:16 +0000 Message-ID: <1449847396.30975.49.camel@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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: wei.liu2@citrix.com, ian.jackson@eu.citrix.com, qemu-devel@nongnu.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org T24gRnJpLCAyMDE1LTEyLTExIGF0IDE0OjI2ICswMDAwLCBTdGVmYW5vIFN0YWJlbGxpbmkgd3Jv dGU6Cj4gT24gV2VkLCA5IERlYyAyMDE1LCBJYW4gQ2FtcGJlbGwgd3JvdGU6Cj4gPiBPbiBUaHUs IDIwMTUtMTItMDMgYXQgMTE6MjMgKzAwMDAsIElhbiBDYW1wYmVsbCB3cm90ZToKPiA+ID4gZGlm ZiAtLWdpdCBhL2h3L2Rpc3BsYXkveGVuZmIuYyBiL2h3L2Rpc3BsYXkveGVuZmIuYwo+ID4gPiBp bmRleCA1ZTMyNGVmLi5jOTZkOTc0IDEwMDY0NAo+ID4gPiAtLS0gYS9ody9kaXNwbGF5L3hlbmZi LmMKPiA+ID4gKysrIGIvaHcvZGlzcGxheS94ZW5mYi5jCj4gPiA+IEBAIC0xMDQsOSArMTA0LDgg QEAgc3RhdGljIGludCBjb21tb25fYmluZChzdHJ1Y3QgY29tbW9uICpjKQo+ID4gPiDCoMKgwqDC oMKgaWYgKHhlbnN0b3JlX3JlYWRfZmVfaW50KCZjLT54ZW5kZXYsICJldmVudC1jaGFubmVsIiwg JmMtCj4gPiA+ID54ZW5kZXYucmVtb3RlX3BvcnQpID09IC0xKQo+ID4gPiDCoAlyZXR1cm4gLTE7 Cj4gPiA+IMKgCj4gPiA+IC3CoMKgwqDCoGMtPnBhZ2UgPSB4Y19tYXBfZm9yZWlnbl9yYW5nZSh4 ZW5feGMsIGMtPnhlbmRldi5kb20sCj4gPiA+IC0JCQkJwqDCoMKgWENfUEFHRV9TSVpFLAo+ID4g PiAtCQkJCcKgwqDCoFBST1RfUkVBRCB8IFBST1RfV1JJVEUsIG1mbik7Cj4gPiA+ICvCoMKgwqDC oGMtPnBhZ2UgPSB4Y19tYXBfZm9yZWlnbl9wYWdlcyh4ZW5feGMsIGMtPnhlbmRldi5kb20sCj4g PiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg wqDCoMKgwqDCoMKgwqDCoMKgUFJPVF9SRUFEIHwgUFJPVF9XUklURSwgJm1mbiwgMSk7Cj4gPiAK PiA+IFRoaXMgZG9lc24ndCBidWlsZCBmb3IgaTM4NiB1c2Vyc3BhY2UsIHNpbmNlIG1mbiBpcyBh IHVpbnQ2NF90IGJ1dAo+ID4geGNfbWFwX2ZvcmVpZ25fcGFnZXMoKSB3YW50cyBhIHhlbl9wZm5f dCAqICh3aGVyZSB4ZW5fcGZuX3QgPT0gdW5zaWduZWQKPiA+IGxvbmcgb24geDg2KS4KPiA+IAo+ ID4gVW50aWwgbm93IHRoYXQgd2FzIGp1c3QgYSB0cnVuY2F0aW9uIHdoaWNoIHdhcyBhbHJlYWR5 IGNoZWNrZWQgZm9yCj4gPiB3aXRoOgo+ID4gCj4gPiDCoCDCoCB1aW50NjRfdCBtZm47Cj4gPiAK PiA+IMKgwqDCoMKgaWYgKHhlbnN0b3JlX3JlYWRfZmVfdWludDY0KCZjLT54ZW5kZXYsICJwYWdl LXJlZiIsICZtZm4pID09IC0xKQo+ID4gwqDCoMKgwqDCoMKgwqDCoHJldHVybiAtMTsKPiA+IMKg wqDCoMKgYXNzZXJ0KG1mbiA9PSAoeGVuX3Bmbl90KW1mbik7Cj4gPiAKPiA+IEkgdGhpbmsgaW4g cHJpbmNpcGFsIHBhc3NpbmcgIih4ZW5fcGZuX3QgKikmbWZuIiB3b3VsZCBvayAoc2luY2UgaXQg aXMKPiA+IGEKPiA+IHNpbmdsZXRvbiBhcnJheSBpbiB0aGlzIGNhc2UpLCBidXQgSSB3YXMgdGhp bmtpbmcgb2YgZ29pbmcgYSBiaXQKPiA+IGZ1cnRoZXIKPiA+IGFuZDoKPiA+IAo+ID4gZGlmZiAt LWdpdCBhL2h3L2Rpc3BsYXkveGVuZmIuYyBiL2h3L2Rpc3BsYXkveGVuZmIuYwo+ID4gaW5kZXgg OGI4NmI0YS4uNTQ1ODVmYSAxMDA2NDQKPiA+IC0tLSBhL2h3L2Rpc3BsYXkveGVuZmIuYwo+ID4g KysrIGIvaHcvZGlzcGxheS94ZW5mYi5jCj4gPiBAQCAtOTUsMTEgKzk1LDEzIEBAIHN0cnVjdCBY ZW5GQiB7Cj4gPiDCoAo+ID4gwqBzdGF0aWMgaW50IGNvbW1vbl9iaW5kKHN0cnVjdCBjb21tb24g KmMpCj4gPiDCoHsKPiA+IC3CoMKgwqDCoHVpbnQ2NF90IG1mbjsKPiA+ICvCoMKgwqDCoHVpbnQ2 NF90IHZhbDsKPiA+ICvCoMKgwqDCoHhlbl9wZm5fdCBtZm47Cj4gPiDCoAo+ID4gLcKgwqDCoMKg aWYgKHhlbnN0b3JlX3JlYWRfZmVfdWludDY0KCZjLT54ZW5kZXYsICJwYWdlLXJlZiIsICZtZm4p ID09IC0xKQo+ID4gK8KgwqDCoMKgaWYgKHhlbnN0b3JlX3JlYWRfZmVfdWludDY0KCZjLT54ZW5k ZXYsICJwYWdlLXJlZiIsICZ2YWwpID09IC0xKQo+ID4gwqAJcmV0dXJuIC0xOwo+ID4gLcKgwqDC oMKgYXNzZXJ0KG1mbiA9PSAoeGVuX3Bmbl90KW1mbik7Cj4gPiArwqDCoMKgwqBtZm4gPSAoeGVu X3Bmbl90KXZhbDsKPiA+ICvCoMKgwqDCoGFzc2VydCh2YWwgPT0gbWZuKTsKPiA+IMKgCj4gPiDC oMKgwqDCoMKgaWYgKHhlbnN0b3JlX3JlYWRfZmVfaW50KCZjLT54ZW5kZXYsICJldmVudC1jaGFu bmVsIiwgJmMtCj4gPiA+eGVuZGV2LnJlbW90ZV9wb3J0KSA9PSAtMSkKPiA+IMKgCXJldHVybiAt MTsKPiA+IAo+ID4gU3RlZmFubywgd2hhdCBkbyB5b3UgdGhpbmsvcHJlZmVyPyBBbiBhbHRlcm5h dGl2ZSB0byB0aGUgYWJvdmUKPiAKPiBJIGxpa2UgdGhpcyBjaGFuZ2UgYmVjYXVzZSBpdCBtYWtl cyB0aGUgY29kZSBtb3JlIG9idmlvdXMKClRoYW5rcywgd2l0aCB0aGF0IGNoYW5nZSBtYXkgSSBr ZWVwIHlvdXIgUmV2aWV3ZWQtYnk/CgoKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fClhlbi1kZXZlbCBtYWlsaW5nIGxpc3QKWGVuLWRldmVsQGxpc3RzLnhl bi5vcmcKaHR0cDovL2xpc3RzLnhlbi5vcmcveGVuLWRldmVsCg==