by Baoquan He

[permalink] [raw]

Subject: Re: [RESEND PATCH 3/3] x86/apic: Clean up the names of legacy irq mode setting related functions

On 01/19/18 at 04:06pm, Dou Liyang wrote:
>
>
> At 01/19/2018 03:21 PM, Baoquan He wrote:
> > On 01/19/18 at 02:42pm, Dou Liyang wrote:
> > > Hi Baoquan,
> > >
> > > At 01/05/2018 12:39 PM, Baoquan He wrote:
> > > [...]
> > > > /*
> > > > - * Not an __init, needed by kexec/kdump code.
> > > > - * For safety IO-APIC and Local APIC need be cleared before this.
> > > > + * In legacy irq mode, full DOS compatibility with the uniprocessor PC/AT is
> > > > + * provided by using the APICs in conjunction with standard 8259A-equivalent
> > > > + * programmable interrupt controllers (PICs). It's necessary to deliver legacy
> > > > + * interrupts even when APIC mode is not enabled. This is required by kexec/
> > > > + * kdump before enter into the 2nd kernel.
> > > > */
> > > > void switch_to_legacy_irq_mode(void)
> > > > {
> > > > if (!nr_legacy_irqs())
> > > > return;
> > > > - x86_io_apic_ops.disable();
> > > > + ioapic_set_virtual_wire_mode();
> > > > +
> > > > + if (boot_cpu_has(X86_FEATURE_APIC) || apic_from_smp_config())
> > > > + lapic_set_legacy_irq_mode(ioapic_i8259.pin != -1);
> > >
> > > Seems these two function, ioapic/lapic_set_legacy_irq_mode should be
> > > exclusive.
> >
> > Thanks for looking into this, dou!
> >
> > It might be not exclusive. You can see mp_spec 3.6.2.2 Virtual Wire Mode
>
> Oops! Sorry to confuse you, here what I want to say is that the code
> make me think that we set through IO-APIC first, then set through
> Local-APIC then. But, we can be only in one mode of them.
>
> Just worry about this code seems ignore the irq remapping situation.
>
> We call switch_to_legacy_irq_mode() directly in machine_kexec(),
> why we also set x86_io_apic_ops:
>
> .disable = switch_to_legacy_irq_mode

You are right, this is not very clear. Since we call switch_to_legacy_irq_mode()
directly, nobody will call x86_io_apic_ops.disable() any more.

Should keep native_disable_io_apic() and let x86_io_apic_ops.disable()
to point at it.

Thanks
Baoquan
>
> > subsection, there are two kinds of virtual wire mode, one is
> > 8259A-Equivalent pics is connected to lint0 of boot cpu LAPIC, the other
> > is 8259A-Equivalent pics go through IO-APIC, then is connected to lint0
> > of LAPIC. Whatever it is, LAPIC need be set as through-lapic.
> >
>
> Yes, Absolutely right!
>
> > Above is what I got from mp_spec. But from function
> > native_disable_io_apic() and disconnect_bsp_APIC(), the code seems to be
> > telling that if io-apic is connected to 8259A-Equivalent pics, we need
> > mask lvt0 of LAPIC. This conflicts with mp_spec 3.6.2.2.
> >
>
> I think so.
>
> Thanks,
> dou.
> > Thanks
> > Baoquan
> > >
> > > But We do that because both the through-lapic and through-ioapic virtual
> > > wire mode need setup the APIC_SPIV_APIC_ENABLED which is only located in
> > > the lapic_set_legacy_irq_mode(). So we need call them both.
> > >
> > > IMO, this cleanup may not make it clear. we can separate these two mode
> > > totally or just keep it like before.
> > >
> > > Thanks,
> > > dou.
> > > > }
> > > > #ifdef CONFIG_X86_32
> > > > diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> > > > index 1151ccd72ce9..c30f0f273dbd 100644
> > > > --- a/arch/x86/kernel/x86_init.c
> > > > +++ b/arch/x86/kernel/x86_init.c
> > > > @@ -148,5 +148,5 @@ void arch_restore_msi_irqs(struct pci_dev *dev)
> > > > struct x86_io_apic_ops x86_io_apic_ops __ro_after_init = {
> > > > .read = native_io_apic_read,
> > > > - .disable = native_disable_io_apic,
> > > > + .disable = switch_to_legacy_irq_mode,
> > > > };
> > > > diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> > > > index 49721b4e1975..751472ddf536 100644
> > > > --- a/drivers/iommu/irq_remapping.c
> > > > +++ b/drivers/iommu/irq_remapping.c
> > > > @@ -37,7 +37,7 @@ static void irq_remapping_disable_io_apic(void)
> > > > * now.
> > > > */
> > > > if (boot_cpu_has(X86_FEATURE_APIC) || apic_from_smp_config())
> > > > - disconnect_bsp_APIC(0);
> > > > + lapic_set_legacy_irq_mode(0);
> > > > }
> > > > static void __init irq_remapping_modify_x86_ops(void)
> > > >
> > >
> > >
> >
> >
> >
>
>

