All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore
@ 2017-05-10 14:35 Bhupinder Thakur
  2017-05-10 14:35 ` [PATCH 07/12 v3] xen/arm: vpl011: Allocate a new GFN in the toolstack for vuart Bhupinder Thakur
                   ` (6 more replies)
  0 siblings, 7 replies; 29+ messages in thread
From: Bhupinder Thakur @ 2017-05-10 14:35 UTC (permalink / raw)
  To: xen-devel; +Cc: Wei Liu, Ian Jackson

Add a new vuart console node to xenstore. This node is added at

/local/domain/$DOMID/vuart/0.

The node contains information such as the ring-ref, event channel,
buffer limit and type of console.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 tools/libxl/libxl_console.c          | 33 ++++++++++++++++++++++++++
 tools/libxl/libxl_create.c           | 24 +++++++++++++++----
 tools/libxl/libxl_device.c           | 45 ++++++++++++++++++++++++++----------
 tools/libxl/libxl_internal.h         |  4 ++++
 tools/libxl/libxl_types_internal.idl |  1 +
 5 files changed, 91 insertions(+), 16 deletions(-)

diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
index 853be15..870cb9c 100644
--- a/tools/libxl/libxl_console.c
+++ b/tools/libxl/libxl_console.c
@@ -344,6 +344,39 @@ out:
     return rc;
 }
 
+int libxl__device_vuart_add(libxl__gc *gc, uint32_t domid,
+                            libxl__device_console *console,
+                            libxl__domain_build_state *state,
+                            libxl__device *device)
+{
+    flexarray_t *ro_front;
+    int rc;
+
+    ro_front = flexarray_make(gc, 16, 1);
+
+    device->backend_devid = console->devid;
+    device->backend_domid = console->backend_domid;
+    device->backend_kind = LIBXL__DEVICE_KIND_VUART;
+    device->devid = console->devid;
+    device->domid = domid;
+    device->kind = LIBXL__DEVICE_KIND_VUART;
+
+    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("%lu", state->vuart_gfn));
+    flexarray_append(ro_front, "limit");
+    flexarray_append(ro_front, GCSPRINTF("%d", LIBXL_XENCONSOLE_LIMIT));
+    flexarray_append(ro_front, "type");
+    flexarray_append(ro_front, "xenconsoled");
+
+    rc = libxl__device_generic_add(gc, XBT_NULL, device,
+                                   NULL,
+                                   NULL,
+                                   libxl__xs_kvs_of_flexarray(gc, ro_front));
+    return rc;
+}
+
 int libxl__init_console_from_channel(libxl__gc *gc,
                                      libxl__device_console *console,
                                      int dev_num,
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 29daa35..39da2d0 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1332,10 +1332,18 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
     switch (d_config->c_info.type) {
     case LIBXL_DOMAIN_TYPE_HVM:
     {
-        libxl__device_console console;
-        libxl__device device;
+        libxl__device_console console, vuart;
+        libxl__device device, vuart_device;
         libxl_device_vkb vkb;
 
+        if (d_config->b_info.vuart)
+        {
+            init_console_info(gc, &vuart, 0);
+            vuart.backend_domid = state->console_domid;
+            libxl__device_vuart_add(gc, domid, &vuart, state, &vuart_device);
+            libxl__device_console_dispose(&vuart);
+        }
+
         init_console_info(gc, &console, 0);
         console.backend_domid = state->console_domid;
         libxl__device_console_add(gc, domid, &console, state, &device);
@@ -1369,14 +1377,22 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
     }
     case LIBXL_DOMAIN_TYPE_PV:
     {
-        libxl__device_console console;
-        libxl__device device;
+        libxl__device_console console, vuart;
+        libxl__device device, vuart_device;
 
         for (i = 0; i < d_config->num_vfbs; i++) {
             libxl__device_vfb_add(gc, domid, &d_config->vfbs[i]);
             libxl__device_vkb_add(gc, domid, &d_config->vkbs[i]);
         }
 
+        if (d_config->b_info.vuart)
+        {
+            init_console_info(gc, &vuart, 0);
+            vuart.backend_domid = state->console_domid;
+            libxl__device_vuart_add(gc, domid, &vuart, state, &vuart_device);
+            libxl__device_console_dispose(&vuart);
+        }
+
         init_console_info(gc, &console, 0);
         console.backend_domid = state->console_domid;
         libxl__device_console_add(gc, domid, &console, state, &device);
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 5e96676..bb25672 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -26,6 +26,9 @@ static char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device)
     if (device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0)
         return GCSPRINTF("%s/console", dom_path);
 
+    if (device->kind == LIBXL__DEVICE_KIND_VUART)
+        return GCSPRINTF("%s/vuart/%d", dom_path, device->devid);
+
     return GCSPRINTF("%s/device/%s/%d", dom_path,
                      libxl__device_kind_to_string(device->kind),
                      device->devid);
@@ -151,13 +154,19 @@ retry_transaction:
     if (rc) goto out;
 
     if (!libxl_only) {
-        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
-                                     frontend_path);
-        if (rc) goto out;
+        if (fents || ro_fents)
+        {
+            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
+                                         frontend_path);
+            if (rc) goto out;
+        }
 
-        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
-                                     backend_path);
-        if (rc) goto out;
+        if (bents)
+        {
+            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
+                                         backend_path);
+            if (rc) goto out;
+        }
     }
 
     /* xxx much of this function lacks error checks! */
@@ -170,14 +179,20 @@ retry_transaction:
          * historically contained other information, such as the
          * vnc-port, which we don't want the guest fiddling with.
          */
-        if (device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0)
+        if ((device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0) ||
+            (device->kind == LIBXL__DEVICE_KIND_VUART))
             xs_set_permissions(ctx->xsh, t, frontend_path,
                                ro_frontend_perms, ARRAY_SIZE(ro_frontend_perms));
         else
             xs_set_permissions(ctx->xsh, t, frontend_path,
                                frontend_perms, ARRAY_SIZE(frontend_perms));
-        xs_write(ctx->xsh, t, GCSPRINTF("%s/backend", frontend_path),
-                 backend_path, strlen(backend_path));
+        /*
+         * Create backend node only if there are entries to be populated in the
+         * backend node.
+         */
+        if (bents)
+            xs_write(ctx->xsh, t, GCSPRINTF("%s/backend", frontend_path),
+                     backend_path, strlen(backend_path));
         if (fents)
             libxl__xs_writev_perms(gc, t, frontend_path, fents,
                                    frontend_perms, ARRAY_SIZE(frontend_perms));
@@ -192,8 +207,13 @@ retry_transaction:
             xs_mkdir(ctx->xsh, t, backend_path);
             xs_set_permissions(ctx->xsh, t, backend_path, backend_perms,
                                ARRAY_SIZE(backend_perms));
-            xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path),
-                     frontend_path, strlen(frontend_path));
+            /*
+            * Create frontend node only if there are entries populated in the
+            * frontend node.
+            */
+            if (fents || ro_fents)
+                xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path),
+                         frontend_path, strlen(frontend_path));
             libxl__xs_writev(gc, t, backend_path, bents);
         }
 
@@ -800,7 +820,8 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
                 dev->domid = domid;
                 dev->kind = kind;
                 dev->devid = atoi(devs[j]);
-                if (dev->backend_kind == LIBXL__DEVICE_KIND_CONSOLE) {
+                if (dev->backend_kind == LIBXL__DEVICE_KIND_CONSOLE ||
+                    dev->backend_kind == LIBXL__DEVICE_KIND_VUART) {
                     /* Currently console devices can be destroyed
                      * synchronously by just removing xenstore entries,
                      * this is what libxl__device_destroy does.
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4e2c247..7a22db0 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1202,6 +1202,10 @@ _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
                                       libxl__device_console *console,
                                       libxl__domain_build_state *state,
                                       libxl__device *device);
+_hidden int libxl__device_vuart_add(libxl__gc *gc, uint32_t domid,
+                                    libxl__device_console *console,
+                                    libxl__domain_build_state *state,
+                                    libxl__device *device);
 
 /* Returns 1 if device exists, 0 if not, ERROR_* (<0) on error. */
 _hidden int libxl__device_exists(libxl__gc *gc, xs_transaction_t t,
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index 7dc4d0f..c463c33 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -26,6 +26,7 @@ libxl__device_kind = Enumeration("device_kind", [
     (9, "VUSB"),
     (10, "QUSB"),
     (11, "9PFS"),
+    (12, "VUART"),
     ])
 
 libxl__console_backend = Enumeration("console_backend", [
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 07/12 v3] xen/arm: vpl011: Allocate a new GFN in the toolstack for vuart
  2017-05-10 14:35 [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Bhupinder Thakur
@ 2017-05-10 14:35 ` Bhupinder Thakur
  2017-05-11 11:10   ` Wei Liu
  2017-05-16 22:59   ` Stefano Stabellini
  2017-05-10 14:35 ` [PATCH 08/12 v3] xen/arm: vpl011: Modify xenconsole to support multiple consoles Bhupinder Thakur
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 29+ messages in thread
From: Bhupinder Thakur @ 2017-05-10 14:35 UTC (permalink / raw)
  To: xen-devel; +Cc: Wei Liu, Ian Jackson

Allocate a new gfn to be used as a ring buffer between xenconsole
and Xen for sending/receiving pl011 data.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---

Changes since v2:

- Removed the DOMCTL call to set the GFN as now this information is passed
  in the DOMCTL call to initialize vpl011 emulation.

 tools/libxc/include/xc_dom.h | 2 ++
 tools/libxc/xc_dom_arm.c     | 5 ++++-
 tools/libxc/xc_dom_boot.c    | 2 ++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index ce47058..6e06ef1 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -216,6 +216,8 @@ struct xc_dom_image {
 
     /* Extra SMBIOS structures passed to HVMLOADER */
     struct xc_hvm_firmware_module smbios_module;
+
+    xen_pfn_t vuart_gfn;
 };
 
 /* --- pluggable kernel loader ------------------------------------- */
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index e7d4bd0..c981b7a 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -26,10 +26,11 @@
 #include "xg_private.h"
 #include "xc_dom.h"
 
-#define NR_MAGIC_PAGES 3
+#define NR_MAGIC_PAGES 4
 #define CONSOLE_PFN_OFFSET 0
 #define XENSTORE_PFN_OFFSET 1
 #define MEMACCESS_PFN_OFFSET 2
+#define VUART_PFN_OFFSET 3
 
 #define LPAE_SHIFT 9
 
@@ -85,10 +86,12 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
 
     dom->console_pfn = base + CONSOLE_PFN_OFFSET;
     dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+    dom->vuart_gfn = base + VUART_PFN_OFFSET;
 
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
     xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, base + VUART_PFN_OFFSET);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
             dom->console_pfn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index c3b44dd..8a376d0 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -226,6 +226,8 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
         return rc;
     if ( (rc = clear_page(dom, dom->xenstore_pfn)) != 0 )
         return rc;
+    if ( (rc = clear_page(dom, dom->vuart_gfn)) != 0 )
+        return rc;
 
     /* start info page */
     if ( dom->arch_hooks->start_info )
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 08/12 v3] xen/arm: vpl011: Modify xenconsole to support multiple consoles
  2017-05-10 14:35 [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Bhupinder Thakur
  2017-05-10 14:35 ` [PATCH 07/12 v3] xen/arm: vpl011: Allocate a new GFN in the toolstack for vuart Bhupinder Thakur
@ 2017-05-10 14:35 ` Bhupinder Thakur
  2017-05-11 11:15   ` Wei Liu
  2017-05-16 23:41   ` Stefano Stabellini
  2017-05-10 14:35 ` [PATCH 09/12 v3] xen/arm: vpl011: Add support for vuart in xenconsole Bhupinder Thakur
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 29+ messages in thread
From: Bhupinder Thakur @ 2017-05-10 14:35 UTC (permalink / raw)
  To: xen-devel; +Cc: Wei Liu, Ian Jackson

Xenconsole supports only PV console currently. This patch adds support
for supporting multiple consoles.

This patch modifies different data structures and APIs used
in xenconsole to support multiple consoles.

Change summary:

1. Split the domain structure into a console structure and the
   domain structure, where each console structure represents one
   console.

2. Modify different APIs such as buffer_append() etc. to take
   console structure as input and perform per console specific
   operations.

3. Define a generic console_create_ring(), which sets up the
   ring buffer and event channel for each console.

3. Modify domain_create_ring() to use console_create_ring().

4. Modifications in handle_ring_read() to read ring buffer data
   from multiple consoles.

5. Add log file support for multiple consoles.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---

Changes since v2:

- Defined a new function console_create_ring() which sets up the ring buffer and 
  event channel a new console. domain_create_ring() uses this function to setup
  a console.
- This patch does not contain vuart specific changes, which would be introduced in
  the next patch.
- Changes for keeping the PV log file name unchanged.

Changes since v1:

- Split the domain struture to a separate console structure
- Modified the functions to operate on the console struture
- Replaced repetitive per console code with generic code

 tools/console/daemon/io.c | 650 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 460 insertions(+), 190 deletions(-)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 7e6a886..9bb14de 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -89,29 +89,139 @@ struct buffer {
 	size_t max_capacity;
 };
 
-struct domain {
-	int domid;
+struct console {
+	char *xsname;
+	char *ttyname;
 	int master_fd;
 	int master_pollfd_idx;
 	int slave_fd;
 	int log_fd;
-	bool is_dead;
-	unsigned last_seen;
 	struct buffer buffer;
-	struct domain *next;
-	char *conspath;
 	int ring_ref;
 	xenevtchn_port_or_error_t local_port;
 	xenevtchn_port_or_error_t remote_port;
+	struct xencons_interface *interface;
+	struct domain *d;  /* Reference to the domain it is contained in. */
+	char *xspath;
+	int (*map_ring_ref)(struct console *, int);
+	bool mandatory;
+};
+
+struct console_data {
+	char *xsname;
+	char *ttyname;
+	int (*mapfunc)(struct console *, int);
+	bool mandatory;
+};
+
+static int map_pvcon_ring_ref(struct console *, int );
+
+static struct console_data console_data[] = {
+
+	{
+		.xsname = "/console",
+		.ttyname = "tty",
+		.mapfunc = map_pvcon_ring_ref,
+		.mandatory = true
+	},
+};
+
+#define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
+
+struct domain {
+	int domid;
+	bool is_dead;
+	unsigned last_seen;
+	struct domain *next;
 	xenevtchn_handle *xce_handle;
 	int xce_pollfd_idx;
-	struct xencons_interface *interface;
 	int event_count;
 	long long next_period;
+	struct console console[MAX_CONSOLE];
 };
 
 static struct domain *dom_head;
 
+typedef void (*VOID_ITER_FUNC_ARG1)(struct console *);
+typedef bool (*BOOL_ITER_FUNC_ARG1)(struct console *);
+typedef int (*INT_ITER_FUNC_ARG1)(struct console *);
+typedef void (*VOID_ITER_FUNC_ARG2)(struct console *,  unsigned int);
+typedef int (*INT_ITER_FUNC_ARG3)(struct console *,
+			 struct domain *dom, void **);
+
+static inline bool console_enabled(struct console *con)
+{
+	return con->local_port != -1;
+}
+
+static inline void console_iter_void_arg1(struct domain *d,
+										   VOID_ITER_FUNC_ARG1 iter_func)
+{
+	int i = 0;
+	struct console *con = &(d->console[0]);
+
+	for (i = 0; i < MAX_CONSOLE; i++, con++)
+	{
+		iter_func(con);
+	}
+}
+
+static inline void console_iter_void_arg2(struct domain *d,
+										   VOID_ITER_FUNC_ARG2 iter_func,
+										   unsigned int iter_data)
+{
+	int i = 0;
+	struct console *con = &(d->console[0]);
+
+	for (i = 0; i < MAX_CONSOLE; i++, con++)
+	{
+		iter_func(con, iter_data);
+	}
+}
+
+static inline bool console_iter_bool_arg1(struct domain *d,
+										   BOOL_ITER_FUNC_ARG1 iter_func)
+{
+	int i = 0;
+	struct console *con = &(d->console[0]);
+
+	for (i = 0; i < MAX_CONSOLE; i++, con++)
+	{
+		if (iter_func(con))
+			return true;
+	}
+	return false;
+}
+
+static inline int console_iter_int_arg1(struct domain *d,
+										 INT_ITER_FUNC_ARG1 iter_func)
+{
+	int i = 0;
+	struct console *con = &(d->console[0]);
+
+	for (i = 0; i < MAX_CONSOLE; i++, con++)
+	{
+		if (iter_func(con))
+			return 1;
+	}
+	return 0;
+}
+
+static inline int console_iter_int_arg3(struct domain *d,
+										 INT_ITER_FUNC_ARG3 iter_func,
+										 void *iter_data)
+{
+	int i = 0;
+	struct console *con = &(d->console[0]);
+
+	for (i = 0; i < MAX_CONSOLE; i++, con++)
+	{
+		if (iter_func(con, d, iter_data))
+			return 1;
+	}
+	return 0;
+}
+
 static int write_all(int fd, const char* buf, size_t len)
 {
 	while (len) {
@@ -158,11 +268,29 @@ static int write_with_timestamp(int fd, const char *data, size_t sz,
 	return 0;
 }
 
-static void buffer_append(struct domain *dom)
+static inline bool buffer_available(struct console *con)
 {
-	struct buffer *buffer = &dom->buffer;
+	if (discard_overflowed_data ||
+		!con->buffer.max_capacity ||
+		con->buffer.size < con->buffer.max_capacity)
+		return true;
+	else
+		return false;
+}
+
+static void buffer_append(struct console *con, unsigned int data)
+{
+	struct buffer *buffer = &con->buffer;
+	struct xencons_interface *intf = con->interface;
+	xenevtchn_port_or_error_t port = con->local_port;
+	struct domain *dom = con->d;
+	xenevtchn_port_or_error_t rxport = (xenevtchn_port_or_error_t)data;
+
 	XENCONS_RING_IDX cons, prod, size;
-	struct xencons_interface *intf = dom->interface;
+
+	/* If incoming data is not for the current console then ignore. */
+	if (port != rxport)
+		return;
 
 	cons = intf->out_cons;
 	prod = intf->out_prod;
@@ -187,22 +315,22 @@ static void buffer_append(struct domain *dom)
 
 	xen_mb();
 	intf->out_cons = cons;
-	xenevtchn_notify(dom->xce_handle, dom->local_port);
+	xenevtchn_notify(dom->xce_handle, port);
 
 	/* Get the data to the logfile as early as possible because if
 	 * no one is listening on the console pty then it will fill up
 	 * and handle_tty_write will stop being called.
 	 */
-	if (dom->log_fd != -1) {
+	if (con->log_fd != -1) {
 		int logret;
 		if (log_time_guest) {
 			logret = write_with_timestamp(
-				dom->log_fd,
+				con->log_fd,
 				buffer->data + buffer->size - size,
 				size, &log_time_guest_needts);
 		} else {
 			logret = write_all(
-				dom->log_fd,
+				con->log_fd,
 				buffer->data + buffer->size - size,
 				size);
 		}
@@ -290,12 +418,13 @@ static int create_hv_log(void)
 	return fd;
 }
 
-static int create_domain_log(struct domain *dom)
+static int create_console_log(struct console *con)
 {
 	char logfile[PATH_MAX];
 	char *namepath, *data, *s;
 	int fd;
 	unsigned int len;
+	struct domain *dom = con->d;
 
 	namepath = xs_get_domain_path(xs, dom->domid);
 	s = realloc(namepath, strlen(namepath) + 6);
@@ -314,7 +443,9 @@ static int create_domain_log(struct domain *dom)
 		return -1;
 	}
 
-	snprintf(logfile, PATH_MAX-1, "%s/guest-%s.log", log_dir, data);
+	snprintf(logfile, PATH_MAX-1, "%s%s/guest-%s.log",
+			 log_dir, con->xspath, data);
+
 	free(data);
 	logfile[PATH_MAX-1] = '\0';
 
@@ -336,19 +467,24 @@ static int create_domain_log(struct domain *dom)
 	return fd;
 }
 
-static void domain_close_tty(struct domain *dom)
+static void console_close_tty(struct console *con)
 {
-	if (dom->master_fd != -1) {
-		close(dom->master_fd);
-		dom->master_fd = -1;
+	if (con->master_fd != -1) {
+		close(con->master_fd);
+		con->master_fd = -1;
 	}
 
-	if (dom->slave_fd != -1) {
-		close(dom->slave_fd);
-		dom->slave_fd = -1;
+	if (con->slave_fd != -1) {
+		close(con->slave_fd);
+		con->slave_fd = -1;
 	}
 }
 
+static void domain_close_tty(struct domain *dom)
+{
+	console_iter_void_arg1(dom, console_close_tty);
+}
+
 #ifdef __sun__
 static int openpty(int *amaster, int *aslave, char *name,
 		   struct termios *termp, struct winsize *winp)
@@ -409,7 +545,7 @@ void cfmakeraw(struct termios *termios_p)
 }
 #endif /* __sun__ */
 
-static int domain_create_tty(struct domain *dom)
+static int console_create_tty(struct console *con)
 {
 	const char *slave;
 	char *path;
@@ -418,19 +554,23 @@ static int domain_create_tty(struct domain *dom)
 	char *data;
 	unsigned int len;
 	struct termios term;
+	struct domain *dom = con->d;
+
+	if (!console_enabled(con))
+		return 0;
 
-	assert(dom->slave_fd == -1);
-	assert(dom->master_fd == -1);
+	assert(con->master_fd == -1);
+	assert(con->slave_fd == -1);
 
-	if (openpty(&dom->master_fd, &dom->slave_fd, NULL, NULL, NULL) < 0) {
+	if (openpty(&con->master_fd, &con->slave_fd, NULL, NULL, NULL) < 0) {
 		err = errno;
 		dolog(LOG_ERR, "Failed to create tty for domain-%d "
-		      "(errno = %i, %s)",
-		      dom->domid, err, strerror(err));
-		return 0;
+			  "(errno = %i, %s)",
+			  dom->domid, err, strerror(err));
+		goto out;
 	}
 
-	if (tcgetattr(dom->slave_fd, &term) < 0) {
+	if (tcgetattr(con->slave_fd, &term) < 0) {
 		err = errno;
 		dolog(LOG_ERR, "Failed to get tty attributes for domain-%d "
 			"(errno = %i, %s)",
@@ -438,7 +578,7 @@ static int domain_create_tty(struct domain *dom)
 		goto out;
 	}
 	cfmakeraw(&term);
-	if (tcsetattr(dom->slave_fd, TCSANOW, &term) < 0) {
+	if (tcsetattr(con->slave_fd, TCSANOW, &term) < 0) {
 		err = errno;
 		dolog(LOG_ERR, "Failed to set tty attributes for domain-%d "
 			"(errno = %i, %s)",
@@ -446,40 +586,54 @@ static int domain_create_tty(struct domain *dom)
 		goto out;
 	}
 
-	if ((slave = ptsname(dom->master_fd)) == NULL) {
+	if ((slave = ptsname(con->master_fd)) == NULL) {
 		err = errno;
 		dolog(LOG_ERR, "Failed to get slave name for domain-%d "
-		      "(errno = %i, %s)",
-		      dom->domid, err, strerror(err));
+			  "(errno = %i, %s)",
+			  dom->domid, err, strerror(err));
 		goto out;
 	}
 
-	success = asprintf(&path, "%s/limit", dom->conspath) !=
+	success = asprintf(&path, "%s/limit", con->xspath) !=
 		-1;
 	if (!success)
 		goto out;
 	data = xs_read(xs, XBT_NULL, path, &len);
 	if (data) {
-		dom->buffer.max_capacity = strtoul(data, 0, 0);
+		con->buffer.max_capacity = strtoul(data, 0, 0);
 		free(data);
 	}
 	free(path);
 
-	success = (asprintf(&path, "%s/tty", dom->conspath) != -1);
+	success = (asprintf(&path, "%s/%s", con->xspath, con->ttyname) != -1);
+
 	if (!success)
 		goto out;
 	success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
 	free(path);
-	if (!success)
+
+	if (fcntl(con->master_fd, F_SETFL, O_NONBLOCK) == -1)
 		goto out;
 
-	if (fcntl(dom->master_fd, F_SETFL, O_NONBLOCK) == -1)
+	if (!success)
 		goto out;
 
-	return 1;
-out:
-	domain_close_tty(dom);
 	return 0;
+
+out:
+	return 1;
+}
+
+static int domain_create_tty(struct domain *dom)
+{
+	int ret;
+
+	ret = console_iter_int_arg1(dom, console_create_tty);
+
+	if (ret)
+		domain_close_tty(dom);
+
+	return ret;
 }
  
 /* Takes tuples of names, scanf-style args, and void **, NULL terminated. */
@@ -517,31 +671,106 @@ static int xs_gather(struct xs_handle *xs, const char *dir, ...)
 	return ret;
 }
 
-static void domain_unmap_interface(struct domain *dom)
+static void console_unmap_interface(struct console *con)
 {
-	if (dom->interface == NULL)
+	if (con->interface == NULL)
 		return;
-	if (xgt_handle && dom->ring_ref == -1)
-		xengnttab_unmap(xgt_handle, dom->interface, 1);
+	if (xgt_handle && con->ring_ref == -1)
+		xengnttab_unmap(xgt_handle, con->interface, 1);
 	else
-		munmap(dom->interface, XC_PAGE_SIZE);
-	dom->interface = NULL;
-	dom->ring_ref = -1;
+		munmap(con->interface, XC_PAGE_SIZE);
+	con->interface = NULL;
+	con->ring_ref = -1;
+}
+
+static void domain_unmap_interface(struct domain *dom)
+{
+	console_iter_void_arg1(dom, console_unmap_interface);
+}
+
+static int bind_event_channel(struct domain *dom,
+							  int new_rport,
+							  int *lport,
+							  int *rport)
+{
+	int err = 0, rc;
+
+	/* Go no further if port has not changed and we are still bound. */
+	if (new_rport == *rport) {
+		xc_evtchn_status_t status = {
+		.dom = DOMID_SELF,
+		.port = *lport };
+		if ((xc_evtchn_status(xc, &status) == 0) &&
+			(status.status == EVTCHNSTAT_interdomain))
+			goto out;
+	}
+
+	*lport = -1;
+	*rport = -1;
+	rc = xenevtchn_bind_interdomain(dom->xce_handle,
+									dom->domid, new_rport);
+
+	if (rc == -1) {
+		err = errno;
+		goto out;
+	}
+
+	*lport = rc;
+	*rport = new_rport;
+out:
+	return err;
 }
  
-static int domain_create_ring(struct domain *dom)
+static int map_pvcon_ring_ref(struct console *con, int ring_ref)
 {
-	int err, remote_port, ring_ref, rc;
+	int err = 0;
+	struct domain *dom = con->d;
+
+	if (!con->interface && xgt_handle) {
+		/* Prefer using grant table */
+		con->interface = xengnttab_map_grant_ref(xgt_handle,
+												 dom->domid, 
+												 GNTTAB_RESERVED_CONSOLE,
+												 PROT_READ|PROT_WRITE);
+		con->ring_ref = -1;
+	}
+
+	if (!con->interface) {
+		con->interface = xc_map_foreign_range(xc,
+											  dom->domid,
+											  XC_PAGE_SIZE,
+											  PROT_READ|PROT_WRITE,
+											  (unsigned long)ring_ref);
+		if (con->interface == NULL) {
+			err = EINVAL;
+			goto out;
+		}
+		con->ring_ref = ring_ref;
+	}
+
+out:
+	return err;
+}
+
+static int console_create_ring(struct console *con)
+{
+	int err, remote_port, ring_ref;
 	char *type, path[PATH_MAX];
+	struct domain *dom = con->d;
+
+	err = xs_gather(xs, con->xspath,
+					"ring-ref", "%u", &ring_ref,
+					"port", "%i", &remote_port,
+					NULL);
 
-	err = xs_gather(xs, dom->conspath,
-			"ring-ref", "%u", &ring_ref,
-			"port", "%i", &remote_port,
-			NULL);
 	if (err)
+	{
+		/* If the console is not mandatory then do not return an error. */
+		if (!con->mandatory)
+			err = 0;
 		goto out;
+	}
 
-	snprintf(path, sizeof(path), "%s/type", dom->conspath);
+	snprintf(path, sizeof(path), "%s/type", con->xspath);
 	type = xs_read(xs, XBT_NULL, path, NULL);
 	if (type && strcmp(type, "xenconsoled") != 0) {
 		free(type);
@@ -550,41 +779,44 @@ static int domain_create_ring(struct domain *dom)
 	free(type);
 
 	/* If using ring_ref and it has changed, remap */
-	if (ring_ref != dom->ring_ref && dom->ring_ref != -1)
-		domain_unmap_interface(dom);
+	if (ring_ref != con->ring_ref && con->ring_ref != -1)
+		console_unmap_interface(con);
 
-	if (!dom->interface && xgt_handle) {
-		/* Prefer using grant table */
-		dom->interface = xengnttab_map_grant_ref(xgt_handle,
-			dom->domid, GNTTAB_RESERVED_CONSOLE,
-			PROT_READ|PROT_WRITE);
-		dom->ring_ref = -1;
-	}
-	if (!dom->interface) {
-		/* Fall back to xc_map_foreign_range */
-		dom->interface = xc_map_foreign_range(
-			xc, dom->domid, XC_PAGE_SIZE,
-			PROT_READ|PROT_WRITE,
-			(unsigned long)ring_ref);
-		if (dom->interface == NULL) {
-			err = EINVAL;
-			goto out;
+	err = con->map_ring_ref(con, ring_ref);
+
+	if (err)
+		goto out;
+
+	err = bind_event_channel(dom, remote_port,
+							 &con->local_port,
+							 &con->remote_port);
+	if (err)
+		goto out1;
+
+	if (con->master_fd == -1) {
+		if (console_create_tty(con)) {
+			err = errno;
+			con->local_port = -1;
+			con->remote_port = -1;
+			goto out1;
 		}
-		dom->ring_ref = ring_ref;
 	}
 
-	/* Go no further if port has not changed and we are still bound. */
-	if (remote_port == dom->remote_port) {
-		xc_evtchn_status_t status = {
-			.dom = DOMID_SELF,
-			.port = dom->local_port };
-		if ((xc_evtchn_status(xc, &status) == 0) &&
-		    (status.status == EVTCHNSTAT_interdomain))
-			goto out;
-	}
+	if (log_guest && (con->log_fd == -1))
+		con->log_fd = create_console_log(con);
+
+	return err;
+
+ out1:
+	console_unmap_interface(con);
+ out:
+	return err;
+}
+
+static int domain_create_ring(struct domain *dom)
+{
+	int err;
 
-	dom->local_port = -1;
-	dom->remote_port = -1;
 	if (dom->xce_handle != NULL)
 		xenevtchn_close(dom->xce_handle);
 
@@ -592,37 +824,17 @@ static int domain_create_ring(struct domain *dom)
 	 * wasteful, but that's how the code is structured... */
 	dom->xce_handle = xenevtchn_open(NULL, 0);
 	if (dom->xce_handle == NULL) {
-		err = errno;
-		goto out;
+		return errno;
 	}
- 
-	rc = xenevtchn_bind_interdomain(dom->xce_handle,
-		dom->domid, remote_port);
 
-	if (rc == -1) {
-		err = errno;
+	err = console_iter_int_arg1(dom, console_create_ring);
+
+	if (err)
+	{
 		xenevtchn_close(dom->xce_handle);
 		dom->xce_handle = NULL;
-		goto out;
-	}
-	dom->local_port = rc;
-	dom->remote_port = remote_port;
-
-	if (dom->master_fd == -1) {
-		if (!domain_create_tty(dom)) {
-			err = errno;
-			xenevtchn_close(dom->xce_handle);
-			dom->xce_handle = NULL;
-			dom->local_port = -1;
-			dom->remote_port = -1;
-			goto out;
-		}
 	}
 
-	if (log_guest && (dom->log_fd == -1))
-		dom->log_fd = create_domain_log(dom);
-
- out:
 	return err;
 }
 
@@ -630,27 +842,66 @@ static bool watch_domain(struct domain *dom, bool watch)
 {
 	char domid_str[3 + MAX_STRLEN(dom->domid)];
 	bool success;
+	char *path = dom->console[0].xspath;
 
 	snprintf(domid_str, sizeof(domid_str), "dom%u", dom->domid);
 	if (watch) {
-		success = xs_watch(xs, dom->conspath, domid_str);
+		success = xs_watch(xs, path, domid_str);
 		if (success)
 			domain_create_ring(dom);
 		else
-			xs_unwatch(xs, dom->conspath, domid_str);
+			xs_unwatch(xs, path, domid_str);
 	} else {
-		success = xs_unwatch(xs, dom->conspath, domid_str);
+		success = xs_unwatch(xs, path, domid_str);
 	}
 
 	return success;
 }
 
+static int console_init(struct console *con, struct domain *dom, void **data)
+{
+	char *s;
+	int err = -1;
+	struct console_data **con_data = (struct console_data **)data;
+
+	con->master_fd = -1;
+	con->master_pollfd_idx = -1;
+	con->slave_fd = -1;
+	con->log_fd = -1;
+	con->ring_ref = -1;
+	con->local_port = -1;
+	con->remote_port = -1;
+	con->d = dom;
+	con->ttyname = (*con_data)->ttyname;
+	con->xsname = (*con_data)->xsname;
+	con->map_ring_ref = (*con_data)->mapfunc;
+	con->mandatory = (*con_data)->mandatory;
+	con->xspath = xs_get_domain_path(xs, dom->domid);
+	s = realloc(con->xspath, strlen(con->xspath) +
+				strlen(con->xsname) + 1);
+
+	(*con_data)++;
+
+	if (s)
+	{
+		con->xspath = s;
+		strcat(con->xspath, con->xsname);
+		err = 0;
+	}
+	return err;
+}
+
+static void console_free(struct console *con)
+{
+	if (con->xspath)
+		free(con->xspath);
+}
 
 static struct domain *create_domain(int domid)
 {
 	struct domain *dom;
-	char *s;
 	struct timespec ts;
+	struct console_data *con_data = &console_data[0];
 
 	if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {
 		dolog(LOG_ERR, "Cannot get time of day %s:%s:L%d",
@@ -667,26 +918,13 @@ static struct domain *create_domain(int domid)
 
 	dom->domid = domid;
 
-	dom->conspath = xs_get_domain_path(xs, dom->domid);
-	s = realloc(dom->conspath, strlen(dom->conspath) +
-		    strlen("/console") + 1);
-	if (s == NULL)
+	if (console_iter_int_arg3(dom, console_init, (void **)&con_data))
 		goto out;
-	dom->conspath = s;
-	strcat(dom->conspath, "/console");
 
-	dom->master_fd = -1;
-	dom->master_pollfd_idx = -1;
-	dom->slave_fd = -1;
-	dom->log_fd = -1;
 	dom->xce_pollfd_idx = -1;
 
 	dom->next_period = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec / 1000000) + RATE_LIMIT_PERIOD;
 
-	dom->ring_ref = -1;
-	dom->local_port = -1;
-	dom->remote_port = -1;
-
 	if (!watch_domain(dom, true))
 		goto out;
 
@@ -696,8 +934,10 @@ static struct domain *create_domain(int domid)
 	dolog(LOG_DEBUG, "New domain %d", domid);
 
 	return dom;
+
  out:
-	free(dom->conspath);
+	console_iter_void_arg1(dom, console_free);
+
 	free(dom);
 	return NULL;
 }
@@ -727,20 +967,24 @@ static void remove_domain(struct domain *dom)
 	}
 }
 
-static void cleanup_domain(struct domain *d)
-{
-	domain_close_tty(d);
 
-	if (d->log_fd != -1) {
-		close(d->log_fd);
-		d->log_fd = -1;
+static void console_cleanup(struct console *con)
+{
+	if (con->log_fd != -1) {
+		close(con->log_fd);
+		con->log_fd = -1;
 	}
+	free(con->buffer.data);
+	con->buffer.data = NULL;
+	free(con->xspath);
+	con->xspath = NULL;
+}
 
-	free(d->buffer.data);
-	d->buffer.data = NULL;
+static void cleanup_domain(struct domain *d)
+{
+	domain_close_tty(d);
 
-	free(d->conspath);
-	d->conspath = NULL;
+	console_iter_void_arg1(d, console_cleanup);
 
 	remove_domain(d);
 }
@@ -780,9 +1024,9 @@ static void enum_domains(void)
 	}
 }
 
-static int ring_free_bytes(struct domain *dom)
+static int ring_free_bytes(struct console *con)
 {
-	struct xencons_interface *intf = dom->interface;
+	struct xencons_interface *intf = con->interface;
 	XENCONS_RING_IDX cons, prod, space;
 
 	cons = intf->in_cons;
@@ -807,25 +1051,27 @@ static void domain_handle_broken_tty(struct domain *dom, int recreate)
 	}
 }
 
-static void handle_tty_read(struct domain *dom)
+static void handle_tty_read(struct console *con)
 {
 	ssize_t len = 0;
 	char msg[80];
 	int i;
-	struct xencons_interface *intf = dom->interface;
 	XENCONS_RING_IDX prod;
+	struct xencons_interface *intf = con->interface;
+	xenevtchn_port_or_error_t port = con->local_port;
+	struct domain *dom = con->d;
 
 	if (dom->is_dead)
 		return;
 
-	len = ring_free_bytes(dom);
+	len = ring_free_bytes(con);
 	if (len == 0)
 		return;
 
 	if (len > sizeof(msg))
 		len = sizeof(msg);
 
-	len = read(dom->master_fd, msg, len);
+	len = read(con->master_fd, msg, len);
 	/*
 	 * Note: on Solaris, len == 0 means the slave closed, and this
 	 * is no problem, but Linux can't handle this usefully, so we
@@ -841,31 +1087,39 @@ static void handle_tty_read(struct domain *dom)
 		}
 		xen_wmb();
 		intf->in_prod = prod;
-		xenevtchn_notify(dom->xce_handle, dom->local_port);
+		xenevtchn_notify(dom->xce_handle, port);
 	} else {
 		domain_close_tty(dom);
 		shutdown_domain(dom);
 	}
 }
 
-static void handle_tty_write(struct domain *dom)
+static void handle_tty_write(struct console *con)
 {
 	ssize_t len;
+	struct domain *dom = con->d;
 
 	if (dom->is_dead)
 		return;
 
-	len = write(dom->master_fd, dom->buffer.data + dom->buffer.consumed,
-		    dom->buffer.size - dom->buffer.consumed);
+	len = write(con->master_fd,
+				con->buffer.data + con->buffer.consumed,
+				con->buffer.size - con->buffer.consumed);
  	if (len < 1) {
 		dolog(LOG_DEBUG, "Write failed on domain %d: %zd, %d\n",
 		      dom->domid, len, errno);
 		domain_handle_broken_tty(dom, domain_is_valid(dom->domid));
 	} else {
-		buffer_advance(&dom->buffer, len);
+		buffer_advance(&con->buffer, len);
 	}
 }
 
+static void console_event_unmask(struct console *con)
+{
+	if (con->local_port != -1)
+		(void)xenevtchn_unmask(con->d->xce_handle, con->local_port);
+}
+
 static void handle_ring_read(struct domain *dom)
 {
 	xenevtchn_port_or_error_t port;
@@ -878,10 +1132,10 @@ static void handle_ring_read(struct domain *dom)
 
 	dom->event_count++;
 
-	buffer_append(dom);
+	console_iter_void_arg2(dom, buffer_append, port);
 
 	if (dom->event_count < RATE_LIMIT_ALLOWANCE)
-		(void)xenevtchn_unmask(dom->xce_handle, port);
+		console_iter_void_arg1(dom, console_event_unmask);
 }
 
 static void handle_xs(void)
@@ -943,14 +1197,22 @@ static void handle_hv_logs(xenevtchn_handle *xce_handle, bool force)
 		(void)xenevtchn_unmask(xce_handle, port);
 }
 
+static void console_open_log(struct console *con)
+{
+	if (console_enabled(con))
+	{
+		if (con->log_fd != -1)
+			close(con->log_fd);
+		con->log_fd = create_console_log(con);
+	}
+}
+
 static void handle_log_reload(void)
 {
 	if (log_guest) {
 		struct domain *d;
 		for (d = dom_head; d; d = d->next) {
-			if (d->log_fd != -1)
-				close(d->log_fd);
-			d->log_fd = create_domain_log(d);
+			console_iter_void_arg1(d, console_open_log);
 		}
 	}
 
@@ -1002,6 +1264,40 @@ static void reset_fds(void)
 		memset(fds, 0, sizeof(struct pollfd) * current_array_size);
 }
 
+static void add_console_fd(struct console *con)
+{
+	if (con->master_fd != -1) {
+		short events = 0;
+		if (!con->d->is_dead && ring_free_bytes(con))
+			events |= POLLIN;
+
+		if (!buffer_empty(&con->buffer))
+			events |= POLLOUT;
+
+		if (events)
+			con->master_pollfd_idx =
+				set_fds(con->master_fd, events|POLLPRI);
+	}
+}
+
+static void process_console(struct console *con)
+{
+	if (con->master_fd != -1 && con->master_pollfd_idx != -1) {
+		if (fds[con->master_pollfd_idx].revents &
+			~(POLLIN|POLLOUT|POLLPRI))
+			domain_handle_broken_tty(con->d, domain_is_valid(con->d->domid));
+		else {
+			if (fds[con->master_pollfd_idx].revents &
+				POLLIN)
+				handle_tty_read(con);
+			if (fds[con->master_pollfd_idx].revents &
+				POLLOUT)
+				handle_tty_write(con);
+		}
+	}
+	con->master_pollfd_idx = -1;
+}
+
 void handle_io(void)
 {
 	int ret;
@@ -1068,7 +1364,7 @@ void handle_io(void)
 			if ((now+5) > d->next_period) {
 				d->next_period = now + RATE_LIMIT_PERIOD;
 				if (d->event_count >= RATE_LIMIT_ALLOWANCE) {
-					(void)xenevtchn_unmask(d->xce_handle, d->local_port);
+					console_iter_void_arg1(d, console_event_unmask);
 				}
 				d->event_count = 0;
 			}
@@ -1081,28 +1377,15 @@ void handle_io(void)
 				    d->next_period < next_timeout)
 					next_timeout = d->next_period;
 			} else if (d->xce_handle != NULL) {
-				if (discard_overflowed_data ||
-				    !d->buffer.max_capacity ||
-				    d->buffer.size < d->buffer.max_capacity) {
+				if (console_iter_bool_arg1(d, buffer_available))
+				{
 					int evtchn_fd = xenevtchn_fd(d->xce_handle);
 					d->xce_pollfd_idx = set_fds(evtchn_fd,
-								    POLLIN|POLLPRI);
+												POLLIN|POLLPRI);
 				}
 			}
 
-			if (d->master_fd != -1) {
-				short events = 0;
-				if (!d->is_dead && ring_free_bytes(d))
-					events |= POLLIN;
-
-				if (!buffer_empty(&d->buffer))
-					events |= POLLOUT;
-
-				if (events)
-					d->master_pollfd_idx =
-						set_fds(d->master_fd,
-							events|POLLPRI);
-			}
+			console_iter_void_arg1(d, add_console_fd);
 		}
 
 		/* If any domain has been rate limited, we need to work
@@ -1170,22 +1453,9 @@ void handle_io(void)
 				    handle_ring_read(d);
 			}
 
-			if (d->master_fd != -1 && d->master_pollfd_idx != -1) {
-				if (fds[d->master_pollfd_idx].revents &
-				    ~(POLLIN|POLLOUT|POLLPRI))
-					domain_handle_broken_tty(d,
-						   domain_is_valid(d->domid));
-				else {
-					if (fds[d->master_pollfd_idx].revents &
-					    POLLIN)
-						handle_tty_read(d);
-					if (fds[d->master_pollfd_idx].revents &
-					    POLLOUT)
-						handle_tty_write(d);
-				}
-			}
+			console_iter_void_arg1(d, process_console);
 
-			d->xce_pollfd_idx = d->master_pollfd_idx = -1;
+			d->xce_pollfd_idx = -1;
 
 			if (d->last_seen != enum_pass)
 				shutdown_domain(d);
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 09/12 v3] xen/arm: vpl011: Add support for vuart in xenconsole
  2017-05-10 14:35 [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Bhupinder Thakur
  2017-05-10 14:35 ` [PATCH 07/12 v3] xen/arm: vpl011: Allocate a new GFN in the toolstack for vuart Bhupinder Thakur
  2017-05-10 14:35 ` [PATCH 08/12 v3] xen/arm: vpl011: Modify xenconsole to support multiple consoles Bhupinder Thakur
@ 2017-05-10 14:35 ` Bhupinder Thakur
  2017-05-11 11:17   ` Wei Liu
  2017-05-16 23:44   ` Stefano Stabellini
  2017-05-10 14:35 ` [PATCH 10/12 v3] xen/arm: vpl011: Add a new vuart console type to xenconsole client Bhupinder Thakur
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 29+ messages in thread
From: Bhupinder Thakur @ 2017-05-10 14:35 UTC (permalink / raw)
  To: xen-devel; +Cc: Wei Liu, Ian Jackson

Xenconsole supports only PV console currently. This patch adds support
for vuart console, which allows emulated pl011 UART to be accessed
as a console.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---

One review comment was to keep the vuart code under CONFIG_ARM64 && CONFIG_ACPI flags.
This code review could not be incorporated as I could not find out the appropriate flags
unders which this code can be kept. Are the CONFIG* flags exported to xenconsole?

 tools/console/daemon/io.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 9bb14de..19a2f35 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -115,6 +115,7 @@ struct console_data {
 };
 
 static int map_pvcon_ring_ref(struct console *, int );
+static int map_vuartcon_ring_ref(struct console *, int );
 
 static struct console_data console_data[] = {
 
@@ -124,6 +125,12 @@ static struct console_data console_data[] = {
 		.mapfunc = map_pvcon_ring_ref,
 		.mandatory = true
 	},
+	{
+		.xsname = "/vuart/0",
+		.ttyname = "tty",
+		.mapfunc = map_vuartcon_ring_ref,
+		.mandatory = false
+	}
 };
 
 #define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
@@ -751,6 +758,28 @@ out:
 	return err;
 }
 
+static int map_vuartcon_ring_ref(struct console *con, int ring_ref)
+{
+	int err = 0;
+	struct domain *dom = con->d;
+
+	if (!con->interface) {
+		con->interface = xc_map_foreign_range(xc,
+											  dom->domid,
+											  XC_PAGE_SIZE,
+											  PROT_READ|PROT_WRITE,
+											  (unsigned long)ring_ref);
+		if (con->interface == NULL) {
+			err = EINVAL;
+			goto out;
+		}
+		con->ring_ref = ring_ref;
+	}
+
+out:
+	return err;
+}
+
 static int console_create_ring(struct console *con)
 {
 	int err, remote_port, ring_ref;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 10/12 v3] xen/arm: vpl011: Add a new vuart console type to xenconsole client
  2017-05-10 14:35 [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Bhupinder Thakur
                   ` (2 preceding siblings ...)
  2017-05-10 14:35 ` [PATCH 09/12 v3] xen/arm: vpl011: Add support for vuart in xenconsole Bhupinder Thakur
@ 2017-05-10 14:35 ` Bhupinder Thakur
  2017-05-11 11:17   ` Wei Liu
  2017-05-16 23:02   ` Stefano Stabellini
  2017-05-10 14:35 ` [PATCH 11/12 v3] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree Bhupinder Thakur
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 29+ messages in thread
From: Bhupinder Thakur @ 2017-05-10 14:35 UTC (permalink / raw)
  To: xen-devel; +Cc: Wei Liu, Ian Jackson

Add a new console type VUART to connect to guest's emualated vuart
console.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 tools/console/client/main.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index 977779f..6f4405f 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -76,7 +76,7 @@ static void usage(const char *program) {
 	       "\n"
 	       "  -h, --help       display this help and exit\n"
 	       "  -n, --num N      use console number N\n"
-	       "  --type TYPE      console type. must be 'pv' or 'serial'\n"
+	       "  --type TYPE      console type. must be 'pv', 'serial' or 'vuart'\n"
 	       "  --start-notify-fd N file descriptor used to notify parent\n"
 	       , program);
 }
@@ -264,6 +264,7 @@ typedef enum {
        CONSOLE_INVAL,
        CONSOLE_PV,
        CONSOLE_SERIAL,
+       CONSOLE_VUART,
 } console_type;
 
 static struct termios stdin_old_attr;
@@ -361,6 +362,8 @@ int main(int argc, char **argv)
 				type = CONSOLE_SERIAL;
 			else if (!strcmp(optarg, "pv"))
 				type = CONSOLE_PV;
+			else if (!strcmp(optarg, "vuart"))
+				type = CONSOLE_VUART;
 			else {
 				fprintf(stderr, "Invalid type argument\n");
 				fprintf(stderr, "Console types supported are: serial, pv\n");
@@ -436,6 +439,9 @@ int main(int argc, char **argv)
 		else
 			snprintf(path, strlen(dom_path) + strlen("/device/console/%d/tty") + 5, "%s/device/console/%d/tty", dom_path, num);
 	}
+	if (type == CONSOLE_VUART) {
+		snprintf(path, strlen(dom_path) + strlen("/vuart/0/tty") + 1, 
+				 "%s/vuart/0/tty", dom_path);
+	}
 
 	/* FIXME consoled currently does not assume domain-0 doesn't have a
 	   console which is good when we break domain-0 up.  To keep us
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 11/12 v3] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree
  2017-05-10 14:35 [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Bhupinder Thakur
                   ` (3 preceding siblings ...)
  2017-05-10 14:35 ` [PATCH 10/12 v3] xen/arm: vpl011: Add a new vuart console type to xenconsole client Bhupinder Thakur
@ 2017-05-10 14:35 ` Bhupinder Thakur
  2017-05-11 11:18   ` Wei Liu
                     ` (2 more replies)
  2017-05-10 14:35 ` [PATCH 12/12 v3] xen/arm: vpl011: Update documentation for vuart console support Bhupinder Thakur
  2017-05-11 11:10 ` [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Wei Liu
  6 siblings, 3 replies; 29+ messages in thread
From: Bhupinder Thakur @ 2017-05-10 14:35 UTC (permalink / raw)
  To: xen-devel; +Cc: Wei Liu, Ian Jackson

The SBSA uart node format is as specified in
Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt and given below:

ARM SBSA defined generic UART
------------------------------
This UART uses a subset of the PL011 registers and consequently lives
in the PL011 driver. It's baudrate and other communication parameters
cannot be adjusted at runtime, so it lacks a clock specifier here.

Required properties:
- compatible: must be "arm,sbsa-uart"
- reg: exactly one register range
- interrupts: exactly one interrupt specifier
- current-speed: the (fixed) baud rate set by the firmware

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---

Changes since v2:
- Currently device discovery using ACPI is not supported.
- Dropped the reviewed-by tag by Stefano as there were some IRQ related changes
  done later.

 tools/libxl/libxl_arm.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index d842d88..f88ef0d 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -44,10 +44,23 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
     uint32_t nr_spis = 0;
     unsigned int i;
 
+    /*
+     * If pl011 vuart is enabled then increment the nr_spis to allow allocation
+     * of SPI VIRQ for pl011.
+     */
+    if (d_config->b_info.vuart)
+        nr_spis += (GUEST_VPL011_SPI - 32) + 1;
+
     for (i = 0; i < d_config->b_info.num_irqs; i++) {
         uint32_t irq = d_config->b_info.irqs[i];
         uint32_t spi;
 
+        if (d_config->b_info.vuart && (irq == GUEST_VPL011_SPI))
+        {
+            LOG(ERROR, "Physical IRQ %u conflicting with pl011 SPI\n", irq);
+            return ERROR_FAIL;
+        }
+
         if (irq < 32)
             continue;
 
@@ -130,9 +143,10 @@ static struct arch_info {
     const char *guest_type;
     const char *timer_compat;
     const char *cpu_compat;
+    const char *uart_compat;
 } arch_info[] = {
-    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15" },
-    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
+    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" },
+    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" },
 };
 
 /*
@@ -590,6 +604,38 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
+static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
+                                 const struct arch_info *ainfo,
+                                 struct xc_dom_image *dom)
+{
+    int res;
+    gic_interrupt intr;
+
+    res = fdt_begin_node(fdt, "sbsa-pl011");
+    if (res) return res;
+
+    res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat);
+    if (res) return res;
+
+    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
+                            1,
+                            GUEST_PL011_BASE, GUEST_PL011_SIZE);
+    if (res) return res;
+
+    set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
+
+    res = fdt_property_interrupts(gc, fdt, &intr, 1);
+    if (res) return res;
+
+    /* Use a default baud rate of 115200. */
+    fdt_property_u32(fdt, "current-speed", 115200);
+
+    res = fdt_end_node(fdt);
+    if (res) return res;
+
+    return 0;
+}
+
 static const struct arch_info *get_arch_info(libxl__gc *gc,
                                              const struct xc_dom_image *dom)
 {
@@ -889,6 +935,9 @@ next_resize:
         FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
         FDT( make_hypervisor_node(gc, fdt, vers) );
 
+        if (info->vuart)
+            FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
+
         if (pfdt)
             FDT( copy_partial_fdt(gc, fdt, pfdt) );
 
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 12/12 v3] xen/arm: vpl011: Update documentation for vuart console support
  2017-05-10 14:35 [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Bhupinder Thakur
                   ` (4 preceding siblings ...)
  2017-05-10 14:35 ` [PATCH 11/12 v3] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree Bhupinder Thakur
@ 2017-05-10 14:35 ` Bhupinder Thakur
  2017-05-11 11:19   ` Wei Liu
  2017-05-16 23:10   ` Stefano Stabellini
  2017-05-11 11:10 ` [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Wei Liu
  6 siblings, 2 replies; 29+ messages in thread
From: Bhupinder Thakur @ 2017-05-10 14:35 UTC (permalink / raw)
  To: xen-devel; +Cc: Wei Liu, Ian Jackson

1. Update documentation for a new vuart option added.
2. Update documentation about SPI irq reserved for vpl011.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---

Changes since v2:

- Incorporated the review comments on the documentation.

 docs/man/xl.cfg.pod.5.in |  9 +++++++++
 docs/misc/console.txt    | 31 ++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 13167ff..3397cda 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -1085,6 +1085,15 @@ Allow a guest to access specific physical IRQs.
 It is recommended to use this option only for trusted VMs under
 administrator control.
 
+If the virtual uart is enabled then irq 32 is reserved for it. By
+default, it is disabled. If the user specifies the following option in
+the VM config file then the vuart gets enabled. Today, only the
+"pl011" model is supported.
+
+vuart = "pl011"
+
+Currently vuart console is available only for ARM64.
+
 =item B<max_event_channels=N>
 
 Limit the guest to using at most N event channels (PV interrupts).
diff --git a/docs/misc/console.txt b/docs/misc/console.txt
index 16da805..9eccfa1 100644
--- a/docs/misc/console.txt
+++ b/docs/misc/console.txt
@@ -19,7 +19,20 @@ The first PV console path in xenstore remains:
 
 /local/domain/$DOMID/console
 
-the other PV consoles follow the conventional xenstore device path and
+The virtual UART console path in xenstore is defined as:
+
+/local/domain/$DOMID/vuart/0
+
+The vuart console provides access to a virtual pl011 UART on ARM64 systems. To
+enable vuart the following line has to be added to the guest configuration
+file:
+
+vuart = "pl011"
+
+In Linux you can select the virtual pl011 UART by using the "ttyAMA0"
+console instead of "hvc0".
+
+The other PV consoles follow the conventional xenstore device path and
 live in:
 
 /local/domain/$DOMID/device/console/$DEVID.
@@ -61,6 +74,14 @@ output = pty
 The backend will write the pty device name to the "tty" node in the
 console frontend.
 
+For the PV console the tty node is added at
+
+/local/domain/$DOMID/console/tty
+
+For the virtual UART console the tty node is added at
+
+/local/domain/$DOMID/vuart/0/tty
+
 If the toolstack wants a listening Unix domain socket to be created at path
 <path>, a connection accepted and data proxied to the console, it will write:
 
@@ -79,8 +100,8 @@ For example:
 ioemu
 
 The supported values are only xenconsoled or ioemu; xenconsoled has
-several limitations: it can only be used for the first PV console and it
-can only connect to a pty.
+several limitations: it can only be used for the first PV or virtual UART console
+and it can only connect to a pty.
 
 Emulated serials are provided by qemu-dm only to hvm guests; the number
 of emulated serials depends on how many "-serial" command line options
@@ -90,8 +111,8 @@ xenstore in the following path:
 
 /local/domain/$DOMID/serial/$SERIAL_NUM/tty
 
-xenconsole is the tool to connect to a PV console or an emulated serial
-that has a pty as output. Xenconsole takes a domid as parameter plus an
+xenconsole is the tool to connect to a PV or virtual UART console or an
+emulated serial that has a pty as output. Xenconsole takes a domid as parameter plus an
 optional console type (pv for PV consoles or serial for emulated
 serials) and console number. Depending on the type and console
 number, xenconsole will look for the tty node in different xenstore
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore
  2017-05-10 14:35 [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Bhupinder Thakur
                   ` (5 preceding siblings ...)
  2017-05-10 14:35 ` [PATCH 12/12 v3] xen/arm: vpl011: Update documentation for vuart console support Bhupinder Thakur
@ 2017-05-11 11:10 ` Wei Liu
  2017-05-12  9:32   ` Bhupinder Thakur
  6 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2017-05-11 11:10 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Wei Liu, Ian Jackson

On Wed, May 10, 2017 at 08:05:12PM +0530, Bhupinder Thakur wrote:
>          libxl__device_console_add(gc, domid, &console, state, &device);
> @@ -1369,14 +1377,22 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>      }
>      case LIBXL_DOMAIN_TYPE_PV:
>      {
> -        libxl__device_console console;
> -        libxl__device device;
> +        libxl__device_console console, vuart;
> +        libxl__device device, vuart_device;
>  
>          for (i = 0; i < d_config->num_vfbs; i++) {
>              libxl__device_vfb_add(gc, domid, &d_config->vfbs[i]);
>              libxl__device_vkb_add(gc, domid, &d_config->vkbs[i]);
>          }
>  
> +        if (d_config->b_info.vuart)
> +        {
> +            init_console_info(gc, &vuart, 0);
> +            vuart.backend_domid = state->console_domid;
> +            libxl__device_vuart_add(gc, domid, &vuart, state, &vuart_device);
> +            libxl__device_console_dispose(&vuart);
> +        }
> +

This is strange. You have code snippet for both PV and HVM. Why?

>          init_console_info(gc, &console, 0);
>          console.backend_domid = state->console_domid;
>          libxl__device_console_add(gc, domid, &console, state, &device);
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 5e96676..bb25672 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -26,6 +26,9 @@ static char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device)
>      if (device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0)
>          return GCSPRINTF("%s/console", dom_path);
>  
> +    if (device->kind == LIBXL__DEVICE_KIND_VUART)
> +        return GCSPRINTF("%s/vuart/%d", dom_path, device->devid);
> +
>      return GCSPRINTF("%s/device/%s/%d", dom_path,
>                       libxl__device_kind_to_string(device->kind),
>                       device->devid);
> @@ -151,13 +154,19 @@ retry_transaction:
>      if (rc) goto out;
>  
>      if (!libxl_only) {
> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
> -                                     frontend_path);
> -        if (rc) goto out;
> +        if (fents || ro_fents)
> +        {
> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
> +                                         frontend_path);
> +            if (rc) goto out;
> +        }
>  
> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
> -                                     backend_path);
> -        if (rc) goto out;
> +        if (bents)
> +        {
> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
> +                                         backend_path);
> +            if (rc) goto out;
> +        }

What is this for?

If there is no fe or be entries you skip the path creation altogether.
But why? This doesn't seem to be related to your patch.

At least explain this a bit in the commit message?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 07/12 v3] xen/arm: vpl011: Allocate a new GFN in the toolstack for vuart
  2017-05-10 14:35 ` [PATCH 07/12 v3] xen/arm: vpl011: Allocate a new GFN in the toolstack for vuart Bhupinder Thakur
@ 2017-05-11 11:10   ` Wei Liu
  2017-05-16 22:59   ` Stefano Stabellini
  1 sibling, 0 replies; 29+ messages in thread
From: Wei Liu @ 2017-05-11 11:10 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Wei Liu, Ian Jackson

On Wed, May 10, 2017 at 08:05:13PM +0530, Bhupinder Thakur wrote:
> Allocate a new gfn to be used as a ring buffer between xenconsole
> and Xen for sending/receiving pl011 data.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 08/12 v3] xen/arm: vpl011: Modify xenconsole to support multiple consoles
  2017-05-10 14:35 ` [PATCH 08/12 v3] xen/arm: vpl011: Modify xenconsole to support multiple consoles Bhupinder Thakur
@ 2017-05-11 11:15   ` Wei Liu
  2017-05-16 23:41   ` Stefano Stabellini
  1 sibling, 0 replies; 29+ messages in thread
From: Wei Liu @ 2017-05-11 11:15 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Ian Jackson, Wei Liu

On Wed, May 10, 2017 at 08:05:14PM +0530, Bhupinder Thakur wrote:
> Xenconsole supports only PV console currently. This patch adds support
> for supporting multiple consoles.
> 
> This patch modifies different data structures and APIs used
> in xenconsole to support multiple consoles.
> 
> Change summary:
> 
> 1. Split the domain structure into a console structure and the
>    domain structure, where each console structure represents one
>    console.
> 
> 2. Modify different APIs such as buffer_append() etc. to take
>    console structure as input and perform per console specific
>    operations.
> 
> 3. Define a generic console_create_ring(), which sets up the
>    ring buffer and event channel for each console.
> 
> 3. Modify domain_create_ring() to use console_create_ring().
> 
> 4. Modifications in handle_ring_read() to read ring buffer data
>    from multiple consoles.
> 
> 5. Add log file support for multiple consoles.

I feel that it would be far easier to review if you spilt this patch
into 5. Can you do that?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 09/12 v3] xen/arm: vpl011: Add support for vuart in xenconsole
  2017-05-10 14:35 ` [PATCH 09/12 v3] xen/arm: vpl011: Add support for vuart in xenconsole Bhupinder Thakur
@ 2017-05-11 11:17   ` Wei Liu
  2017-05-16 23:44   ` Stefano Stabellini
  1 sibling, 0 replies; 29+ messages in thread
From: Wei Liu @ 2017-05-11 11:17 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Ian Jackson, Wei Liu

On Wed, May 10, 2017 at 08:05:15PM +0530, Bhupinder Thakur wrote:
> Xenconsole supports only PV console currently. This patch adds support
> for vuart console, which allows emulated pl011 UART to be accessed
> as a console.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> 
> One review comment was to keep the vuart code under CONFIG_ARM64 && CONFIG_ACPI flags.
> This code review could not be incorporated as I could not find out the appropriate flags
> unders which this code can be kept. Are the CONFIG* flags exported to xenconsole?

IIRC they are exported.

> 
>  tools/console/daemon/io.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 9bb14de..19a2f35 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -115,6 +115,7 @@ struct console_data {
>  };
>  
>  static int map_pvcon_ring_ref(struct console *, int );
> +static int map_vuartcon_ring_ref(struct console *, int );
>  
>  static struct console_data console_data[] = {
>  
> @@ -124,6 +125,12 @@ static struct console_data console_data[] = {
>  		.mapfunc = map_pvcon_ring_ref,
>  		.mandatory = true
>  	},
> +	{
> +		.xsname = "/vuart/0",
> +		.ttyname = "tty",
> +		.mapfunc = map_vuartcon_ring_ref,
> +		.mandatory = false
> +	}
>  };
>  
>  #define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
> @@ -751,6 +758,28 @@ out:
>  	return err;
>  }
>  
> +static int map_vuartcon_ring_ref(struct console *con, int ring_ref)
> +{
> +	int err = 0;
> +	struct domain *dom = con->d;
> +
> +	if (!con->interface) {
> +		con->interface = xc_map_foreign_range(xc,
> +											  dom->domid,
> +											  XC_PAGE_SIZE,
> +											  PROT_READ|PROT_WRITE,
> +											  (unsigned long)ring_ref);

Indentation.

> +		if (con->interface == NULL) {
> +			err = EINVAL;
> +			goto out;
> +		}
> +		con->ring_ref = ring_ref;
> +	}
> +
> +out:
> +	return err;
> +}
> +
>  static int console_create_ring(struct console *con)
>  {
>  	int err, remote_port, ring_ref;
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 10/12 v3] xen/arm: vpl011: Add a new vuart console type to xenconsole client
  2017-05-10 14:35 ` [PATCH 10/12 v3] xen/arm: vpl011: Add a new vuart console type to xenconsole client Bhupinder Thakur
@ 2017-05-11 11:17   ` Wei Liu
  2017-05-16 23:02   ` Stefano Stabellini
  1 sibling, 0 replies; 29+ messages in thread
From: Wei Liu @ 2017-05-11 11:17 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Ian Jackson, Wei Liu

On Wed, May 10, 2017 at 08:05:16PM +0530, Bhupinder Thakur wrote:
> Add a new console type VUART to connect to guest's emualated vuart
> console.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  tools/console/client/main.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/console/client/main.c b/tools/console/client/main.c
> index 977779f..6f4405f 100644
> --- a/tools/console/client/main.c
> +++ b/tools/console/client/main.c
> @@ -76,7 +76,7 @@ static void usage(const char *program) {
>  	       "\n"
>  	       "  -h, --help       display this help and exit\n"
>  	       "  -n, --num N      use console number N\n"
> -	       "  --type TYPE      console type. must be 'pv' or 'serial'\n"
> +	       "  --type TYPE      console type. must be 'pv', 'serial' or 'vuart'\n"
>  	       "  --start-notify-fd N file descriptor used to notify parent\n"
>  	       , program);
>  }
> @@ -264,6 +264,7 @@ typedef enum {
>         CONSOLE_INVAL,
>         CONSOLE_PV,
>         CONSOLE_SERIAL,
> +       CONSOLE_VUART,
>  } console_type;
>  
>  static struct termios stdin_old_attr;
> @@ -361,6 +362,8 @@ int main(int argc, char **argv)
>  				type = CONSOLE_SERIAL;
>  			else if (!strcmp(optarg, "pv"))
>  				type = CONSOLE_PV;
> +			else if (!strcmp(optarg, "vuart"))
> +				type = CONSOLE_VUART;
>  			else {
>  				fprintf(stderr, "Invalid type argument\n");
>  				fprintf(stderr, "Console types supported are: serial, pv\n");
> @@ -436,6 +439,9 @@ int main(int argc, char **argv)
>  		else
>  			snprintf(path, strlen(dom_path) + strlen("/device/console/%d/tty") + 5, "%s/device/console/%d/tty", dom_path, num);
>  	}
> +	if (type == CONSOLE_VUART) {
> +		snprintf(path, strlen(dom_path) + strlen("/vuart/0/tty") + 1, 
> +				 "%s/vuart/0/tty", dom_path);

Indentation.

With this fixed:

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 11/12 v3] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree
  2017-05-10 14:35 ` [PATCH 11/12 v3] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree Bhupinder Thakur
@ 2017-05-11 11:18   ` Wei Liu
  2017-05-16 23:05   ` Stefano Stabellini
  2017-05-22 14:42   ` Julien Grall
  2 siblings, 0 replies; 29+ messages in thread
From: Wei Liu @ 2017-05-11 11:18 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Wei Liu, Ian Jackson

On Wed, May 10, 2017 at 08:05:17PM +0530, Bhupinder Thakur wrote:
> The SBSA uart node format is as specified in
> Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt and given below:
> 
> ARM SBSA defined generic UART
> ------------------------------
> This UART uses a subset of the PL011 registers and consequently lives
> in the PL011 driver. It's baudrate and other communication parameters
> cannot be adjusted at runtime, so it lacks a clock specifier here.
> 
> Required properties:
> - compatible: must be "arm,sbsa-uart"
> - reg: exactly one register range
> - interrupts: exactly one interrupt specifier
> - current-speed: the (fixed) baud rate set by the firmware
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

I will leave this to arm folks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 12/12 v3] xen/arm: vpl011: Update documentation for vuart console support
  2017-05-10 14:35 ` [PATCH 12/12 v3] xen/arm: vpl011: Update documentation for vuart console support Bhupinder Thakur
@ 2017-05-11 11:19   ` Wei Liu
  2017-05-16 23:10   ` Stefano Stabellini
  1 sibling, 0 replies; 29+ messages in thread
From: Wei Liu @ 2017-05-11 11:19 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Ian Jackson, Wei Liu

On Wed, May 10, 2017 at 08:05:18PM +0530, Bhupinder Thakur wrote:
> 1. Update documentation for a new vuart option added.
> 2. Update documentation about SPI irq reserved for vpl011.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore
  2017-05-11 11:10 ` [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Wei Liu
@ 2017-05-12  9:32   ` Bhupinder Thakur
  2017-05-16 15:18     ` Wei Liu
  0 siblings, 1 reply; 29+ messages in thread
From: Bhupinder Thakur @ 2017-05-12  9:32 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson

Hi Wei,

On 11 May 2017 at 16:40, Wei Liu <wei.liu2@citrix.com> wrote:
> On Wed, May 10, 2017 at 08:05:12PM +0530, Bhupinder Thakur wrote:
>>          libxl__device_console_add(gc, domid, &console, state, &device);
>> @@ -1369,14 +1377,22 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>>      }
>>      case LIBXL_DOMAIN_TYPE_PV:
>>      {
>> -        libxl__device_console console;
>> -        libxl__device device;
>> +        libxl__device_console console, vuart;
>> +        libxl__device device, vuart_device;
>>
>>          for (i = 0; i < d_config->num_vfbs; i++) {
>>              libxl__device_vfb_add(gc, domid, &d_config->vfbs[i]);
>>              libxl__device_vkb_add(gc, domid, &d_config->vkbs[i]);
>>          }
>>
>> +        if (d_config->b_info.vuart)
>> +        {
>> +            init_console_info(gc, &vuart, 0);
>> +            vuart.backend_domid = state->console_domid;
>> +            libxl__device_vuart_add(gc, domid, &vuart, state, &vuart_device);
>> +            libxl__device_console_dispose(&vuart);
>> +        }
>> +
>
> This is strange. You have code snippet for both PV and HVM. Why?
Yes I believe I should have added only for PV guest but to keep the
code consistent for both type of guests, I added for both.
I will remove it for HVM.

>
>>          init_console_info(gc, &console, 0);
>>          console.backend_domid = state->console_domid;
>>          libxl__device_console_add(gc, domid, &console, state, &device);
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index 5e96676..bb25672 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -26,6 +26,9 @@ static char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device)
>>      if (device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0)
>>          return GCSPRINTF("%s/console", dom_path);
>>
>> +    if (device->kind == LIBXL__DEVICE_KIND_VUART)
>> +        return GCSPRINTF("%s/vuart/%d", dom_path, device->devid);
>> +
>>      return GCSPRINTF("%s/device/%s/%d", dom_path,
>>                       libxl__device_kind_to_string(device->kind),
>>                       device->devid);
>> @@ -151,13 +154,19 @@ retry_transaction:
>>      if (rc) goto out;
>>
>>      if (!libxl_only) {
>> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
>> -                                     frontend_path);
>> -        if (rc) goto out;
>> +        if (fents || ro_fents)
>> +        {
>> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
>> +                                         frontend_path);
>> +            if (rc) goto out;
>> +        }
>>
>> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
>> -                                     backend_path);
>> -        if (rc) goto out;
>> +        if (bents)
>> +        {
>> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
>> +                                         backend_path);
>> +            if (rc) goto out;
>> +        }
>
> What is this for?
>
> If there is no fe or be entries you skip the path creation altogether.
> But why? This doesn't seem to be related to your patch.
For vuart, I am adding only a front end node but the
libxl__device_generic_add() creates the backend path also,even though
there is no backend node. To remove that hanging be path, I added this
check.
>
> At least explain this a bit in the commit message?
I will add more details in the commit message.

Regards,
Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore
  2017-05-12  9:32   ` Bhupinder Thakur
@ 2017-05-16 15:18     ` Wei Liu
  2017-05-22 11:03       ` Bhupinder Thakur
  0 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2017-05-16 15:18 UTC (permalink / raw)
  To: Bhupinder Thakur
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Wei Liu, Ian Jackson

On Fri, May 12, 2017 at 03:02:29PM +0530, Bhupinder Thakur wrote:
[...]
> >> @@ -151,13 +154,19 @@ retry_transaction:
> >>      if (rc) goto out;
> >>
> >>      if (!libxl_only) {
> >> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
> >> -                                     frontend_path);
> >> -        if (rc) goto out;
> >> +        if (fents || ro_fents)
> >> +        {
> >> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
> >> +                                         frontend_path);
> >> +            if (rc) goto out;
> >> +        }
> >>
> >> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
> >> -                                     backend_path);
> >> -        if (rc) goto out;
> >> +        if (bents)
> >> +        {
> >> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
> >> +                                         backend_path);
> >> +            if (rc) goto out;
> >> +        }
> >
> > What is this for?
> >
> > If there is no fe or be entries you skip the path creation altogether.
> > But why? This doesn't seem to be related to your patch.
> For vuart, I am adding only a front end node but the
> libxl__device_generic_add() creates the backend path also,even though
> there is no backend node. To remove that hanging be path, I added this
> check.
> >
> > At least explain this a bit in the commit message?
> I will add more details in the commit message.
> 

Preferable it should be in a separate patch. That would make review
easier.

But there is another question: how do you know if Dom0 is servicing a
DomU? How do you construct a libxl__device struct should you want to
manipulate vuart?

Wei.

> Regards,
> Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 07/12 v3] xen/arm: vpl011: Allocate a new GFN in the toolstack for vuart
  2017-05-10 14:35 ` [PATCH 07/12 v3] xen/arm: vpl011: Allocate a new GFN in the toolstack for vuart Bhupinder Thakur
  2017-05-11 11:10   ` Wei Liu
@ 2017-05-16 22:59   ` Stefano Stabellini
  1 sibling, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2017-05-16 22:59 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Wei Liu, Ian Jackson

