| Message ID | 1569957357-20803-1-git-send-email-igor.druzhinin@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.13] x86/crash: force unlock console before printing on kexec crash | expand |
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
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
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
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
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
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
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(+)