linux-kernel - Re: [Patch v4 12/16] x86, irq, ACPI: Implement interface to support ACPI based IOAPIC hot-addition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.10.1409102202530.4178@nanos>
Date:	Wed, 10 Sep 2014 22:06:02 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Jiang Liu <jiang.liu@...ux.intel.com>
cc:	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Yinghai Lu <yinghai@...nel.org>,
	Borislav Petkov <bp@...en8.de>,
	Grant Likely <grant.likely@...aro.org>,
	Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
	x86@...nel.org, Prarit Bhargava <prarit@...hat.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Tony Luck <tony.luck@...el.com>,
	Joerg Roedel <joro@...tes.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-acpi@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	linux-pm@...r.kernel.org
Subject: Re: [Patch v4 12/16] x86, irq, ACPI: Implement interface to support
 ACPI based IOAPIC hot-addition

On Wed, 10 Sep 2014, Jiang Liu wrote:
> >>  int mp_register_ioapic(int id, u32 address, u32 gsi_base,
> >> @@ -3867,8 +3873,15 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
> >>  	}
> >>  	for_each_ioapic(ioapic)
> >>  		if (ioapics[ioapic].mp_config.apicaddr == address) {
> >> -			pr_warn("address 0x%x conflicts with IOAPIC%d\n",
> >> -				address, ioapic);
> >> +			/*
> >> +			 * IOAPIC unit may also be visible in PCI scope.
> >> +			 * When ioapic PCI driver's probe() is called,
> >> +			 * the IOAPIC unit may have already been initialized
> >> +			 * at boot time.
> >> +			 */
> >> +			if (!ioapic_initialized)
> >> +				pr_warn("address 0x%x conflicts with IOAPIC%d\n",
> >> +					address, ioapic);
> > 
> > Hmm. This smells fishy. Why do we allow multiple initializations of
> > the same IOAPIC in the first place. Either it's done via ACPI or via
> > PCI, but not both.
> The ACPI subsystem will register and initialize all IOAPICs when walking
> ACPI MADT table during boot, before initializing PCI subsystem.
> Later when binding ioapic PCI driver to IOAPIC PCI device, it will try
> to register the IOAPIC device again.
> 
> After this patchset is applied, we could remove the !ioapic_initialized
> check. We check acpi_ioapic_register() before calling
> acpi_register_ioapic(). So the check becomes redundant.
> Or we could remove the temporary code from this patch.

How about removing the disfunctional ioapic PCI driver first and then
implementing the whole thing cleanly?
 
> > 
> >>  			return -EEXIST;
> >>  		}
> >>  
> >> @@ -3918,6 +3931,14 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
> >>  	ioapics[idx].irqdomain = NULL;
> >>  	ioapics[idx].irqdomain_cfg = *cfg;
> >>  
> >> +	if (ioapic_initialized) {
> > 
> > I have a hard time to understand this conditional. Why can't we do
> > that unconditionally?
> How about following comments?
> /*
>  * If mp_register_ioapic() is called during early boot stage when
>  * walking ACPI/SFI/DT tables, it's too early to create irqdomain,
>  * we are still using bootmem allocator. So delay it to setup_IO_APIC().
>  */

Fine, but then the "if (ioapic_initialized)" conditional still does
not make sense. We surely have some global non ioapic specific
indicator that bootmem is done and the proper memory allocator is
available, right?

Aside of that is there a point to walk those tables before we actually
can make any use of their content?

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/