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) > > if (xenstore_read_fe_int(&c->xendev, "event-channel", > > &c->xendev.remote_port) == -1) > > return -1; > > > > - c->page = xc_map_foreign_range(xen_xc, c->xendev.dom, > > - XC_PAGE_SIZE, > > - PROT_READ | PROT_WRITE, mfn); > > + c->page = xc_map_foreign_pages(xen_xc, c->xendev.dom, > > + PROT_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 == unsigned > long on x86). > > Until now that was just a truncation which was already checked for with: > > uint64_t mfn; > > if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1) > return -1; > assert(mfn == (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 { > > static int common_bind(struct common *c) > { > - uint64_t mfn; > + uint64_t val; > + xen_pfn_t mfn; > > - if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1) > + if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &val) == -1) > return -1; > - assert(mfn == (xen_pfn_t)mfn); > + mfn = (xen_pfn_t)val; > + assert(val == mfn); > > if (xenstore_read_fe_int(&c->xendev, "event-channel", > &c->xendev.remote_port) == -1) > return -1; > > Stefano, what do you think/prefer? An alternative to the above
I like this change because it makes the code more obvious