Xen devel - Jul 2013 - xen/conring: Improvements

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

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

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

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);
>      }
>  }

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)

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

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>

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>