* [PATCH v2] xen/console: buffer and show origin of guest PV writes
@ 2013-08-16 19:01 Daniel De Graaf
2013-09-09 11:13 ` Keir Fraser
0 siblings, 1 reply; 8+ messages in thread
From: Daniel De Graaf @ 2013-08-16 19:01 UTC (permalink / raw)
To: keir; +Cc: andrew.cooper3, Daniel De Graaf, JBeulich, xen-devel
Guests other than domain 0 using the console output have previously been
controlled by the VERBOSE define, but with no designation of which
guest's output was on the console. This patch converts the HVM output
buffering to be used by all domains, line buffering their output and
prefixing it with the domain ID. This is especially useful for debugging
stub domains.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
Changes since v1 (RFC):
- Use prefix of (d%d) in place of (XEN) for both PV and HVM output
- Guests other than dom0 have non-printable characters stripped
- Use unsigned type for pbuf_idx
- Formatting fixes
xen/arch/x86/hvm/hvm.c | 27 +++++-------
xen/common/domain.c | 8 ++++
xen/drivers/char/console.c | 94 ++++++++++++++++++++++++++++++++--------
xen/include/asm-x86/hvm/domain.h | 6 ---
xen/include/xen/lib.h | 2 +
xen/include/xen/sched.h | 6 +++
6 files changed, 103 insertions(+), 40 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 1fcaed0..4ff76cc 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -485,8 +485,7 @@ static int hvm_set_ioreq_page(
static int hvm_print_line(
int dir, uint32_t port, uint32_t bytes, uint32_t *val)
{
- struct vcpu *curr = current;
- struct hvm_domain *hd = &curr->domain->arch.hvm_domain;
+ struct domain *cd = current->domain;
char c = *val;
BUG_ON(bytes != 1);
@@ -495,17 +494,16 @@ static int hvm_print_line(
if ( !isprint(c) && (c != '\n') && (c != '\t') )
return X86EMUL_OKAY;
- spin_lock(&hd->pbuf_lock);
- hd->pbuf[hd->pbuf_idx++] = c;
- if ( (hd->pbuf_idx == (HVM_PBUF_SIZE - 2)) || (c == '\n') )
+ spin_lock(&cd->pbuf_lock);
+ if ( c != '\n' )
+ cd->pbuf[cd->pbuf_idx++] = c;
+ if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
{
- if ( c != '\n' )
- hd->pbuf[hd->pbuf_idx++] = '\n';
- hd->pbuf[hd->pbuf_idx] = '\0';
- printk(XENLOG_G_DEBUG "HVM%u: %s", curr->domain->domain_id, hd->pbuf);
- hd->pbuf_idx = 0;
+ cd->pbuf[cd->pbuf_idx] = '\0';
+ guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
+ cd->pbuf_idx = 0;
}
- spin_unlock(&hd->pbuf_lock);
+ spin_unlock(&cd->pbuf_lock);
return X86EMUL_OKAY;
}
@@ -521,19 +519,16 @@ int hvm_domain_initialise(struct domain *d)
return -EINVAL;
}
- spin_lock_init(&d->arch.hvm_domain.pbuf_lock);
spin_lock_init(&d->arch.hvm_domain.irq_lock);
spin_lock_init(&d->arch.hvm_domain.uc_lock);
INIT_LIST_HEAD(&d->arch.hvm_domain.msixtbl_list);
spin_lock_init(&d->arch.hvm_domain.msixtbl_list_lock);
- d->arch.hvm_domain.pbuf = xzalloc_array(char, HVM_PBUF_SIZE);
d->arch.hvm_domain.params = xzalloc_array(uint64_t, HVM_NR_PARAMS);
d->arch.hvm_domain.io_handler = xmalloc(struct hvm_io_handler);
rc = -ENOMEM;
- if ( !d->arch.hvm_domain.pbuf || !d->arch.hvm_domain.params ||
- !d->arch.hvm_domain.io_handler )
+ if ( !d->arch.hvm_domain.params || !d->arch.hvm_domain.io_handler )
goto fail0;
d->arch.hvm_domain.io_handler->num_slot = 0;
@@ -578,7 +573,6 @@ int hvm_domain_initialise(struct domain *d)
fail0:
xfree(d->arch.hvm_domain.io_handler);
xfree(d->arch.hvm_domain.params);
- xfree(d->arch.hvm_domain.pbuf);
return rc;
}
@@ -603,7 +597,6 @@ void hvm_domain_relinquish_resources(struct domain *d)
xfree(d->arch.hvm_domain.io_handler);
xfree(d->arch.hvm_domain.params);
- xfree(d->arch.hvm_domain.pbuf);
}
void hvm_domain_destroy(struct domain *d)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6c264a5..daac2c9 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -231,6 +231,8 @@ struct domain *domain_create(
spin_lock_init(&d->shutdown_lock);
d->shutdown_code = -1;
+ spin_lock_init(&d->pbuf_lock);
+
err = -ENOMEM;
if ( !zalloc_cpumask_var(&d->domain_dirty_cpumask) )
goto fail;
@@ -286,6 +288,10 @@ struct domain *domain_create(
d->mem_event = xzalloc(struct mem_event_per_domain);
if ( !d->mem_event )
goto fail;
+
+ d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
+ if ( !d->pbuf )
+ goto fail;
}
if ( (err = arch_domain_create(d, domcr_flags)) != 0 )
@@ -318,6 +324,7 @@ struct domain *domain_create(
d->is_dying = DOMDYING_dead;
atomic_set(&d->refcnt, DOMAIN_DESTROYED);
xfree(d->mem_event);
+ xfree(d->pbuf);
if ( init_status & INIT_arch )
arch_domain_destroy(d);
if ( init_status & INIT_gnttab )
@@ -730,6 +737,7 @@ static void complete_domain_destroy(struct rcu_head *head)
#endif
xfree(d->mem_event);
+ xfree(d->pbuf);
for ( i = d->max_vcpus - 1; i >= 0; i-- )
if ( (v = d->vcpu[i]) != NULL )
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 8ac32e4..b8d9a9f 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -24,6 +24,7 @@
#include <xen/shutdown.h>
#include <xen/video.h>
#include <xen/kexec.h>
+#include <xen/ctype.h>
#include <asm/debugger.h>
#include <asm/div64.h>
#include <xen/hypercall.h> /* for do_console_io */
@@ -375,6 +376,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
{
char kbuf[128];
int kcount;
+ struct domain *cd = current->domain;
while ( count > 0 )
{
@@ -388,19 +390,60 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
return -EFAULT;
kbuf[kcount] = '\0';
- spin_lock_irq(&console_lock);
+ if ( is_hardware_domain(cd) )
+ {
+ /* Use direct console output as it could be interactive */
+ spin_lock_irq(&console_lock);
+
+ sercon_puts(kbuf);
+ video_puts(kbuf);
- sercon_puts(kbuf);
- video_puts(kbuf);
+ if ( opt_console_to_ring )
+ {
+ conring_puts(kbuf);
+ tasklet_schedule(¬ify_dom0_con_ring_tasklet);
+ }
- if ( opt_console_to_ring )
+ spin_unlock_irq(&console_lock);
+ }
+ else
{
- conring_puts(kbuf);
- tasklet_schedule(¬ify_dom0_con_ring_tasklet);
+ char *kin = kbuf;
+ char *kout = kbuf;
+ char c;
+ /* Strip non-printable characters */
+ for ( ; ; )
+ {
+ c = *kin++;
+ if ( c == '\0' || c == '\n' )
+ break;
+ if ( isprint(c) || c == '\t' )
+ *kout++ = c;
+ }
+ *kout = '\0';
+ spin_lock(&cd->pbuf_lock);
+ if ( c == '\n' )
+ {
+ kcount = kin - kbuf;
+ cd->pbuf[cd->pbuf_idx] = '\0';
+ guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
+ cd->pbuf_idx = 0;
+ }
+ else if ( cd->pbuf_idx + kcount < (DOMAIN_PBUF_SIZE - 1) )
+ {
+ /* buffer the output until a newline */
+ memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kcount);
+ cd->pbuf_idx += kcount;
+ }
+ else
+ {
+ cd->pbuf[cd->pbuf_idx] = '\0';
+ guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
+ cd->pbuf_idx = 0;
+ }
+ spin_unlock(&cd->pbuf_lock);
}
- spin_unlock_irq(&console_lock);
-
guest_handle_add_offset(buffer, kcount);
count -= kcount;
}
@@ -504,12 +547,12 @@ static int printk_prefix_check(char *p, char **pp)
((loglvl < upper_thresh) && printk_ratelimit()));
}
-static void printk_start_of_line(void)
+static void printk_start_of_line(const char *prefix)
{
struct tm tm;
char tstr[32];
- __putstr("(XEN) ");
+ __putstr(prefix);
if ( !opt_console_timestamps )
return;
@@ -524,12 +567,11 @@ static void printk_start_of_line(void)
__putstr(tstr);
}
-void printk(const char *fmt, ...)
+static void vprintk_common(const char *prefix, const char *fmt, va_list args)
{
static char buf[1024];
static int start_of_line = 1, do_print;
- va_list args;
char *p, *q;
unsigned long flags;
@@ -537,9 +579,7 @@ void printk(const char *fmt, ...)
local_irq_save(flags);
spin_lock_recursive(&console_lock);
- va_start(args, fmt);
(void)vsnprintf(buf, sizeof(buf), fmt, args);
- va_end(args);
p = buf;
@@ -551,7 +591,7 @@ void printk(const char *fmt, ...)
if ( do_print )
{
if ( start_of_line )
- printk_start_of_line();
+ printk_start_of_line(prefix);
__putstr(p);
__putstr("\n");
}
@@ -566,7 +606,7 @@ void printk(const char *fmt, ...)
if ( do_print )
{
if ( start_of_line )
- printk_start_of_line();
+ printk_start_of_line(prefix);
__putstr(p);
}
start_of_line = 0;
@@ -576,6 +616,26 @@ void printk(const char *fmt, ...)
local_irq_restore(flags);
}
+void printk(const char *fmt, ...)
+{
+ va_list args;
+ va_start(args, fmt);
+ vprintk_common("(XEN) ", fmt, args);
+ va_end(args);
+}
+
+void guest_printk(struct domain *d, const char *fmt, ...)
+{
+ va_list args;
+ char prefix[16];
+
+ snprintf(prefix, sizeof(prefix), "(d%d) ", d->domain_id);
+
+ va_start(args, fmt);
+ vprintk_common(prefix, fmt, args);
+ va_end(args);
+}
+
void __init console_init_preirq(void)
{
char *p;
@@ -791,7 +851,7 @@ int __printk_ratelimit(int ratelimit_ms, int ratelimit_burst)
snprintf(lost_str, sizeof(lost_str), "%d", lost);
/* console_lock may already be acquired by printk(). */
spin_lock_recursive(&console_lock);
- printk_start_of_line();
+ printk_start_of_line("(XEN) ");
__putstr("printk: ");
__putstr(lost_str);
__putstr(" messages suppressed.\n");
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 27b3de5..b1e3187 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -62,12 +62,6 @@ struct hvm_domain {
/* emulated irq to pirq */
struct radix_tree_root emuirq_pirq;
- /* hvm_print_line() logging. */
-#define HVM_PBUF_SIZE 80
- char *pbuf;
- int pbuf_idx;
- spinlock_t pbuf_lock;
-
uint64_t *params;
/* Memory ranges with pinned cache attributes. */
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 74b34eb..40afe12 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -83,6 +83,8 @@ extern void debugtrace_printk(const char *fmt, ...);
#define _p(_x) ((void *)(unsigned long)(_x))
extern void printk(const char *format, ...)
__attribute__ ((format (printf, 1, 2)));
+extern void guest_printk(struct domain *d, const char *format, ...)
+ __attribute__ ((format (printf, 2, 3)));
extern void panic(const char *format, ...)
__attribute__ ((format (printf, 1, 2)));
extern long vm_assist(struct domain *, unsigned int, unsigned int);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index ae6a3b8..0013a8d 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -341,6 +341,12 @@ struct domain
/* Control-plane tools handle for this domain. */
xen_domain_handle_t handle;
+ /* hvm_print_line() and guest_console_write() logging. */
+#define DOMAIN_PBUF_SIZE 80
+ char *pbuf;
+ unsigned pbuf_idx;
+ spinlock_t pbuf_lock;
+
/* OProfile support. */
struct xenoprof *xenoprof;
int32_t time_offset_seconds;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xen/console: buffer and show origin of guest PV writes
2013-08-16 19:01 [PATCH v2] xen/console: buffer and show origin of guest PV writes Daniel De Graaf
@ 2013-09-09 11:13 ` Keir Fraser
2013-09-09 12:13 ` Ian Campbell
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Keir Fraser @ 2013-09-09 11:13 UTC (permalink / raw)
To: Daniel De Graaf; +Cc: Andrew Cooper, JBeulich, xen-devel
On 16/08/2013 20:01, "Daniel De Graaf" <dgdegra@tycho.nsa.gov> wrote:
> Guests other than domain 0 using the console output have previously been
> controlled by the VERBOSE define, but with no designation of which
> guest's output was on the console. This patch converts the HVM output
> buffering to be used by all domains, line buffering their output and
> prefixing it with the domain ID. This is especially useful for debugging
> stub domains.
>
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
This seems good, but, if we process and buffer dom0's output, we lose the
possibility of running a terminal session in dom0 over the Xen console.
Personally I do that quite a bit -- serial access only, get Xen's debugging
there, but also can log in to dom0. Does noone else??
-- Keir
> ---
>
> Changes since v1 (RFC):
> - Use prefix of (d%d) in place of (XEN) for both PV and HVM output
> - Guests other than dom0 have non-printable characters stripped
> - Use unsigned type for pbuf_idx
> - Formatting fixes
>
> xen/arch/x86/hvm/hvm.c | 27 +++++-------
> xen/common/domain.c | 8 ++++
> xen/drivers/char/console.c | 94
> ++++++++++++++++++++++++++++++++--------
> xen/include/asm-x86/hvm/domain.h | 6 ---
> xen/include/xen/lib.h | 2 +
> xen/include/xen/sched.h | 6 +++
> 6 files changed, 103 insertions(+), 40 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 1fcaed0..4ff76cc 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -485,8 +485,7 @@ static int hvm_set_ioreq_page(
> static int hvm_print_line(
> int dir, uint32_t port, uint32_t bytes, uint32_t *val)
> {
> - struct vcpu *curr = current;
> - struct hvm_domain *hd = &curr->domain->arch.hvm_domain;
> + struct domain *cd = current->domain;
> char c = *val;
>
> BUG_ON(bytes != 1);
> @@ -495,17 +494,16 @@ static int hvm_print_line(
> if ( !isprint(c) && (c != '\n') && (c != '\t') )
> return X86EMUL_OKAY;
>
> - spin_lock(&hd->pbuf_lock);
> - hd->pbuf[hd->pbuf_idx++] = c;
> - if ( (hd->pbuf_idx == (HVM_PBUF_SIZE - 2)) || (c == '\n') )
> + spin_lock(&cd->pbuf_lock);
> + if ( c != '\n' )
> + cd->pbuf[cd->pbuf_idx++] = c;
> + if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
> {
> - if ( c != '\n' )
> - hd->pbuf[hd->pbuf_idx++] = '\n';
> - hd->pbuf[hd->pbuf_idx] = '\0';
> - printk(XENLOG_G_DEBUG "HVM%u: %s", curr->domain->domain_id,
> hd->pbuf);
> - hd->pbuf_idx = 0;
> + cd->pbuf[cd->pbuf_idx] = '\0';
> + guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
> + cd->pbuf_idx = 0;
> }
> - spin_unlock(&hd->pbuf_lock);
> + spin_unlock(&cd->pbuf_lock);
>
> return X86EMUL_OKAY;
> }
> @@ -521,19 +519,16 @@ int hvm_domain_initialise(struct domain *d)
> return -EINVAL;
> }
>
> - spin_lock_init(&d->arch.hvm_domain.pbuf_lock);
> spin_lock_init(&d->arch.hvm_domain.irq_lock);
> spin_lock_init(&d->arch.hvm_domain.uc_lock);
>
> INIT_LIST_HEAD(&d->arch.hvm_domain.msixtbl_list);
> spin_lock_init(&d->arch.hvm_domain.msixtbl_list_lock);
>
> - d->arch.hvm_domain.pbuf = xzalloc_array(char, HVM_PBUF_SIZE);
> d->arch.hvm_domain.params = xzalloc_array(uint64_t, HVM_NR_PARAMS);
> d->arch.hvm_domain.io_handler = xmalloc(struct hvm_io_handler);
> rc = -ENOMEM;
> - if ( !d->arch.hvm_domain.pbuf || !d->arch.hvm_domain.params ||
> - !d->arch.hvm_domain.io_handler )
> + if ( !d->arch.hvm_domain.params || !d->arch.hvm_domain.io_handler )
> goto fail0;
> d->arch.hvm_domain.io_handler->num_slot = 0;
>
> @@ -578,7 +573,6 @@ int hvm_domain_initialise(struct domain *d)
> fail0:
> xfree(d->arch.hvm_domain.io_handler);
> xfree(d->arch.hvm_domain.params);
> - xfree(d->arch.hvm_domain.pbuf);
> return rc;
> }
>
> @@ -603,7 +597,6 @@ void hvm_domain_relinquish_resources(struct domain *d)
>
> xfree(d->arch.hvm_domain.io_handler);
> xfree(d->arch.hvm_domain.params);
> - xfree(d->arch.hvm_domain.pbuf);
> }
>
> void hvm_domain_destroy(struct domain *d)
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 6c264a5..daac2c9 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -231,6 +231,8 @@ struct domain *domain_create(
> spin_lock_init(&d->shutdown_lock);
> d->shutdown_code = -1;
>
> + spin_lock_init(&d->pbuf_lock);
> +
> err = -ENOMEM;
> if ( !zalloc_cpumask_var(&d->domain_dirty_cpumask) )
> goto fail;
> @@ -286,6 +288,10 @@ struct domain *domain_create(
> d->mem_event = xzalloc(struct mem_event_per_domain);
> if ( !d->mem_event )
> goto fail;
> +
> + d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
> + if ( !d->pbuf )
> + goto fail;
> }
>
> if ( (err = arch_domain_create(d, domcr_flags)) != 0 )
> @@ -318,6 +324,7 @@ struct domain *domain_create(
> d->is_dying = DOMDYING_dead;
> atomic_set(&d->refcnt, DOMAIN_DESTROYED);
> xfree(d->mem_event);
> + xfree(d->pbuf);
> if ( init_status & INIT_arch )
> arch_domain_destroy(d);
> if ( init_status & INIT_gnttab )
> @@ -730,6 +737,7 @@ static void complete_domain_destroy(struct rcu_head *head)
> #endif
>
> xfree(d->mem_event);
> + xfree(d->pbuf);
>
> for ( i = d->max_vcpus - 1; i >= 0; i-- )
> if ( (v = d->vcpu[i]) != NULL )
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 8ac32e4..b8d9a9f 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -24,6 +24,7 @@
> #include <xen/shutdown.h>
> #include <xen/video.h>
> #include <xen/kexec.h>
> +#include <xen/ctype.h>
> #include <asm/debugger.h>
> #include <asm/div64.h>
> #include <xen/hypercall.h> /* for do_console_io */
> @@ -375,6 +376,7 @@ static long
> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
> {
> char kbuf[128];
> int kcount;
> + struct domain *cd = current->domain;
>
> while ( count > 0 )
> {
> @@ -388,19 +390,60 @@ static long
> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
> return -EFAULT;
> kbuf[kcount] = '\0';
>
> - spin_lock_irq(&console_lock);
> + if ( is_hardware_domain(cd) )
> + {
> + /* Use direct console output as it could be interactive */
> + spin_lock_irq(&console_lock);
> +
> + sercon_puts(kbuf);
> + video_puts(kbuf);
>
> - sercon_puts(kbuf);
> - video_puts(kbuf);
> + if ( opt_console_to_ring )
> + {
> + conring_puts(kbuf);
> + tasklet_schedule(¬ify_dom0_con_ring_tasklet);
> + }
>
> - if ( opt_console_to_ring )
> + spin_unlock_irq(&console_lock);
> + }
> + else
> {
> - conring_puts(kbuf);
> - tasklet_schedule(¬ify_dom0_con_ring_tasklet);
> + char *kin = kbuf;
> + char *kout = kbuf;
> + char c;
> + /* Strip non-printable characters */
> + for ( ; ; )
> + {
> + c = *kin++;
> + if ( c == '\0' || c == '\n' )
> + break;
> + if ( isprint(c) || c == '\t' )
> + *kout++ = c;
> + }
> + *kout = '\0';
> + spin_lock(&cd->pbuf_lock);
> + if ( c == '\n' )
> + {
> + kcount = kin - kbuf;
> + cd->pbuf[cd->pbuf_idx] = '\0';
> + guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
> + cd->pbuf_idx = 0;
> + }
> + else if ( cd->pbuf_idx + kcount < (DOMAIN_PBUF_SIZE - 1) )
> + {
> + /* buffer the output until a newline */
> + memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kcount);
> + cd->pbuf_idx += kcount;
> + }
> + else
> + {
> + cd->pbuf[cd->pbuf_idx] = '\0';
> + guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
> + cd->pbuf_idx = 0;
> + }
> + spin_unlock(&cd->pbuf_lock);
> }
>
> - spin_unlock_irq(&console_lock);
> -
> guest_handle_add_offset(buffer, kcount);
> count -= kcount;
> }
> @@ -504,12 +547,12 @@ static int printk_prefix_check(char *p, char **pp)
> ((loglvl < upper_thresh) && printk_ratelimit()));
> }
>
> -static void printk_start_of_line(void)
> +static void printk_start_of_line(const char *prefix)
> {
> struct tm tm;
> char tstr[32];
>
> - __putstr("(XEN) ");
> + __putstr(prefix);
>
> if ( !opt_console_timestamps )
> return;
> @@ -524,12 +567,11 @@ static void printk_start_of_line(void)
> __putstr(tstr);
> }
>
> -void printk(const char *fmt, ...)
> +static void vprintk_common(const char *prefix, const char *fmt, va_list args)
> {
> static char buf[1024];
> static int start_of_line = 1, do_print;
>
> - va_list args;
> char *p, *q;
> unsigned long flags;
>
> @@ -537,9 +579,7 @@ void printk(const char *fmt, ...)
> local_irq_save(flags);
> spin_lock_recursive(&console_lock);
>
> - va_start(args, fmt);
> (void)vsnprintf(buf, sizeof(buf), fmt, args);
> - va_end(args);
>
> p = buf;
>
> @@ -551,7 +591,7 @@ void printk(const char *fmt, ...)
> if ( do_print )
> {
> if ( start_of_line )
> - printk_start_of_line();
> + printk_start_of_line(prefix);
> __putstr(p);
> __putstr("\n");
> }
> @@ -566,7 +606,7 @@ void printk(const char *fmt, ...)
> if ( do_print )
> {
> if ( start_of_line )
> - printk_start_of_line();
> + printk_start_of_line(prefix);
> __putstr(p);
> }
> start_of_line = 0;
> @@ -576,6 +616,26 @@ void printk(const char *fmt, ...)
> local_irq_restore(flags);
> }
>
> +void printk(const char *fmt, ...)
> +{
> + va_list args;
> + va_start(args, fmt);
> + vprintk_common("(XEN) ", fmt, args);
> + va_end(args);
> +}
> +
> +void guest_printk(struct domain *d, const char *fmt, ...)
> +{
> + va_list args;
> + char prefix[16];
> +
> + snprintf(prefix, sizeof(prefix), "(d%d) ", d->domain_id);
> +
> + va_start(args, fmt);
> + vprintk_common(prefix, fmt, args);
> + va_end(args);
> +}
> +
> void __init console_init_preirq(void)
> {
> char *p;
> @@ -791,7 +851,7 @@ int __printk_ratelimit(int ratelimit_ms, int
> ratelimit_burst)
> snprintf(lost_str, sizeof(lost_str), "%d", lost);
> /* console_lock may already be acquired by printk(). */
> spin_lock_recursive(&console_lock);
> - printk_start_of_line();
> + printk_start_of_line("(XEN) ");
> __putstr("printk: ");
> __putstr(lost_str);
> __putstr(" messages suppressed.\n");
> diff --git a/xen/include/asm-x86/hvm/domain.h
> b/xen/include/asm-x86/hvm/domain.h
> index 27b3de5..b1e3187 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -62,12 +62,6 @@ struct hvm_domain {
> /* emulated irq to pirq */
> struct radix_tree_root emuirq_pirq;
>
> - /* hvm_print_line() logging. */
> -#define HVM_PBUF_SIZE 80
> - char *pbuf;
> - int pbuf_idx;
> - spinlock_t pbuf_lock;
> -
> uint64_t *params;
>
> /* Memory ranges with pinned cache attributes. */
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 74b34eb..40afe12 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -83,6 +83,8 @@ extern void debugtrace_printk(const char *fmt, ...);
> #define _p(_x) ((void *)(unsigned long)(_x))
> extern void printk(const char *format, ...)
> __attribute__ ((format (printf, 1, 2)));
> +extern void guest_printk(struct domain *d, const char *format, ...)
> + __attribute__ ((format (printf, 2, 3)));
> extern void panic(const char *format, ...)
> __attribute__ ((format (printf, 1, 2)));
> extern long vm_assist(struct domain *, unsigned int, unsigned int);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ae6a3b8..0013a8d 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -341,6 +341,12 @@ struct domain
> /* Control-plane tools handle for this domain. */
> xen_domain_handle_t handle;
>
> + /* hvm_print_line() and guest_console_write() logging. */
> +#define DOMAIN_PBUF_SIZE 80
> + char *pbuf;
> + unsigned pbuf_idx;
> + spinlock_t pbuf_lock;
> +
> /* OProfile support. */
> struct xenoprof *xenoprof;
> int32_t time_offset_seconds;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xen/console: buffer and show origin of guest PV writes
2013-09-09 11:13 ` Keir Fraser
@ 2013-09-09 12:13 ` Ian Campbell
2013-09-09 13:43 ` Keir Fraser
2013-09-09 12:48 ` Jan Beulich
2013-09-09 13:46 ` Daniel De Graaf
2 siblings, 1 reply; 8+ messages in thread
From: Ian Campbell @ 2013-09-09 12:13 UTC (permalink / raw)
To: Keir Fraser; +Cc: Andrew Cooper, Daniel De Graaf, JBeulich, xen-devel
On Mon, 2013-09-09 at 04:13 -0700, Keir Fraser wrote:
> On 16/08/2013 20:01, "Daniel De Graaf" <dgdegra@tycho.nsa.gov> wrote:
>
> > Guests other than domain 0 using the console output have previously been
> > controlled by the VERBOSE define, but with no designation of which
> > guest's output was on the console. This patch converts the HVM output
> > buffering to be used by all domains, line buffering their output and
> > prefixing it with the domain ID. This is especially useful for debugging
> > stub domains.
> >
> > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>
> This seems good, but, if we process and buffer dom0's output, we lose the
> possibility of running a terminal session in dom0 over the Xen console.
> Personally I do that quite a bit -- serial access only, get Xen's debugging
> there, but also can log in to dom0. Does noone else??
I think I do too, I'm more likely to on ARM as well since there is often
only a single debug serial port and no graphics.
Oh, doesn't code like:
> + if ( is_hardware_domain(cd) )
> > + {
> > + /* Use direct console output as it could be interactive */
> > + spin_lock_irq(&console_lock);
> > +
> > + sercon_puts(kbuf);
> > + video_puts(kbuf);
...mean that Daniel has already done the right thing for dom0 here?
>
> -- Keir
>
> > ---
> >
> > Changes since v1 (RFC):
> > - Use prefix of (d%d) in place of (XEN) for both PV and HVM output
> > - Guests other than dom0 have non-printable characters stripped
> > - Use unsigned type for pbuf_idx
> > - Formatting fixes
> >
> > xen/arch/x86/hvm/hvm.c | 27 +++++-------
> > xen/common/domain.c | 8 ++++
> > xen/drivers/char/console.c | 94
> > ++++++++++++++++++++++++++++++++--------
> > xen/include/asm-x86/hvm/domain.h | 6 ---
> > xen/include/xen/lib.h | 2 +
> > xen/include/xen/sched.h | 6 +++
> > 6 files changed, 103 insertions(+), 40 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 1fcaed0..4ff76cc 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -485,8 +485,7 @@ static int hvm_set_ioreq_page(
> > static int hvm_print_line(
> > int dir, uint32_t port, uint32_t bytes, uint32_t *val)
> > {
> > - struct vcpu *curr = current;
> > - struct hvm_domain *hd = &curr->domain->arch.hvm_domain;
> > + struct domain *cd = current->domain;
> > char c = *val;
> >
> > BUG_ON(bytes != 1);
> > @@ -495,17 +494,16 @@ static int hvm_print_line(
> > if ( !isprint(c) && (c != '\n') && (c != '\t') )
> > return X86EMUL_OKAY;
> >
> > - spin_lock(&hd->pbuf_lock);
> > - hd->pbuf[hd->pbuf_idx++] = c;
> > - if ( (hd->pbuf_idx == (HVM_PBUF_SIZE - 2)) || (c == '\n') )
> > + spin_lock(&cd->pbuf_lock);
> > + if ( c != '\n' )
> > + cd->pbuf[cd->pbuf_idx++] = c;
> > + if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
> > {
> > - if ( c != '\n' )
> > - hd->pbuf[hd->pbuf_idx++] = '\n';
> > - hd->pbuf[hd->pbuf_idx] = '\0';
> > - printk(XENLOG_G_DEBUG "HVM%u: %s", curr->domain->domain_id,
> > hd->pbuf);
> > - hd->pbuf_idx = 0;
> > + cd->pbuf[cd->pbuf_idx] = '\0';
> > + guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
> > + cd->pbuf_idx = 0;
> > }
> > - spin_unlock(&hd->pbuf_lock);
> > + spin_unlock(&cd->pbuf_lock);
> >
> > return X86EMUL_OKAY;
> > }
> > @@ -521,19 +519,16 @@ int hvm_domain_initialise(struct domain *d)
> > return -EINVAL;
> > }
> >
> > - spin_lock_init(&d->arch.hvm_domain.pbuf_lock);
> > spin_lock_init(&d->arch.hvm_domain.irq_lock);
> > spin_lock_init(&d->arch.hvm_domain.uc_lock);
> >
> > INIT_LIST_HEAD(&d->arch.hvm_domain.msixtbl_list);
> > spin_lock_init(&d->arch.hvm_domain.msixtbl_list_lock);
> >
> > - d->arch.hvm_domain.pbuf = xzalloc_array(char, HVM_PBUF_SIZE);
> > d->arch.hvm_domain.params = xzalloc_array(uint64_t, HVM_NR_PARAMS);
> > d->arch.hvm_domain.io_handler = xmalloc(struct hvm_io_handler);
> > rc = -ENOMEM;
> > - if ( !d->arch.hvm_domain.pbuf || !d->arch.hvm_domain.params ||
> > - !d->arch.hvm_domain.io_handler )
> > + if ( !d->arch.hvm_domain.params || !d->arch.hvm_domain.io_handler )
> > goto fail0;
> > d->arch.hvm_domain.io_handler->num_slot = 0;
> >
> > @@ -578,7 +573,6 @@ int hvm_domain_initialise(struct domain *d)
> > fail0:
> > xfree(d->arch.hvm_domain.io_handler);
> > xfree(d->arch.hvm_domain.params);
> > - xfree(d->arch.hvm_domain.pbuf);
> > return rc;
> > }
> >
> > @@ -603,7 +597,6 @@ void hvm_domain_relinquish_resources(struct domain *d)
> >
> > xfree(d->arch.hvm_domain.io_handler);
> > xfree(d->arch.hvm_domain.params);
> > - xfree(d->arch.hvm_domain.pbuf);
> > }
> >
> > void hvm_domain_destroy(struct domain *d)
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 6c264a5..daac2c9 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -231,6 +231,8 @@ struct domain *domain_create(
> > spin_lock_init(&d->shutdown_lock);
> > d->shutdown_code = -1;
> >
> > + spin_lock_init(&d->pbuf_lock);
> > +
> > err = -ENOMEM;
> > if ( !zalloc_cpumask_var(&d->domain_dirty_cpumask) )
> > goto fail;
> > @@ -286,6 +288,10 @@ struct domain *domain_create(
> > d->mem_event = xzalloc(struct mem_event_per_domain);
> > if ( !d->mem_event )
> > goto fail;
> > +
> > + d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
> > + if ( !d->pbuf )
> > + goto fail;
> > }
> >
> > if ( (err = arch_domain_create(d, domcr_flags)) != 0 )
> > @@ -318,6 +324,7 @@ struct domain *domain_create(
> > d->is_dying = DOMDYING_dead;
> > atomic_set(&d->refcnt, DOMAIN_DESTROYED);
> > xfree(d->mem_event);
> > + xfree(d->pbuf);
> > if ( init_status & INIT_arch )
> > arch_domain_destroy(d);
> > if ( init_status & INIT_gnttab )
> > @@ -730,6 +737,7 @@ static void complete_domain_destroy(struct rcu_head *head)
> > #endif
> >
> > xfree(d->mem_event);
> > + xfree(d->pbuf);
> >
> > for ( i = d->max_vcpus - 1; i >= 0; i-- )
> > if ( (v = d->vcpu[i]) != NULL )
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 8ac32e4..b8d9a9f 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -24,6 +24,7 @@
> > #include <xen/shutdown.h>
> > #include <xen/video.h>
> > #include <xen/kexec.h>
> > +#include <xen/ctype.h>
> > #include <asm/debugger.h>
> > #include <asm/div64.h>
> > #include <xen/hypercall.h> /* for do_console_io */
> > @@ -375,6 +376,7 @@ static long
> > guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
> > {
> > char kbuf[128];
> > int kcount;
> > + struct domain *cd = current->domain;
> >
> > while ( count > 0 )
> > {
> > @@ -388,19 +390,60 @@ static long
> > guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
> > return -EFAULT;
> > kbuf[kcount] = '\0';
> >
> > - spin_lock_irq(&console_lock);
> > + if ( is_hardware_domain(cd) )
> > + {
> > + /* Use direct console output as it could be interactive */
> > + spin_lock_irq(&console_lock);
> > +
> > + sercon_puts(kbuf);
> > + video_puts(kbuf);
> >
> > - sercon_puts(kbuf);
> > - video_puts(kbuf);
> > + if ( opt_console_to_ring )
> > + {
> > + conring_puts(kbuf);
> > + tasklet_schedule(¬ify_dom0_con_ring_tasklet);
> > + }
> >
> > - if ( opt_console_to_ring )
> > + spin_unlock_irq(&console_lock);
> > + }
> > + else
> > {
> > - conring_puts(kbuf);
> > - tasklet_schedule(¬ify_dom0_con_ring_tasklet);
> > + char *kin = kbuf;
> > + char *kout = kbuf;
> > + char c;
> > + /* Strip non-printable characters */
> > + for ( ; ; )
> > + {
> > + c = *kin++;
> > + if ( c == '\0' || c == '\n' )
> > + break;
> > + if ( isprint(c) || c == '\t' )
> > + *kout++ = c;
> > + }
> > + *kout = '\0';
> > + spin_lock(&cd->pbuf_lock);
> > + if ( c == '\n' )
> > + {
> > + kcount = kin - kbuf;
> > + cd->pbuf[cd->pbuf_idx] = '\0';
> > + guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
> > + cd->pbuf_idx = 0;
> > + }
> > + else if ( cd->pbuf_idx + kcount < (DOMAIN_PBUF_SIZE - 1) )
> > + {
> > + /* buffer the output until a newline */
> > + memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kcount);
> > + cd->pbuf_idx += kcount;
> > + }
> > + else
> > + {
> > + cd->pbuf[cd->pbuf_idx] = '\0';
> > + guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
> > + cd->pbuf_idx = 0;
> > + }
> > + spin_unlock(&cd->pbuf_lock);
> > }
> >
> > - spin_unlock_irq(&console_lock);
> > -
> > guest_handle_add_offset(buffer, kcount);
> > count -= kcount;
> > }
> > @@ -504,12 +547,12 @@ static int printk_prefix_check(char *p, char **pp)
> > ((loglvl < upper_thresh) && printk_ratelimit()));
> > }
> >
> > -static void printk_start_of_line(void)
> > +static void printk_start_of_line(const char *prefix)
> > {
> > struct tm tm;
> > char tstr[32];
> >
> > - __putstr("(XEN) ");
> > + __putstr(prefix);
> >
> > if ( !opt_console_timestamps )
> > return;
> > @@ -524,12 +567,11 @@ static void printk_start_of_line(void)
> > __putstr(tstr);
> > }
> >
> > -void printk(const char *fmt, ...)
> > +static void vprintk_common(const char *prefix, const char *fmt, va_list args)
> > {
> > static char buf[1024];
> > static int start_of_line = 1, do_print;
> >
> > - va_list args;
> > char *p, *q;
> > unsigned long flags;
> >
> > @@ -537,9 +579,7 @@ void printk(const char *fmt, ...)
> > local_irq_save(flags);
> > spin_lock_recursive(&console_lock);
> >
> > - va_start(args, fmt);
> > (void)vsnprintf(buf, sizeof(buf), fmt, args);
> > - va_end(args);
> >
> > p = buf;
> >
> > @@ -551,7 +591,7 @@ void printk(const char *fmt, ...)
> > if ( do_print )
> > {
> > if ( start_of_line )
> > - printk_start_of_line();
> > + printk_start_of_line(prefix);
> > __putstr(p);
> > __putstr("\n");
> > }
> > @@ -566,7 +606,7 @@ void printk(const char *fmt, ...)
> > if ( do_print )
> > {
> > if ( start_of_line )
> > - printk_start_of_line();
> > + printk_start_of_line(prefix);
> > __putstr(p);
> > }
> > start_of_line = 0;
> > @@ -576,6 +616,26 @@ void printk(const char *fmt, ...)
> > local_irq_restore(flags);
> > }
> >
> > +void printk(const char *fmt, ...)
> > +{
> > + va_list args;
> > + va_start(args, fmt);
> > + vprintk_common("(XEN) ", fmt, args);
> > + va_end(args);
> > +}
> > +
> > +void guest_printk(struct domain *d, const char *fmt, ...)
> > +{
> > + va_list args;
> > + char prefix[16];
> > +
> > + snprintf(prefix, sizeof(prefix), "(d%d) ", d->domain_id);
> > +
> > + va_start(args, fmt);
> > + vprintk_common(prefix, fmt, args);
> > + va_end(args);
> > +}
> > +
> > void __init console_init_preirq(void)
> > {
> > char *p;
> > @@ -791,7 +851,7 @@ int __printk_ratelimit(int ratelimit_ms, int
> > ratelimit_burst)
> > snprintf(lost_str, sizeof(lost_str), "%d", lost);
> > /* console_lock may already be acquired by printk(). */
> > spin_lock_recursive(&console_lock);
> > - printk_start_of_line();
> > + printk_start_of_line("(XEN) ");
> > __putstr("printk: ");
> > __putstr(lost_str);
> > __putstr(" messages suppressed.\n");
> > diff --git a/xen/include/asm-x86/hvm/domain.h
> > b/xen/include/asm-x86/hvm/domain.h
> > index 27b3de5..b1e3187 100644
> > --- a/xen/include/asm-x86/hvm/domain.h
> > +++ b/xen/include/asm-x86/hvm/domain.h
> > @@ -62,12 +62,6 @@ struct hvm_domain {
> > /* emulated irq to pirq */
> > struct radix_tree_root emuirq_pirq;
> >
> > - /* hvm_print_line() logging. */
> > -#define HVM_PBUF_SIZE 80
> > - char *pbuf;
> > - int pbuf_idx;
> > - spinlock_t pbuf_lock;
> > -
> > uint64_t *params;
> >
> > /* Memory ranges with pinned cache attributes. */
> > diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> > index 74b34eb..40afe12 100644
> > --- a/xen/include/xen/lib.h
> > +++ b/xen/include/xen/lib.h
> > @@ -83,6 +83,8 @@ extern void debugtrace_printk(const char *fmt, ...);
> > #define _p(_x) ((void *)(unsigned long)(_x))
> > extern void printk(const char *format, ...)
> > __attribute__ ((format (printf, 1, 2)));
> > +extern void guest_printk(struct domain *d, const char *format, ...)
> > + __attribute__ ((format (printf, 2, 3)));
> > extern void panic(const char *format, ...)
> > __attribute__ ((format (printf, 1, 2)));
> > extern long vm_assist(struct domain *, unsigned int, unsigned int);
> > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > index ae6a3b8..0013a8d 100644
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -341,6 +341,12 @@ struct domain
> > /* Control-plane tools handle for this domain. */
> > xen_domain_handle_t handle;
> >
> > + /* hvm_print_line() and guest_console_write() logging. */
> > +#define DOMAIN_PBUF_SIZE 80
> > + char *pbuf;
> > + unsigned pbuf_idx;
> > + spinlock_t pbuf_lock;
> > +
> > /* OProfile support. */
> > struct xenoprof *xenoprof;
> > int32_t time_offset_seconds;
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xen/console: buffer and show origin of guest PV writes
2013-09-09 11:13 ` Keir Fraser
2013-09-09 12:13 ` Ian Campbell
@ 2013-09-09 12:48 ` Jan Beulich
2013-09-09 13:46 ` Daniel De Graaf
2 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2013-09-09 12:48 UTC (permalink / raw)
To: Keir Fraser, Daniel De Graaf; +Cc: Andrew Cooper, xen-devel
>>> On 09.09.13 at 13:13, Keir Fraser <keir.xen@gmail.com> wrote:
> On 16/08/2013 20:01, "Daniel De Graaf" <dgdegra@tycho.nsa.gov> wrote:
>
>> Guests other than domain 0 using the console output have previously been
>> controlled by the VERBOSE define, but with no designation of which
>> guest's output was on the console. This patch converts the HVM output
>> buffering to be used by all domains, line buffering their output and
>> prefixing it with the domain ID. This is especially useful for debugging
>> stub domains.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>
> This seems good, but, if we process and buffer dom0's output, we lose the
> possibility of running a terminal session in dom0 over the Xen console.
> Personally I do that quite a bit -- serial access only, get Xen's debugging
> there, but also can log in to dom0. Does noone else??
I use this only rarely; where possible I prefer using the normal
(VGA) console, and in most other cases ssh access to a remote
machine is available. But yes, I view this as an important feature
too, and simply failed to realize it would get broken by the change
here.
Jan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xen/console: buffer and show origin of guest PV writes
2013-09-09 12:13 ` Ian Campbell
@ 2013-09-09 13:43 ` Keir Fraser
2013-09-09 13:46 ` Keir Fraser
0 siblings, 1 reply; 8+ messages in thread
From: Keir Fraser @ 2013-09-09 13:43 UTC (permalink / raw)
To: Ian Campbell; +Cc: Andrew Cooper, Daniel De Graaf, JBeulich, xen-devel
On 09/09/2013 05:13, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:
> On Mon, 2013-09-09 at 04:13 -0700, Keir Fraser wrote:
>> On 16/08/2013 20:01, "Daniel De Graaf" <dgdegra@tycho.nsa.gov> wrote:
>>
>>> Guests other than domain 0 using the console output have previously been
>>> controlled by the VERBOSE define, but with no designation of which
>>> guest's output was on the console. This patch converts the HVM output
>>> buffering to be used by all domains, line buffering their output and
>>> prefixing it with the domain ID. This is especially useful for debugging
>>> stub domains.
>>>
>>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>
>> This seems good, but, if we process and buffer dom0's output, we lose the
>> possibility of running a terminal session in dom0 over the Xen console.
>> Personally I do that quite a bit -- serial access only, get Xen's debugging
>> there, but also can log in to dom0. Does noone else??
>
> I think I do too, I'm more likely to on ARM as well since there is often
> only a single debug serial port and no graphics.
>
> Oh, doesn't code like:
>> + if ( is_hardware_domain(cd) )
>>> + {
>>> + /* Use direct console output as it could be interactive */
>>> + spin_lock_irq(&console_lock);
>>> +
>>> + sercon_puts(kbuf);
>>> + video_puts(kbuf);
>
> ...mean that Daniel has already done the right thing for dom0 here?
Ah, you could be right. Well if he will confirm that then I will give my
Ack.
-- Keir
>>
>> -- Keir
>>
>>> ---
>>>
>>> Changes since v1 (RFC):
>>> - Use prefix of (d%d) in place of (XEN) for both PV and HVM output
>>> - Guests other than dom0 have non-printable characters stripped
>>> - Use unsigned type for pbuf_idx
>>> - Formatting fixes
>>>
>>> xen/arch/x86/hvm/hvm.c | 27 +++++-------
>>> xen/common/domain.c | 8 ++++
>>> xen/drivers/char/console.c | 94
>>> ++++++++++++++++++++++++++++++++--------
>>> xen/include/asm-x86/hvm/domain.h | 6 ---
>>> xen/include/xen/lib.h | 2 +
>>> xen/include/xen/sched.h | 6 +++
>>> 6 files changed, 103 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index 1fcaed0..4ff76cc 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -485,8 +485,7 @@ static int hvm_set_ioreq_page(
>>> static int hvm_print_line(
>>> int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>>> {
>>> - struct vcpu *curr = current;
>>> - struct hvm_domain *hd = &curr->domain->arch.hvm_domain;
>>> + struct domain *cd = current->domain;
>>> char c = *val;
>>>
>>> BUG_ON(bytes != 1);
>>> @@ -495,17 +494,16 @@ static int hvm_print_line(
>>> if ( !isprint(c) && (c != '\n') && (c != '\t') )
>>> return X86EMUL_OKAY;
>>>
>>> - spin_lock(&hd->pbuf_lock);
>>> - hd->pbuf[hd->pbuf_idx++] = c;
>>> - if ( (hd->pbuf_idx == (HVM_PBUF_SIZE - 2)) || (c == '\n') )
>>> + spin_lock(&cd->pbuf_lock);
>>> + if ( c != '\n' )
>>> + cd->pbuf[cd->pbuf_idx++] = c;
>>> + if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
>>> {
>>> - if ( c != '\n' )
>>> - hd->pbuf[hd->pbuf_idx++] = '\n';
>>> - hd->pbuf[hd->pbuf_idx] = '\0';
>>> - printk(XENLOG_G_DEBUG "HVM%u: %s", curr->domain->domain_id,
>>> hd->pbuf);
>>> - hd->pbuf_idx = 0;
>>> + cd->pbuf[cd->pbuf_idx] = '\0';
>>> + guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
>>> + cd->pbuf_idx = 0;
>>> }
>>> - spin_unlock(&hd->pbuf_lock);
>>> + spin_unlock(&cd->pbuf_lock);
>>>
>>> return X86EMUL_OKAY;
>>> }
>>> @@ -521,19 +519,16 @@ int hvm_domain_initialise(struct domain *d)
>>> return -EINVAL;
>>> }
>>>
>>> - spin_lock_init(&d->arch.hvm_domain.pbuf_lock);
>>> spin_lock_init(&d->arch.hvm_domain.irq_lock);
>>> spin_lock_init(&d->arch.hvm_domain.uc_lock);
>>>
>>> INIT_LIST_HEAD(&d->arch.hvm_domain.msixtbl_list);
>>> spin_lock_init(&d->arch.hvm_domain.msixtbl_list_lock);
>>>
>>> - d->arch.hvm_domain.pbuf = xzalloc_array(char, HVM_PBUF_SIZE);
>>> d->arch.hvm_domain.params = xzalloc_array(uint64_t, HVM_NR_PARAMS);
>>> d->arch.hvm_domain.io_handler = xmalloc(struct hvm_io_handler);
>>> rc = -ENOMEM;
>>> - if ( !d->arch.hvm_domain.pbuf || !d->arch.hvm_domain.params ||
>>> - !d->arch.hvm_domain.io_handler )
>>> + if ( !d->arch.hvm_domain.params || !d->arch.hvm_domain.io_handler )
>>> goto fail0;
>>> d->arch.hvm_domain.io_handler->num_slot = 0;
>>>
>>> @@ -578,7 +573,6 @@ int hvm_domain_initialise(struct domain *d)
>>> fail0:
>>> xfree(d->arch.hvm_domain.io_handler);
>>> xfree(d->arch.hvm_domain.params);
>>> - xfree(d->arch.hvm_domain.pbuf);
>>> return rc;
>>> }
>>>
>>> @@ -603,7 +597,6 @@ void hvm_domain_relinquish_resources(struct domain *d)
>>>
>>> xfree(d->arch.hvm_domain.io_handler);
>>> xfree(d->arch.hvm_domain.params);
>>> - xfree(d->arch.hvm_domain.pbuf);
>>> }
>>>
>>> void hvm_domain_destroy(struct domain *d)
>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>> index 6c264a5..daac2c9 100644
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -231,6 +231,8 @@ struct domain *domain_create(
>>> spin_lock_init(&d->shutdown_lock);
>>> d->shutdown_code = -1;
>>>
>>> + spin_lock_init(&d->pbuf_lock);
>>> +
>>> err = -ENOMEM;
>>> if ( !zalloc_cpumask_var(&d->domain_dirty_cpumask) )
>>> goto fail;
>>> @@ -286,6 +288,10 @@ struct domain *domain_create(
>>> d->mem_event = xzalloc(struct mem_event_per_domain);
>>> if ( !d->mem_event )
>>> goto fail;
>>> +
>>> + d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
>>> + if ( !d->pbuf )
>>> + goto fail;
>>> }
>>>
>>> if ( (err = arch_domain_create(d, domcr_flags)) != 0 )
>>> @@ -318,6 +324,7 @@ struct domain *domain_create(
>>> d->is_dying = DOMDYING_dead;
>>> atomic_set(&d->refcnt, DOMAIN_DESTROYED);
>>> xfree(d->mem_event);
>>> + xfree(d->pbuf);
>>> if ( init_status & INIT_arch )
>>> arch_domain_destroy(d);
>>> if ( init_status & INIT_gnttab )
>>> @@ -730,6 +737,7 @@ static void complete_domain_destroy(struct rcu_head
>>> *head)
>>> #endif
>>>
>>> xfree(d->mem_event);
>>> + xfree(d->pbuf);
>>>
>>> for ( i = d->max_vcpus - 1; i >= 0; i-- )
>>> if ( (v = d->vcpu[i]) != NULL )
>>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
>>> index 8ac32e4..b8d9a9f 100644
>>> --- a/xen/drivers/char/console.c
>>> +++ b/xen/drivers/char/console.c
>>> @@ -24,6 +24,7 @@
>>> #include <xen/shutdown.h>
>>> #include <xen/video.h>
>>> #include <xen/kexec.h>
>>> +#include <xen/ctype.h>
>>> #include <asm/debugger.h>
>>> #include <asm/div64.h>
>>> #include <xen/hypercall.h> /* for do_console_io */
>>> @@ -375,6 +376,7 @@ static long
>>> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
>>> {
>>> char kbuf[128];
>>> int kcount;
>>> + struct domain *cd = current->domain;
>>>
>>> while ( count > 0 )
>>> {
>>> @@ -388,19 +390,60 @@ static long
>>> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
>>> return -EFAULT;
>>> kbuf[kcount] = '\0';
>>>
>>> - spin_lock_irq(&console_lock);
>>> + if ( is_hardware_domain(cd) )
>>> + {
>>> + /* Use direct console output as it could be interactive */
>>> + spin_lock_irq(&console_lock);
>>> +
>>> + sercon_puts(kbuf);
>>> + video_puts(kbuf);
>>>
>>> - sercon_puts(kbuf);
>>> - video_puts(kbuf);
>>> + if ( opt_console_to_ring )
>>> + {
>>> + conring_puts(kbuf);
>>> + tasklet_schedule(¬ify_dom0_con_ring_tasklet);
>>> + }
>>>
>>> - if ( opt_console_to_ring )
>>> + spin_unlock_irq(&console_lock);
>>> + }
>>> + else
>>> {
>>> - conring_puts(kbuf);
>>> - tasklet_schedule(¬ify_dom0_con_ring_tasklet);
>>> + char *kin = kbuf;
>>> + char *kout = kbuf;
>>> + char c;
>>> + /* Strip non-printable characters */
>>> + for ( ; ; )
>>> + {
>>> + c = *kin++;
>>> + if ( c == '\0' || c == '\n' )
>>> + break;
>>> + if ( isprint(c) || c == '\t' )
>>> + *kout++ = c;
>>> + }
>>> + *kout = '\0';
>>> + spin_lock(&cd->pbuf_lock);
>>> + if ( c == '\n' )
>>> + {
>>> + kcount = kin - kbuf;
>>> + cd->pbuf[cd->pbuf_idx] = '\0';
>>> + guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
>>> + cd->pbuf_idx = 0;
>>> + }
>>> + else if ( cd->pbuf_idx + kcount < (DOMAIN_PBUF_SIZE - 1) )
>>> + {
>>> + /* buffer the output until a newline */
>>> + memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kcount);
>>> + cd->pbuf_idx += kcount;
>>> + }
>>> + else
>>> + {
>>> + cd->pbuf[cd->pbuf_idx] = '\0';
>>> + guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
>>> + cd->pbuf_idx = 0;
>>> + }
>>> + spin_unlock(&cd->pbuf_lock);
>>> }
>>>
>>> - spin_unlock_irq(&console_lock);
>>> -
>>> guest_handle_add_offset(buffer, kcount);
>>> count -= kcount;
>>> }
>>> @@ -504,12 +547,12 @@ static int printk_prefix_check(char *p, char **pp)
>>> ((loglvl < upper_thresh) && printk_ratelimit()));
>>> }
>>>
>>> -static void printk_start_of_line(void)
>>> +static void printk_start_of_line(const char *prefix)
>>> {
>>> struct tm tm;
>>> char tstr[32];
>>>
>>> - __putstr("(XEN) ");
>>> + __putstr(prefix);
>>>
>>> if ( !opt_console_timestamps )
>>> return;
>>> @@ -524,12 +567,11 @@ static void printk_start_of_line(void)
>>> __putstr(tstr);
>>> }
>>>
>>> -void printk(const char *fmt, ...)
>>> +static void vprintk_common(const char *prefix, const char *fmt, va_list
>>> args)
>>> {
>>> static char buf[1024];
>>> static int start_of_line = 1, do_print;
>>>
>>> - va_list args;
>>> char *p, *q;
>>> unsigned long flags;
>>>
>>> @@ -537,9 +579,7 @@ void printk(const char *fmt, ...)
>>> local_irq_save(flags);
>>> spin_lock_recursive(&console_lock);
>>>
>>> - va_start(args, fmt);
>>> (void)vsnprintf(buf, sizeof(buf), fmt, args);
>>> - va_end(args);
>>>
>>> p = buf;
>>>
>>> @@ -551,7 +591,7 @@ void printk(const char *fmt, ...)
>>> if ( do_print )
>>> {
>>> if ( start_of_line )
>>> - printk_start_of_line();
>>> + printk_start_of_line(prefix);
>>> __putstr(p);
>>> __putstr("\n");
>>> }
>>> @@ -566,7 +606,7 @@ void printk(const char *fmt, ...)
>>> if ( do_print )
>>> {
>>> if ( start_of_line )
>>> - printk_start_of_line();
>>> + printk_start_of_line(prefix);
>>> __putstr(p);
>>> }
>>> start_of_line = 0;
>>> @@ -576,6 +616,26 @@ void printk(const char *fmt, ...)
>>> local_irq_restore(flags);
>>> }
>>>
>>> +void printk(const char *fmt, ...)
>>> +{
>>> + va_list args;
>>> + va_start(args, fmt);
>>> + vprintk_common("(XEN) ", fmt, args);
>>> + va_end(args);
>>> +}
>>> +
>>> +void guest_printk(struct domain *d, const char *fmt, ...)
>>> +{
>>> + va_list args;
>>> + char prefix[16];
>>> +
>>> + snprintf(prefix, sizeof(prefix), "(d%d) ", d->domain_id);
>>> +
>>> + va_start(args, fmt);
>>> + vprintk_common(prefix, fmt, args);
>>> + va_end(args);
>>> +}
>>> +
>>> void __init console_init_preirq(void)
>>> {
>>> char *p;
>>> @@ -791,7 +851,7 @@ int __printk_ratelimit(int ratelimit_ms, int
>>> ratelimit_burst)
>>> snprintf(lost_str, sizeof(lost_str), "%d", lost);
>>> /* console_lock may already be acquired by printk(). */
>>> spin_lock_recursive(&console_lock);
>>> - printk_start_of_line();
>>> + printk_start_of_line("(XEN) ");
>>> __putstr("printk: ");
>>> __putstr(lost_str);
>>> __putstr(" messages suppressed.\n");
>>> diff --git a/xen/include/asm-x86/hvm/domain.h
>>> b/xen/include/asm-x86/hvm/domain.h
>>> index 27b3de5..b1e3187 100644
>>> --- a/xen/include/asm-x86/hvm/domain.h
>>> +++ b/xen/include/asm-x86/hvm/domain.h
>>> @@ -62,12 +62,6 @@ struct hvm_domain {
>>> /* emulated irq to pirq */
>>> struct radix_tree_root emuirq_pirq;
>>>
>>> - /* hvm_print_line() logging. */
>>> -#define HVM_PBUF_SIZE 80
>>> - char *pbuf;
>>> - int pbuf_idx;
>>> - spinlock_t pbuf_lock;
>>> -
>>> uint64_t *params;
>>>
>>> /* Memory ranges with pinned cache attributes. */
>>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>>> index 74b34eb..40afe12 100644
>>> --- a/xen/include/xen/lib.h
>>> +++ b/xen/include/xen/lib.h
>>> @@ -83,6 +83,8 @@ extern void debugtrace_printk(const char *fmt, ...);
>>> #define _p(_x) ((void *)(unsigned long)(_x))
>>> extern void printk(const char *format, ...)
>>> __attribute__ ((format (printf, 1, 2)));
>>> +extern void guest_printk(struct domain *d, const char *format, ...)
>>> + __attribute__ ((format (printf, 2, 3)));
>>> extern void panic(const char *format, ...)
>>> __attribute__ ((format (printf, 1, 2)));
>>> extern long vm_assist(struct domain *, unsigned int, unsigned int);
>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>> index ae6a3b8..0013a8d 100644
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -341,6 +341,12 @@ struct domain
>>> /* Control-plane tools handle for this domain. */
>>> xen_domain_handle_t handle;
>>>
>>> + /* hvm_print_line() and guest_console_write() logging. */
>>> +#define DOMAIN_PBUF_SIZE 80
>>> + char *pbuf;
>>> + unsigned pbuf_idx;
>>> + spinlock_t pbuf_lock;
>>> +
>>> /* OProfile support. */
>>> struct xenoprof *xenoprof;
>>> int32_t time_offset_seconds;
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xen/console: buffer and show origin of guest PV writes
2013-09-09 11:13 ` Keir Fraser
2013-09-09 12:13 ` Ian Campbell
2013-09-09 12:48 ` Jan Beulich
@ 2013-09-09 13:46 ` Daniel De Graaf
2013-09-09 14:14 ` Keir Fraser
2 siblings, 1 reply; 8+ messages in thread
From: Daniel De Graaf @ 2013-09-09 13:46 UTC (permalink / raw)
To: Keir Fraser; +Cc: Andrew Cooper, JBeulich, xen-devel
On 09/09/2013 07:13 AM, Keir Fraser wrote:
> On 16/08/2013 20:01, "Daniel De Graaf" <dgdegra@tycho.nsa.gov> wrote:
>
>> Guests other than domain 0 using the console output have previously been
>> controlled by the VERBOSE define, but with no designation of which
>> guest's output was on the console. This patch converts the HVM output
>> buffering to be used by all domains, line buffering their output and
>> prefixing it with the domain ID. This is especially useful for debugging
>> stub domains.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>
> This seems good, but, if we process and buffer dom0's output, we lose the
> possibility of running a terminal session in dom0 over the Xen console.
> Personally I do that quite a bit -- serial access only, get Xen's debugging
> there, but also can log in to dom0. Does noone else??
I do care about this use case (I also use it rather often), and it is preserved
- this patch explicitly does not buffer or insert characters in dom0's output.
This means that we waste the 80-byte buffer for dom0, but I didn't think it was
worth special-casing dom0 there too (also, doing that might break a PVH dom0
that uses the HVM output - if that method is available, which I did not check).
>
> -- Keir
>
>> ---
>>
>> Changes since v1 (RFC):
>> - Use prefix of (d%d) in place of (XEN) for both PV and HVM output
>> - Guests other than dom0 have non-printable characters stripped
>> - Use unsigned type for pbuf_idx
>> - Formatting fixes
>>
>> xen/arch/x86/hvm/hvm.c | 27 +++++-------
>> xen/common/domain.c | 8 ++++
>> xen/drivers/char/console.c | 94
>> ++++++++++++++++++++++++++++++++--------
>> xen/include/asm-x86/hvm/domain.h | 6 ---
>> xen/include/xen/lib.h | 2 +
>> xen/include/xen/sched.h | 6 +++
>> 6 files changed, 103 insertions(+), 40 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 1fcaed0..4ff76cc 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -485,8 +485,7 @@ static int hvm_set_ioreq_page(
>> static int hvm_print_line(
>> int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>> {
>> - struct vcpu *curr = current;
>> - struct hvm_domain *hd = &curr->domain->arch.hvm_domain;
>> + struct domain *cd = current->domain;
>> char c = *val;
>>
>> BUG_ON(bytes != 1);
>> @@ -495,17 +494,16 @@ static int hvm_print_line(
>> if ( !isprint(c) && (c != '\n') && (c != '\t') )
>> return X86EMUL_OKAY;
>>
>> - spin_lock(&hd->pbuf_lock);
>> - hd->pbuf[hd->pbuf_idx++] = c;
>> - if ( (hd->pbuf_idx == (HVM_PBUF_SIZE - 2)) || (c == '\n') )
>> + spin_lock(&cd->pbuf_lock);
>> + if ( c != '\n' )
>> + cd->pbuf[cd->pbuf_idx++] = c;
>> + if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
>> {
>> - if ( c != '\n' )
>> - hd->pbuf[hd->pbuf_idx++] = '\n';
>> - hd->pbuf[hd->pbuf_idx] = '\0';
>> - printk(XENLOG_G_DEBUG "HVM%u: %s", curr->domain->domain_id,
>> hd->pbuf);
>> - hd->pbuf_idx = 0;
>> + cd->pbuf[cd->pbuf_idx] = '\0';
>> + guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
>> + cd->pbuf_idx = 0;
>> }
>> - spin_unlock(&hd->pbuf_lock);
>> + spin_unlock(&cd->pbuf_lock);
>>
>> return X86EMUL_OKAY;
>> }
>> @@ -521,19 +519,16 @@ int hvm_domain_initialise(struct domain *d)
>> return -EINVAL;
>> }
>>
>> - spin_lock_init(&d->arch.hvm_domain.pbuf_lock);
>> spin_lock_init(&d->arch.hvm_domain.irq_lock);
>> spin_lock_init(&d->arch.hvm_domain.uc_lock);
>>
>> INIT_LIST_HEAD(&d->arch.hvm_domain.msixtbl_list);
>> spin_lock_init(&d->arch.hvm_domain.msixtbl_list_lock);
>>
>> - d->arch.hvm_domain.pbuf = xzalloc_array(char, HVM_PBUF_SIZE);
>> d->arch.hvm_domain.params = xzalloc_array(uint64_t, HVM_NR_PARAMS);
>> d->arch.hvm_domain.io_handler = xmalloc(struct hvm_io_handler);
>> rc = -ENOMEM;
>> - if ( !d->arch.hvm_domain.pbuf || !d->arch.hvm_domain.params ||
>> - !d->arch.hvm_domain.io_handler )
>> + if ( !d->arch.hvm_domain.params || !d->arch.hvm_domain.io_handler )
>> goto fail0;
>> d->arch.hvm_domain.io_handler->num_slot = 0;
>>
>> @@ -578,7 +573,6 @@ int hvm_domain_initialise(struct domain *d)
>> fail0:
>> xfree(d->arch.hvm_domain.io_handler);
>> xfree(d->arch.hvm_domain.params);
>> - xfree(d->arch.hvm_domain.pbuf);
>> return rc;
>> }
>>
>> @@ -603,7 +597,6 @@ void hvm_domain_relinquish_resources(struct domain *d)
>>
>> xfree(d->arch.hvm_domain.io_handler);
>> xfree(d->arch.hvm_domain.params);
>> - xfree(d->arch.hvm_domain.pbuf);
>> }
>>
>> void hvm_domain_destroy(struct domain *d)
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 6c264a5..daac2c9 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -231,6 +231,8 @@ struct domain *domain_create(
>> spin_lock_init(&d->shutdown_lock);
>> d->shutdown_code = -1;
>>
>> + spin_lock_init(&d->pbuf_lock);
>> +
>> err = -ENOMEM;
>> if ( !zalloc_cpumask_var(&d->domain_dirty_cpumask) )
>> goto fail;
>> @@ -286,6 +288,10 @@ struct domain *domain_create(
>> d->mem_event = xzalloc(struct mem_event_per_domain);
>> if ( !d->mem_event )
>> goto fail;
>> +
>> + d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
>> + if ( !d->pbuf )
>> + goto fail;
>> }
>>
>> if ( (err = arch_domain_create(d, domcr_flags)) != 0 )
>> @@ -318,6 +324,7 @@ struct domain *domain_create(
>> d->is_dying = DOMDYING_dead;
>> atomic_set(&d->refcnt, DOMAIN_DESTROYED);
>> xfree(d->mem_event);
>> + xfree(d->pbuf);
>> if ( init_status & INIT_arch )
>> arch_domain_destroy(d);
>> if ( init_status & INIT_gnttab )
>> @@ -730,6 +737,7 @@ static void complete_domain_destroy(struct rcu_head *head)
>> #endif
>>
>> xfree(d->mem_event);
>> + xfree(d->pbuf);
>>
>> for ( i = d->max_vcpus - 1; i >= 0; i-- )
>> if ( (v = d->vcpu[i]) != NULL )
>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
>> index 8ac32e4..b8d9a9f 100644
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -24,6 +24,7 @@
>> #include <xen/shutdown.h>
>> #include <xen/video.h>
>> #include <xen/kexec.h>
>> +#include <xen/ctype.h>
>> #include <asm/debugger.h>
>> #include <asm/div64.h>
>> #include <xen/hypercall.h> /* for do_console_io */
>> @@ -375,6 +376,7 @@ static long
>> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
>> {
>> char kbuf[128];
>> int kcount;
>> + struct domain *cd = current->domain;
>>
>> while ( count > 0 )
>> {
>> @@ -388,19 +390,60 @@ static long
>> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
>> return -EFAULT;
>> kbuf[kcount] = '\0';
>>
>> - spin_lock_irq(&console_lock);
>> + if ( is_hardware_domain(cd) )
>> + {
>> + /* Use direct console output as it could be interactive */
>> + spin_lock_irq(&console_lock);
>> +
>> + sercon_puts(kbuf);
>> + video_puts(kbuf);
>>
>> - sercon_puts(kbuf);
>> - video_puts(kbuf);
>> + if ( opt_console_to_ring )
>> + {
>> + conring_puts(kbuf);
>> + tasklet_schedule(¬ify_dom0_con_ring_tasklet);
>> + }
>>
>> - if ( opt_console_to_ring )
>> + spin_unlock_irq(&console_lock);
>> + }
>> + else
>> {
>> - conring_puts(kbuf);
>> - tasklet_schedule(¬ify_dom0_con_ring_tasklet);
>> + char *kin = kbuf;
>> + char *kout = kbuf;
>> + char c;
>> + /* Strip non-printable characters */
>> + for ( ; ; )
>> + {
>> + c = *kin++;
>> + if ( c == '\0' || c == '\n' )
>> + break;
>> + if ( isprint(c) || c == '\t' )
>> + *kout++ = c;
>> + }
>> + *kout = '\0';
>> + spin_lock(&cd->pbuf_lock);
>> + if ( c == '\n' )
>> + {
>> + kcount = kin - kbuf;
>> + cd->pbuf[cd->pbuf_idx] = '\0';
>> + guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
>> + cd->pbuf_idx = 0;
>> + }
>> + else if ( cd->pbuf_idx + kcount < (DOMAIN_PBUF_SIZE - 1) )
>> + {
>> + /* buffer the output until a newline */
>> + memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kcount);
>> + cd->pbuf_idx += kcount;
>> + }
>> + else
>> + {
>> + cd->pbuf[cd->pbuf_idx] = '\0';
>> + guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
>> + cd->pbuf_idx = 0;
>> + }
>> + spin_unlock(&cd->pbuf_lock);
>> }
>>
>> - spin_unlock_irq(&console_lock);
>> -
>> guest_handle_add_offset(buffer, kcount);
>> count -= kcount;
>> }
>> @@ -504,12 +547,12 @@ static int printk_prefix_check(char *p, char **pp)
>> ((loglvl < upper_thresh) && printk_ratelimit()));
>> }
>>
>> -static void printk_start_of_line(void)
>> +static void printk_start_of_line(const char *prefix)
>> {
>> struct tm tm;
>> char tstr[32];
>>
>> - __putstr("(XEN) ");
>> + __putstr(prefix);
>>
>> if ( !opt_console_timestamps )
>> return;
>> @@ -524,12 +567,11 @@ static void printk_start_of_line(void)
>> __putstr(tstr);
>> }
>>
>> -void printk(const char *fmt, ...)
>> +static void vprintk_common(const char *prefix, const char *fmt, va_list args)
>> {
>> static char buf[1024];
>> static int start_of_line = 1, do_print;
>>
>> - va_list args;
>> char *p, *q;
>> unsigned long flags;
>>
>> @@ -537,9 +579,7 @@ void printk(const char *fmt, ...)
>> local_irq_save(flags);
>> spin_lock_recursive(&console_lock);
>>
>> - va_start(args, fmt);
>> (void)vsnprintf(buf, sizeof(buf), fmt, args);
>> - va_end(args);
>>
>> p = buf;
>>
>> @@ -551,7 +591,7 @@ void printk(const char *fmt, ...)
>> if ( do_print )
>> {
>> if ( start_of_line )
>> - printk_start_of_line();
>> + printk_start_of_line(prefix);
>> __putstr(p);
>> __putstr("\n");
>> }
>> @@ -566,7 +606,7 @@ void printk(const char *fmt, ...)
>> if ( do_print )
>> {
>> if ( start_of_line )
>> - printk_start_of_line();
>> + printk_start_of_line(prefix);
>> __putstr(p);
>> }
>> start_of_line = 0;
>> @@ -576,6 +616,26 @@ void printk(const char *fmt, ...)
>> local_irq_restore(flags);
>> }
>>
>> +void printk(const char *fmt, ...)
>> +{
>> + va_list args;
>> + va_start(args, fmt);
>> + vprintk_common("(XEN) ", fmt, args);
>> + va_end(args);
>> +}
>> +
>> +void guest_printk(struct domain *d, const char *fmt, ...)
>> +{
>> + va_list args;
>> + char prefix[16];
>> +
>> + snprintf(prefix, sizeof(prefix), "(d%d) ", d->domain_id);
>> +
>> + va_start(args, fmt);
>> + vprintk_common(prefix, fmt, args);
>> + va_end(args);
>> +}
>> +
>> void __init console_init_preirq(void)
>> {
>> char *p;
>> @@ -791,7 +851,7 @@ int __printk_ratelimit(int ratelimit_ms, int
>> ratelimit_burst)
>> snprintf(lost_str, sizeof(lost_str), "%d", lost);
>> /* console_lock may already be acquired by printk(). */
>> spin_lock_recursive(&console_lock);
>> - printk_start_of_line();
>> + printk_start_of_line("(XEN) ");
>> __putstr("printk: ");
>> __putstr(lost_str);
>> __putstr(" messages suppressed.\n");
>> diff --git a/xen/include/asm-x86/hvm/domain.h
>> b/xen/include/asm-x86/hvm/domain.h
>> index 27b3de5..b1e3187 100644
>> --- a/xen/include/asm-x86/hvm/domain.h
>> +++ b/xen/include/asm-x86/hvm/domain.h
>> @@ -62,12 +62,6 @@ struct hvm_domain {
>> /* emulated irq to pirq */
>> struct radix_tree_root emuirq_pirq;
>>
>> - /* hvm_print_line() logging. */
>> -#define HVM_PBUF_SIZE 80
>> - char *pbuf;
>> - int pbuf_idx;
>> - spinlock_t pbuf_lock;
>> -
>> uint64_t *params;
>>
>> /* Memory ranges with pinned cache attributes. */
>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>> index 74b34eb..40afe12 100644
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -83,6 +83,8 @@ extern void debugtrace_printk(const char *fmt, ...);
>> #define _p(_x) ((void *)(unsigned long)(_x))
>> extern void printk(const char *format, ...)
>> __attribute__ ((format (printf, 1, 2)));
>> +extern void guest_printk(struct domain *d, const char *format, ...)
>> + __attribute__ ((format (printf, 2, 3)));
>> extern void panic(const char *format, ...)
>> __attribute__ ((format (printf, 1, 2)));
>> extern long vm_assist(struct domain *, unsigned int, unsigned int);
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index ae6a3b8..0013a8d 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -341,6 +341,12 @@ struct domain
>> /* Control-plane tools handle for this domain. */
>> xen_domain_handle_t handle;
>>
>> + /* hvm_print_line() and guest_console_write() logging. */
>> +#define DOMAIN_PBUF_SIZE 80
>> + char *pbuf;
>> + unsigned pbuf_idx;
>> + spinlock_t pbuf_lock;
>> +
>> /* OProfile support. */
>> struct xenoprof *xenoprof;
>> int32_t time_offset_seconds;
>
>
--
Daniel De Graaf
National Security Agency
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xen/console: buffer and show origin of guest PV writes
2013-09-09 13:43 ` Keir Fraser
@ 2013-09-09 13:46 ` Keir Fraser
0 siblings, 0 replies; 8+ messages in thread
From: Keir Fraser @ 2013-09-09 13:46 UTC (permalink / raw)
To: Ian Campbell; +Cc: Andrew Cooper, Daniel De Graaf, JBeulich, xen-devel
On 09/09/2013 06:43, "Keir Fraser" <keir.xen@gmail.com> wrote:
>> ...mean that Daniel has already done the right thing for dom0 here?
>
> Ah, you could be right. Well if he will confirm that then I will give my
> Ack.
>
> -- Keir
Also, the unchanged behaviour for dom0 should be stated in the patch header.
As it stands it actually explicitly says that the buffering/cooking is
applied to all domains, with dom0 excluded only from stripping of
non-printable chars. So either the patch header is wrong/misleading, or the
code needs changing.
-- Keir
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xen/console: buffer and show origin of guest PV writes
2013-09-09 13:46 ` Daniel De Graaf
@ 2013-09-09 14:14 ` Keir Fraser
0 siblings, 0 replies; 8+ messages in thread
From: Keir Fraser @ 2013-09-09 14:14 UTC (permalink / raw)
To: Daniel De Graaf; +Cc: Andrew Cooper, JBeulich, xen-devel
On 09/09/2013 06:46, "Daniel De Graaf" <dgdegra@tycho.nsa.gov> wrote:
> On 09/09/2013 07:13 AM, Keir Fraser wrote:
>> On 16/08/2013 20:01, "Daniel De Graaf" <dgdegra@tycho.nsa.gov> wrote:
>>
>>> Guests other than domain 0 using the console output have previously been
>>> controlled by the VERBOSE define, but with no designation of which
>>> guest's output was on the console. This patch converts the HVM output
>>> buffering to be used by all domains, line buffering their output and
>>> prefixing it with the domain ID. This is especially useful for debugging
>>> stub domains.
>>>
>>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>
>> This seems good, but, if we process and buffer dom0's output, we lose the
>> possibility of running a terminal session in dom0 over the Xen console.
>> Personally I do that quite a bit -- serial access only, get Xen's debugging
>> there, but also can log in to dom0. Does noone else??
>
> I do care about this use case (I also use it rather often), and it is
> preserved
> - this patch explicitly does not buffer or insert characters in dom0's output.
> This means that we waste the 80-byte buffer for dom0, but I didn't think it
> was
> worth special-casing dom0 there too (also, doing that might break a PVH dom0
> that uses the HVM output - if that method is available, which I did not
> check).
That's great, but then the patch header is wrong as the output buffering is
not used by *all* domains. Make it clear that dom0 is unaffected and then:
Acked-by: Keir Fraser <keir@xen.org>
-- Keir
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-09-09 14:14 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-16 19:01 [PATCH v2] xen/console: buffer and show origin of guest PV writes Daniel De Graaf
2013-09-09 11:13 ` Keir Fraser
2013-09-09 12:13 ` Ian Campbell
2013-09-09 13:43 ` Keir Fraser
2013-09-09 13:46 ` Keir Fraser
2013-09-09 12:48 ` Jan Beulich
2013-09-09 13:46 ` Daniel De Graaf
2013-09-09 14:14 ` Keir Fraser
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.