Date: Wed, 17 Jan 2018 17:42:58 +0800
From: Baoquan He <bhe@redhat.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: linux-kernel@vger.kernel.org, fenghua.yu@intel.com,
        mingo@redhat.com, tglx@linutronix.de, hpa@zytor.com,
        x86@kernel.org, rostedt@goodmis.org, jgross@suse.com,
        peterz@infradead.org, uobergfe@redhat.com, joro@8bytes.org
Subject: Re: [RESEND PATCH 0/3] x86/apic/kexec: Enable legacy irq mode before
 jump to kexec/kdump kernel
Message-ID: <20180117094258.GE2321@localhost.localdomain>
References: <1515123732-28908-1-git-send-email-bhe@redhat.com>
 <20180111020532.GC6243@localhost.localdomain>
 <871siwb66u.fsf@xmission.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <871siwb66u.fsf@xmission.com>
User-Agent: Mutt/1.9.1 (2017-09-22)
Sender: linux-kernel-owner@vger.kernel.org

Hi Eric,

Sorry I missed your mail contact in the CC list when posted patches.
Have CC-ed them to you one by one. I can send the patches to you
privately if the format is messy and not good for reviewing.

On 01/11/18 at 12:28pm, Eric W. Biederman wrote:
> Baoquan He <bhe@redhat.com> writes:
> 
> > Hi all,
> >
> > PING!
> >
> > (Add Fenghua and Eric to this thread)
> 
> Absolutely not.  There are very strong reasons not to anything in the
> kexec on panic path that we don't need to.

Agreed that we should stop adding anything in the path if we don't need
to. Now the legacy irq mode enabling could not be the not needed case.
As you can see we will see the warning in kdump boot log if irq pending
in APIC_RR.

And people may not always want to take the APIC mode in kernel. E.g if
they specify 'noapic' to disable IO-APIC in system explicitly, for
testing purpuse or other special reason. Furthermore, the apic mode
enabling has been put earlier by Dou's patchset. This change will enables
legacy irq mode before jump to kdump won't break that.

After all, a warning seen in boot log of kdump kernel, bug is reported,
we need fix.

Any idea or suggestion about this?

Thanks
Baoquan

> 
> Among them legacy mode only works when the kernel starts up on the boot
> cpu and kexec on panic absolutely can not guarantee the kernel will be
> crashing and thus the new kernel starting on the boot cpu.
> 
> What we need to be doing (and I have seen patches around to this effect)
> is teaching the kernel to always use non-legacy mode.  Which should
> remove all of these issues as then legacy mode won't matter in the
> slightest.
> 
> Eric
> 
> 
> 
> >
> > On 01/05/18 at 11:42am, Baoquan He wrote:
> >> On kvm guest, the latest kernel will always print warning during kdump kernel boots
> >> as below. The reaons is the legacy irq mode is disabled before jump to kexec/kdump
> >> kernel. So in setup_local_APIC(), the do { xxx } while (queued && max_loops > 0)
> >> can't handle if pending irq exists in APIC IRR since LAPIC is disabled. It will
> >> terminate the do while loop finally when max_loops overflows by subtraction. Then
> >> WARN_ON(max_loops <= 0) is triggered.
> >> 
> >> [    0.001000] WARNING: CPU: 0 PID: 0 at arch/x86/kernel/apic/apic.c:1467 setup_local_APIC+0x228/0x330
> >> [    0.001000] Modules linked in:
> >> [    0.001000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.15.0-rc5+ #3
> >> [    0.001000] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1.fc26 04/01/2014
> >> [    0.001000] RIP: 0010:setup_local_APIC+0x228/0x330
> >> [    0.001000] RSP: 0000:ffffffffb6e03eb8 EFLAGS: 00010286
> >> [    0.001000] RAX: 0000009edb4c4d84 RBX: 0000000000000000 RCX: 00000000b099d800
> >> [    0.001000] RDX: 0000009e00000000 RSI: 0000000000000000 RDI: 0000000000000810
> >> [    0.001000] RBP: 0000000000000000 R08: ffffffffffffffff R09: 0000000000000001
> >> [    0.001000] R10: ffff98ce6a801c00 R11: 0761076d072f0776 R12: 0000000000000001
> >> [    0.001000] R13: 00000000000000f0 R14: 0000000000004000 R15: ffffffffffffc6ff
> >> [    0.001000] FS:  0000000000000000(0000) GS:ffff98ce6bc00000(0000) knlGS:0000000000000000
> >> [    0.001000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [    0.001000] CR2: 00000000ffffffff CR3: 0000000022209000 CR4: 00000000000406b0
> >> [    0.001000] Call Trace:
> >> [    0.001000]  apic_bsp_setup+0x56/0x74
> >> [    0.001000]  x86_late_time_init+0x11/0x16
> >> [    0.001000]  start_kernel+0x3c9/0x486
> >> [    0.001000]  secondary_startup_64+0xa5/0xb0
> >> [    0.001000] Code: 00 85 c9 74 2d 0f 31 c1 e1 0a 48 c1 e2 20 41 89 cf 4c 03 7c 24 08 48 09 d0 49 29 c7 4c 89 3c 24 48 83 3c 24 00 0f 8f 8f fe ff ff <0f> ff e9 10 ff ff ff 48 83 2c 24 01 eb e7 48 83 c4 18 5b 5d 41 
> >> [    0.001000] ---[ end trace b88e71b9a6ebebdd ]---
> >> [    0.001000] masked ExtINT on CPU#0
> >> 
> >> With patch 2/3 applied, the above warning disappeared. And with patch 2/3
> >> applied, the issue mentioned in patch 1/3 can also be fixed because the LAPIC
> >> has been set as ExtINT before jump to kdump kernel, while we had better set it
> >> explicitly. Seems no reason not to enable legacy irq mode before jump to
> >> kexec/kdump kernel, and can make it be consistent with normal kernel.
> >> 
> >> Patch 3/3 is doing clean up, I am fine if people think it's unnecessary.
> >> 
> >> 
> >> Baoquan He (3):
> >>   x86/apic: Set up through LAPIC on boot CPU's LINT0 if ioapic is
> >>     disabled
> >>   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
> >> 
> >>  arch/x86/include/asm/apic.h        |  2 +-
> >>  arch/x86/include/asm/io_apic.h     |  6 +++---
> >>  arch/x86/kernel/apic/apic.c        | 13 +++++++------
> >>  arch/x86/kernel/apic/io_apic.c     | 25 ++++++++++++-------------
> >>  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 +-
> >>  10 files changed, 37 insertions(+), 47 deletions(-)
> >> 
> >> -- 
> >> 2.5.5
> >>