Baoquan He <b...@redhat.com> writes:
> Hi Eric,
>
> On 02/11/18 at 09:08pm, Eric W. Biederman wrote:
>> Baoquan He <b...@redhat.com> writes:
>>
>> > This is a regression fix.
>> >
>> > Before, to fix erratum AVR31, commit 522e66464467 ("x86/apic: Disable
>> > I/O APIC before shutdown of the local APIC") moved lapic_shutdown()
>> > calling after disable_IO_APIC(). This introdued a regression. The
>> > root cause is that disable_IO_APIC() not only clears IO_APIC, also
>> > restore boot irq mode by setting LAPIC/APIC/IMCR, lapic_shutdown()
>> > after disable_IO_APIC() will disable LAPIC and ruin the possible
>> > virtual wire mode setting which the code has been trying to do all
>> > along.
>> > To fix this, just break down disable_IO_APIC(), then call
>> > clear_IO_APIC() to stop IO_APIC where disable_IO_APIC() was called,
>> > and call restore_boot_irq_mode() to restore boot irq mode before
>> > reboot or kexec/kdump jump.
>>
>> Two things here.
>> a) This is missing a fixes tag and a CC stable.
>> b) What makes your change to the KEXEC_JUMP code path safe?
>> Have the lapic and ioapic already been shut down?
>>
>> The KEXEC_JUMP changes to machine_kexec_32.c and machine_kexec_64.c
>> either need to be documented in the change long why they are safe
>> so that this change becomes obviously safe and correct.
>
> Re-read the code, I have to admit I didn't check the KEXEC_JUMP code
> path carefully.
>
> kernel_kexec() {
> if (kexec_image->preserve_context) {
> ...
> freeze_processes();
> ...
> disable_nonboot_cpus();
> ...
>
> else {
> ...
> machine_shutdown();
> ...
> }
> machine_kexec(kexec_image);
> ...
> }
>
> --machine_shutdown()
> --native_machine_shutdown()
> --disable_IO_APIC()
> --lapic_shutdown()
>
> machine_kexec() {
> ...
> if (image->preserve_context) {
> disable_IO_APIC();
> }
> ...
> }
>
> KEXEC_JUMP code path is different than kexec/kdump, it doesn't call
> lapic_shutdown() before jump. So commit 522e66464467
> ("x86/apic: Disable I/O APIC before shutdown of the local APIC") didn't
> impact it. And here I break down disable_IO_APIC() and change to only
> call restore_boot_irq_mode() to make a possible danger. I am not an
> expert on KEXEC_JUMP, and don't know how to test it, so will keep the
> code implementation consistent as before. For now, I plan to change it
> as below if you don't object. As you pointed out, I will describe this
> in patch log.
>
> diff --git a/arch/x86/kernel/machine_kexec_64.c
> b/arch/x86/kernel/machine_kexec_64.c
> index 1f790cf9d38f..cb0c2d0a4c99 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -297,7 +297,7 @@ void machine_kexec(struct kimage *image)
> * one form or other. kexec jump path also need
> * one.
> */
> - disable_IO_APIC();
> + clear_IO_APIC();
> + restore_boot_irq_mode();
> #endif
> }
>
>
Let me give a very concrete suggestion:
Patch 1) Replace "disable_IO_APIC();" with "clear_IO_APIC();
restore_boot_irq_mode();"
Patch 2) Move restore_boot_irq_mode(); to fix the regression.
I think that will be a slightly shorter patch sequence than what you are
dealing with and one that is slightly easier to read.
We need to sort out KEXEC_JUMP but that is something for another time.
Eric
