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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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've not spotted any other similar constructs, xenfb is a bit unusual here
in that it apparently uses foreign mappings rather than grants like most
drivers do.

Ian.

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