|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC] xen/console: buffer and show origin of guest PV writes
On 13/08/13 17:34, Daniel De Graaf 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 during early boot.
>
> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>
> ---
>
> A few questions for discussion (the reason this is an RFC):
>
> 1. HVM guests' output is currently limited to printable characters; do
> we want to implement the same restriction on PV guests?
No. If a guest is doing something such as listing HP ACPI tables
(something dom0 would very reasonably do on HP hardware), restricting to
printable characters will leave strange omissions.
HVM guests should be relaxed, with PVH on the way.
>
> 2. The prefix on the serial console for PV output is "(XEN) d5: ", while
> HVM output is still "(XEN) HVM5: "; should these be made consistent?
I tend to find it useful to distinguish between HVM and PV at a glance,
but would agree that something more consistent would be better.
>
> 3. Should we change to allowing console output by default, since it is
> now controlled by log levels?
>
> xen/arch/x86/hvm/hvm.c | 27 ++++++++++----------------
> xen/common/domain.c | 8 ++++++++
> xen/drivers/char/console.c | 41
> +++++++++++++++++++++++++++++++---------
> xen/include/asm-x86/hvm/domain.h | 6 ------
> xen/include/xen/sched.h | 6 ++++++
> 5 files changed, 56 insertions(+), 32 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 1fcaed0..fa843b5 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';
> + printk(XENLOG_G_DEBUG "HVM%u: %s\n", cd->domain_id, 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..1509260 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -230,6 +230,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) )
> @@ -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..36ed100 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -375,6 +375,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,18 +389,40 @@ 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 )
> + {
> + conring_puts(kbuf);
> + tasklet_schedule(¬ify_dom0_con_ring_tasklet);
> + }
>
> - spin_unlock_irq(&console_lock);
> + spin_unlock_irq(&console_lock);
> + } else {
> + char *kptr = strchr(kbuf, '\n');
> + spin_lock(&cd->pbuf_lock);
> + if ( kptr ) {
> + *kptr = '\0';
> + kcount = kptr - kbuf + 1;
> + cd->pbuf[cd->pbuf_idx] = '\0';
> + printk(XENLOG_G_DEBUG "d%u: %s%s\n", cd->domain_id,
> 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';
> + printk(XENLOG_G_DEBUG "d%u: %s%s\n", cd->domain_id,
> cd->pbuf, kbuf);
> + cd->pbuf_idx = 0;
> + }
> + spin_unlock(&cd->pbuf_lock);
> + }
>
> guest_handle_add_offset(buffer, kcount);
> count -= kcount;
> 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/sched.h b/xen/include/xen/sched.h
> index ae6a3b8..5a5d7ba 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;
> + int pbuf_idx;
It might have been wrong before, but as it is changing, can we please
use the correct type, unsigned, for an index.
~Andrew
> + spinlock_t pbuf_lock;
> +
> /* OProfile support. */
> struct xenoprof *xenoprof;
> int32_t time_offset_seconds;
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |