[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] libxl/xenconsole: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add



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@xxxxxxxxxx>
---

This patch was verified for arm64 and arm32 toolstack compilation.

CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
CC: Julien Grall <julien.grall@xxxxxxx>
CC: Jan Beulich <jbeulich@xxxxxxxx>
CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

  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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.