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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
