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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/12/15 13:41, 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:

It is never ok to convert a pointer like this.  In 32bit (little endian)
userspace, it will leave the upper half of mfn uninitialised on the stack.

~Andrew