Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 25 Jan 2018 10:48:08 +0800
From:   Baoquan He <bhe@redhat.com>
To:     Dou Liyang <douly.fnst@cn.fujitsu.com>
Cc:     linux-kernel@vger.kernel.org, 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 3/3] x86/apic: Clean up the names of legacy irq
 mode setting related functions
Message-ID: <20180125024808.GA5512@localhost.localdomain>
References: <1515123732-28908-1-git-send-email-bhe@redhat.com>
 <20180105043929.GL7235@x1>
 <ff101d07-2a59-75f1-7e4b-834a8990bc70@cn.fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ff101d07-2a59-75f1-7e4b-834a8990bc70@cn.fujitsu.com>
User-Agent: Mutt/1.9.1 (2017-09-22)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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)
> > 
> 
>