| Message ID | 1508144567-23300-1-git-send-email-bhupinder.thakur@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [Xen-devel] libxl/xenconsole: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add | expand |
Hi Bhupinder, On 16/10/17 10:02, Bhupinder Thakur wrote: > In libxl__device_vuart_add vuart_gfn is getting stored as a hex value: > >> flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn)); > > However, xenstore reads this value as a decimal value and tries to map the > wrong address and fails. > > Introduced a new format string "PRIu_xen_pfn" which formats the value as a > decimal value. Also corrected the type of ring_ref to xen_pfn_t in xenconsole. The commit message does not give any reason why xc_map_foreign_page(...) was switch to xen_paddr_t. But I think this patch is doing too much. You try to fix too distinct errors here: 1) Fixing the bug introduced by commit 2b668a84e5 "libxl: use correct type modifier for vuart_gfn" 2) The fact that the other side will at best truncate the value With that you also modify the xc_map_foreign_page() to avoid truncating the value. So you at least need to split this patch in 3 if not more. > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > > This patch was verified for arm64 and arm32 toolstack compilation. > > CC: Ian Jackson <ian.jackson@eu.citrix.com> > CC: Wei Liu <wei.liu2@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien.grall@arm.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > > tools/console/daemon/io.c | 20 +++++++++++--------- > tools/libxc/include/xenctrl_compat.h | 2 +- > tools/libxc/xc_foreign_memory.c | 2 +- > tools/libxl/libxl_console.c | 4 ++-- > tools/libxl/libxl_dom.c | 2 +- > tools/libxl/libxl_internal.h | 2 +- > xen/include/public/arch-arm.h | 1 + > xen/include/public/arch-x86/xen.h | 1 + > 8 files changed, 19 insertions(+), 15 deletions(-) > > diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c > index e22009a..6369bf4 100644 > --- a/tools/console/daemon/io.c > +++ b/tools/console/daemon/io.c > @@ -80,6 +80,7 @@ static unsigned int current_array_size; > static unsigned int nr_fds; > > #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1)) > +#define INVALID_RING_REF (~(xen_pfn_t)0) I am a bit surprised we don't have an INVALID_XEN_PFN definition in the public header. Jan would you have an objection to introduce that? > > struct buffer { > char *data; > @@ -98,7 +99,7 @@ struct console { > struct buffer buffer; > char *xspath; > char *log_suffix; > - int ring_ref; > + xen_pfn_t ring_ref; > xenevtchn_handle *xce_handle; > int xce_pollfd_idx; > int event_count; > @@ -651,22 +652,23 @@ static void console_unmap_interface(struct console *con) > { > if (con->interface == NULL) > return; > - if (xgt_handle && con->ring_ref == -1) > + if (xgt_handle && con->ring_ref == INVALID_RING_REF) > xengnttab_unmap(xgt_handle, con->interface, 1); > else > munmap(con->interface, XC_PAGE_SIZE); > con->interface = NULL; > - con->ring_ref = -1; > + con->ring_ref = INVALID_RING_REF; > } > > static int console_create_ring(struct console *con) > { > - int err, remote_port, ring_ref, rc; > + int err, remote_port, rc; > + xen_pfn_t ring_ref; > char *type, path[PATH_MAX]; > struct domain *dom = con->d; > > err = xs_gather(xs, con->xspath, > - "ring-ref", "%u", &ring_ref, > + "ring-ref", "%i", &ring_ref, Hmmm, I think %i is wrong here. You don't fix the problem that the GFN can be wider than 32-bit. > "port", "%i", &remote_port, > NULL); > > @@ -690,7 +692,7 @@ static int console_create_ring(struct console *con) > free(type); > > /* If using ring_ref and it has changed, remap */ > - if (ring_ref != con->ring_ref && con->ring_ref != -1) > + if (ring_ref != con->ring_ref && con->ring_ref != INVALID_RING_REF) > console_unmap_interface(con); > > if (!con->interface && xgt_handle && con->use_gnttab) { > @@ -698,14 +700,14 @@ static int console_create_ring(struct console *con) > con->interface = xengnttab_map_grant_ref(xgt_handle, > dom->domid, GNTTAB_RESERVED_CONSOLE, > PROT_READ|PROT_WRITE); > - con->ring_ref = -1; > + con->ring_ref = INVALID_RING_REF; > } > if (!con->interface) { > /* Fall back to xc_map_foreign_range */ > con->interface = xc_map_foreign_range( > xc, dom->domid, XC_PAGE_SIZE, > PROT_READ|PROT_WRITE, > - (unsigned long)ring_ref); > + ring_ref); > if (con->interface == NULL) { > err = EINVAL; > goto out; > @@ -804,7 +806,7 @@ static int console_init(struct console *con, struct domain *dom, void **data) > con->master_pollfd_idx = -1; > con->slave_fd = -1; > con->log_fd = -1; > - con->ring_ref = -1; > + con->ring_ref = INVALID_RING_REF; > con->local_port = -1; > con->remote_port = -1; > con->xce_pollfd_idx = -1; > diff --git a/tools/libxc/include/xenctrl_compat.h b/tools/libxc/include/xenctrl_compat.h > index a655e47..2d930d8 100644 > --- a/tools/libxc/include/xenctrl_compat.h > +++ b/tools/libxc/include/xenctrl_compat.h > @@ -26,7 +26,7 @@ > */ > void *xc_map_foreign_range(xc_interface *xch, uint32_t dom, > int size, int prot, > - unsigned long mfn ); > + xen_pfn_t mfn ); Can you please drop the spurious space before ')' at the same time. But the term mfn here is slightly odd as you pass GFN for HVM/PVH/ARM Guest. You might want to rename it to 'pfn' I think. Am I right? > > void *xc_map_foreign_pages(xc_interface *xch, uint32_t dom, int prot, > const xen_pfn_t *arr, int num ); > diff --git a/tools/libxc/xc_foreign_memory.c b/tools/libxc/xc_foreign_memory.c > index 4053d26..8b65e52 100644 > --- a/tools/libxc/xc_foreign_memory.c > +++ b/tools/libxc/xc_foreign_memory.c > @@ -33,7 +33,7 @@ void *xc_map_foreign_pages(xc_interface *xch, uint32_t dom, int prot, > > void *xc_map_foreign_range(xc_interface *xch, > uint32_t dom, int size, int prot, > - unsigned long mfn) > + xen_pfn_t mfn) > { > xen_pfn_t *arr; > int num; > diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c > index c05dc28..f2ca689 100644 > --- a/tools/libxl/libxl_console.c > +++ b/tools/libxl/libxl_console.c > @@ -329,7 +329,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid, > flexarray_append(ro_front, "port"); > flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port)); > flexarray_append(ro_front, "ring-ref"); > - flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn)); > + flexarray_append(ro_front, GCSPRINTF("%"PRIu_xen_pfn, state->console_mfn)); All the changes regarding console_mfn in libxl should really be separated. > } else { > flexarray_append(front, "state"); > flexarray_append(front, GCSPRINTF("%d", XenbusStateInitialising)); > @@ -376,7 +376,7 @@ int libxl__device_vuart_add(libxl__gc *gc, uint32_t domid, > flexarray_append(ro_front, "port"); > flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vuart_port)); > flexarray_append(ro_front, "ring-ref"); > - flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn)); > + flexarray_append(ro_front, GCSPRINTF("%"PRIu_xen_pfn, state->vuart_gfn)); > flexarray_append(ro_front, "limit"); > flexarray_append(ro_front, GCSPRINTF("%d", LIBXL_XENCONSOLE_LIMIT)); > flexarray_append(ro_front, "type"); > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index ef834e6..a58e74f 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -869,7 +869,7 @@ out: > static int hvm_build_set_params(xc_interface *handle, uint32_t domid, > libxl_domain_build_info *info, > int store_evtchn, unsigned long *store_mfn, > - int console_evtchn, unsigned long *console_mfn, > + int console_evtchn, xen_pfn_t *console_mfn, > domid_t store_domid, domid_t console_domid) > { > struct hvm_info_table *va_hvm; > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index 45e6df6..f52aeb3 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -1128,7 +1128,7 @@ typedef struct { > > uint32_t console_port; > uint32_t console_domid; > - unsigned long console_mfn; > + xen_pfn_t console_mfn; > char *console_tty; > > char *saved_state; > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index 5708cd2..05fd11c 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -274,6 +274,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t); > > typedef uint64_t xen_pfn_t; > #define PRI_xen_pfn PRIx64 > +#define PRIu_xen_pfn PRIu64 > > /* Maximum number of virtual CPUs in legacy multi-processor guests. */ > /* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */ > diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h > index ff91831..3b0b1d6 100644 > --- a/xen/include/public/arch-x86/xen.h > +++ b/xen/include/public/arch-x86/xen.h > @@ -75,6 +75,7 @@ __DeFiNe__ __DECL_REG_LO16(name) e ## name > #ifndef __ASSEMBLY__ > typedef unsigned long xen_pfn_t; > #define PRI_xen_pfn "lx" > +#define PRIu_xen_pfn "lu" > #endif > > #define XEN_HAVE_PV_GUEST_ENTRY 1 > Cheers,
>>> On 16.10.17 at 11:02, <bhupinder.thakur@linaro.org> wrote: > static int console_create_ring(struct console *con) > { > - int err, remote_port, ring_ref, rc; > + int err, remote_port, rc; > + xen_pfn_t ring_ref; > char *type, path[PATH_MAX]; > struct domain *dom = con->d; > > err = xs_gather(xs, con->xspath, > - "ring-ref", "%u", &ring_ref, > + "ring-ref", "%i", &ring_ref, How would you gather a 64-bit value using %i without any length modifier? With just %i you're even going to use partially initialized data, so unless somewhere else the upper 32 bits got clipped off again the console wouldn't work anymore. Jan
>>> On 16.10.17 at 12:18, <julien.grall@linaro.org> wrote: > On 16/10/17 10:02, Bhupinder Thakur wrote: >> --- a/tools/console/daemon/io.c >> +++ b/tools/console/daemon/io.c >> @@ -80,6 +80,7 @@ static unsigned int current_array_size; >> static unsigned int nr_fds; >> >> #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1)) >> +#define INVALID_RING_REF (~(xen_pfn_t)0) > > I am a bit surprised we don't have an INVALID_XEN_PFN definition in the > public header. Jan would you have an objection to introduce that? If this was for a use with a hypercall, I probably wouldn't mind. But here this is a completely local constant, unrelated to anything Xen produces or consumes. Of course seeing mention of INVALID_GFN in a public header comment, there likely are hypercalls that could benefit from such a constant (or maybe more than one, for the various frame number flavors). Jan
On 16 October 2017 at 15:53, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 16.10.17 at 11:02, <bhupinder.thakur@linaro.org> wrote: >> static int console_create_ring(struct console *con) >> { >> - int err, remote_port, ring_ref, rc; >> + int err, remote_port, rc; >> + xen_pfn_t ring_ref; >> char *type, path[PATH_MAX]; >> struct domain *dom = con->d; >> >> err = xs_gather(xs, con->xspath, >> - "ring-ref", "%u", &ring_ref, >> + "ring-ref", "%i", &ring_ref, > > How would you gather a 64-bit value using %i without any length > modifier? With just %i you're even going to use partially initialized > data, so unless somewhere else the upper 32 bits got clipped off > again the console wouldn't work anymore. > I should use "%lli" here to read it as a 64-bit value for all architectures. Correct? Regards, Bhupinder
>>> On 16.10.17 at 17:47, <bhupinder.thakur@linaro.org> wrote: > On 16 October 2017 at 15:53, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 16.10.17 at 11:02, <bhupinder.thakur@linaro.org> wrote: >>> static int console_create_ring(struct console *con) >>> { >>> - int err, remote_port, ring_ref, rc; >>> + int err, remote_port, rc; >>> + xen_pfn_t ring_ref; >>> char *type, path[PATH_MAX]; >>> struct domain *dom = con->d; >>> >>> err = xs_gather(xs, con->xspath, >>> - "ring-ref", "%u", &ring_ref, >>> + "ring-ref", "%i", &ring_ref, >> >> How would you gather a 64-bit value using %i without any length >> modifier? With just %i you're even going to use partially initialized >> data, so unless somewhere else the upper 32 bits got clipped off >> again the console wouldn't work anymore. >> > I should use "%lli" here to read it as a 64-bit value for all > architectures. Correct? No, that's break for a 32-bit tool stack on x86. You really need an abstraction similar to PRIu_xen_pfn, just that I'd rather not see SCNi_xen_pfn added to the public headers. Jan
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c index e22009a..6369bf4 100644 --- a/tools/console/daemon/io.c +++ b/tools/console/daemon/io.c @@ -80,6 +80,7 @@ static unsigned int current_array_size; static unsigned int nr_fds; #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1)) +#define INVALID_RING_REF (~(xen_pfn_t)0) struct buffer { char *data; @@ -98,7 +99,7 @@ struct console { struct buffer buffer; char *xspath; char *log_suffix; - int ring_ref; + xen_pfn_t ring_ref; xenevtchn_handle *xce_handle; int xce_pollfd_idx; int event_count; @@ -651,22 +652,23 @@ static void console_unmap_interface(struct console *con) { if (con->interface == NULL) return; - if (xgt_handle && con->ring_ref == -1) + if (xgt_handle && con->ring_ref == INVALID_RING_REF) xengnttab_unmap(xgt_handle, con->interface, 1); else munmap(con->interface, XC_PAGE_SIZE); con->interface = NULL; - con->ring_ref = -1; + con->ring_ref = INVALID_RING_REF; } static int console_create_ring(struct console *con) { - int err, remote_port, ring_ref, rc; + int err, remote_port, rc; + xen_pfn_t ring_ref; char *type, path[PATH_MAX]; struct domain *dom = con->d; err = xs_gather(xs, con->xspath, - "ring-ref", "%u", &ring_ref, + "ring-ref", "%i", &ring_ref, "port", "%i", &remote_port, NULL); @@ -690,7 +692,7 @@ static int console_create_ring(struct console *con) free(type); /* If using ring_ref and it has changed, remap */ - if (ring_ref != con->ring_ref && con->ring_ref != -1) + if (ring_ref != con->ring_ref && con->ring_ref != INVALID_RING_REF) console_unmap_interface(con); if (!con->interface && xgt_handle && con->use_gnttab) { @@ -698,14 +700,14 @@ static int console_create_ring(struct console *con) con->interface = xengnttab_map_grant_ref(xgt_handle, dom->domid, GNTTAB_RESERVED_CONSOLE, PROT_READ|PROT_WRITE); - con->ring_ref = -1; + con->ring_ref = INVALID_RING_REF; } if (!con->interface) { /* Fall back to xc_map_foreign_range */ con->interface = xc_map_foreign_range( xc, dom->domid, XC_PAGE_SIZE, PROT_READ|PROT_WRITE, - (unsigned long)ring_ref); + ring_ref); if (con->interface == NULL) { err = EINVAL; goto out; @@ -804,7 +806,7 @@ static int console_init(struct console *con, struct domain *dom, void **data) con->master_pollfd_idx = -1; con->slave_fd = -1; con->log_fd = -1; - con->ring_ref = -1; + con->ring_ref = INVALID_RING_REF; con->local_port = -1; con->remote_port = -1; con->xce_pollfd_idx = -1; diff --git a/tools/libxc/include/xenctrl_compat.h b/tools/libxc/include/xenctrl_compat.h index a655e47..2d930d8 100644 --- a/tools/libxc/include/xenctrl_compat.h +++ b/tools/libxc/include/xenctrl_compat.h @@ -26,7 +26,7 @@ */ void *xc_map_foreign_range(xc_interface *xch, uint32_t dom, int size, int prot, - unsigned long mfn ); + xen_pfn_t mfn ); void *xc_map_foreign_pages(xc_interface *xch, uint32_t dom, int prot, const xen_pfn_t *arr, int num ); diff --git a/tools/libxc/xc_foreign_memory.c b/tools/libxc/xc_foreign_memory.c index 4053d26..8b65e52 100644 --- a/tools/libxc/xc_foreign_memory.c +++ b/tools/libxc/xc_foreign_memory.c @@ -33,7 +33,7 @@ void *xc_map_foreign_pages(xc_interface *xch, uint32_t dom, int prot, void *xc_map_foreign_range(xc_interface *xch, uint32_t dom, int size, int prot, - unsigned long mfn) + xen_pfn_t mfn) { xen_pfn_t *arr; int num; diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c index c05dc28..f2ca689 100644 --- a/tools/libxl/libxl_console.c +++ b/tools/libxl/libxl_console.c @@ -329,7 +329,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid, flexarray_append(ro_front, "port"); flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port)); flexarray_append(ro_front, "ring-ref"); - flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn)); + flexarray_append(ro_front, GCSPRINTF("%"PRIu_xen_pfn, state->console_mfn)); } else { flexarray_append(front, "state"); flexarray_append(front, GCSPRINTF("%d", XenbusStateInitialising)); @@ -376,7 +376,7 @@ int libxl__device_vuart_add(libxl__gc *gc, uint32_t domid, flexarray_append(ro_front, "port"); flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vuart_port)); flexarray_append(ro_front, "ring-ref"); - flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn)); + flexarray_append(ro_front, GCSPRINTF("%"PRIu_xen_pfn, state->vuart_gfn)); flexarray_append(ro_front, "limit"); flexarray_append(ro_front, GCSPRINTF("%d", LIBXL_XENCONSOLE_LIMIT)); flexarray_append(ro_front, "type"); diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index ef834e6..a58e74f 100644 --- a/tools/libxl/libxl_dom.c +++ b/tools/libxl/libxl_dom.c @@ -869,7 +869,7 @@ out: static int hvm_build_set_params(xc_interface *handle, uint32_t domid, libxl_domain_build_info *info, int store_evtchn, unsigned long *store_mfn, - int console_evtchn, unsigned long *console_mfn, + int console_evtchn, xen_pfn_t *console_mfn, domid_t store_domid, domid_t console_domid) { struct hvm_info_table *va_hvm; diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 45e6df6..f52aeb3 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -1128,7 +1128,7 @@ typedef struct { uint32_t console_port; uint32_t console_domid; - unsigned long console_mfn; + xen_pfn_t console_mfn; char *console_tty; char *saved_state; diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index 5708cd2..05fd11c 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -274,6 +274,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t); typedef uint64_t xen_pfn_t; #define PRI_xen_pfn PRIx64 +#define PRIu_xen_pfn PRIu64 /* Maximum number of virtual CPUs in legacy multi-processor guests. */ /* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */ diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h index ff91831..3b0b1d6 100644 --- a/xen/include/public/arch-x86/xen.h +++ b/xen/include/public/arch-x86/xen.h @@ -75,6 +75,7 @@ __DeFiNe__ __DECL_REG_LO16(name) e ## name #ifndef __ASSEMBLY__ typedef unsigned long xen_pfn_t; #define PRI_xen_pfn "lx" +#define PRIu_xen_pfn "lu" #endif #define XEN_HAVE_PV_GUEST_ENTRY 1