Message-ID: <20160720062833.GB2608@x1.redhat.com>
Date:	Wed, 20 Jul 2016 14:28:33 +0800
From:	"bhe@...hat.com" <bhe@...hat.com>
To:	"Wei, Jiangang" <weijg.fnst@...fujitsu.com>
Cc:	"fenghua.yu@...el.com" <fenghua.yu@...el.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"kexec@...ts.infradead.org" <kexec@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"ebiederm@...ssion.com" <ebiederm@...ssion.com>,
	"hpa@...or.com" <hpa@...or.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"jiang.liu@...ux.intel.com" <jiang.liu@...ux.intel.com>,
	"vgoyal@...hat.com" <vgoyal@...hat.com>
Subject: Re: [PATCH 0/3] Enable legacy irq mode before jump to kexec/kdump
 kernel

On 07/20/16 at 03:54am, Wei, Jiangang wrote:
> Hi Baoquan He,
> 
> Well, Indeed there‘s a relationship between the dump-capture hangs in
> calibrate_delay_converge() and the interrupt mode.
> 
> but there‘s no essential difference between your patches and mine that
> calls disable_IO_APIC() again.  
> Actually, disable_IO_APIC will set APIC to virtual wire mode.

Well, about this I want to explain a little bit. Usually people posted a
patch, reviewers can give comments, suggestions or other ideas. During
reviewing stage patch author need answer questions from people
interested. So this is an interactive action, reviewers can also learn
knowledge, meanwhile give comments.

When reviewing your patch, I have many questions, and only get two
pieces of information, kdump kernel hang with notsc, and disable_IO_APIC
can save it. You even can't answer people's question why disable_IO_APIC
need be called twice with your patch. I have to dig code and read intel
arch manual and MP spec and Eric's original patch thread, and add debug
bug to verify all. How can it be like you said "no essential difference
between your patches and mine"? 


> 
> In fact,
> Eric and Ingo suggested that "it should be fixed in the bootup path of
> the dump kernel, not the crash kernel reboot path", which is convincing
> and reasonable.
> 
> And i find a better method can fix the problem.
> It's better to set virtual wire mode for apic in init_bsp_APIC(), which
> in the bootup path of dump kernel.
> But now, init_bsp_APIC doesn't initialize the apic to vitual wire mode
> when smp_found_config is non-zero.

This may be do-able, let's see what Eric and Ingo will say.

> 
> FYI, I'm working on this point. later i will send patches to mail list.
> 
> Wei
> 
> On Wed, 2016-07-20 at 10:58 +0800, Baoquan He wrote:
> > Wei Jiangang reported kdump kernel always hang when "notsc" is specified
> > in boot parameter. After debugging I found there's no timer interrupt
> > in the current kexec/kdump kernel. This is caused by commit 522e66464467
> > ("x86/apic: Disable I/O APIC before shutdown of the local APIC"). Originally
> > Eric posted below patch to make system be virtual wire mode in which 8259-
> > equivalent PIC fields all interrupts and the LAPIC becomes a virtual wire.
> > Like this interrupts can be delivered from PIC to CPU via the LAPIC's local
> > interrupt 0 (LINTIN0). In virtual wire APIC mode is disabled while LAPIC
> > is software enabled and its LINT0 and LINT1 need be programmed specifically.
> > 
> > https://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.11/2.6.11-mm1/broken-out/x86_64-apic-virtwire-on-shutdown.patch
> > 
> > But with commit 522e66464 you can see after disable_IO_APIC had setting
> > virtual wire mode, lapic_shutdown disabled LAPIC again. Now virtual wire
> > mode doesn't work, then it cause no timer interrupt during kdump kernel
> > initialization stage until system enter into APIC mode.
> > 
> > So people may be wondering why only kdump kernel hang, the normal kernel
> > with "notsc" can still work. This is because BIOS has already built PIC mode
> > or virtual wire mode while kexec/kdump kernel doesn't go through BIOS
> > initialization. That is why we have to change system to be PIC mode or
> > virtual wire mode before jump to kexec/kdump kernel.
> > 
> > Then why kdump kernel didn't hang when "notsc" is not specified. This is
> > because tsc_init will assign the already calibrated value to lpj_fine.
> > Then kernel doesn't need to count cpu loops between jiffies with the help
> > of timer interrupt. So "notsc" is not victim, but a informer.
> > 
> > In patch 1/3 disable_IO_APIC is changed to only contain code of changeing
> > system to be PIC mode or virtual wire mode and is renamed as
> > switch_to_legacy_irq_mode. Now only call clear_IO_APIC where IO-APIC need
> > be disabled, and call switch_to_legacy_irq_mode before jump to kexe/kdump
> > kernel.
> > 
> > Patch 2/3 and 3/3 are clean up patch.
> > 
> > Baoquan He (3):
> >   x86/apic/kexec: Enable legacy irq mode before jump to kexec/kdump
> >     kernel
> >   x86/apic: Clean up the names of legacy irq mode setting related
> >     functions
> >   x86/apic: Clean up the apic delivery mode macro definition
> > 
> >  arch/x86/include/asm/apic.h        |  2 +-
> >  arch/x86/include/asm/apicdef.h     |  1 -
> >  arch/x86/include/asm/io_apic.h     |  6 +++---
> >  arch/x86/kernel/apic/apic.c        | 19 +++++++++++--------
> >  arch/x86/kernel/apic/io_apic.c     | 32 +++++++++++++++++---------------
> >  arch/x86/kernel/crash.c            |  2 +-
> >  arch/x86/kernel/machine_kexec_32.c | 15 +++++----------
> >  arch/x86/kernel/machine_kexec_64.c | 15 +++++----------
> >  arch/x86/kernel/reboot.c           |  2 +-
> >  arch/x86/kernel/x86_init.c         |  2 +-
> >  drivers/iommu/irq_remapping.c      |  2 +-
> >  11 files changed, 46 insertions(+), 52 deletions(-)
> > 
> 
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Powered by blists - more mailing lists