[PATCH RFC] xen/console: buffer and show origin of guest PV writes

[PATCH RFC] xen/console: buffer and show origin of guest PV writes

[Daniel De Graaf]

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@tycho.nsa.gov>

---

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?

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?

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(&notify_dom0_con_ring_tasklet);
-        }
+            if ( opt_console_to_ring )
+            {
+                conring_puts(kbuf);
+                tasklet_schedule(&notify_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;
+    spinlock_t  pbuf_lock;
+
     /* OProfile support. */
     struct xenoprof *xenoprof;
     int32_t time_offset_seconds;
-- 
1.8.1.4

Re: [PATCH RFC] xen/console: buffer and show origin of guest PV writes

[Pasi Kärkkäinen]

On Tue, Aug 13, 2013 at 12:34:19PM -0400, 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@tycho.nsa.gov>
> 
> ---
> 
> 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?
> 
> 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?
> 
> 3. Should we change to allowing console output by default, since it is
> now controlled by log levels?
>

Also having timestamps before the log entries would be nice (like Linux has).. 

-- Pasi

Re: [PATCH RFC] xen/console: buffer and show origin of guest PV writes

[Jan Beulich]

>>> On 13.08.13 at 18:34, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
> 1. HVM guests' output is currently limited to printable characters; do
> we want to implement the same restriction on PV guests?

Would seem to make sense.

> 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?

Uniformly using e.g. "(Dom%d) " instead of "(XEN) " for domain output
might be best?

> 3. Should we change to allowing console output by default, since it is
> now controlled by log levels?

I don't think we should, not the least because guest output doesn't
really specify a log level, and default log level messages make it
through with default options.

Will have to look at the patch itself later, but you should have Cc-ed
Keir in any case (as he will eventually need to ack it).

Jan

Re: [PATCH RFC] xen/console: buffer and show origin of guest PV writes

[Daniel De Graaf]

On 08/13/2013 12:40 PM, Pasi Kärkkäinen wrote:
> On Tue, Aug 13, 2013 at 12:34:19PM -0400, 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@tycho.nsa.gov>
>>
>> ---
>>
>> 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?
>>
>> 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?
>>
>> 3. Should we change to allowing console output by default, since it is
>> now controlled by log levels?
>>
>
> Also having timestamps before the log entries would be nice (like Linux has)..
>
> -- Pasi

Since this uses printk, if timestamps are turned on with opt_console_timestamps,
they will beadded to the output. It looks like the timestamp only has 1 second
resolution, unlike Linux - it might be useful to update that.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH RFC] xen/console: buffer and show origin of guest PV writes

[Andrew Cooper]

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@tycho.nsa.gov>
>
> ---
>
> 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(&notify_dom0_con_ring_tasklet);
> -        }
> +            if ( opt_console_to_ring )
> +            {
> +                conring_puts(kbuf);
> +                tasklet_schedule(&notify_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;

Re: [PATCH RFC] xen/console: buffer and show origin of guest PV writes

[Daniel De Graaf]

On 08/13/2013 01:18 PM, Andrew Cooper wrote:
> 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@tycho.nsa.gov>
>>
>> ---
>>
>> 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.

Dom0 would not be included in this restriction, in order to allow other
control characters (using console=hvc0 with Fedora includes color codes
and other control characters that would turn into a mess with such
stripping enabled). I have not observed a domU trying to do this since
they will switch to the shared-page PV console prior to this output.

> HVM guests should be relaxed, with PVH on the way.

HVM guests can still use the PV output - they just need to use the console
write hypercall instead of the HVM I/O port. I would think that PVH guests
would default to using the hypercall as it is more efficient (it takes a
string rather than one character per write).

Actually, checking... the console_io hypercall would need to be added to
the hvm_hypercall{32,64}_table for an HVM guest to be able to use it; they
currently must use the I/O port. I didn't check the PVH patches.

>>
>> 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.

Agreed.

[...]
>> 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

Seems like a good idea.

>> +    spinlock_t  pbuf_lock;
>> +
>>       /* OProfile support. */
>>       struct xenoprof *xenoprof;
>>       int32_t time_offset_seconds;

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH RFC] xen/console: buffer and show origin of guest PV writes

[Daniel De Graaf]

On 08/13/2013 12:52 PM, Jan Beulich wrote:
>>>> On 13.08.13 at 18:34, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> 1. HVM guests' output is currently limited to printable characters; do
>> we want to implement the same restriction on PV guests?
>
> Would seem to make sense.

It seems only dom0 will try to use control characters under normal
conditions, so this shouldn't cause any issues (hopefully).

>> 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?
>
> Uniformly using e.g. "(Dom%d) " instead of "(XEN) " for domain output
> might be best?

I think this makes sense as a way to distinguish guest output from Xen
output regarding a guest. This will probably require introducing a
vprintk-ish function to add the guest domid argument.

The original version of this patch used "(%d) ", but I think "(d%d) " or
"(dom%d) " may end up being clearer.

>> 3. Should we change to allowing console output by default, since it is
>> now controlled by log levels?
>
> I don't think we should, not the least because guest output doesn't
> really specify a log level, and default log level messages make it
> through with default options.

The existing HVM guest output is done with XENLOG_G_DEBUG, which was
copied for the PV guest output. The default value of guest_loglvl on a
debug build includes this output, but on a non-debug build hides it.

> Will have to look at the patch itself later, but you should have Cc-ed
> Keir in any case (as he will eventually need to ack it).
>
> Jan

Ah, for some reason I didn't have him on the original list. I have added
him to this message to highlight the thread and will fix that on a resend.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH RFC] xen/console: buffer and show origin of guest PV writes

[Jan Beulich]

>>> On 13.08.13 at 19:47, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
> On 08/13/2013 12:52 PM, Jan Beulich wrote:
>>>>> On 13.08.13 at 18:34, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>>> 3. Should we change to allowing console output by default, since it is
>>> now controlled by log levels?
>>
>> I don't think we should, not the least because guest output doesn't
>> really specify a log level, and default log level messages make it
>> through with default options.
> 
> The existing HVM guest output is done with XENLOG_G_DEBUG, which was
> copied for the PV guest output. The default value of guest_loglvl on a
> debug build includes this output, but on a non-debug build hides it.

Okay, I didn't look closely enough then. Yet nevertheless, at least
for debugging purposes we routinely tell people to lower the log
level, and some tend to keep things that way in order to avoid
having to re-create an eventual problem with maximum output
enforced. And we wouldn't want to spam their consoles (and at
the same time make our lives harder when it comes to going
through those logs).

Jan

Re: [PATCH RFC] xen/console: buffer and show origin of guest PV writes

[Jan Beulich]

>>> On 13.08.13 at 18:34, 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 during early boot.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Patch looks okay apart from a few formatting issues (braces not
on separate lines).

Jan