thr3ads.net - Xen devel - [PATCH 0 of 5 v3] Tools: Console ring improvements [Mar 2013]

If this information is useful, please help other people find it:
Share via:

Andrew Cooper

2013-Mar-12 18:09 UTC

[PATCH 0 of 5 v3] Tools: Console ring improvements

Hello,

The following set of patches came about when trying to fix the 32K limitation
with stub_xc_readconsolering() in the ocaml bindings.

Patch 1 is a hypervisor patch adding a new SYSCTL hypercall to query the size
of the console ring, which is fixed but otherwise inaccessible after boot.

Patches 2 thru 5 are tools patches:
 * Patch 2 adds a libxc function to use the new SYSCTL hypercall.
 * Patch 3 implements an alternative xc_readconsolering() to prevent bounce
   buffering.
 * Patch 4 uses the preceding patches to fix the implementation of
   stub_xc_readconsolering() in the ocaml bindings.
 * Patch 5 is a misc threading fix in the ocaml bindings, discovered when
   trying to fix the console functionality.

There are minimal changes in v3.  The changes are mostly style tweaks, extra
comments and rebasing on top of unstable.

CC''d is xen-api@lists.xen.org as well, for comment on the ocaml
patches.

This patch set has been compile-tested on unstable, and functionally tested
via backport to 4.2 (which was only one whitespace fuzz issue and no code
change).

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Andrew Cooper

2013-Mar-12 18:09 UTC

head link

[PATCH 1 of 5 v3] common/sysctl: Introduce hypercall to query the console ring size

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

--
Changes since v2:
 * Rebase on top of coverage patches.
 * Reword hypercall comments.

diff -r a6b81234b189 -r 89f3c6846f6b xen/common/sysctl.c
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -358,6 +358,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
     }
     break;
 
+    case XEN_SYSCTL_consoleringsize:
+        ret = console_ring_size(&op->u.consoleringsize);
+        break;
+
 #ifdef TEST_COVERAGE
     case XEN_SYSCTL_coverage_op:
         ret = sysctl_coverage_op(&op->u.coverage_op);
diff -r a6b81234b189 -r 89f3c6846f6b xen/drivers/char/console.c
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -226,6 +226,12 @@ long read_console_ring(struct xen_sysctl
     return 0;
 }
 
+long console_ring_size(struct xen_sysctl_consoleringsize * op)
+{
+    op->size = conring_size;
+    return 0;
+}
+
 
 /*
  * *******************************************************
diff -r a6b81234b189 -r 89f3c6846f6b xen/include/public/sysctl.h
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -632,6 +632,14 @@ typedef struct xen_sysctl_coverage_op xe
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
 
 
+/* XEN_SYSCTL_consoleringsize */
+/* Get the size of the hypervisor console ring in bytes. */
+struct xen_sysctl_consoleringsize {
+    uint64_t size; /* OUT */
+};
+typedef struct xen_sysctl_consoleringsize xen_sysctl_consoleringsize_t;
+DEFINE_XEN_GUEST_HANDLE(xen_sysctl_consoleringsize_t);
+
 struct xen_sysctl {
     uint32_t cmd;
 #define XEN_SYSCTL_readconsole                    1
@@ -653,6 +661,7 @@ struct xen_sysctl {
 #define XEN_SYSCTL_cpupool_op                    18
 #define XEN_SYSCTL_scheduler_op                  19
 #define XEN_SYSCTL_coverage_op                   20
+#define XEN_SYSCTL_consoleringsize               21
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
@@ -674,6 +683,7 @@ struct xen_sysctl {
         struct xen_sysctl_cpupool_op        cpupool_op;
         struct xen_sysctl_scheduler_op      scheduler_op;
         struct xen_sysctl_coverage_op       coverage_op;
+        struct xen_sysctl_consoleringsize   consoleringsize;
         uint8_t                             pad[128];
     } u;
 };
diff -r a6b81234b189 -r 89f3c6846f6b xen/include/xen/console.h
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -12,6 +12,8 @@
 
 struct xen_sysctl_readconsole;
 long read_console_ring(struct xen_sysctl_readconsole *op);
+struct xen_sysctl_consoleringsize;
+long console_ring_size(struct xen_sysctl_consoleringsize *op);
 
 void console_init_preirq(void);
 void console_init_postirq(void);

Andrew Cooper

2013-Mar-12 18:09 UTC

head link

[PATCH 2 of 5 v3] tools/libxc: Helper function for XEN_SYSCTL_consoleringsize

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

---
Changes since v2:
 * Tweak style and errno in case of invalid pointer.

diff -r 89f3c6846f6b -r 0bd32c19873e tools/libxc/xc_misc.c
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -100,6 +100,26 @@ int xc_readconsolering(xc_interface *xch
     return ret;
 }
 
+int xc_consoleringsize(xc_interface *xch, uint64_t *psize)
+{
+    int ret = -1;
+    DECLARE_SYSCTL;
+
+    if ( !psize )
+    {
+        errno = EINVAL;
+        return ret;
+    }
+
+    sysctl.cmd = XEN_SYSCTL_consoleringsize;
+    ret = do_sysctl(xch, &sysctl);
+
+    if ( !ret )
+        *psize = sysctl.u.consoleringsize.size;
+
+    return ret;
+}
+
 int xc_send_debug_keys(xc_interface *xch, char *keys)
 {
     int ret, len = strlen(keys);
diff -r 89f3c6846f6b -r 0bd32c19873e tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -998,6 +998,7 @@ int xc_readconsolering(xc_interface *xch
                        char *buffer,
                        unsigned int *pnr_chars,
                        int clear, int incremental, uint32_t *pindex);
+int xc_consoleringsize(xc_interface *xch, uint64_t *psize);
 
 int xc_send_debug_keys(xc_interface *xch, char *keys);

Andrew Cooper

2013-Mar-12 18:09 UTC

head link

[PATCH 3 of 5 v3] tools/libxc: Implement xc_readconsolering_buffer()

Functions identically to xc_readconsolering(), but uses a user-provided
xc_hypercall_buffer_t to save using a bounce buffer.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

--
Changes since v2:
 * Document xc_readconsolering() and xc_readconsolering_buffer() functions
Changes since v1:
 * Reduce xc_readconsolering() to use xc_readconsolering_buffer()

diff -r 0bd32c19873e -r c7b82dfbec34 tools/libxc/xc_misc.c
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -70,13 +70,29 @@ int xc_readconsolering(xc_interface *xch
                        int clear, int incremental, uint32_t *pindex)
 {
     int ret;
-    unsigned int nr_chars = *pnr_chars;
-    DECLARE_SYSCTL;
-    DECLARE_HYPERCALL_BOUNCE(buffer, nr_chars, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_HYPERCALL_BOUNCE(buffer, *pnr_chars,
XC_HYPERCALL_BUFFER_BOUNCE_OUT);
 
     if ( xc_hypercall_bounce_pre(xch, buffer) )
         return -1;
 
+    ret = xc_readconsolering_buffer(xch, HYPERCALL_BUFFER(buffer),
+                                    pnr_chars, clear, incremental, pindex);
+
+    xc_hypercall_bounce_post(xch, buffer);
+
+    return ret;
+}
+
+int xc_readconsolering_buffer(xc_interface *xch,
+                              xc_hypercall_buffer_t *buffer,
+                              unsigned int *pnr_chars,
+                              int clear, int incremental, uint32_t *pindex)
+{
+    int ret;
+    unsigned int nr_chars = *pnr_chars;
+    DECLARE_SYSCTL;
+    DECLARE_HYPERCALL_BUFFER_ARGUMENT(buffer);
+
     sysctl.cmd = XEN_SYSCTL_readconsole;
     set_xen_guest_handle(sysctl.u.readconsole.buffer, buffer);
     sysctl.u.readconsole.count = nr_chars;
@@ -95,8 +111,6 @@ int xc_readconsolering(xc_interface *xch
             *pindex = sysctl.u.readconsole.index;
     }
 
-    xc_hypercall_bounce_post(xch, buffer);
-
     return ret;
 }
 
diff -r 0bd32c19873e -r c7b82dfbec34 tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -994,10 +994,26 @@ int xc_physdev_pci_access_modify(xc_inte
                                  int func,
                                  int enable);
 
+/*
+ * For both readconsolering functions, *pnr_chars is both an input and
+ * output.  As an input, it specifies the size of *buffer, and as an output
+ * indicates now many character Xen wrote into *buffer.
+ *
+ * The ''clear'' parameter indicates whether Xen should clear
the buffer or not.
+ * If incremental is set, *pindex is an input and output parameter to aid with
+ * sequential small reads of the console ring.
+ *
+ * xc_readconsolering_buffer is preferred to avoid an extra copy of the console
+ * ring buffer.
+ */
 int xc_readconsolering(xc_interface *xch,
                        char *buffer,
                        unsigned int *pnr_chars,
                        int clear, int incremental, uint32_t *pindex);
+int xc_readconsolering_buffer(xc_interface *xch,
+                              xc_hypercall_buffer_t *buffer,
+                              unsigned int *pnr_chars,
+                              int clear, int incremental, uint32_t *pindex);
 int xc_consoleringsize(xc_interface *xch, uint64_t *psize);
 
 int xc_send_debug_keys(xc_interface *xch, char *keys);

Andrew Cooper

2013-Mar-12 18:09 UTC

head link

[PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

There are two problems with this function:
  * The caml_{enter,leave}_blocking_section() calls mean that two threads can
    compete for use of the static ring[] buffer.
  * It fails to retrieve the entire buffer if the hypervisor has used 32K or
    more of its console ring.

Rewrite the function using the new xc_consoleringsize() and
xc_readconsolering_buffer() functions to efficiently grab the entire console
ring.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

--
Changes since v2:
 * Tweak style and remove redundant variables
Changes since v1:
 * Convert conring_size to being static to avoid needless hypercalls
 * Fix memory due to ordering of failwith_xc() and xc_hypercall_buffer_free()
 * Remove useless CAMLreturns()

diff -r c7b82dfbec34 -r 7a33b4fdb977 tools/ocaml/libs/xc/xenctrl_stubs.c
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -523,27 +523,47 @@ CAMLprim value stub_xc_evtchn_reset(valu
 	CAMLreturn(Val_unit);
 }
 
-
-#define RING_SIZE 32768
-static char ring[RING_SIZE];
-
 CAMLprim value stub_xc_readconsolering(value xch)
 {
-	unsigned int size = RING_SIZE - 1;
-	char *ring_ptr = ring;
+	static uint64_t conring_size = 0;
+	unsigned int nr_chars;
+	DECLARE_HYPERCALL_BUFFER(char, ring);
 	int retval;
 
 	CAMLparam1(xch);
+	CAMLlocal1(conring);
+
+	if (!conring_size)
+	{
+		if (xc_consoleringsize(_H(xch), &conring_size))
+			failwith_xc(_H(xch));
+
+		/* Round conring_size up to the next page, for NULL terminator
+		   and slop from a race with printk() in the hypervisor. */
+		conring_size = (conring_size + XC_PAGE_SIZE) & XC_PAGE_MASK;
+	}
+
+	nr_chars = conring_size - 1;
+	ring = xc_hypercall_buffer_alloc(_H(xch), ring, conring_size);
+
+	if (!ring)
+		caml_raise_out_of_memory();
 
 	caml_enter_blocking_section();
-	retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
+	retval = xc_readconsolering_buffer(_H(xch), HYPERCALL_BUFFER(ring),
+					   &nr_chars, 0, 0, NULL);
 	caml_leave_blocking_section();
 
 	if (retval)
+	{
+		xc_hypercall_buffer_free(_H(xch), ring);
 		failwith_xc(_H(xch));
+	}
 
-	ring[size] = ''\0'';
-	CAMLreturn(caml_copy_string(ring));
+	ring[nr_chars] = ''\0'';
+	conring = caml_copy_string(ring);
+	xc_hypercall_buffer_free(_H(xch), ring);
+	CAMLreturn(conring);
 }
 
 CAMLprim value stub_xc_send_debug_keys(value xch, value keys)

Andrew Cooper

2013-Mar-12 18:09 UTC

head link

[PATCH 5 of 5 v3] tools/ocaml: libxc bindings: Fix failwith_xc()

The static error_str[] buffer is not thread-safe, and 1024 bytes is
unreasonably large.  Reduce to 256 bytes (which is still much larger than any
current use), and move it to being a stack variable.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

--
Changes since v1:
 * Mark as Noreturn, due to unconditional use of caml_raise_with_string()

diff -r 7a33b4fdb977 -r 189d76dca2e4 tools/ocaml/libs/xc/xenctrl_stubs.c
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -51,21 +51,22 @@
 	i1 = (uint32_t) Int64_val(Field(input, 0)); \
 	i2 = ((Field(input, 1) == Val_none) ? 0xffffffff : (uint32_t)
Int64_val(Field(Field(input, 1), 0)));
 
-#define ERROR_STRLEN 1024
-void failwith_xc(xc_interface *xch)
+static void Noreturn failwith_xc(xc_interface *xch)
 {
-	static char error_str[ERROR_STRLEN];
+	char error_str[256];
 	if (xch) {
 		const xc_error *error = xc_get_last_error(xch);
 		if (error->code == XC_ERROR_NONE)
-                	snprintf(error_str, ERROR_STRLEN, "%d: %s", errno,
strerror(errno));
+                	snprintf(error_str, sizeof(error_str),
+				 "%d: %s", errno, strerror(errno));
 		else
-			snprintf(error_str, ERROR_STRLEN, "%d: %s: %s",
-				 error->code,
+			snprintf(error_str, sizeof(error_str),
+				 "%d: %s: %s", error->code,
 				 xc_error_code_to_desc(error->code),
 				 error->message);
 	} else {
-		snprintf(error_str, ERROR_STRLEN, "Unable to open XC interface");
+		snprintf(error_str, sizeof(error_str),
+			 "Unable to open XC interface");
 	}
 	caml_raise_with_string(*caml_named_value("xc.error"), error_str);
 }

Dave Scott

2013-Mar-13 10:48 UTC

head link

Re: [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

Hi,

This looks fine to me. I assume the console ring is a C string which
doesn''t contain NULLs in the middle and hence it''s sensible to
use "caml_copy_string" (the alternative would be to treat it as a raw
block of bytes using "caml_alloc_string" and memcpy-- OCaml strings
can contain NULLs safely)

Cheers,
Dave
> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Andrew Cooper
> Sent: 12 March 2013 6:09 PM
> To: xen-devel@lists.xen.org
> Cc: Ian Jackson; Keir (Xen.org); Ian Campbell; Jan Beulich; xen-
> api@lists.xen.org
> Subject: [Xen-devel] [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix
> stub_xc_readconsolering()
> 
> There are two problems with this function:
>   * The caml_{enter,leave}_blocking_section() calls mean that two threads
> can
>     compete for use of the static ring[] buffer.
>   * It fails to retrieve the entire buffer if the hypervisor has used 32K
or
>     more of its console ring.
> 
> Rewrite the function using the new xc_consoleringsize() and
> xc_readconsolering_buffer() functions to efficiently grab the entire
console
> ring.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> --
> Changes since v2:
>  * Tweak style and remove redundant variables Changes since v1:
>  * Convert conring_size to being static to avoid needless hypercalls
>  * Fix memory due to ordering of failwith_xc() and
> xc_hypercall_buffer_free()
>  * Remove useless CAMLreturns()
> 
> diff -r c7b82dfbec34 -r 7a33b4fdb977 tools/ocaml/libs/xc/xenctrl_stubs.c
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -523,27 +523,47 @@ CAMLprim value stub_xc_evtchn_reset(valu
>  	CAMLreturn(Val_unit);
>  }
> 
> -
> -#define RING_SIZE 32768
> -static char ring[RING_SIZE];
> -
>  CAMLprim value stub_xc_readconsolering(value xch)  {
> -	unsigned int size = RING_SIZE - 1;
> -	char *ring_ptr = ring;
> +	static uint64_t conring_size = 0;
> +	unsigned int nr_chars;
> +	DECLARE_HYPERCALL_BUFFER(char, ring);
>  	int retval;
> 
>  	CAMLparam1(xch);
> +	CAMLlocal1(conring);
> +
> +	if (!conring_size)
> +	{
> +		if (xc_consoleringsize(_H(xch), &conring_size))
> +			failwith_xc(_H(xch));
> +
> +		/* Round conring_size up to the next page, for NULL
> terminator
> +		   and slop from a race with printk() in the hypervisor. */
> +		conring_size = (conring_size + XC_PAGE_SIZE) &
> XC_PAGE_MASK;
> +	}
> +
> +	nr_chars = conring_size - 1;
> +	ring = xc_hypercall_buffer_alloc(_H(xch), ring, conring_size);
> +
> +	if (!ring)
> +		caml_raise_out_of_memory();
> 
>  	caml_enter_blocking_section();
> -	retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
> +	retval = xc_readconsolering_buffer(_H(xch),
> HYPERCALL_BUFFER(ring),
> +					   &nr_chars, 0, 0, NULL);
>  	caml_leave_blocking_section();
> 
>  	if (retval)
> +	{
> +		xc_hypercall_buffer_free(_H(xch), ring);
>  		failwith_xc(_H(xch));
> +	}
> 
> -	ring[size] = ''\0'';
> -	CAMLreturn(caml_copy_string(ring));
> +	ring[nr_chars] = ''\0'';
> +	conring = caml_copy_string(ring);
> +	xc_hypercall_buffer_free(_H(xch), ring);
> +	CAMLreturn(conring);
>  }
> 
>  CAMLprim value stub_xc_send_debug_keys(value xch, value keys)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Dave Scott

2013-Mar-13 10:49 UTC

head link

Re: [PATCH 5 of 5 v3] tools/ocaml: libxc bindings: Fix failwith_xc()

This looks fine to me.

Cheers,
Dave
> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Andrew Cooper
> Sent: 12 March 2013 6:09 PM
> To: xen-devel@lists.xen.org
> Cc: Ian Jackson; Keir (Xen.org); Ian Campbell; Jan Beulich; xen-
> api@lists.xen.org
> Subject: [Xen-devel] [PATCH 5 of 5 v3] tools/ocaml: libxc bindings: Fix
> failwith_xc()
> 
> The static error_str[] buffer is not thread-safe, and 1024 bytes is
> unreasonably large.  Reduce to 256 bytes (which is still much larger than
any
> current use), and move it to being a stack variable.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> --
> Changes since v1:
>  * Mark as Noreturn, due to unconditional use of caml_raise_with_string()
> 
> diff -r 7a33b4fdb977 -r 189d76dca2e4 tools/ocaml/libs/xc/xenctrl_stubs.c
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -51,21 +51,22 @@
>  	i1 = (uint32_t) Int64_val(Field(input, 0)); \
>  	i2 = ((Field(input, 1) == Val_none) ? 0xffffffff : (uint32_t)
> Int64_val(Field(Field(input, 1), 0)));
> 
> -#define ERROR_STRLEN 1024
> -void failwith_xc(xc_interface *xch)
> +static void Noreturn failwith_xc(xc_interface *xch)
>  {
> -	static char error_str[ERROR_STRLEN];
> +	char error_str[256];
>  	if (xch) {
>  		const xc_error *error = xc_get_last_error(xch);
>  		if (error->code == XC_ERROR_NONE)
> -                	snprintf(error_str, ERROR_STRLEN, "%d: %s",
errno,
> strerror(errno));
> +                	snprintf(error_str, sizeof(error_str),
> +				 "%d: %s", errno, strerror(errno));
>  		else
> -			snprintf(error_str, ERROR_STRLEN, "%d: %s: %s",
> -				 error->code,
> +			snprintf(error_str, sizeof(error_str),
> +				 "%d: %s: %s", error->code,
>  				 xc_error_code_to_desc(error->code),
>  				 error->message);
>  	} else {
> -		snprintf(error_str, ERROR_STRLEN, "Unable to open XC
> interface");
> +		snprintf(error_str, sizeof(error_str),
> +			 "Unable to open XC interface");
>  	}
>  	caml_raise_with_string(*caml_named_value("xc.error"),
error_str);  }
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Ian Campbell

2013-Mar-13 10:55 UTC

head link

Re: [PATCH 5 of 5 v3] tools/ocaml: libxc bindings: Fix failwith_xc()

On Wed, 2013-03-13 at 10:49 +0000, Dave Scott wrote:> This looks fine to me.
Can we/I take that as a formal "Acked-by: Dave Scott
<Dave.Scott@eu.citrix.com>" ?

Dave Scott

2013-Mar-13 10:56 UTC

head link

Re: [PATCH 5 of 5 v3] tools/ocaml: libxc bindings: Fix failwith_xc()

> On Wed, 2013-03-13 at 10:49 +0000, Dave Scott wrote:
> > This looks fine to me.
> 
> Can we/I take that as a formal "Acked-by: Dave Scott
> <Dave.Scott@eu.citrix.com>" ?
Please do!

Acked-by: Dave Scott <Dave.Scott@eu.citrix.com>

Thanks,
Dave

Andrew Cooper

2013-Mar-13 11:05 UTC

head link

Re: [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

On 13/03/13 10:48, Dave Scott wrote:> Hi,
>
> This looks fine to me. I assume the console ring is a C string which
doesn''t contain NULLs in the middle and hence it''s sensible to
use "caml_copy_string" (the alternative would be to treat it as a raw
block of bytes using "caml_alloc_string" and memcpy-- OCaml strings
can contain NULLs safely)
>
> Cheers,
> Dave
Hmm - that is a good point.  We expect anything in the console ring to
be regular ASCII text, but already know that ACPI tables from certain
vendors contain non ASCII text.  We certainly cant rule out the presence
of NULL characters.

Furthermore, your suggestion of caml_alloc_string() has given me an idea
to remove another copy of the buffer.  I will respin the patch.

~Andrew
>
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of Andrew Cooper
>> Sent: 12 March 2013 6:09 PM
>> To: xen-devel@lists.xen.org
>> Cc: Ian Jackson; Keir (Xen.org); Ian Campbell; Jan Beulich; xen-
>> api@lists.xen.org
>> Subject: [Xen-devel] [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix
>> stub_xc_readconsolering()
>>
>> There are two problems with this function:
>>   * The caml_{enter,leave}_blocking_section() calls mean that two
threads
>> can
>>     compete for use of the static ring[] buffer.
>>   * It fails to retrieve the entire buffer if the hypervisor has used
32K or
>>     more of its console ring.
>>
>> Rewrite the function using the new xc_consoleringsize() and
>> xc_readconsolering_buffer() functions to efficiently grab the entire
console
>> ring.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> --
>> Changes since v2:
>>  * Tweak style and remove redundant variables Changes since v1:
>>  * Convert conring_size to being static to avoid needless hypercalls
>>  * Fix memory due to ordering of failwith_xc() and
>> xc_hypercall_buffer_free()
>>  * Remove useless CAMLreturns()
>>
>> diff -r c7b82dfbec34 -r 7a33b4fdb977
tools/ocaml/libs/xc/xenctrl_stubs.c
>> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
>> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
>> @@ -523,27 +523,47 @@ CAMLprim value stub_xc_evtchn_reset(valu
>>  	CAMLreturn(Val_unit);
>>  }
>>
>> -
>> -#define RING_SIZE 32768
>> -static char ring[RING_SIZE];
>> -
>>  CAMLprim value stub_xc_readconsolering(value xch)  {
>> -	unsigned int size = RING_SIZE - 1;
>> -	char *ring_ptr = ring;
>> +	static uint64_t conring_size = 0;
>> +	unsigned int nr_chars;
>> +	DECLARE_HYPERCALL_BUFFER(char, ring);
>>  	int retval;
>>
>>  	CAMLparam1(xch);
>> +	CAMLlocal1(conring);
>> +
>> +	if (!conring_size)
>> +	{
>> +		if (xc_consoleringsize(_H(xch), &conring_size))
>> +			failwith_xc(_H(xch));
>> +
>> +		/* Round conring_size up to the next page, for NULL
>> terminator
>> +		   and slop from a race with printk() in the hypervisor. */
>> +		conring_size = (conring_size + XC_PAGE_SIZE) &
>> XC_PAGE_MASK;
>> +	}
>> +
>> +	nr_chars = conring_size - 1;
>> +	ring = xc_hypercall_buffer_alloc(_H(xch), ring, conring_size);
>> +
>> +	if (!ring)
>> +		caml_raise_out_of_memory();
>>
>>  	caml_enter_blocking_section();
>> -	retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0,
NULL);
>> +	retval = xc_readconsolering_buffer(_H(xch),
>> HYPERCALL_BUFFER(ring),
>> +					   &nr_chars, 0, 0, NULL);
>>  	caml_leave_blocking_section();
>>
>>  	if (retval)
>> +	{
>> +		xc_hypercall_buffer_free(_H(xch), ring);
>>  		failwith_xc(_H(xch));
>> +	}
>>
>> -	ring[size] = ''\0'';
>> -	CAMLreturn(caml_copy_string(ring));
>> +	ring[nr_chars] = ''\0'';
>> +	conring = caml_copy_string(ring);
>> +	xc_hypercall_buffer_free(_H(xch), ring);
>> +	CAMLreturn(conring);
>>  }
>>
>>  CAMLprim value stub_xc_send_debug_keys(value xch, value keys)
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel

Ian Jackson

2013-Mar-13 11:15 UTC

head link

Re: [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

Dave Scott writes ("RE: [Xen-devel] [PATCH 4 of 5 v3] tools/ocaml: libxc
bindings: Fix stub_xc_readconsolering()"):> This looks fine to me. I assume the console ring is a C string which
> doesn''t contain NULLs in the middle and hence it''s
sensible to use
> "caml_copy_string" (the alternative would be to treat it as a raw
> block of bytes using "caml_alloc_string" and memcpy-- OCaml
strings
> can contain NULLs safely)
The guest is certainly able to write nuls into the console ring if it
wants to.

Ian.

Ian Campbell

2013-Mar-13 11:19 UTC

head link

Re: [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

On Wed, 2013-03-13 at 11:15 +0000, Ian Jackson wrote:> Dave Scott writes ("RE: [Xen-devel] [PATCH 4 of 5 v3] tools/ocaml:
libxc bindings: Fix stub_xc_readconsolering()"):
> > This looks fine to me. I assume the console ring is a C string which
> > doesn''t contain NULLs in the middle and hence it''s
sensible to use
> > "caml_copy_string" (the alternative would be to treat it as
a raw
> > block of bytes using "caml_alloc_string" and memcpy-- OCaml
strings
> > can contain NULLs safely)
> 
> The guest is certainly able to write nuls into the console ring if it
> wants to.
This is the hypervisor''s console ring, not the guests.

But you are right that a guest can write to that in debug builds with
sufficient logging enabled on the command line.

Ian.

Xen devel - Mar 2013 - [PATCH 0 of 5 v3] Tools: Console ring improvements

[PATCH 0 of 5 v3] Tools: Console ring improvements

[PATCH 1 of 5 v3] common/sysctl: Introduce hypercall to query the console ring size

[PATCH 2 of 5 v3] tools/libxc: Helper function for XEN_SYSCTL_consoleringsize

[PATCH 3 of 5 v3] tools/libxc: Implement xc_readconsolering_buffer()

[PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

[PATCH 5 of 5 v3] tools/ocaml: libxc bindings: Fix failwith_xc()

Re: [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

Re: [PATCH 5 of 5 v3] tools/ocaml: libxc bindings: Fix failwith_xc()

Re: [PATCH 5 of 5 v3] tools/ocaml: libxc bindings: Fix failwith_xc()

Re: [PATCH 5 of 5 v3] tools/ocaml: libxc bindings: Fix failwith_xc()

Re: [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

Re: [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

Re: [PATCH 4 of 5 v3] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()