x86/crash: force unlock console before printing on kexec crash

[Xen-devel] [PATCH for-4.13] x86/crash: force unlock console before printing on kexec crash

Posted by Igor Druzhinin 4 years, 6 months ago

There is a small window where shootdown NMI might come to a CPU
(e.g. in serial interrupt handler) where console lock is taken. In order
not to leave following console prints waiting infinitely for shot down
CPUs to free the lock - force unlock the console.

The race has been frequently observed while crashing nested Xen in
an HVM domain.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 xen/arch/x86/crash.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 6e1d3d3..a20ec8a 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -29,6 +29,7 @@
 #include <asm/io_apic.h>
 #include <xen/iommu.h>
 #include <asm/hpet.h>
+#include <xen/console.h>
 
 static cpumask_t waiting_to_crash;
 static unsigned int crashing_cpu;
@@ -155,6 +156,7 @@ static void nmi_shootdown_cpus(void)
     }
 
     /* Leave a hint of how well we did trying to shoot down the other cpus */
+    console_force_unlock();
     if ( cpumask_empty(&waiting_to_crash) )
         printk("Shot down all CPUs\n");
     else
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/crash: force unlock console before printing on kexec crash

Posted by Andrew Cooper 4 years, 6 months ago

On 01/10/2019 20:15, Igor Druzhinin wrote:
> There is a small window where shootdown NMI might come to a CPU
> (e.g. in serial interrupt handler) where console lock is taken. In order
> not to leave following console prints waiting infinitely for shot down
> CPUs to free the lock - force unlock the console.
>
> The race has been frequently observed while crashing nested Xen in
> an HVM domain.
>
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
>  xen/arch/x86/crash.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
> index 6e1d3d3..a20ec8a 100644
> --- a/xen/arch/x86/crash.c
> +++ b/xen/arch/x86/crash.c
> @@ -29,6 +29,7 @@
>  #include <asm/io_apic.h>
>  #include <xen/iommu.h>
>  #include <asm/hpet.h>
> +#include <xen/console.h>
>  
>  static cpumask_t waiting_to_crash;
>  static unsigned int crashing_cpu;
> @@ -155,6 +156,7 @@ static void nmi_shootdown_cpus(void)
>      }
>  
>      /* Leave a hint of how well we did trying to shoot down the other cpus */
> +    console_force_unlock();
>      if ( cpumask_empty(&waiting_to_crash) )
>          printk("Shot down all CPUs\n");
>      else

The overall change, R-by me, but I'd prefer something along the lines of:

diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 6e1d3d3a84..41687465ac 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -154,6 +154,12 @@ static void nmi_shootdown_cpus(void)
         msecs--;
     }
 
+    /*
+     * We may have NMI'd another CPU while it was holding the console lock.
+     * It won't be in a position to release the lock...
+     */
+    console_force_unlock();
+
     /* Leave a hint of how well we did trying to shoot down the other
cpus */
     if ( cpumask_empty(&waiting_to_crash) )
         printk("Shot down all CPUs\n");


If you're happy, I can fold this in on commit.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/crash: force unlock console before printing on kexec crash

Posted by Igor Druzhinin 4 years, 6 months ago

On 01/10/2019 20:48, Andrew Cooper wrote:
> On 01/10/2019 20:15, Igor Druzhinin wrote:
>> There is a small window where shootdown NMI might come to a CPU
>> (e.g. in serial interrupt handler) where console lock is taken. In order
>> not to leave following console prints waiting infinitely for shot down
>> CPUs to free the lock - force unlock the console.
>>
>> The race has been frequently observed while crashing nested Xen in
>> an HVM domain.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>> ---
>>  xen/arch/x86/crash.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
>> index 6e1d3d3..a20ec8a 100644
>> --- a/xen/arch/x86/crash.c
>> +++ b/xen/arch/x86/crash.c
>> @@ -29,6 +29,7 @@
>>  #include <asm/io_apic.h>
>>  #include <xen/iommu.h>
>>  #include <asm/hpet.h>
>> +#include <xen/console.h>
>>  
>>  static cpumask_t waiting_to_crash;
>>  static unsigned int crashing_cpu;
>> @@ -155,6 +156,7 @@ static void nmi_shootdown_cpus(void)
>>      }
>>  
>>      /* Leave a hint of how well we did trying to shoot down the other cpus */
>> +    console_force_unlock();
>>      if ( cpumask_empty(&waiting_to_crash) )
>>          printk("Shot down all CPUs\n");
>>      else
> 
> The overall change, R-by me, but I'd prefer something along the lines of:
> 
> diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
> index 6e1d3d3a84..41687465ac 100644
> --- a/xen/arch/x86/crash.c
> +++ b/xen/arch/x86/crash.c
> @@ -154,6 +154,12 @@ static void nmi_shootdown_cpus(void)
>          msecs--;
>      }
>  
> +    /*
> +     * We may have NMI'd another CPU while it was holding the console lock.
> +     * It won't be in a position to release the lock...
> +     */
> +    console_force_unlock();
> +
>      /* Leave a hint of how well we did trying to shoot down the other
> cpus */
>      if ( cpumask_empty(&waiting_to_crash) )
>          printk("Shot down all CPUs\n");
> 
> 
> If you're happy, I can fold this in on commit.

Have no objections but there are other places we call
console_force_unlock() for similar purposes and those don't have
explanatory comments like that. From my point of view the reason here is
kind of obvious but if you prefer...

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/crash: force unlock console before printing on kexec crash

Posted by Jan Beulich 4 years, 6 months ago

On 01.10.2019 21:51, Igor Druzhinin wrote:
> On 01/10/2019 20:48, Andrew Cooper wrote:
>> On 01/10/2019 20:15, Igor Druzhinin wrote:
>>> There is a small window where shootdown NMI might come to a CPU
>>> (e.g. in serial interrupt handler) where console lock is taken. In order
>>> not to leave following console prints waiting infinitely for shot down
>>> CPUs to free the lock - force unlock the console.
>>>
>>> The race has been frequently observed while crashing nested Xen in
>>> an HVM domain.
>>>
>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>> ---
>>>  xen/arch/x86/crash.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
>>> index 6e1d3d3..a20ec8a 100644
>>> --- a/xen/arch/x86/crash.c
>>> +++ b/xen/arch/x86/crash.c
>>> @@ -29,6 +29,7 @@
>>>  #include <asm/io_apic.h>
>>>  #include <xen/iommu.h>
>>>  #include <asm/hpet.h>
>>> +#include <xen/console.h>
>>>  
>>>  static cpumask_t waiting_to_crash;
>>>  static unsigned int crashing_cpu;
>>> @@ -155,6 +156,7 @@ static void nmi_shootdown_cpus(void)
>>>      }
>>>  
>>>      /* Leave a hint of how well we did trying to shoot down the other cpus */
>>> +    console_force_unlock();
>>>      if ( cpumask_empty(&waiting_to_crash) )
>>>          printk("Shot down all CPUs\n");
>>>      else
>>
>> The overall change, R-by me, but I'd prefer something along the lines of:
>>
>> diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
>> index 6e1d3d3a84..41687465ac 100644
>> --- a/xen/arch/x86/crash.c
>> +++ b/xen/arch/x86/crash.c
>> @@ -154,6 +154,12 @@ static void nmi_shootdown_cpus(void)
>>          msecs--;
>>      }
>>  
>> +    /*
>> +     * We may have NMI'd another CPU while it was holding the console lock.
>> +     * It won't be in a position to release the lock...
>> +     */
>> +    console_force_unlock();
>> +
>>      /* Leave a hint of how well we did trying to shoot down the other
>> cpus */
>>      if ( cpumask_empty(&waiting_to_crash) )
>>          printk("Shot down all CPUs\n");
>>
>>
>> If you're happy, I can fold this in on commit.
> 
> Have no objections but there are other places we call
> console_force_unlock() for similar purposes and those don't have
> explanatory comments like that. From my point of view the reason here is
> kind of obvious but if you prefer...

+1 for the variant with comment.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/crash: force unlock console before printing on kexec crash

Posted by Jan Beulich 4 years, 6 months ago

On 01.10.2019 21:15, Igor Druzhinin wrote:
> There is a small window where shootdown NMI might come to a CPU
> (e.g. in serial interrupt handler) where console lock is taken. In order
> not to leave following console prints waiting infinitely for shot down
> CPUs to free the lock - force unlock the console.
> 
> The race has been frequently observed while crashing nested Xen in
> an HVM domain.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

This should have been Cc-ed to Jürgen (now done), for him to release-ack
it.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/crash: force unlock console before printing on kexec crash

Posted by Jürgen Groß 4 years, 6 months ago

On 02.10.19 12:25, Jan Beulich wrote:
> On 01.10.2019 21:15, Igor Druzhinin wrote:
>> There is a small window where shootdown NMI might come to a CPU
>> (e.g. in serial interrupt handler) where console lock is taken. In order
>> not to leave following console prints waiting infinitely for shot down
>> CPUs to free the lock - force unlock the console.
>>
>> The race has been frequently observed while crashing nested Xen in
>> an HVM domain.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> 
> This should have been Cc-ed to Jürgen (now done), for him to release-ack
> it.

Already done via IRC


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel