xen/conring: Improvements

xen/conring: Improvements

[Andrew Cooper]

These two changes have come from an investgation of a cascade crash
deliberatly triggered using an NMI from an IPMI controller.  As the NMI/MCE/DF
stack trace is particularly important to be captured in the crash environment,
I recomend these patches for backport to all Xen trees.

~Andrew

[PATCH 1/2] xen/conring: Clean up writing to the console ring.

[Andrew Cooper]

Refactor putchar_console_ring() to conring_puts().  This allows for
consistency with {sercon,vga}_puts(), prevents needless recalculation of
the conring consumer index, and slight cleanup at the two callsites.

There is no functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/drivers/char/console.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index b696b3e..45b81b3 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -175,10 +175,15 @@ static char * __init loglvl_str(int lvl)
  * ********************************************************
  */
 
-static void putchar_console_ring(int c)
+static void conring_puts(const char *str)
 {
+    char c;
+
     ASSERT(spin_is_locked(&console_lock));
-    conring[CONRING_IDX_MASK(conringp++)] = c;
+
+    while ( (c = *str++) != '\0' )
+        conring[CONRING_IDX_MASK(conringp++)] = c;
+
     if ( (uint32_t)(conringp - conringc) > conring_size )
         conringc = conringp - conring_size;
 }
