| Message ID | 1403530078-13640-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
Julien Grall writes ("[PATCH] libxc: Fix xc_mem_event.c compilation for ARM"):
> The commit 6ae2df9 "mem_access: Add helper API to setup ring and enable
> mem_access¨ break libxc compilation for ARM.
>
> This is because xc_map_foreign_map and xc_domain_decrease_reservation_exact
> is taking an xen_pfn_t in parameters. On ARM, xen_pfn_t is always an uin64_t.
I have applied this, mostly because we don't want to have a broken
build for any length of time, but:
> - unsigned long ring_pfn, mmap_pfn;
> + unsigned long pfn;
> + xen_pfn_t ring_pfn, mmap_pfn;
...
> - rc1 = xc_get_hvm_param(xch, domain_id, param, &ring_pfn);
> + rc1 = xc_get_hvm_param(xch, domain_id, param, &pfn);
...
> - mmap_pfn = ring_pfn;
> + ring_pfn = pfn;
> + mmap_pfn = pfn;
I asked on IRC:
16:05 <@Diziet> julieng: If xen_pfn_t is uint64_t and on 32-bit ARM unsigned
long is uint32_t, how can the xc_get_hvm_param in
xc_mem_event_enable DTRT ?
I don't know whether that's a real problem. (Related seems to be the
question of whether we support a 32-bit dom0 with 64-bit guests on ARM
and if not what stops the user from setting up such a thing.)
Thanks,
Ian.
On Mon, 2014-06-23 at 16:34 +0100, Ian Jackson wrote: > Julien Grall writes ("[PATCH] libxc: Fix xc_mem_event.c compilation for ARM"): > > The commit 6ae2df9 "mem_access: Add helper API to setup ring and enable > > mem_access¨ break libxc compilation for ARM. > > > > This is because xc_map_foreign_map and xc_domain_decrease_reservation_exact > > is taking an xen_pfn_t in parameters. On ARM, xen_pfn_t is always an uin64_t. > > I have applied this, mostly because we don't want to have a broken > build for any length of time, but: > > > - unsigned long ring_pfn, mmap_pfn; > > + unsigned long pfn; > > + xen_pfn_t ring_pfn, mmap_pfn; > ... > > - rc1 = xc_get_hvm_param(xch, domain_id, param, &ring_pfn); > > + rc1 = xc_get_hvm_param(xch, domain_id, param, &pfn); > ... > > - mmap_pfn = ring_pfn; > > + ring_pfn = pfn; > > + mmap_pfn = pfn; > > I asked on IRC: > > 16:05 <@Diziet> julieng: If xen_pfn_t is uint64_t and on 32-bit ARM unsigned > long is uint32_t, how can the xc_get_hvm_param in > xc_mem_event_enable DTRT ? > > I don't know whether that's a real problem. (Related seems to be the > question of whether we support a 32-bit dom0 with 64-bit guests on ARM > and if not what stops the user from setting up such a thing.) It's supported, although it's not clear that it is such a useful thing to be doing on ARM (on x86 32-bit PAE was quite a nice trade off between memory size vs performance due to the 64-bit syscall overhead, which doesn't apply to ARM). I'm _pretty_ sure that the only things which we pass as a pfn to HVM params are the "magic pfns", which are at a hardcoded guest location which needs to be under 4GB anyway so that 32-bit non-LPAE guests can map them, so no danger of an actual overflow. We never see the actual MFN. FWIW I think the same issue with HVM parameters would impact 32-bit dom0 on x86 too, but I suppose the magic PFNs have the same property there too. Luckily I think David's patches sorts it out at the root. > > Thanks, > Ian.
On 06/24/2014 02:16 PM, Ian Campbell wrote: > On Mon, 2014-06-23 at 16:34 +0100, Ian Jackson wrote: > FWIW I think the same issue with HVM parameters would impact 32-bit dom0 > on x86 too, but I suppose the magic PFNs have the same property there > too. Luckily I think David's patches sorts it out at the root. On IRC, I pointed that the HVM param hypercall structure is using uint64_t for the value rather than the helpers in libxc use unsigned long. Even tho, it's not an issue for the magic PFN right now, a developer may want to add a new HVM param which requires a 64 bit value. Does David's series sort it out this problem?
On 24/06/14 14:46, Julien Grall wrote: > On 06/24/2014 02:16 PM, Ian Campbell wrote: >> On Mon, 2014-06-23 at 16:34 +0100, Ian Jackson wrote: >> FWIW I think the same issue with HVM parameters would impact 32-bit dom0 >> on x86 too, but I suppose the magic PFNs have the same property there >> too. Luckily I think David's patches sorts it out at the root. > > On IRC, I pointed that the HVM param hypercall structure is using > uint64_t for the value rather than the helpers in libxc use unsigned long. > > Even tho, it's not an issue for the magic PFN right now, a developer may > want to add a new HVM param which requires a 64 bit value. > > Does David's series sort it out this problem? Yes. It adds new xc_hvm_param_get() and xc_hvm_param_set() functions that take uint64_t values and changes all callers (in tools/) to use them. David
>The commit 6ae2df9 "mem_access: Add helper API to setup ring and enable >mem_access¨ break libxc compilation for ARM. Sorry about breaking the ARM build and taking so long to respond. I was on vacation and just got back today. I will make a habit of cross compiling ARM from here on before submitting patches. Thanks, Aravindh
diff --git a/tools/libxc/xc_mem_event.c b/tools/libxc/xc_mem_event.c index be7c63d..0b2eecb 100644 --- a/tools/libxc/xc_mem_event.c +++ b/tools/libxc/xc_mem_event.c @@ -60,7 +60,8 @@ void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param, uint32_t *port) { void *ring_page = NULL; - unsigned long ring_pfn, mmap_pfn; + unsigned long pfn; + xen_pfn_t ring_pfn, mmap_pfn; unsigned int op, mode; int rc1, rc2, saved_errno; @@ -79,14 +80,15 @@ void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param, } /* Get the pfn of the ring page */ - rc1 = xc_get_hvm_param(xch, domain_id, param, &ring_pfn); + rc1 = xc_get_hvm_param(xch, domain_id, param, &pfn); if ( rc1 != 0 ) { PERROR("Failed to get pfn of ring page\n"); goto out; } - mmap_pfn = ring_pfn; + ring_pfn = pfn; + mmap_pfn = pfn; ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ | PROT_WRITE, &mmap_pfn, 1); if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
The commit 6ae2df9 "mem_access: Add helper API to setup ring and enable mem_access¨ break libxc compilation for ARM. This is because xc_map_foreign_map and xc_domain_decrease_reservation_exact is taking an xen_pfn_t in parameters. On ARM, xen_pfn_t is always an uin64_t. Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Aravindh Puthiyaparambil <aravindp@cisco.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> --- tools/libxc/xc_mem_event.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)