2018-01-25 02:49:47

by Baoquan He

[permalink] [raw]

Subject: Re: [RESEND PATCH 3/3] x86/apic: Clean up the names of legacy irq mode setting related functions

On 01/19/18 at 02:42pm, Dou Liyang wrote:
> Hi Baoquan,
>
> At 01/05/2018 12:39 PM, Baoquan He wrote:
> [...]
> > /*
> > - * Not an __init, needed by kexec/kdump code.
> > - * For safety IO-APIC and Local APIC need be cleared before this.
> > + * In legacy irq mode, full DOS compatibility with the uniprocessor PC/AT is
> > + * provided by using the APICs in conjunction with standard 8259A-equivalent
> > + * programmable interrupt controllers (PICs). It's necessary to deliver legacy
> > + * interrupts even when APIC mode is not enabled. This is required by kexec/
> > + * kdump before enter into the 2nd kernel.
> > */
> > void switch_to_legacy_irq_mode(void)
> > {
> > if (!nr_legacy_irqs())
> > return;
> > - x86_io_apic_ops.disable();
> > + ioapic_set_virtual_wire_mode();
> > +
> > + if (boot_cpu_has(X86_FEATURE_APIC) || apic_from_smp_config())
> > + lapic_set_legacy_irq_mode(ioapic_i8259.pin != -1);
>
> Seems these two function, ioapic/lapic_set_legacy_irq_mode should be
> exclusive.
>
> But We do that because both the through-lapic and through-ioapic virtual
> wire mode need setup the APIC_SPIV_APIC_ENABLED which is only located in
> the lapic_set_legacy_irq_mode(). So we need call them both.
>
> IMO, this cleanup may not make it clear. we can separate these two mode
> totally or just keep it like before.

OK, I tried to find document about the virtual wire mode with
through-ioapic, but failed. The code has been there for a long time and
everything works well, that proves it's good. While the concept is still
not clear to me, at least there isn't description to tell explicitly.
And also we still need the x86_io_apic_ops.disable() hooker which irq
remapping need to use as dou commented.

So drop this clean up patch for now. I will repost the patchset
including patch 1 and 2.

Thanks
Baoquan

> > }
> > #ifdef CONFIG_X86_32
> > diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> > index 1151ccd72ce9..c30f0f273dbd 100644
> > --- a/arch/x86/kernel/x86_init.c
> > +++ b/arch/x86/kernel/x86_init.c
> > @@ -148,5 +148,5 @@ void arch_restore_msi_irqs(struct pci_dev *dev)
> > struct x86_io_apic_ops x86_io_apic_ops __ro_after_init = {
> > .read = native_io_apic_read,
> > - .disable = native_disable_io_apic,
> > + .disable = switch_to_legacy_irq_mode,
> > };
> > diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> > index 49721b4e1975..751472ddf536 100644
> > --- a/drivers/iommu/irq_remapping.c
> > +++ b/drivers/iommu/irq_remapping.c
> > @@ -37,7 +37,7 @@ static void irq_remapping_disable_io_apic(void)
> > * now.
> > */
> > if (boot_cpu_has(X86_FEATURE_APIC) || apic_from_smp_config())
> > - disconnect_bsp_APIC(0);
> > + lapic_set_legacy_irq_mode(0);
> > }
> > static void __init irq_remapping_modify_x86_ops(void)
> >
>
>