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. 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-Feb-20 18:05 UTC
[PATCH 1 of 5] common/sysctl: Introduce hypercall to query the console ring size
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff -r 66f563be41d9 -r a380b2169803 xen/common/sysctl.c
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -357,6 +357,9 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
}
break;
+ case XEN_SYSCTL_consoleringsize:
+ ret = console_ring_size(&op->u.consoleringsize);
+ break;
default:
ret = arch_do_sysctl(op, u_sysctl);
diff -r 66f563be41d9 -r a380b2169803 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 66f563be41d9 -r a380b2169803 xen/include/public/sysctl.h
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -596,6 +596,14 @@ struct xen_sysctl_scheduler_op {
typedef struct xen_sysctl_scheduler_op xen_sysctl_scheduler_op_t;
DEFINE_XEN_GUEST_HANDLE(xen_sysctl_scheduler_op_t);
+/* XEN_SYSCTL_consoleringsize */
+/* Get the size of the hypervisor console ring. */
+struct xen_sysctl_consoleringsize {
+ uint64_t size; /* Written by Xen. */
+};
+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
@@ -616,6 +624,7 @@ struct xen_sysctl {
#define XEN_SYSCTL_numainfo 17
#define XEN_SYSCTL_cpupool_op 18
#define XEN_SYSCTL_scheduler_op 19
+#define XEN_SYSCTL_consoleringsize 20
uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
union {
struct xen_sysctl_readconsole readconsole;
@@ -636,6 +645,7 @@ struct xen_sysctl {
struct xen_sysctl_lockprof_op lockprof_op;
struct xen_sysctl_cpupool_op cpupool_op;
struct xen_sysctl_scheduler_op scheduler_op;
+ struct xen_sysctl_consoleringsize consoleringsize;
uint8_t pad[128];
} u;
};
diff -r 66f563be41d9 -r a380b2169803 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-Feb-20 18:05 UTC
[PATCH 2 of 5] tools/libxc: Helper function for XEN_SYSCTL_consoleringsize
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff -r a380b2169803 -r 40d1bab1b996 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 = EFAULT;
+ 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 a380b2169803 -r 40d1bab1b996 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-Feb-20 18:05 UTC
[PATCH 3 of 5] tools/libxc: Implement of 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>
diff -r 40d1bab1b996 -r 6b8c513cff4f tools/libxc/xc_misc.c
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -100,6 +100,37 @@ int xc_readconsolering(xc_interface *xch
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;
+ sysctl.u.readconsole.clear = clear;
+ sysctl.u.readconsole.incremental = 0;
+ if ( pindex )
+ {
+ sysctl.u.readconsole.index = *pindex;
+ sysctl.u.readconsole.incremental = incremental;
+ }
+
+ if ( (ret = do_sysctl(xch, &sysctl)) == 0 )
+ {
+ *pnr_chars = sysctl.u.readconsole.count;
+ if ( pindex )
+ *pindex = sysctl.u.readconsole.index;
+ }
+
+ return ret;
+}
+
int xc_consoleringsize(xc_interface *xch, uint64_t * psize)
{
int ret = -1;
diff -r 40d1bab1b996 -r 6b8c513cff4f tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -998,6 +998,10 @@ 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-Feb-20 18:05 UTC
[PATCH 4 of 5] 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>
diff -r 6b8c513cff4f -r a73d0c5a5b24 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,61 @@ CAMLprim value stub_xc_evtchn_reset(valu
CAMLreturn(Val_unit);
}
-
-#define RING_SIZE 32768
-static char ring[RING_SIZE];
-
+/* Maximum size of console ring we are prepared to read, 16MB */
+#define MAX_CONSOLE_RING_SIZE (1U << 24)
CAMLprim value stub_xc_readconsolering(value xch)
{
- unsigned int size = RING_SIZE - 1;
- char *ring_ptr = ring;
- int retval;
+ uint64_t conring_size;
+ DECLARE_HYPERCALL_BUFFER(char, ring);
+ unsigned int nr_chars;
+ int r;
CAMLparam1(xch);
+ CAMLlocal1(conring);
+
+ r = xc_consoleringsize(_H(xch), &conring_size);
+
+ if ( r )
+ {
+ failwith_xc(_H(xch));
+ CAMLreturn(Val_unit);
+ }
+
+ /* 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;
+
+ if ( conring_size > MAX_CONSOLE_RING_SIZE )
+ {
+ caml_failwith("Console ring too large");
+ CAMLreturn(Val_unit);
+ }
+
+ ring = xc_hypercall_buffer_alloc(_H(xch), ring, conring_size);
+
+ if ( ! ring )
+ {
+ failwith_xc(_H(xch));
+ CAMLreturn(Val_unit);
+ }
caml_enter_blocking_section();
- retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
+ r = xc_readconsolering_buffer(_H(xch), HYPERCALL_BUFFER(ring),
+ &nr_chars, 0, 0, NULL);
caml_leave_blocking_section();
- if (retval)
+ if ( r )
+ {
failwith_xc(_H(xch));
+ xc_hypercall_buffer_free(_H(xch), ring);
+ CAMLreturn(Val_unit);
+ }
- 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-Feb-20 18:05 UTC
[PATCH 5 of 5] 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>
diff -r a73d0c5a5b24 -r 4ba606ff8cf9 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 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);
}
Ian Campbell
2013-Feb-21 10:01 UTC
Re: [PATCH 3 of 5] tools/libxc: Implement of xc_readconsolering_buffer
On Wed, 2013-02-20 at 18:05 +0000, Andrew Cooper wrote:> Functions identically to xc_readconsolering(), but uses a user-provided > xc_hypercall_buffer_t to save using a bounce buffer.Can we now trivially reimplement xc_readconsolering in terms of this function? Should just be a case of replacing all the code between bounce_pre and bounce_post with a call xc_readconsolering_buffer(xch, HYPERCALL_BUFFER_AS_ARG(buffer), pnr_chars, clear, incremental, pindex); ? We should probably also deprecate the old one and at least have a vague idea that we might eventually switch all the callers over (you don''t need to here though, unless you are super keen). Ian.
Ian Campbell
2013-Feb-21 10:14 UTC
Re: [PATCH 4 of 5] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()
On Wed, 2013-02-20 at 18:05 +0000, Andrew Cooper wrote:> 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> > > diff -r 6b8c513cff4f -r a73d0c5a5b24 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,61 @@ CAMLprim value stub_xc_evtchn_reset(valu > CAMLreturn(Val_unit); > } > > - > -#define RING_SIZE 32768 > -static char ring[RING_SIZE]; > - > +/* Maximum size of console ring we are prepared to read, 16MB */ > +#define MAX_CONSOLE_RING_SIZE (1U << 24)A bit arbitrary, why not just let it handle whatever the hypervisor throws at it. If there is to be a limit it probably belongs on the hypervisor side, but in the meantime someone who passes consring=1G can keep all the pieces ;-)> CAMLprim value stub_xc_readconsolering(value xch) > { > - unsigned int size = RING_SIZE - 1; > - char *ring_ptr = ring; > - int retval; > + uint64_t conring_size; > + DECLARE_HYPERCALL_BUFFER(char, ring); > + unsigned int nr_chars; > + int r; > > CAMLparam1(xch); > + CAMLlocal1(conring); > + > + r = xc_consoleringsize(_H(xch), &conring_size);static int consoleringsize and only call this once the first time?> + > + if ( r ) > + { > + failwith_xc(_H(xch)); > + CAMLreturn(Val_unit); > + } > + > + /* 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; > + > + if ( conring_size > MAX_CONSOLE_RING_SIZE ) > + { > + caml_failwith("Console ring too large"); > + CAMLreturn(Val_unit); > + } > + > + ring = xc_hypercall_buffer_alloc(_H(xch), ring, conring_size); > + > + if ( ! ring ) > + { > + failwith_xc(_H(xch)); > + CAMLreturn(Val_unit); > + } > > caml_enter_blocking_section(); > - retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL); > + r = xc_readconsolering_buffer(_H(xch), HYPERCALL_BUFFER(ring), > + &nr_chars, 0, 0, NULL); > caml_leave_blocking_section(); > > - if (retval) > + if ( r ) > + { > failwith_xc(_H(xch));Doesn''t this (due to caml_raise...) end up exiting synchronously via a longjmp back to the runtime? i.e. it leaks the buffer which is only free''d after. It''s a good idea to cc xen-api@ with ocaml changes, for this sort of reason...> + xc_hypercall_buffer_free(_H(xch), ring); > + CAMLreturn(Val_unit);I think CAMLnoreturn here.> + } > > - 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-Feb-21 11:07 UTC
Re: [PATCH 3 of 5] tools/libxc: Implement of xc_readconsolering_buffer
On 21/02/13 10:01, Ian Campbell wrote:> On Wed, 2013-02-20 at 18:05 +0000, Andrew Cooper wrote: >> Functions identically to xc_readconsolering(), but uses a user-provided >> xc_hypercall_buffer_t to save using a bounce buffer. > Can we now trivially reimplement xc_readconsolering in terms of this > function? > > Should just be a case of replacing all the code between bounce_pre and > bounce_post with a call > xc_readconsolering_buffer(xch, HYPERCALL_BUFFER_AS_ARG(buffer), > pnr_chars, clear, incremental, pindex); > ?Ok> > We should probably also deprecate the old one and at least have a vague > idea that we might eventually switch all the callers over (you don''t > need to here though, unless you are super keen). > > Ian. >Depending on the quantity of callers, I may or may not due to time. ~Andrew
Andrew Cooper
2013-Feb-21 11:12 UTC
Re: [PATCH 4 of 5] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()
On 21/02/13 10:14, Ian Campbell wrote:> On Wed, 2013-02-20 at 18:05 +0000, Andrew Cooper wrote: >> 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> >> >> diff -r 6b8c513cff4f -r a73d0c5a5b24 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,61 @@ CAMLprim value stub_xc_evtchn_reset(valu >> CAMLreturn(Val_unit); >> } >> >> - >> -#define RING_SIZE 32768 >> -static char ring[RING_SIZE]; >> - >> +/* Maximum size of console ring we are prepared to read, 16MB */ >> +#define MAX_CONSOLE_RING_SIZE (1U << 24) > A bit arbitrary, why not just let it handle whatever the hypervisor > throws at it. If there is to be a limit it probably belongs on the > hypervisor side, but in the meantime someone who passes consring=1G can > keep all the pieces ;-) > >> CAMLprim value stub_xc_readconsolering(value xch) >> { >> - unsigned int size = RING_SIZE - 1; >> - char *ring_ptr = ring; >> - int retval; >> + uint64_t conring_size; >> + DECLARE_HYPERCALL_BUFFER(char, ring); >> + unsigned int nr_chars; >> + int r; >> >> CAMLparam1(xch); >> + CAMLlocal1(conring); >> + >> + r = xc_consoleringsize(_H(xch), &conring_size); > static int consoleringsize and only call this once the first time?One of the point of this patch was to remove the static buffer for thread safety reasons. Having said that, even if you do race on this static variable, it should be filled in with the same value, so is probably safe?> >> + >> + if ( r ) >> + { >> + failwith_xc(_H(xch)); >> + CAMLreturn(Val_unit); >> + } >> + >> + /* 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; >> + >> + if ( conring_size > MAX_CONSOLE_RING_SIZE ) >> + { >> + caml_failwith("Console ring too large"); >> + CAMLreturn(Val_unit); >> + } >> + >> + ring = xc_hypercall_buffer_alloc(_H(xch), ring, conring_size); >> + >> + if ( ! ring ) >> + { >> + failwith_xc(_H(xch)); >> + CAMLreturn(Val_unit); >> + } >> >> caml_enter_blocking_section(); >> - retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL); >> + r = xc_readconsolering_buffer(_H(xch), HYPERCALL_BUFFER(ring), >> + &nr_chars, 0, 0, NULL); >> caml_leave_blocking_section(); >> >> - if (retval) >> + if ( r ) >> + { >> failwith_xc(_H(xch)); > Doesn''t this (due to caml_raise...) end up exiting synchronously via a > longjmp back to the runtime? i.e. it leaks the buffer which is only > free''d after. > > It''s a good idea to cc xen-api@ with ocaml changes, for this sort of > reason...I will double check that. If so, then I have some memory leaks to fix up in other bits of Ocaml/C bindings, which I was using as an example here... I will cc xen-api@ for the next submission of this series.> >> + xc_hypercall_buffer_free(_H(xch), ring); >> + CAMLreturn(Val_unit); > I think CAMLnoreturn here.Ok. ~Andrew> >> + } >> >> - 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) >