On Wed, 10 May 2017, Bhupinder Thakur wrote:
> Allocate a new gfn to be used as a ring buffer between xenconsole
> and Xen for sending/receiving pl011 data.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> Changes since v2:
> 
> - Removed the DOMCTL call to set the GFN as now this information is passed
>   in the DOMCTL call to initialize vpl011 emulation.
> 
>  tools/libxc/include/xc_dom.h | 2 ++
>  tools/libxc/xc_dom_arm.c     | 5 ++++-
>  tools/libxc/xc_dom_boot.c    | 2 ++
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index ce47058..6e06ef1 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -216,6 +216,8 @@ struct xc_dom_image {
>  
>      /* Extra SMBIOS structures passed to HVMLOADER */
>      struct xc_hvm_firmware_module smbios_module;
> +
> +    xen_pfn_t vuart_gfn;
>  };
>  
>  /* --- pluggable kernel loader ------------------------------------- */
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index e7d4bd0..c981b7a 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -26,10 +26,11 @@
>  #include "xg_private.h"
>  #include "xc_dom.h"
>  
> -#define NR_MAGIC_PAGES 3
> +#define NR_MAGIC_PAGES 4
>  #define CONSOLE_PFN_OFFSET 0
>  #define XENSTORE_PFN_OFFSET 1
>  #define MEMACCESS_PFN_OFFSET 2
> +#define VUART_PFN_OFFSET 3
>  
>  #define LPAE_SHIFT 9
>  
> @@ -85,10 +86,12 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>  
>      dom->console_pfn = base + CONSOLE_PFN_OFFSET;
>      dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> +    dom->vuart_gfn = base + VUART_PFN_OFFSET;
>  
>      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
>      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
>      xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
> +    xc_clear_domain_page(dom->xch, dom->guest_domid, base + VUART_PFN_OFFSET);
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
>              dom->console_pfn);
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index c3b44dd..8a376d0 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -226,6 +226,8 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
>          return rc;
>      if ( (rc = clear_page(dom, dom->xenstore_pfn)) != 0 )
>          return rc;
> +    if ( (rc = clear_page(dom, dom->vuart_gfn)) != 0 )
> +        return rc;
>  
>      /* start info page */
>      if ( dom->arch_hooks->start_info )
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 10/12 v3] xen/arm: vpl011: Add a new vuart console type to xenconsole client
  2017-05-10 14:35 ` [PATCH 10/12 v3] xen/arm: vpl011: Add a new vuart console type to xenconsole client Bhupinder Thakur
  2017-05-11 11:17   ` Wei Liu
@ 2017-05-16 23:02   ` Stefano Stabellini
  1 sibling, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2017-05-16 23:02 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Wei Liu, Ian Jackson

On Wed, 10 May 2017, Bhupinder Thakur wrote:
> Add a new console type VUART to connect to guest's emualated vuart
> console.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  tools/console/client/main.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/console/client/main.c b/tools/console/client/main.c
> index 977779f..6f4405f 100644
> --- a/tools/console/client/main.c
> +++ b/tools/console/client/main.c
> @@ -76,7 +76,7 @@ static void usage(const char *program) {
>  	       "\n"
>  	       "  -h, --help       display this help and exit\n"
>  	       "  -n, --num N      use console number N\n"
> -	       "  --type TYPE      console type. must be 'pv' or 'serial'\n"
> +	       "  --type TYPE      console type. must be 'pv', 'serial' or 'vuart'\n"
>  	       "  --start-notify-fd N file descriptor used to notify parent\n"
>  	       , program);
>  }
> @@ -264,6 +264,7 @@ typedef enum {
>         CONSOLE_INVAL,
>         CONSOLE_PV,
>         CONSOLE_SERIAL,
> +       CONSOLE_VUART,
>  } console_type;
>  
>  static struct termios stdin_old_attr;
> @@ -361,6 +362,8 @@ int main(int argc, char **argv)
>  				type = CONSOLE_SERIAL;
>  			else if (!strcmp(optarg, "pv"))
>  				type = CONSOLE_PV;
> +			else if (!strcmp(optarg, "vuart"))
> +				type = CONSOLE_VUART;
>  			else {
>  				fprintf(stderr, "Invalid type argument\n");
>  				fprintf(stderr, "Console types supported are: serial, pv\n");
> @@ -436,6 +439,9 @@ int main(int argc, char **argv)
>  		else
>  			snprintf(path, strlen(dom_path) + strlen("/device/console/%d/tty") + 5, "%s/device/console/%d/tty", dom_path, num);
>  	}
> +	if (type == CONSOLE_VUART) {
> +		snprintf(path, strlen(dom_path) + strlen("/vuart/0/tty") + 1, 
> +				 "%s/vuart/0/tty", dom_path);
> +	}
>  
>  	/* FIXME consoled currently does not assume domain-0 doesn't have a
>  	   console which is good when we break domain-0 up.  To keep us
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 11/12 v3] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree
  2017-05-10 14:35 ` [PATCH 11/12 v3] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree Bhupinder Thakur
  2017-05-11 11:18   ` Wei Liu
@ 2017-05-16 23:05   ` Stefano Stabellini
  2017-05-22 14:42   ` Julien Grall
  2 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2017-05-16 23:05 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Wei Liu, Ian Jackson

On Wed, 10 May 2017, Bhupinder Thakur wrote:
> The SBSA uart node format is as specified in
> Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt and given below:
> 
> ARM SBSA defined generic UART
> ------------------------------
> This UART uses a subset of the PL011 registers and consequently lives
> in the PL011 driver. It's baudrate and other communication parameters
> cannot be adjusted at runtime, so it lacks a clock specifier here.
> 
> Required properties:
> - compatible: must be "arm,sbsa-uart"
> - reg: exactly one register range
> - interrupts: exactly one interrupt specifier
> - current-speed: the (fixed) baud rate set by the firmware
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> 
> Changes since v2:
> - Currently device discovery using ACPI is not supported.
> - Dropped the reviewed-by tag by Stefano as there were some IRQ related changes
>   done later.
> 
>  tools/libxl/libxl_arm.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d842d88..f88ef0d 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -44,10 +44,23 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>      uint32_t nr_spis = 0;
>      unsigned int i;
>  
> +    /*
> +     * If pl011 vuart is enabled then increment the nr_spis to allow allocation
> +     * of SPI VIRQ for pl011.
> +     */
> +    if (d_config->b_info.vuart)
> +        nr_spis += (GUEST_VPL011_SPI - 32) + 1;
> +
>      for (i = 0; i < d_config->b_info.num_irqs; i++) {
>          uint32_t irq = d_config->b_info.irqs[i];
>          uint32_t spi;
>  
> +        if (d_config->b_info.vuart && (irq == GUEST_VPL011_SPI))
> +        {

code style for libxl is

   if () {

sorry about all the different code styles, it's a mess

Aside from that:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> +            LOG(ERROR, "Physical IRQ %u conflicting with pl011 SPI\n", irq);
> +            return ERROR_FAIL;
> +        }
> +
>          if (irq < 32)
>              continue;
>  
> @@ -130,9 +143,10 @@ static struct arch_info {
>      const char *guest_type;
>      const char *timer_compat;
>      const char *cpu_compat;
> +    const char *uart_compat;
>  } arch_info[] = {
> -    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15" },
> -    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
> +    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" },
> +    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" },
>  };
>  
>  /*
> @@ -590,6 +604,38 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>  
> +static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
> +                                 const struct arch_info *ainfo,
> +                                 struct xc_dom_image *dom)
> +{
> +    int res;
> +    gic_interrupt intr;
> +
> +    res = fdt_begin_node(fdt, "sbsa-pl011");
> +    if (res) return res;
> +
> +    res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat);
> +    if (res) return res;
> +
> +    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
> +                            1,
> +                            GUEST_PL011_BASE, GUEST_PL011_SIZE);
> +    if (res) return res;
> +
> +    set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> +
> +    res = fdt_property_interrupts(gc, fdt, &intr, 1);
> +    if (res) return res;
> +
> +    /* Use a default baud rate of 115200. */
> +    fdt_property_u32(fdt, "current-speed", 115200);
> +
> +    res = fdt_end_node(fdt);
> +    if (res) return res;
> +
> +    return 0;
> +}
> +
>  static const struct arch_info *get_arch_info(libxl__gc *gc,
>                                               const struct xc_dom_image *dom)
>  {
> @@ -889,6 +935,9 @@ next_resize:
>          FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
>          FDT( make_hypervisor_node(gc, fdt, vers) );
>  
> +        if (info->vuart)
> +            FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
> +
>          if (pfdt)
>              FDT( copy_partial_fdt(gc, fdt, pfdt) );
>  
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 12/12 v3] xen/arm: vpl011: Update documentation for vuart console support
  2017-05-10 14:35 ` [PATCH 12/12 v3] xen/arm: vpl011: Update documentation for vuart console support Bhupinder Thakur
  2017-05-11 11:19   ` Wei Liu
@ 2017-05-16 23:10   ` Stefano Stabellini
  1 sibling, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2017-05-16 23:10 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Wei Liu, Ian Jackson

On Wed, 10 May 2017, Bhupinder Thakur wrote:
> 1. Update documentation for a new vuart option added.
> 2. Update documentation about SPI irq reserved for vpl011.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> 
> Changes since v2:
> 
> - Incorporated the review comments on the documentation.
> 
>  docs/man/xl.cfg.pod.5.in |  9 +++++++++
>  docs/misc/console.txt    | 31 ++++++++++++++++++++++++++-----
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index 13167ff..3397cda 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
> @@ -1085,6 +1085,15 @@ Allow a guest to access specific physical IRQs.
>  It is recommended to use this option only for trusted VMs under
>  administrator control.
>  
> +If the virtual uart is enabled then irq 32 is reserved for it. By
> +default, it is disabled. If the user specifies the following option in
> +the VM config file then the vuart gets enabled. Today, only the
> +"pl011" model is supported.
> +
> +vuart = "pl011"
> +
> +Currently vuart console is available only for ARM64.
> +
>  =item B<max_event_channels=N>
>  
>  Limit the guest to using at most N event channels (PV interrupts).
> diff --git a/docs/misc/console.txt b/docs/misc/console.txt
> index 16da805..9eccfa1 100644
> --- a/docs/misc/console.txt
> +++ b/docs/misc/console.txt
> @@ -19,7 +19,20 @@ The first PV console path in xenstore remains:
>  
>  /local/domain/$DOMID/console
>  
> -the other PV consoles follow the conventional xenstore device path and
> +The virtual UART console path in xenstore is defined as:
> +
> +/local/domain/$DOMID/vuart/0
> +
> +The vuart console provides access to a virtual pl011 UART on ARM64 systems. To
> +enable vuart the following line has to be added to the guest configuration
> +file:
> +
> +vuart = "pl011"
> +
> +In Linux you can select the virtual pl011 UART by using the "ttyAMA0"
> +console instead of "hvc0".
> +
> +The other PV consoles follow the conventional xenstore device path and
>  live in:
>  
>  /local/domain/$DOMID/device/console/$DEVID.
> @@ -61,6 +74,14 @@ output = pty
>  The backend will write the pty device name to the "tty" node in the
>  console frontend.
>  
> +For the PV console the tty node is added at
> +
> +/local/domain/$DOMID/console/tty
> +
> +For the virtual UART console the tty node is added at
> +
> +/local/domain/$DOMID/vuart/0/tty
> +
>  If the toolstack wants a listening Unix domain socket to be created at path
>  <path>, a connection accepted and data proxied to the console, it will write:
>  
> @@ -79,8 +100,8 @@ For example:
>  ioemu
>  
>  The supported values are only xenconsoled or ioemu; xenconsoled has
> -several limitations: it can only be used for the first PV console and it
> -can only connect to a pty.
> +several limitations: it can only be used for the first PV or virtual UART console
> +and it can only connect to a pty.
>  
>  Emulated serials are provided by qemu-dm only to hvm guests; the number
>  of emulated serials depends on how many "-serial" command line options
> @@ -90,8 +111,8 @@ xenstore in the following path:
>  
>  /local/domain/$DOMID/serial/$SERIAL_NUM/tty
>  
> -xenconsole is the tool to connect to a PV console or an emulated serial
> -that has a pty as output. Xenconsole takes a domid as parameter plus an
> +xenconsole is the tool to connect to a PV or virtual UART console or an
> +emulated serial that has a pty as output. Xenconsole takes a domid as parameter plus an

This line is too long. Aside from that:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


>  optional console type (pv for PV consoles or serial for emulated
>  serials) and console number. Depending on the type and console
>  number, xenconsole will look for the tty node in different xenstore
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 08/12 v3] xen/arm: vpl011: Modify xenconsole to support multiple consoles
  2017-05-10 14:35 ` [PATCH 08/12 v3] xen/arm: vpl011: Modify xenconsole to support multiple consoles Bhupinder Thakur
  2017-05-11 11:15   ` Wei Liu
@ 2017-05-16 23:41   ` Stefano Stabellini
  2017-05-25 10:57     ` Bhupinder Thakur
  1 sibling, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2017-05-16 23:41 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Wei Liu, Ian Jackson

On Wed, 10 May 2017, Bhupinder Thakur wrote:
> Xenconsole supports only PV console currently. This patch adds support
> for supporting multiple consoles.
> 
> This patch modifies different data structures and APIs used
> in xenconsole to support multiple consoles.
> 
> Change summary:
> 
> 1. Split the domain structure into a console structure and the
>    domain structure, where each console structure represents one
>    console.
> 
> 2. Modify different APIs such as buffer_append() etc. to take
>    console structure as input and perform per console specific
>    operations.
> 
> 3. Define a generic console_create_ring(), which sets up the
>    ring buffer and event channel for each console.
> 
> 3. Modify domain_create_ring() to use console_create_ring().
> 
> 4. Modifications in handle_ring_read() to read ring buffer data
>    from multiple consoles.
> 
> 5. Add log file support for multiple consoles.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

There is something wrong with this patch: I cannot apply it.

Also, it is still way to big for me to review. I cannot track all the
changes and figure out if they are correct.

One option is to introduce struct console in one patch, with only one
struct console per domain. Then the second patch could introduce
multiple struct console with the helpers such as console_iter_void_arg1.

Finally the third patch could add vuart support.


> ---
> 
> Changes since v2:
> 
> - Defined a new function console_create_ring() which sets up the ring buffer and 
>   event channel a new console. domain_create_ring() uses this function to setup
>   a console.
> - This patch does not contain vuart specific changes, which would be introduced in
>   the next patch.
> - Changes for keeping the PV log file name unchanged.
> 
> Changes since v1:
> 
> - Split the domain struture to a separate console structure
> - Modified the functions to operate on the console struture
> - Replaced repetitive per console code with generic code
> 
>  tools/console/daemon/io.c | 650 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 460 insertions(+), 190 deletions(-)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 7e6a886..9bb14de 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -89,29 +89,139 @@ struct buffer {
>  	size_t max_capacity;
>  };
>  
> -struct domain {
> -	int domid;
> +struct console {
> +	char *xsname;
> +	char *ttyname;
>  	int master_fd;
>  	int master_pollfd_idx;
>  	int slave_fd;
>  	int log_fd;
> -	bool is_dead;
> -	unsigned last_seen;
>  	struct buffer buffer;
> -	struct domain *next;
> -	char *conspath;
>  	int ring_ref;
>  	xenevtchn_port_or_error_t local_port;
>  	xenevtchn_port_or_error_t remote_port;
> +	struct xencons_interface *interface;
> +	struct domain *d;  /* Reference to the domain it is contained in. */
> +	char *xspath;
> +	int (*map_ring_ref)(struct console *, int);
> +	bool mandatory;
> +};
> +
> +struct console_data {
> +	char *xsname;
> +	char *ttyname;
> +	int (*mapfunc)(struct console *, int);
> +	bool mandatory;
> +};
> +
> +static int map_pvcon_ring_ref(struct console *, int );
> +
> +static struct console_data console_data[] = {
> +
> +	{
> +		.xsname = "/console",
> +		.ttyname = "tty",
> +		.mapfunc = map_pvcon_ring_ref,
> +		.mandatory = true
> +	},
> +};
> +
> +#define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
> +
> +struct domain {
> +	int domid;
> +	bool is_dead;
> +	unsigned last_seen;
> +	struct domain *next;
>  	xenevtchn_handle *xce_handle;
>  	int xce_pollfd_idx;
> -	struct xencons_interface *interface;
>  	int event_count;
>  	long long next_period;
> +	struct console console[MAX_CONSOLE];
>  };
>  
>  static struct domain *dom_head;
>  
> +typedef void (*VOID_ITER_FUNC_ARG1)(struct console *);
> +typedef bool (*BOOL_ITER_FUNC_ARG1)(struct console *);
> +typedef int (*INT_ITER_FUNC_ARG1)(struct console *);
> +typedef void (*VOID_ITER_FUNC_ARG2)(struct console *,  unsigned int);
> +typedef int (*INT_ITER_FUNC_ARG3)(struct console *,
> +			 struct domain *dom, void **);
> +
> +static inline bool console_enabled(struct console *con)
> +{
> +	return con->local_port != -1;
> +}
> +
> +static inline void console_iter_void_arg1(struct domain *d,
> +										   VOID_ITER_FUNC_ARG1 iter_func)
> +{
> +	int i = 0;
> +	struct console *con = &(d->console[0]);
> +
> +	for (i = 0; i < MAX_CONSOLE; i++, con++)
> +	{
> +		iter_func(con);
> +	}
> +}
> +
> +static inline void console_iter_void_arg2(struct domain *d,
> +										   VOID_ITER_FUNC_ARG2 iter_func,
> +										   unsigned int iter_data)
> +{
> +	int i = 0;
> +	struct console *con = &(d->console[0]);
> +
> +	for (i = 0; i < MAX_CONSOLE; i++, con++)
> +	{
> +		iter_func(con, iter_data);
> +	}
> +}
> +
> +static inline bool console_iter_bool_arg1(struct domain *d,
> +										   BOOL_ITER_FUNC_ARG1 iter_func)
> +{
> +	int i = 0;
> +	struct console *con = &(d->console[0]);
> +
> +	for (i = 0; i < MAX_CONSOLE; i++, con++)
> +	{
> +		if (iter_func(con))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static inline int console_iter_int_arg1(struct domain *d,
> +										 INT_ITER_FUNC_ARG1 iter_func)
> +{
> +	int i = 0;
> +	struct console *con = &(d->console[0]);
> +
> +	for (i = 0; i < MAX_CONSOLE; i++, con++)
> +	{
> +		if (iter_func(con))
> +			return 1;
> +	}
> +	return 0;
> +}
> +
> +static inline int console_iter_int_arg3(struct domain *d,
> +										 INT_ITER_FUNC_ARG3 iter_func,
> +										 void *iter_data)
> +{
> +	int i = 0;
> +	struct console *con = &(d->console[0]);
> +
> +	for (i = 0; i < MAX_CONSOLE; i++, con++)
> +	{
> +		if (iter_func(con, d, iter_data))
> +			return 1;
> +	}
> +	return 0;
> +}
> +
>  static int write_all(int fd, const char* buf, size_t len)
>  {
>  	while (len) {
> @@ -158,11 +268,29 @@ static int write_with_timestamp(int fd, const char *data, size_t sz,
>  	return 0;
>  }
>  
> -static void buffer_append(struct domain *dom)
> +static inline bool buffer_available(struct console *con)
>  {
> -	struct buffer *buffer = &dom->buffer;
> +	if (discard_overflowed_data ||
> +		!con->buffer.max_capacity ||
> +		con->buffer.size < con->buffer.max_capacity)
> +		return true;
> +	else
> +		return false;
> +}
> +
> +static void buffer_append(struct console *con, unsigned int data)
> +{
> +	struct buffer *buffer = &con->buffer;
> +	struct xencons_interface *intf = con->interface;
> +	xenevtchn_port_or_error_t port = con->local_port;
> +	struct domain *dom = con->d;
> +	xenevtchn_port_or_error_t rxport = (xenevtchn_port_or_error_t)data;
> +
>  	XENCONS_RING_IDX cons, prod, size;
> -	struct xencons_interface *intf = dom->interface;
> +
> +	/* If incoming data is not for the current console then ignore. */
> +	if (port != rxport)
> +		return;
>  
>  	cons = intf->out_cons;
>  	prod = intf->out_prod;
> @@ -187,22 +315,22 @@ static void buffer_append(struct domain *dom)
>  
>  	xen_mb();
>  	intf->out_cons = cons;
> -	xenevtchn_notify(dom->xce_handle, dom->local_port);
> +	xenevtchn_notify(dom->xce_handle, port);
>  
>  	/* Get the data to the logfile as early as possible because if
>  	 * no one is listening on the console pty then it will fill up
>  	 * and handle_tty_write will stop being called.
>  	 */
> -	if (dom->log_fd != -1) {
> +	if (con->log_fd != -1) {
>  		int logret;
>  		if (log_time_guest) {
>  			logret = write_with_timestamp(
> -				dom->log_fd,
> +				con->log_fd,
>  				buffer->data + buffer->size - size,
>  				size, &log_time_guest_needts);
>  		} else {
>  			logret = write_all(
> -				dom->log_fd,
> +				con->log_fd,
>  				buffer->data + buffer->size - size,
>  				size);
>  		}
> @@ -290,12 +418,13 @@ static int create_hv_log(void)
>  	return fd;
>  }
>  
> -static int create_domain_log(struct domain *dom)
> +static int create_console_log(struct console *con)
>  {
>  	char logfile[PATH_MAX];
>  	char *namepath, *data, *s;
>  	int fd;
>  	unsigned int len;
> +	struct domain *dom = con->d;
>  
>  	namepath = xs_get_domain_path(xs, dom->domid);
>  	s = realloc(namepath, strlen(namepath) + 6);
> @@ -314,7 +443,9 @@ static int create_domain_log(struct domain *dom)
>  		return -1;
>  	}
>  
> -	snprintf(logfile, PATH_MAX-1, "%s/guest-%s.log", log_dir, data);
> +	snprintf(logfile, PATH_MAX-1, "%s%s/guest-%s.log",
> +			 log_dir, con->xspath, data);

This changes the log directory, right? Are the new directories created
correctly by the install scripts?


>  	free(data);
>  	logfile[PATH_MAX-1] = '\0';
>  
> @@ -336,19 +467,24 @@ static int create_domain_log(struct domain *dom)
>  	return fd;
>  }
>  
> -static void domain_close_tty(struct domain *dom)
> +static void console_close_tty(struct console *con)
>  {
> -	if (dom->master_fd != -1) {
> -		close(dom->master_fd);
> -		dom->master_fd = -1;
> +	if (con->master_fd != -1) {
> +		close(con->master_fd);
> +		con->master_fd = -1;
>  	}
>  
> -	if (dom->slave_fd != -1) {
> -		close(dom->slave_fd);
> -		dom->slave_fd = -1;
> +	if (con->slave_fd != -1) {
> +		close(con->slave_fd);
> +		con->slave_fd = -1;
>  	}
>  }
>  
> +static void domain_close_tty(struct domain *dom)
> +{
> +	console_iter_void_arg1(dom, console_close_tty);
> +}
> +
>  #ifdef __sun__
>  static int openpty(int *amaster, int *aslave, char *name,
>  		   struct termios *termp, struct winsize *winp)
> @@ -409,7 +545,7 @@ void cfmakeraw(struct termios *termios_p)
>  }
>  #endif /* __sun__ */
>  
> -static int domain_create_tty(struct domain *dom)
> +static int console_create_tty(struct console *con)
>  {
>  	const char *slave;
>  	char *path;
> @@ -418,19 +554,23 @@ static int domain_create_tty(struct domain *dom)
>  	char *data;
>  	unsigned int len;
>  	struct termios term;
> +	struct domain *dom = con->d;
> +
> +	if (!console_enabled(con))
> +		return 0;
>  
> -	assert(dom->slave_fd == -1);
> -	assert(dom->master_fd == -1);
> +	assert(con->master_fd == -1);
> +	assert(con->slave_fd == -1);
>  
> -	if (openpty(&dom->master_fd, &dom->slave_fd, NULL, NULL, NULL) < 0) {
> +	if (openpty(&con->master_fd, &con->slave_fd, NULL, NULL, NULL) < 0) {
>  		err = errno;
>  		dolog(LOG_ERR, "Failed to create tty for domain-%d "
> -		      "(errno = %i, %s)",
> -		      dom->domid, err, strerror(err));
> -		return 0;
> +			  "(errno = %i, %s)",
> +			  dom->domid, err, strerror(err));
> +		goto out;

still changing the return into a goto?


>  	}
>  
> -	if (tcgetattr(dom->slave_fd, &term) < 0) {
> +	if (tcgetattr(con->slave_fd, &term) < 0) {
>  		err = errno;
>  		dolog(LOG_ERR, "Failed to get tty attributes for domain-%d "
>  			"(errno = %i, %s)",
> @@ -438,7 +578,7 @@ static int domain_create_tty(struct domain *dom)
>  		goto out;
>  	}
>  	cfmakeraw(&term);
> -	if (tcsetattr(dom->slave_fd, TCSANOW, &term) < 0) {
> +	if (tcsetattr(con->slave_fd, TCSANOW, &term) < 0) {
>  		err = errno;
>  		dolog(LOG_ERR, "Failed to set tty attributes for domain-%d "
>  			"(errno = %i, %s)",
> @@ -446,40 +586,54 @@ static int domain_create_tty(struct domain *dom)
>  		goto out;
>  	}
>  
> -	if ((slave = ptsname(dom->master_fd)) == NULL) {
> +	if ((slave = ptsname(con->master_fd)) == NULL) {
>  		err = errno;
>  		dolog(LOG_ERR, "Failed to get slave name for domain-%d "
> -		      "(errno = %i, %s)",
> -		      dom->domid, err, strerror(err));
> +			  "(errno = %i, %s)",
> +			  dom->domid, err, strerror(err));
>  		goto out;
>  	}
>  
> -	success = asprintf(&path, "%s/limit", dom->conspath) !=
> +	success = asprintf(&path, "%s/limit", con->xspath) !=
>  		-1;
>  	if (!success)
>  		goto out;
>  	data = xs_read(xs, XBT_NULL, path, &len);
>  	if (data) {
> -		dom->buffer.max_capacity = strtoul(data, 0, 0);
> +		con->buffer.max_capacity = strtoul(data, 0, 0);
>  		free(data);
>  	}
>  	free(path);
>  
> -	success = (asprintf(&path, "%s/tty", dom->conspath) != -1);
> +	success = (asprintf(&path, "%s/%s", con->xspath, con->ttyname) != -1);
> +
>  	if (!success)
>  		goto out;
>  	success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
>  	free(path);
> -	if (!success)
> +
> +	if (fcntl(con->master_fd, F_SETFL, O_NONBLOCK) == -1)
>  		goto out;
>  
> -	if (fcntl(dom->master_fd, F_SETFL, O_NONBLOCK) == -1)
> +	if (!success)
>  		goto out;
>  
> -	return 1;
> -out:
> -	domain_close_tty(dom);
>  	return 0;
> +
> +out:
> +	return 1;
> +}
> +
> +static int domain_create_tty(struct domain *dom)
> +{
> +	int ret;
> +
> +	ret = console_iter_int_arg1(dom, console_create_tty);
> +
> +	if (ret)
> +		domain_close_tty(dom);
> +
> +	return ret;
>  }
>   
>  /* Takes tuples of names, scanf-style args, and void **, NULL terminated. */
> @@ -517,31 +671,106 @@ static int xs_gather(struct xs_handle *xs, const char *dir, ...)
>  	return ret;
>  }
>  
> -static void domain_unmap_interface(struct domain *dom)
> +static void console_unmap_interface(struct console *con)
>  {
> -	if (dom->interface == NULL)
> +	if (con->interface == NULL)
>  		return;
> -	if (xgt_handle && dom->ring_ref == -1)
> -		xengnttab_unmap(xgt_handle, dom->interface, 1);
> +	if (xgt_handle && con->ring_ref == -1)
> +		xengnttab_unmap(xgt_handle, con->interface, 1);
>  	else
> -		munmap(dom->interface, XC_PAGE_SIZE);
> -	dom->interface = NULL;
> -	dom->ring_ref = -1;
> +		munmap(con->interface, XC_PAGE_SIZE);
> +	con->interface = NULL;
> +	con->ring_ref = -1;
> +}
> +
> +static void domain_unmap_interface(struct domain *dom)
> +{
> +	console_iter_void_arg1(dom, console_unmap_interface);
> +}
> +
> +static int bind_event_channel(struct domain *dom,
> +							  int new_rport,
> +							  int *lport,
> +							  int *rport)
> +{
> +	int err = 0, rc;
> +
> +	/* Go no further if port has not changed and we are still bound. */
> +	if (new_rport == *rport) {
> +		xc_evtchn_status_t status = {
> +		.dom = DOMID_SELF,
> +		.port = *lport };
> +		if ((xc_evtchn_status(xc, &status) == 0) &&
> +			(status.status == EVTCHNSTAT_interdomain))
> +			goto out;
> +	}
> +
> +	*lport = -1;
> +	*rport = -1;
> +	rc = xenevtchn_bind_interdomain(dom->xce_handle,
> +									dom->domid, new_rport);
> +
> +	if (rc == -1) {
> +		err = errno;
> +		goto out;
> +	}
> +
> +	*lport = rc;
> +	*rport = new_rport;
> +out:
> +	return err;
>  }
>   
> -static int domain_create_ring(struct domain *dom)
> +static int map_pvcon_ring_ref(struct console *con, int ring_ref)
>  {
> -	int err, remote_port, ring_ref, rc;
> +	int err = 0;
> +	struct domain *dom = con->d;
> +
> +	if (!con->interface && xgt_handle) {
> +		/* Prefer using grant table */
> +		con->interface = xengnttab_map_grant_ref(xgt_handle,
> +												 dom->domid, 
> +												 GNTTAB_RESERVED_CONSOLE,
> +												 PROT_READ|PROT_WRITE);
> +		con->ring_ref = -1;
> +	}
> +
> +	if (!con->interface) {
> +		con->interface = xc_map_foreign_range(xc,
> +											  dom->domid,
> +											  XC_PAGE_SIZE,
> +											  PROT_READ|PROT_WRITE,
> +											  (unsigned long)ring_ref);
> +		if (con->interface == NULL) {
> +			err = EINVAL;
> +			goto out;
> +		}
> +		con->ring_ref = ring_ref;
> +	}
> +
> +out:
> +	return err;
> +}
> +
> +static int console_create_ring(struct console *con)
> +{
> +	int err, remote_port, ring_ref;
>  	char *type, path[PATH_MAX];
> +	struct domain *dom = con->d;
> +
> +	err = xs_gather(xs, con->xspath,
> +					"ring-ref", "%u", &ring_ref,
> +					"port", "%i", &remote_port,
> +					NULL);
>  
> -	err = xs_gather(xs, dom->conspath,
> -			"ring-ref", "%u", &ring_ref,
> -			"port", "%i", &remote_port,
> -			NULL);
>  	if (err)
> +	{
> +		/* If the console is not mandatory then do not return an error. */
> +		if (!con->mandatory)
> +			err = 0;
>  		goto out;
> +	}
>  
> -	snprintf(path, sizeof(path), "%s/type", dom->conspath);
> +	snprintf(path, sizeof(path), "%s/type", con->xspath);
>  	type = xs_read(xs, XBT_NULL, path, NULL);
>  	if (type && strcmp(type, "xenconsoled") != 0) {
>  		free(type);
> @@ -550,41 +779,44 @@ static int domain_create_ring(struct domain *dom)
>  	free(type);
>  
>  	/* If using ring_ref and it has changed, remap */
> -	if (ring_ref != dom->ring_ref && dom->ring_ref != -1)
> -		domain_unmap_interface(dom);
> +	if (ring_ref != con->ring_ref && con->ring_ref != -1)
> +		console_unmap_interface(con);
>  
> -	if (!dom->interface && xgt_handle) {
> -		/* Prefer using grant table */
> -		dom->interface = xengnttab_map_grant_ref(xgt_handle,
> -			dom->domid, GNTTAB_RESERVED_CONSOLE,
> -			PROT_READ|PROT_WRITE);
> -		dom->ring_ref = -1;
> -	}
> -	if (!dom->interface) {
> -		/* Fall back to xc_map_foreign_range */
> -		dom->interface = xc_map_foreign_range(
> -			xc, dom->domid, XC_PAGE_SIZE,
> -			PROT_READ|PROT_WRITE,
> -			(unsigned long)ring_ref);
> -		if (dom->interface == NULL) {
> -			err = EINVAL;
> -			goto out;
> +	err = con->map_ring_ref(con, ring_ref);
> +
> +	if (err)
> +		goto out;
> +
> +	err = bind_event_channel(dom, remote_port,
> +							 &con->local_port,
> +							 &con->remote_port);
> +	if (err)
> +		goto out1;
> +
> +	if (con->master_fd == -1) {
> +		if (console_create_tty(con)) {
> +			err = errno;
> +			con->local_port = -1;
> +			con->remote_port = -1;
> +			goto out1;
>  		}
> -		dom->ring_ref = ring_ref;
>  	}
>  
> -	/* Go no further if port has not changed and we are still bound. */
> -	if (remote_port == dom->remote_port) {
> -		xc_evtchn_status_t status = {
> -			.dom = DOMID_SELF,
> -			.port = dom->local_port };
> -		if ((xc_evtchn_status(xc, &status) == 0) &&
> -		    (status.status == EVTCHNSTAT_interdomain))
> -			goto out;
> -	}
> +	if (log_guest && (con->log_fd == -1))
> +		con->log_fd = create_console_log(con);
> +
> +	return err;
> +
> + out1:
> +	console_unmap_interface(con);
> + out:
> +	return err;
> +}
> +
> +static int domain_create_ring(struct domain *dom)
> +{
> +	int err;
>  
> -	dom->local_port = -1;
> -	dom->remote_port = -1;
>  	if (dom->xce_handle != NULL)
>  		xenevtchn_close(dom->xce_handle);
>  
> @@ -592,37 +824,17 @@ static int domain_create_ring(struct domain *dom)
>  	 * wasteful, but that's how the code is structured... */
>  	dom->xce_handle = xenevtchn_open(NULL, 0);
>  	if (dom->xce_handle == NULL) {
> -		err = errno;
> -		goto out;
> +		return errno;
>  	}
> - 
> -	rc = xenevtchn_bind_interdomain(dom->xce_handle,
> -		dom->domid, remote_port);
>  
> -	if (rc == -1) {
> -		err = errno;
> +	err = console_iter_int_arg1(dom, console_create_ring);
> +
> +	if (err)
> +	{
>  		xenevtchn_close(dom->xce_handle);
>  		dom->xce_handle = NULL;
> -		goto out;
> -	}
> -	dom->local_port = rc;
> -	dom->remote_port = remote_port;
> -
> -	if (dom->master_fd == -1) {
> -		if (!domain_create_tty(dom)) {
> -			err = errno;
> -			xenevtchn_close(dom->xce_handle);
> -			dom->xce_handle = NULL;
> -			dom->local_port = -1;
> -			dom->remote_port = -1;
> -			goto out;
> -		}
>  	}
>  
> -	if (log_guest && (dom->log_fd == -1))
> -		dom->log_fd = create_domain_log(dom);
> -
> - out:
>  	return err;
>  }
>  
> @@ -630,27 +842,66 @@ static bool watch_domain(struct domain *dom, bool watch)
>  {
>  	char domid_str[3 + MAX_STRLEN(dom->domid)];
>  	bool success;
> +	char *path = dom->console[0].xspath;
>  
>  	snprintf(domid_str, sizeof(domid_str), "dom%u", dom->domid);
>  	if (watch) {
> -		success = xs_watch(xs, dom->conspath, domid_str);
> +		success = xs_watch(xs, path, domid_str);
>  		if (success)
>  			domain_create_ring(dom);
>  		else
> -			xs_unwatch(xs, dom->conspath, domid_str);
> +			xs_unwatch(xs, path, domid_str);
>  	} else {
> -		success = xs_unwatch(xs, dom->conspath, domid_str);
> +		success = xs_unwatch(xs, path, domid_str);
>  	}
>  
>  	return success;
>  }
>  
> +static int console_init(struct console *con, struct domain *dom, void **data)
> +{
> +	char *s;
> +	int err = -1;
> +	struct console_data **con_data = (struct console_data **)data;
> +
> +	con->master_fd = -1;
> +	con->master_pollfd_idx = -1;
> +	con->slave_fd = -1;
> +	con->log_fd = -1;
> +	con->ring_ref = -1;
> +	con->local_port = -1;
> +	con->remote_port = -1;
> +	con->d = dom;
> +	con->ttyname = (*con_data)->ttyname;
> +	con->xsname = (*con_data)->xsname;
> +	con->map_ring_ref = (*con_data)->mapfunc;
> +	con->mandatory = (*con_data)->mandatory;
> +	con->xspath = xs_get_domain_path(xs, dom->domid);
> +	s = realloc(con->xspath, strlen(con->xspath) +
> +				strlen(con->xsname) + 1);
> +
> +	(*con_data)++;
> +
> +	if (s)
> +	{
> +		con->xspath = s;
> +		strcat(con->xspath, con->xsname);
> +		err = 0;
> +	}
> +	return err;
> +}
> +
> +static void console_free(struct console *con)
> +{
> +	if (con->xspath)
> +		free(con->xspath);
> +}
>  
>  static struct domain *create_domain(int domid)
>  {
>  	struct domain *dom;
> -	char *s;
>  	struct timespec ts;
> +	struct console_data *con_data = &console_data[0];
>  
>  	if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {
>  		dolog(LOG_ERR, "Cannot get time of day %s:%s:L%d",
> @@ -667,26 +918,13 @@ static struct domain *create_domain(int domid)
>  
>  	dom->domid = domid;
>  
> -	dom->conspath = xs_get_domain_path(xs, dom->domid);
> -	s = realloc(dom->conspath, strlen(dom->conspath) +
> -		    strlen("/console") + 1);
> -	if (s == NULL)
> +	if (console_iter_int_arg3(dom, console_init, (void **)&con_data))
>  		goto out;
> -	dom->conspath = s;
> -	strcat(dom->conspath, "/console");
>  
> -	dom->master_fd = -1;
> -	dom->master_pollfd_idx = -1;
> -	dom->slave_fd = -1;
> -	dom->log_fd = -1;
>  	dom->xce_pollfd_idx = -1;
>  
>  	dom->next_period = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec / 1000000) + RATE_LIMIT_PERIOD;
>  
> -	dom->ring_ref = -1;
> -	dom->local_port = -1;
> -	dom->remote_port = -1;
> -
>  	if (!watch_domain(dom, true))
>  		goto out;
>  
> @@ -696,8 +934,10 @@ static struct domain *create_domain(int domid)
>  	dolog(LOG_DEBUG, "New domain %d", domid);
>  
>  	return dom;
> +
>   out:
> -	free(dom->conspath);
> +	console_iter_void_arg1(dom, console_free);
> +
>  	free(dom);
>  	return NULL;
>  }
> @@ -727,20 +967,24 @@ static void remove_domain(struct domain *dom)
>  	}
>  }
>  
> -static void cleanup_domain(struct domain *d)
> -{
> -	domain_close_tty(d);
>  
> -	if (d->log_fd != -1) {
> -		close(d->log_fd);
> -		d->log_fd = -1;
> +static void console_cleanup(struct console *con)
> +{
> +	if (con->log_fd != -1) {
> +		close(con->log_fd);
> +		con->log_fd = -1;
>  	}
> +	free(con->buffer.data);
> +	con->buffer.data = NULL;
> +	free(con->xspath);
> +	con->xspath = NULL;
> +}
>  
> -	free(d->buffer.data);
> -	d->buffer.data = NULL;
> +static void cleanup_domain(struct domain *d)
> +{
> +	domain_close_tty(d);
>  
> -	free(d->conspath);
> -	d->conspath = NULL;
> +	console_iter_void_arg1(d, console_cleanup);
>  
>  	remove_domain(d);
>  }
> @@ -780,9 +1024,9 @@ static void enum_domains(void)
>  	}
>  }
>  
> -static int ring_free_bytes(struct domain *dom)
> +static int ring_free_bytes(struct console *con)
>  {
> -	struct xencons_interface *intf = dom->interface;
> +	struct xencons_interface *intf = con->interface;
>  	XENCONS_RING_IDX cons, prod, space;
>  
>  	cons = intf->in_cons;
> @@ -807,25 +1051,27 @@ static void domain_handle_broken_tty(struct domain *dom, int recreate)
>  	}
>  }
>  
> -static void handle_tty_read(struct domain *dom)
> +static void handle_tty_read(struct console *con)
>  {
>  	ssize_t len = 0;
>  	char msg[80];
>  	int i;
> -	struct xencons_interface *intf = dom->interface;
>  	XENCONS_RING_IDX prod;
> +	struct xencons_interface *intf = con->interface;
> +	xenevtchn_port_or_error_t port = con->local_port;
> +	struct domain *dom = con->d;
>  
>  	if (dom->is_dead)
>  		return;
>  
> -	len = ring_free_bytes(dom);
> +	len = ring_free_bytes(con);
>  	if (len == 0)
>  		return;
>  
>  	if (len > sizeof(msg))
>  		len = sizeof(msg);
>  
> -	len = read(dom->master_fd, msg, len);
> +	len = read(con->master_fd, msg, len);
>  	/*
>  	 * Note: on Solaris, len == 0 means the slave closed, and this
>  	 * is no problem, but Linux can't handle this usefully, so we
> @@ -841,31 +1087,39 @@ static void handle_tty_read(struct domain *dom)
>  		}
>  		xen_wmb();
>  		intf->in_prod = prod;
> -		xenevtchn_notify(dom->xce_handle, dom->local_port);
> +		xenevtchn_notify(dom->xce_handle, port);
>  	} else {
>  		domain_close_tty(dom);
>  		shutdown_domain(dom);
>  	}
>  }
>  
> -static void handle_tty_write(struct domain *dom)
> +static void handle_tty_write(struct console *con)
>  {
>  	ssize_t len;
> +	struct domain *dom = con->d;
>  
>  	if (dom->is_dead)
>  		return;
>  
> -	len = write(dom->master_fd, dom->buffer.data + dom->buffer.consumed,
> -		    dom->buffer.size - dom->buffer.consumed);
> +	len = write(con->master_fd,
> +				con->buffer.data + con->buffer.consumed,
> +				con->buffer.size - con->buffer.consumed);
>   	if (len < 1) {
>  		dolog(LOG_DEBUG, "Write failed on domain %d: %zd, %d\n",
>  		      dom->domid, len, errno);
>  		domain_handle_broken_tty(dom, domain_is_valid(dom->domid));
>  	} else {
> -		buffer_advance(&dom->buffer, len);
> +		buffer_advance(&con->buffer, len);
>  	}
>  }
>  
> +static void console_event_unmask(struct console *con)
> +{
> +	if (con->local_port != -1)
> +		(void)xenevtchn_unmask(con->d->xce_handle, con->local_port);
> +}
> +
>  static void handle_ring_read(struct domain *dom)
>  {
>  	xenevtchn_port_or_error_t port;
> @@ -878,10 +1132,10 @@ static void handle_ring_read(struct domain *dom)
>  
>  	dom->event_count++;
>  
> -	buffer_append(dom);
> +	console_iter_void_arg2(dom, buffer_append, port);
>  
>  	if (dom->event_count < RATE_LIMIT_ALLOWANCE)
> -		(void)xenevtchn_unmask(dom->xce_handle, port);
> +		console_iter_void_arg1(dom, console_event_unmask);
>  }
>  
>  static void handle_xs(void)
> @@ -943,14 +1197,22 @@ static void handle_hv_logs(xenevtchn_handle *xce_handle, bool force)
>  		(void)xenevtchn_unmask(xce_handle, port);
>  }
>  
> +static void console_open_log(struct console *con)
> +{
> +	if (console_enabled(con))
> +	{
> +		if (con->log_fd != -1)
> +			close(con->log_fd);
> +		con->log_fd = create_console_log(con);
> +	}
> +}
> +
>  static void handle_log_reload(void)
>  {
>  	if (log_guest) {
>  		struct domain *d;
>  		for (d = dom_head; d; d = d->next) {
> -			if (d->log_fd != -1)
> -				close(d->log_fd);
> -			d->log_fd = create_domain_log(d);
> +			console_iter_void_arg1(d, console_open_log);
>  		}
>  	}
>  
> @@ -1002,6 +1264,40 @@ static void reset_fds(void)
>  		memset(fds, 0, sizeof(struct pollfd) * current_array_size);
>  }
>  
> +static void add_console_fd(struct console *con)
> +{
> +	if (con->master_fd != -1) {
> +		short events = 0;
> +		if (!con->d->is_dead && ring_free_bytes(con))
> +			events |= POLLIN;
> +
> +		if (!buffer_empty(&con->buffer))
> +			events |= POLLOUT;
> +
> +		if (events)
> +			con->master_pollfd_idx =
> +				set_fds(con->master_fd, events|POLLPRI);
> +	}
> +}
> +
> +static void process_console(struct console *con)
> +{
> +	if (con->master_fd != -1 && con->master_pollfd_idx != -1) {
> +		if (fds[con->master_pollfd_idx].revents &
> +			~(POLLIN|POLLOUT|POLLPRI))
> +			domain_handle_broken_tty(con->d, domain_is_valid(con->d->domid));
> +		else {
> +			if (fds[con->master_pollfd_idx].revents &
> +				POLLIN)
> +				handle_tty_read(con);
> +			if (fds[con->master_pollfd_idx].revents &
> +				POLLOUT)
> +				handle_tty_write(con);
> +		}
> +	}
> +	con->master_pollfd_idx = -1;
> +}
> +
>  void handle_io(void)
>  {
>  	int ret;
> @@ -1068,7 +1364,7 @@ void handle_io(void)
>  			if ((now+5) > d->next_period) {
>  				d->next_period = now + RATE_LIMIT_PERIOD;
>  				if (d->event_count >= RATE_LIMIT_ALLOWANCE) {
> -					(void)xenevtchn_unmask(d->xce_handle, d->local_port);
> +					console_iter_void_arg1(d, console_event_unmask);
>  				}
>  				d->event_count = 0;
>  			}
> @@ -1081,28 +1377,15 @@ void handle_io(void)
>  				    d->next_period < next_timeout)
>  					next_timeout = d->next_period;
>  			} else if (d->xce_handle != NULL) {
> -				if (discard_overflowed_data ||
> -				    !d->buffer.max_capacity ||
> -				    d->buffer.size < d->buffer.max_capacity) {
> +				if (console_iter_bool_arg1(d, buffer_available))
> +				{
>  					int evtchn_fd = xenevtchn_fd(d->xce_handle);
>  					d->xce_pollfd_idx = set_fds(evtchn_fd,
> -								    POLLIN|POLLPRI);
> +												POLLIN|POLLPRI);

spurious change


>  				}
>  			}
>  
> -			if (d->master_fd != -1) {
> -				short events = 0;
> -				if (!d->is_dead && ring_free_bytes(d))
> -					events |= POLLIN;
> -
> -				if (!buffer_empty(&d->buffer))
> -					events |= POLLOUT;
> -
> -				if (events)
> -					d->master_pollfd_idx =
> -						set_fds(d->master_fd,
> -							events|POLLPRI);
> -			}
> +			console_iter_void_arg1(d, add_console_fd);
>  		}
>  
>  		/* If any domain has been rate limited, we need to work
> @@ -1170,22 +1453,9 @@ void handle_io(void)
>  				    handle_ring_read(d);
>  			}
>  
> -			if (d->master_fd != -1 && d->master_pollfd_idx != -1) {
> -				if (fds[d->master_pollfd_idx].revents &
> -				    ~(POLLIN|POLLOUT|POLLPRI))
> -					domain_handle_broken_tty(d,
> -						   domain_is_valid(d->domid));
> -				else {
> -					if (fds[d->master_pollfd_idx].revents &
> -					    POLLIN)
> -						handle_tty_read(d);
> -					if (fds[d->master_pollfd_idx].revents &
> -					    POLLOUT)
> -						handle_tty_write(d);
> -				}
> -			}
> +			console_iter_void_arg1(d, process_console);
>  
> -			d->xce_pollfd_idx = d->master_pollfd_idx = -1;
> +			d->xce_pollfd_idx = -1;
>  
>  			if (d->last_seen != enum_pass)
>  				shutdown_domain(d);

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 09/12 v3] xen/arm: vpl011: Add support for vuart in xenconsole
  2017-05-10 14:35 ` [PATCH 09/12 v3] xen/arm: vpl011: Add support for vuart in xenconsole Bhupinder Thakur
  2017-05-11 11:17   ` Wei Liu
@ 2017-05-16 23:44   ` Stefano Stabellini
  2017-05-22 14:37     ` Julien Grall
  1 sibling, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2017-05-16 23:44 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Wei Liu, Ian Jackson

On Wed, 10 May 2017, Bhupinder Thakur wrote:
> Xenconsole supports only PV console currently. This patch adds support
> for vuart console, which allows emulated pl011 UART to be accessed
> as a console.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> 
> One review comment was to keep the vuart code under CONFIG_ARM64 && CONFIG_ACPI flags.
> This code review could not be incorporated as I could not find out the appropriate flags
> unders which this code can be kept. Are the CONFIG* flags exported to xenconsole?

Not sure about ACPI, but CONFIG_ARM_64 definitely should be.

This patch looks good, however I don't feel confident giving my acked-by
until I can review properly the previous one.


>  tools/console/daemon/io.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 9bb14de..19a2f35 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -115,6 +115,7 @@ struct console_data {
>  };
>  
>  static int map_pvcon_ring_ref(struct console *, int );
> +static int map_vuartcon_ring_ref(struct console *, int );
>  
>  static struct console_data console_data[] = {
>  
> @@ -124,6 +125,12 @@ static struct console_data console_data[] = {
>  		.mapfunc = map_pvcon_ring_ref,
>  		.mandatory = true
>  	},
> +	{
> +		.xsname = "/vuart/0",
> +		.ttyname = "tty",
> +		.mapfunc = map_vuartcon_ring_ref,
> +		.mandatory = false
> +	}
>  };
>  
>  #define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
> @@ -751,6 +758,28 @@ out:
>  	return err;
>  }
>  
> +static int map_vuartcon_ring_ref(struct console *con, int ring_ref)
> +{
> +	int err = 0;
> +	struct domain *dom = con->d;
> +
> +	if (!con->interface) {
> +		con->interface = xc_map_foreign_range(xc,
> +											  dom->domid,
> +											  XC_PAGE_SIZE,
> +											  PROT_READ|PROT_WRITE,
> +											  (unsigned long)ring_ref);
> +		if (con->interface == NULL) {
> +			err = EINVAL;
> +			goto out;
> +		}
> +		con->ring_ref = ring_ref;
> +	}
> +
> +out:
> +	return err;
> +}
> +
>  static int console_create_ring(struct console *con)
>  {
>  	int err, remote_port, ring_ref;
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore
  2017-05-16 15:18     ` Wei Liu
@ 2017-05-22 11:03       ` Bhupinder Thakur
  2017-05-30 11:58         ` Wei Liu
  0 siblings, 1 reply; 29+ messages in thread
From: Bhupinder Thakur @ 2017-05-22 11:03 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson

Hi Wei,


> [...]
>> >> @@ -151,13 +154,19 @@ retry_transaction:
>> >>      if (rc) goto out;
>> >>
>> >>      if (!libxl_only) {
>> >> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
>> >> -                                     frontend_path);
>> >> -        if (rc) goto out;
>> >> +        if (fents || ro_fents)
>> >> +        {
>> >> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
>> >> +                                         frontend_path);
>> >> +            if (rc) goto out;
>> >> +        }
>> >>
>> >> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
>> >> -                                     backend_path);
>> >> -        if (rc) goto out;
>> >> +        if (bents)
>> >> +        {
>> >> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
>> >> +                                         backend_path);
>> >> +            if (rc) goto out;
>> >> +        }
>> >
>> > What is this for?
>> >
>> > If there is no fe or be entries you skip the path creation altogether.
>> > But why? This doesn't seem to be related to your patch.
>> For vuart, I am adding only a front end node but the
>> libxl__device_generic_add() creates the backend path also,even though
>> there is no backend node. To remove that hanging be path, I added this
>> check.
>> >
>> > At least explain this a bit in the commit message?
>> I will add more details in the commit message.
>>
>
> Preferable it should be in a separate patch. That would make review
> easier.
>
> But there is another question: how do you know if Dom0 is servicing a
> DomU? How do you construct a libxl__device struct should you want to
> manipulate vuart?
>
Can you please elaborate more on this question? I am adding the vuart
console node at the same place where the PV console node is added. I
believe, the check that I have added should be a generic check valid
for any device creation.

Regards,
Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 09/12 v3] xen/arm: vpl011: Add support for vuart in xenconsole
  2017-05-16 23:44   ` Stefano Stabellini
@ 2017-05-22 14:37     ` Julien Grall
  0 siblings, 0 replies; 29+ messages in thread
From: Julien Grall @ 2017-05-22 14:37 UTC (permalink / raw)
  To: Stefano Stabellini, Bhupinder Thakur; +Cc: xen-devel, Wei Liu, Ian Jackson

Hi,

On 17/05/17 00:44, Stefano Stabellini wrote:
> On Wed, 10 May 2017, Bhupinder Thakur wrote:
>> Xenconsole supports only PV console currently. This patch adds support
>> for vuart console, which allows emulated pl011 UART to be accessed
>> as a console.
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> ---
>>
>> One review comment was to keep the vuart code under CONFIG_ARM64 && CONFIG_ACPI flags.
>> This code review could not be incorporated as I could not find out the appropriate flags
>> unders which this code can be kept. Are the CONFIG* flags exported to xenconsole?
>
> Not sure about ACPI, but CONFIG_ARM_64 definitely should be.

Why only CONFIG_ARM_64? I don't see any restriction preventing to be 
used for ARM32 bit also.

If we decide to limit to ARM_64 here, then we should do the same on the 
hypervisor side.

But I am wondering whether we should have a configure option for that as 
the hypervisor may not support pl011 (the user is allowed to disable it).

Cheers,

>
> This patch looks good, however I don't feel confident giving my acked-by
> until I can review properly the previous one.
>
>
>>  tools/console/daemon/io.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
>> index 9bb14de..19a2f35 100644
>> --- a/tools/console/daemon/io.c
>> +++ b/tools/console/daemon/io.c
>> @@ -115,6 +115,7 @@ struct console_data {
>>  };
>>
>>  static int map_pvcon_ring_ref(struct console *, int );
>> +static int map_vuartcon_ring_ref(struct console *, int );
>>
>>  static struct console_data console_data[] = {
>>
>> @@ -124,6 +125,12 @@ static struct console_data console_data[] = {
>>  		.mapfunc = map_pvcon_ring_ref,
>>  		.mandatory = true
>>  	},
>> +	{
>> +		.xsname = "/vuart/0",
>> +		.ttyname = "tty",
>> +		.mapfunc = map_vuartcon_ring_ref,
>> +		.mandatory = false
>> +	}
>>  };
>>
>>  #define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
>> @@ -751,6 +758,28 @@ out:
>>  	return err;
>>  }
>>
>> +static int map_vuartcon_ring_ref(struct console *con, int ring_ref)
>> +{
>> +	int err = 0;
>> +	struct domain *dom = con->d;
>> +
>> +	if (!con->interface) {
>> +		con->interface = xc_map_foreign_range(xc,
>> +											  dom->domid,
>> +											  XC_PAGE_SIZE,
>> +											  PROT_READ|PROT_WRITE,
>> +											  (unsigned long)ring_ref);
>> +		if (con->interface == NULL) {
>> +			err = EINVAL;
>> +			goto out;
>> +		}
>> +		con->ring_ref = ring_ref;
>> +	}
>> +
>> +out:
>> +	return err;
>> +}
>> +
>>  static int console_create_ring(struct console *con)
>>  {
>>  	int err, remote_port, ring_ref;
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 11/12 v3] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree
  2017-05-10 14:35 ` [PATCH 11/12 v3] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree Bhupinder Thakur
  2017-05-11 11:18   ` Wei Liu
  2017-05-16 23:05   ` Stefano Stabellini
@ 2017-05-22 14:42   ` Julien Grall
  2 siblings, 0 replies; 29+ messages in thread
From: Julien Grall @ 2017-05-22 14:42 UTC (permalink / raw)
  To: Bhupinder Thakur, xen-devel; +Cc: Ian Jackson, Wei Liu

Hi Bhupinder,

On 10/05/17 15:35, Bhupinder Thakur wrote:
> The SBSA uart node format is as specified in
> Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt and given below:
>
> ARM SBSA defined generic UART
> ------------------------------
> This UART uses a subset of the PL011 registers and consequently lives
> in the PL011 driver. It's baudrate and other communication parameters
> cannot be adjusted at runtime, so it lacks a clock specifier here.
>
> Required properties:
> - compatible: must be "arm,sbsa-uart"
> - reg: exactly one register range
> - interrupts: exactly one interrupt specifier
> - current-speed: the (fixed) baud rate set by the firmware
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>
> Changes since v2:
> - Currently device discovery using ACPI is not supported.
> - Dropped the reviewed-by tag by Stefano as there were some IRQ related changes
>   done later.
>
>  tools/libxl/libxl_arm.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d842d88..f88ef0d 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -44,10 +44,23 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>      uint32_t nr_spis = 0;
>      unsigned int i;
>
> +    /*
> +     * If pl011 vuart is enabled then increment the nr_spis to allow allocation
> +     * of SPI VIRQ for pl011.
> +     */
> +    if (d_config->b_info.vuart)
> +        nr_spis += (GUEST_VPL011_SPI - 32) + 1;
> +
>      for (i = 0; i < d_config->b_info.num_irqs; i++) {
>          uint32_t irq = d_config->b_info.irqs[i];
>          uint32_t spi;
>
> +        if (d_config->b_info.vuart && (irq == GUEST_VPL011_SPI))
> +        {
> +            LOG(ERROR, "Physical IRQ %u conflicting with pl011 SPI\n", irq);
> +            return ERROR_FAIL;
> +        }
> +
>          if (irq < 32)
>              continue;
>
> @@ -130,9 +143,10 @@ static struct arch_info {
>      const char *guest_type;
>      const char *timer_compat;
>      const char *cpu_compat;
> +    const char *uart_compat;
>  } arch_info[] = {
> -    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15" },
> -    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
> +    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" },
> +    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" },
>  };
>
>  /*
> @@ -590,6 +604,38 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>
> +static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
> +                                 const struct arch_info *ainfo,
> +                                 struct xc_dom_image *dom)
> +{
> +    int res;
> +    gic_interrupt intr;
> +
> +    res = fdt_begin_node(fdt, "sbsa-pl011");
> +    if (res) return res;
> +
> +    res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat);
> +    if (res) return res;
> +
> +    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
> +                            1,
> +                            GUEST_PL011_BASE, GUEST_PL011_SIZE);
> +    if (res) return res;
> +
> +    set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> +
> +    res = fdt_property_interrupts(gc, fdt, &intr, 1);
> +    if (res) return res;
> +
> +    /* Use a default baud rate of 115200. */
> +    fdt_property_u32(fdt, "current-speed", 115200);

Again, please explain in the commit message why 115200. How this is 
going to affect the driver?

> +
> +    res = fdt_end_node(fdt);
> +    if (res) return res;
> +
> +    return 0;
> +}
> +
>  static const struct arch_info *get_arch_info(libxl__gc *gc,
>                                               const struct xc_dom_image *dom)
>  {
> @@ -889,6 +935,9 @@ next_resize:
>          FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
>          FDT( make_hypervisor_node(gc, fdt, vers) );
>
> +        if (info->vuart)
> +            FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
> +
>          if (pfdt)
>              FDT( copy_partial_fdt(gc, fdt, pfdt) );
>
>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 08/12 v3] xen/arm: vpl011: Modify xenconsole to support multiple consoles
  2017-05-16 23:41   ` Stefano Stabellini
@ 2017-05-25 10:57     ` Bhupinder Thakur
  2017-05-25 18:24       ` Stefano Stabellini
  0 siblings, 1 reply; 29+ messages in thread
From: Bhupinder Thakur @ 2017-05-25 10:57 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, Wei Liu, Ian Jackson

Hi,

On 17 May 2017 at 05:11, Stefano Stabellini <sstabellini@kernel.org> wrote:
> On Wed, 10 May 2017, Bhupinder Thakur wrote:
>> Xenconsole supports only PV console currently. This patch adds support
>> for supporting multiple consoles.
>>
>> This patch modifies different data structures and APIs used
>> in xenconsole to support multiple consoles.
>>
>> Change summary:
>>
>> 1. Split the domain structure into a console structure and the
>>    domain structure, where each console structure represents one
>>    console.
>>
>> 2. Modify different APIs such as buffer_append() etc. to take
>>    console structure as input and perform per console specific
>>    operations.
>>
>> 3. Define a generic console_create_ring(), which sets up the
>>    ring buffer and event channel for each console.
>>
>> 3. Modify domain_create_ring() to use console_create_ring().
>>
>> 4. Modifications in handle_ring_read() to read ring buffer data
>>    from multiple consoles.
>>
>> 5. Add log file support for multiple consoles.
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>
> There is something wrong with this patch: I cannot apply it.
>
> Also, it is still way to big for me to review. I cannot track all the
> changes and figure out if they are correct.
>
> One option is to introduce struct console in one patch, with only one
> struct console per domain. Then the second patch could introduce
> multiple struct console with the helpers such as console_iter_void_arg1.
>
> Finally the third patch could add vuart support.
>
I have divided the changes into 4 patches:

patch#1: This patch introduces the console structure and modifies the
code to use the new console structure.

patch#2: This patch modifies the functions to take console structure
as input instead of domain structure. Also it renames the console
specific functions to start with "console_" prefix instead of
"domain_" prefix. For example - domain_create_tty() is renamed to
console_create_tty().

patch#3: This patch adds the support for multiple consoles and
introduces the iterator functions to operate on multiple consoles.

patch#4: Finally this patch adds support for a new vuart console.

>> -static int create_domain_log(struct domain *dom)
>> +static int create_console_log(struct console *con)
>>  {
>>       char logfile[PATH_MAX];
>>       char *namepath, *data, *s;
>>       int fd;
>>       unsigned int len;
>> +     struct domain *dom = con->d;
>>
>>       namepath = xs_get_domain_path(xs, dom->domid);
>>       s = realloc(namepath, strlen(namepath) + 6);
>> @@ -314,7 +443,9 @@ static int create_domain_log(struct domain *dom)
>>               return -1;
>>       }
>>
>> -     snprintf(logfile, PATH_MAX-1, "%s/guest-%s.log", log_dir, data);
>> +     snprintf(logfile, PATH_MAX-1, "%s%s/guest-%s.log",
>> +                      log_dir, con->xspath, data);
>
> This changes the log directory, right? Are the new directories created
> correctly by the install scripts?
I will correct this. There should be no change in the path for PV
console log. I think by default guest logging is disabled. How can I
enable the logging to test it? I believe some option needs to be
passed while spawning xenconsoled?

Regards,
Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 08/12 v3] xen/arm: vpl011: Modify xenconsole to support multiple consoles
  2017-05-25 10:57     ` Bhupinder Thakur
@ 2017-05-25 18:24       ` Stefano Stabellini
  0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2017-05-25 18:24 UTC (permalink / raw)
  To: Bhupinder Thakur; +Cc: xen-devel, Stefano Stabellini, Wei Liu, Ian Jackson

On Thu, 25 May 2017, Bhupinder Thakur wrote:
> Hi,
> 
> On 17 May 2017 at 05:11, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > On Wed, 10 May 2017, Bhupinder Thakur wrote:
> >> Xenconsole supports only PV console currently. This patch adds support
> >> for supporting multiple consoles.
> >>
> >> This patch modifies different data structures and APIs used
> >> in xenconsole to support multiple consoles.
> >>
> >> Change summary:
> >>
> >> 1. Split the domain structure into a console structure and the
> >>    domain structure, where each console structure represents one
> >>    console.
> >>
> >> 2. Modify different APIs such as buffer_append() etc. to take
> >>    console structure as input and perform per console specific
> >>    operations.
> >>
> >> 3. Define a generic console_create_ring(), which sets up the
> >>    ring buffer and event channel for each console.
> >>
> >> 3. Modify domain_create_ring() to use console_create_ring().
> >>
> >> 4. Modifications in handle_ring_read() to read ring buffer data
> >>    from multiple consoles.
> >>
> >> 5. Add log file support for multiple consoles.
> >>
> >> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> >
> > There is something wrong with this patch: I cannot apply it.
> >
> > Also, it is still way to big for me to review. I cannot track all the
> > changes and figure out if they are correct.
> >
> > One option is to introduce struct console in one patch, with only one
> > struct console per domain. Then the second patch could introduce
> > multiple struct console with the helpers such as console_iter_void_arg1.
> >
> > Finally the third patch could add vuart support.
> >
> I have divided the changes into 4 patches:
> 
> patch#1: This patch introduces the console structure and modifies the
> code to use the new console structure.
> 
> patch#2: This patch modifies the functions to take console structure
> as input instead of domain structure. Also it renames the console
> specific functions to start with "console_" prefix instead of
> "domain_" prefix. For example - domain_create_tty() is renamed to
> console_create_tty().
> 
> patch#3: This patch adds the support for multiple consoles and
> introduces the iterator functions to operate on multiple consoles.
> 
> patch#4: Finally this patch adds support for a new vuart console.

Thank you, it looks better on paper


> >> -static int create_domain_log(struct domain *dom)
> >> +static int create_console_log(struct console *con)
> >>  {
> >>       char logfile[PATH_MAX];
> >>       char *namepath, *data, *s;
> >>       int fd;
> >>       unsigned int len;
> >> +     struct domain *dom = con->d;
> >>
> >>       namepath = xs_get_domain_path(xs, dom->domid);
> >>       s = realloc(namepath, strlen(namepath) + 6);
> >> @@ -314,7 +443,9 @@ static int create_domain_log(struct domain *dom)
> >>               return -1;
> >>       }
> >>
> >> -     snprintf(logfile, PATH_MAX-1, "%s/guest-%s.log", log_dir, data);
> >> +     snprintf(logfile, PATH_MAX-1, "%s%s/guest-%s.log",
> >> +                      log_dir, con->xspath, data);
> >
> > This changes the log directory, right? Are the new directories created
> > correctly by the install scripts?
> I will correct this. There should be no change in the path for PV
> console log. I think by default guest logging is disabled. How can I
> enable the logging to test it? I believe some option needs to be
> passed while spawning xenconsoled?

Yes, it looks like it's the -l option, see:

		case 'l':
		        if (!strcmp(optarg, "all")) {
			      log_hv = 1;
			      log_guest = 1;
			} else if (!strcmp(optarg, "hv")) {
			      log_hv = 1;
			} else if (!strcmp(optarg, "guest")) {
			      log_guest = 1;
			}

in main.c

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore
  2017-05-22 11:03       ` Bhupinder Thakur
@ 2017-05-30 11:58         ` Wei Liu
  2017-06-01  8:43           ` Bhupinder Thakur
  0 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2017-05-30 11:58 UTC (permalink / raw)
  To: Bhupinder Thakur
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Wei Liu, Ian Jackson

On Mon, May 22, 2017 at 04:33:15PM +0530, Bhupinder Thakur wrote:
> Hi Wei,
> 
> 
> > [...]
> >> >> @@ -151,13 +154,19 @@ retry_transaction:
> >> >>      if (rc) goto out;
> >> >>
> >> >>      if (!libxl_only) {
> >> >> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
> >> >> -                                     frontend_path);
> >> >> -        if (rc) goto out;
> >> >> +        if (fents || ro_fents)
> >> >> +        {
> >> >> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
> >> >> +                                         frontend_path);
> >> >> +            if (rc) goto out;
> >> >> +        }
> >> >>
> >> >> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
> >> >> -                                     backend_path);
> >> >> -        if (rc) goto out;
> >> >> +        if (bents)
> >> >> +        {
> >> >> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
> >> >> +                                         backend_path);
> >> >> +            if (rc) goto out;
> >> >> +        }
> >> >
> >> > What is this for?
> >> >
> >> > If there is no fe or be entries you skip the path creation altogether.
> >> > But why? This doesn't seem to be related to your patch.
> >> For vuart, I am adding only a front end node but the
> >> libxl__device_generic_add() creates the backend path also,even though
> >> there is no backend node. To remove that hanging be path, I added this
> >> check.
> >> >
> >> > At least explain this a bit in the commit message?
> >> I will add more details in the commit message.
> >>
> >
> > Preferable it should be in a separate patch. That would make review
> > easier.
> >
> > But there is another question: how do you know if Dom0 is servicing a
> > DomU? How do you construct a libxl__device struct should you want to
> > manipulate vuart?
> >
> Can you please elaborate more on this question? I am adding the vuart
> console node at the same place where the PV console node is added. I
> believe, the check that I have added should be a generic check valid
> for any device creation.
> 

Sorry if that question confuses you. Let me try to rephrase.

Nowadays you can use xl block-list / network-list etc do list respective
devices for a DomU. The underlying libxl functions normally rely on
xenstore for device information.

Now AIUI (but I could be wrong!) you don't have an backend entry for
vuart in xenstore. In a hypothetical situation, we want to add a xl
console-list to list all consoles for a DomU, where should libxl get
relevant information from?

Wei.

> Regards,
> Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore
  2017-05-30 11:58         ` Wei Liu
@ 2017-06-01  8:43           ` Bhupinder Thakur
  0 siblings, 0 replies; 29+ messages in thread
From: Bhupinder Thakur @ 2017-06-01  8:43 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson

Hi Wei,

Thanks for your explanation.


>> >> >> @@ -151,13 +154,19 @@ retry_transaction:
>> >> >>      if (rc) goto out;
>> >> >>
>> >> >>      if (!libxl_only) {
>> >> >> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
>> >> >> -                                     frontend_path);
>> >> >> -        if (rc) goto out;
>> >> >> +        if (fents || ro_fents)
>> >> >> +        {
>> >> >> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
>> >> >> +                                         frontend_path);
>> >> >> +            if (rc) goto out;
>> >> >> +        }
>> >> >>
>> >> >> -        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
>> >> >> -                                     backend_path);
>> >> >> -        if (rc) goto out;
>> >> >> +        if (bents)
>> >> >> +        {
>> >> >> +            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
>> >> >> +                                         backend_path);
>> >> >> +            if (rc) goto out;
>> >> >> +        }
>> >> >
>> >> > What is this for?
>> >> >
>> >> > If there is no fe or be entries you skip the path creation altogether.
>> >> > But why? This doesn't seem to be related to your patch.
>> >> For vuart, I am adding only a front end node but the
>> >> libxl__device_generic_add() creates the backend path also,even though
>> >> there is no backend node. To remove that hanging be path, I added this
>> >> check.
>> >> >
>> >> > At least explain this a bit in the commit message?
>> >> I will add more details in the commit message.
>> >>
>> >
>> > Preferable it should be in a separate patch. That would make review
>> > easier.
>> >
>> > But there is another question: how do you know if Dom0 is servicing a
>> > DomU? How do you construct a libxl__device struct should you want to
>> > manipulate vuart?
>> >
>> Can you please elaborate more on this question? I am adding the vuart
>> console node at the same place where the PV console node is added. I
>> believe, the check that I have added should be a generic check valid
>> for any device creation.
>>
>
> Sorry if that question confuses you. Let me try to rephrase.
>
> Nowadays you can use xl block-list / network-list etc do list respective
> devices for a DomU. The underlying libxl functions normally rely on
> xenstore for device information.
>
> Now AIUI (but I could be wrong!) you don't have an backend entry for
> vuart in xenstore. In a hypothetical situation, we want to add a xl
> console-list to list all consoles for a DomU, where should libxl get
> relevant information from?
>
I did not create a backend entry because I thought domU won't need it.
But as you have mentioned, there are other consumers of the backend
information (like xl) so I will add the backend information for vuart
in xenstore.

In that case, the checks that I have added in
libxl__device_generic_add()  are not required because there is no use
case where a device is created with backend or frontend as null.

Regards,
Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2017-06-01  8:43 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-10 14:35 [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Bhupinder Thakur
2017-05-10 14:35 ` [PATCH 07/12 v3] xen/arm: vpl011: Allocate a new GFN in the toolstack for vuart Bhupinder Thakur
2017-05-11 11:10   ` Wei Liu
2017-05-16 22:59   ` Stefano Stabellini
2017-05-10 14:35 ` [PATCH 08/12 v3] xen/arm: vpl011: Modify xenconsole to support multiple consoles Bhupinder Thakur
2017-05-11 11:15   ` Wei Liu
2017-05-16 23:41   ` Stefano Stabellini
2017-05-25 10:57     ` Bhupinder Thakur
2017-05-25 18:24       ` Stefano Stabellini
2017-05-10 14:35 ` [PATCH 09/12 v3] xen/arm: vpl011: Add support for vuart in xenconsole Bhupinder Thakur
2017-05-11 11:17   ` Wei Liu
2017-05-16 23:44   ` Stefano Stabellini
2017-05-22 14:37     ` Julien Grall
2017-05-10 14:35 ` [PATCH 10/12 v3] xen/arm: vpl011: Add a new vuart console type to xenconsole client Bhupinder Thakur
2017-05-11 11:17   ` Wei Liu
2017-05-16 23:02   ` Stefano Stabellini
2017-05-10 14:35 ` [PATCH 11/12 v3] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree Bhupinder Thakur
2017-05-11 11:18   ` Wei Liu
2017-05-16 23:05   ` Stefano Stabellini
2017-05-22 14:42   ` Julien Grall
2017-05-10 14:35 ` [PATCH 12/12 v3] xen/arm: vpl011: Update documentation for vuart console support Bhupinder Thakur
2017-05-11 11:19   ` Wei Liu
2017-05-16 23:10   ` Stefano Stabellini
2017-05-11 11:10 ` [PATCH 06/12 v3] xen/arm: vpl011: Add a new vuart node in the xenstore Wei Liu
2017-05-12  9:32   ` Bhupinder Thakur
2017-05-16 15:18     ` Wei Liu
2017-05-22 11:03       ` Bhupinder Thakur
2017-05-30 11:58         ` Wei Liu
2017-06-01  8:43           ` Bhupinder Thakur

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.