@@ -368,7 +373,7 @@ static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet,
 
 static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
 {
-    char kbuf[128], *kptr;
+    char kbuf[128];
     int kcount;
 
     while ( count > 0 )
@@ -390,8 +395,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
 
         if ( opt_console_to_ring )
         {
-            for ( kptr = kbuf; *kptr != '\0'; kptr++ )
-                putchar_console_ring(*kptr);
+            conring_puts(kbuf);
             tasklet_schedule(&notify_dom0_con_ring_tasklet);
         }
 
@@ -456,8 +460,6 @@ static bool_t console_locks_busted;
 
 static void __putstr(const char *str)
 {
-    int c;
-
     ASSERT(spin_is_locked(&console_lock));
 
     sercon_puts(str);
@@ -465,8 +467,7 @@ static void __putstr(const char *str)
 
     if ( !console_locks_busted )
     {
-        while ( (c = *str++) != '\0' )
-            putchar_console_ring(c);
+        conring_puts(str);
         tasklet_schedule(&notify_dom0_con_ring_tasklet);
     }
 }
-- 
1.7.10.4

Re: [PATCH 1/2] xen/conring: Clean up writing to the console ring.

[Matt Wilson]

On Wed, Jul 31, 2013 at 03:22:51PM +0100, Andrew Cooper wrote:
> Refactor putchar_console_ring() to conring_puts().  This allows for
> consistency with {sercon,vga}_puts(), prevents needless recalculation of
> the conring consumer index, and slight cleanup at the two callsites.
> 
> There is no functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Matt Wilson <msw@amazon.com>

> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> ---
>  xen/drivers/char/console.c |   19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index b696b3e..45b81b3 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -175,10 +175,15 @@ static char * __init loglvl_str(int lvl)
>   * ********************************************************
>   */
>  
> -static void putchar_console_ring(int c)
> +static void conring_puts(const char *str)
>  {
> +    char c;
> +
>      ASSERT(spin_is_locked(&console_lock));
> -    conring[CONRING_IDX_MASK(conringp++)] = c;
> +
> +    while ( (c = *str++) != '\0' )
> +        conring[CONRING_IDX_MASK(conringp++)] = c;
> +
>      if ( (uint32_t)(conringp - conringc) > conring_size )
>          conringc = conringp - conring_size;
>  }
> @@ -368,7 +373,7 @@ static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet,
>  
>  static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
>  {
> -    char kbuf[128], *kptr;
> +    char kbuf[128];
>      int kcount;
>  
>      while ( count > 0 )
> @@ -390,8 +395,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
>  
>          if ( opt_console_to_ring )
>          {
> -            for ( kptr = kbuf; *kptr != '\0'; kptr++ )
> -                putchar_console_ring(*kptr);
> +            conring_puts(kbuf);
>              tasklet_schedule(&notify_dom0_con_ring_tasklet);
>          }
>  
> @@ -456,8 +460,6 @@ static bool_t console_locks_busted;
>  
>  static void __putstr(const char *str)
>  {
> -    int c;
> -
>      ASSERT(spin_is_locked(&console_lock));
>  
>      sercon_puts(str);
> @@ -465,8 +467,7 @@ static void __putstr(const char *str)
>  
>      if ( !console_locks_busted )
>      {
> -        while ( (c = *str++) != '\0' )
> -            putchar_console_ring(c);
> +        conring_puts(str);
>          tasklet_schedule(&notify_dom0_con_ring_tasklet);
>      }
>  }

Re: [PATCH 1/2] xen/conring: Clean up writing to the console ring.

[Keir Fraser]

On 31/07/2013 15:22, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> Refactor putchar_console_ring() to conring_puts().  This allows for
> consistency with {sercon,vga}_puts(), prevents needless recalculation of
> the conring consumer index, and slight cleanup at the two callsites.
> 
> There is no functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>

Acked-by: Keir Fraser <keir@xen.org>

[PATCH 2/2] xen/conring: Write to console ring even if console lock is busted.

[Andrew Cooper]

console_lock_busted gets set when an NMI/MCE/Double Fault handler decides to
bring Xen down in an emergency.  conring_puts() cannot block and does
not have problematic interactions with the console_lock.

Therefore, choosing to not put the string into the console ring simply means
that the kexec environment cant find any panic() message caused by an IST
interrupt, which is unhelpful for debugging purposes.

In the case that two pcpus fight with console_force_unlock(), having slightly
garbled strings in the console ring is far more useful than having nothing at
all.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/drivers/char/console.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 45b81b3..8ac32e4 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -465,11 +465,10 @@ static void __putstr(const char *str)
     sercon_puts(str);
     video_puts(str);
 
+    conring_puts(str);
+
     if ( !console_locks_busted )
-    {
-        conring_puts(str);
         tasklet_schedule(&notify_dom0_con_ring_tasklet);
-    }
 }
 
 static int printk_prefix_check(char *p, char **pp)
-- 
1.7.10.4

Re: [PATCH 2/2] xen/conring: Write to console ring even if console lock is busted.

[Matt Wilson]

On Wed, Jul 31, 2013 at 03:22:52PM +0100, Andrew Cooper wrote:
> console_lock_busted gets set when an NMI/MCE/Double Fault handler decides to
> bring Xen down in an emergency.  conring_puts() cannot block and does
> not have problematic interactions with the console_lock.
> 
> Therefore, choosing to not put the string into the console ring simply means
> that the kexec environment cant find any panic() message caused by an IST
> interrupt, which is unhelpful for debugging purposes.
> 
> In the case that two pcpus fight with console_force_unlock(), having slightly
> garbled strings in the console ring is far more useful than having nothing at
> all.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Matt Wilson <msw@amazon.com>

> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> ---
>  xen/drivers/char/console.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 45b81b3..8ac32e4 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -465,11 +465,10 @@ static void __putstr(const char *str)
>      sercon_puts(str);
>      video_puts(str);
>  
> +    conring_puts(str);
> +
>      if ( !console_locks_busted )
> -    {
> -        conring_puts(str);
>          tasklet_schedule(&notify_dom0_con_ring_tasklet);
> -    }
>  }
>  
>  static int printk_prefix_check(char *p, char **pp)

Re: [PATCH 2/2] xen/conring: Write to console ring even if console lock is busted.

[Keir Fraser]

On 31/07/2013 15:22, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> console_lock_busted gets set when an NMI/MCE/Double Fault handler decides to
> bring Xen down in an emergency.  conring_puts() cannot block and does
> not have problematic interactions with the console_lock.
> 
> Therefore, choosing to not put the string into the console ring simply means
> that the kexec environment cant find any panic() message caused by an IST
> interrupt, which is unhelpful for debugging purposes.
> 
> In the case that two pcpus fight with console_force_unlock(), having slightly
> garbled strings in the console ring is far more useful than having nothing at
> all.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>

Acked-by: Keir Fraser <keir@xen.org>

Re: xen/conring: Improvements

[Andrew Cooper]

On 31/07/13 15:22, Andrew Cooper wrote:
> These two changes have come from an investgation of a cascade crash
> deliberatly triggered using an NMI from an IPMI controller.  As the NMI/MCE/DF
> stack trace is particularly important to be captured in the crash environment,
> I recomend these patches for backport to all Xen trees.
>
> ~Andrew

Ping?

>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel