Re: [Xen-devel] [PATCH QEMU-XEN v6 4/8] xen: Switch uses of xc_map_foreign_range into xc_map_foreign_pages

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 11 Dec 2015, Ian Campbell wrote:
> 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)
> > > > ÂÂÂÂÂ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
>
> Thanks, with that change may I keep your Reviewed-by?

Sure

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel