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.tha...@linaro.org>
---
This patch was verified for arm64 and arm32 toolstack compilation.
CC: Ian Jackson <ian.jack...@eu.citrix.com>
CC: Wei Liu <wei.l...@citrix.com>
CC: Stefano Stabellini <sstabell...@kernel.org>
CC: Julien Grall <julien.gr...@arm.com>
CC: Jan Beulich <jbeul...@suse.com>
CC: Andrew Cooper <andrew.coop...@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,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
