[Patch v4 11/16] x86, irq, ACPI: Introduce a rwsem to protect IOAPIC operations from hotplug

[Patch v4 11/16] x86, irq, ACPI: Introduce a rwsem to protect IOAPIC operations from hotplug

[Jiang Liu]

We are going to support ACPI based IOAPIC hotplug, so introduce a rwsem
to protect IOAPIC data structures from IOAPIC hotplug. We choose to
serialize in ACPI instead of in the IOAPIC core because:
1) currently we are only plan to support ACPI based IOAPIC hotplug
2) it's much more cleaner and easier
3) It does't affect IOAPIC discovered by devicetree, SFI and mpparse.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 arch/x86/kernel/acpi/boot.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index b436fc735aa4..e23f7460c3f8 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -76,6 +76,8 @@ int acpi_fix_pin2_polarity __initdata;
 static u64 acpi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;
 #endif
 
+static DECLARE_RWSEM(acpi_ioapic_rwsem);
+
 /* --------------------------------------------------------------------------
                               Boot-time Configuration
    -------------------------------------------------------------------------- */
@@ -604,8 +606,11 @@ void __init acpi_pic_sci_set_trigger(unsigned int irq, u16 trigger)
 
 int acpi_gsi_to_irq(u32 gsi, unsigned int *irqp)
 {
-	int irq = mp_map_gsi_to_irq(gsi, IOAPIC_MAP_ALLOC | IOAPIC_MAP_CHECK);
+	int irq;
 
+	down_read(&acpi_ioapic_rwsem);
+	irq = mp_map_gsi_to_irq(gsi, IOAPIC_MAP_ALLOC | IOAPIC_MAP_CHECK);
+	up_read(&acpi_ioapic_rwsem);
 	if (irq >= 0) {
 		*irqp = irq;
 		return 0;
@@ -646,7 +651,9 @@ static int acpi_register_gsi_ioapic(struct device *dev, u32 gsi,
 	int irq = gsi;
 
 #ifdef CONFIG_X86_IO_APIC
+	down_read(&acpi_ioapic_rwsem);
 	irq = mp_register_gsi(dev, gsi, trigger, polarity);
+	up_read(&acpi_ioapic_rwsem);
 #endif
 
 	return irq;
@@ -655,7 +662,9 @@ static int acpi_register_gsi_ioapic(struct device *dev, u32 gsi,
 static void acpi_unregister_gsi_ioapic(u32 gsi)
 {
 #ifdef CONFIG_X86_IO_APIC
+	down_read(&acpi_ioapic_rwsem);
 	mp_unregister_gsi(gsi);
+	up_read(&acpi_ioapic_rwsem);
 #endif
 }
 
@@ -1181,7 +1190,9 @@ static void __init acpi_process_madt(void)
 			/*
 			 * Parse MADT IO-APIC entries
 			 */
+			down_write(&acpi_ioapic_rwsem);
 			error = acpi_parse_madt_ioapic_entries();
+			up_write(&acpi_ioapic_rwsem);
 			if (!error) {
 				acpi_set_irq_model_ioapic();
 
-- 
1.7.10.4

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

[Jiang Liu]

Implement acpi_register_ioapic() and enhance mp_register_ioapic()
to support ACPI based IOAPIC hot-addition.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 arch/x86/kernel/acpi/boot.c    |   31 +++++++++++++++++++++++++++++--
 arch/x86/kernel/apic/io_apic.c |   27 ++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index e23f7460c3f8..796cd9e31ef3 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -764,8 +764,35 @@ EXPORT_SYMBOL(acpi_unmap_lsapic);
 
 int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base)
 {
-	/* TBD */
-	return -EINVAL;
+	int ret = -ENOSYS;
+#ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
+	int ioapic_id;
+	u64 addr;
+	struct ioapic_domain_cfg cfg = {
+		.type = IOAPIC_DOMAIN_DYNAMIC,
+		.ops = &acpi_irqdomain_ops,
+	};
+
+	ioapic_id = acpi_get_ioapic_id(handle, gsi_base, &addr);
+	if (ioapic_id < 0) {
+		unsigned long long uid;
+		acpi_status status;
+
+		status = acpi_evaluate_integer(handle, METHOD_NAME__UID,
+					       NULL, &uid);
+		if (ACPI_FAILURE(status)) {
+			acpi_handle_warn(handle, "failed to get IOAPIC ID.\n");
+			return -EINVAL;
+		}
+		ioapic_id = (int)uid;
+	}
+
+	down_write(&acpi_ioapic_rwsem);
+	ret  = mp_register_ioapic(ioapic_id, phys_addr, gsi_base, &cfg);
+	up_write(&acpi_ioapic_rwsem);
+#endif
+
+	return ret;
 }
 
 EXPORT_SYMBOL(acpi_register_ioapic);
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 6e67af0c5f99..b286461cabf9 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3851,7 +3851,13 @@ static int bad_ioapic_register(int idx)
 
 static int find_free_ioapic_entry(void)
 {
-	return nr_ioapics;
+	int idx;
+
+	for (idx = 0; idx < MAX_IO_APICS; idx++)
+		if (ioapics[idx].nr_registers == 0)
+			return idx;
+
+	return MAX_IO_APICS;
 }
 
 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);
 			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) {
+		if (mp_irqdomain_create(idx)) {
+			clear_fixmap(FIX_IO_APIC_BASE_0 + idx);
+			return -ENOMEM;
+		}
+		alloc_ioapic_saved_registers(idx);
+	}
+
 	if (gsi_cfg->gsi_end >= gsi_top)
 		gsi_top = gsi_cfg->gsi_end + 1;
 	if (nr_ioapics <= idx)
-- 
1.7.10.4

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

[Thomas Gleixner]

On Thu, 28 Aug 2014, Jiang Liu wrote:
>  EXPORT_SYMBOL(acpi_register_ioapic);
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 6e67af0c5f99..b286461cabf9 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -3851,7 +3851,13 @@ static int bad_ioapic_register(int idx)
>  
>  static int find_free_ioapic_entry(void)
>  {
> -	return nr_ioapics;
> +	int idx;
> +
> +	for (idx = 0; idx < MAX_IO_APICS; idx++)
> +		if (ioapics[idx].nr_registers == 0)
> +			return idx;
> +
> +	return MAX_IO_APICS;
>  }
>  
>  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.

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

> +		if (mp_irqdomain_create(idx)) {
> +			clear_fixmap(FIX_IO_APIC_BASE_0 + idx);
> +			return -ENOMEM;
> +		}
> +		alloc_ioapic_saved_registers(idx);
> +	}
> +
>  	if (gsi_cfg->gsi_end >= gsi_top)
>  		gsi_top = gsi_cfg->gsi_end + 1;
>  	if (nr_ioapics <= idx)

Thanks,

	tglx

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

[Jiang Liu]

On 2014/9/9 20:20, Thomas Gleixner wrote:
> On Thu, 28 Aug 2014, Jiang Liu wrote:
>>  EXPORT_SYMBOL(acpi_register_ioapic);
>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>> index 6e67af0c5f99..b286461cabf9 100644
>> --- a/arch/x86/kernel/apic/io_apic.c
>> +++ b/arch/x86/kernel/apic/io_apic.c
>> @@ -3851,7 +3851,13 @@ static int bad_ioapic_register(int idx)
>>  
>>  static int find_free_ioapic_entry(void)
>>  {
>> -	return nr_ioapics;
>> +	int idx;
>> +
>> +	for (idx = 0; idx < MAX_IO_APICS; idx++)
>> +		if (ioapics[idx].nr_registers == 0)
>> +			return idx;
>> +
>> +	return MAX_IO_APICS;
>>  }
>>  
>>  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.

> 
>>  			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().
 */
Regards!
Gerry
> 
>> +		if (mp_irqdomain_create(idx)) {
>> +			clear_fixmap(FIX_IO_APIC_BASE_0 + idx);
>> +			return -ENOMEM;
>> +		}
>> +		alloc_ioapic_saved_registers(idx);
>> +	}
>> +
>>  	if (gsi_cfg->gsi_end >= gsi_top)
>>  		gsi_top = gsi_cfg->gsi_end + 1;
>>  	if (nr_ioapics <= idx)
> 
> Thanks,
> 
> 	tglx
> 

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

[Thomas Gleixner]

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

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

[Jiang Liu]

On 2014/9/11 4:06, Thomas Gleixner wrote:
> 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?
Good suggestion:)

>  
>>>
>>>>  			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?
Flag ioapic_initialized will be used to check whether we have created
irqdomains for IOAPICs. Currently function arch_dynirq_lower_bound()
uses that flag, and alloc_irq_from_domain() will use it too later.

> 
> Aside of that is there a point to walk those tables before we actually
> can make any use of their content?
At least we depend on walking those tables to detect whether system has
IOAPICs available:)

Regards!
Gerry
> 
> Thanks,
> 
> 	tglx
> 

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

[Jiang Liu]

On 2014/9/11 4:06, Thomas Gleixner wrote:
> 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?
Maybe a good name helps here. How about
bool hotplug = !!ioapic_initialized;

if (hotplug)
	mp_irqdomain_create(idx);

Regards!
Gerry	

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

[Patch v4 13/16] x86, irq, ACPI: Implement interfaces to support ACPI based IOAPIC hot-removal

[Jiang Liu]

Implement acpi_unregister_ioapic() to support ACPI based IOAPIC hot-removal.
An IOAPIC could only be removed when all its pins are unused.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 arch/x86/include/asm/io_apic.h |    1 +
 arch/x86/kernel/acpi/boot.c    |   13 +++++++---
 arch/x86/kernel/apic/io_apic.c |   55 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 94d05bd6586f..ce63cf34c93c 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -190,6 +190,7 @@ extern int mp_map_gsi_to_irq(u32 gsi, unsigned int flags);
 extern void mp_unmap_irq(int irq);
 extern int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 			      struct ioapic_domain_cfg *cfg);
+extern int mp_unregister_ioapic(u32 gsi_base);
 extern int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq,
 			    irq_hw_number_t hwirq);
 extern void mp_irqdomain_unmap(struct irq_domain *domain, unsigned int virq);
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 796cd9e31ef3..560a6d1cb0f4 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -794,15 +794,20 @@ int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base)
 
 	return ret;
 }
-
 EXPORT_SYMBOL(acpi_register_ioapic);
 
 int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base)
 {
-	/* TBD */
-	return -EINVAL;
-}
+	int ret = -ENOSYS;
+
+#ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
+	down_write(&acpi_ioapic_rwsem);
+	ret  = mp_unregister_ioapic(gsi_base);
+	up_write(&acpi_ioapic_rwsem);
+#endif
 
+	return ret;
+}
 EXPORT_SYMBOL(acpi_unregister_ioapic);
 
 static int __init acpi_parse_sbf(struct acpi_table_header *table)
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index b286461cabf9..fc1e1f9567bc 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -112,6 +112,7 @@ static struct ioapic {
 	struct ioapic_domain_cfg irqdomain_cfg;
 	struct irq_domain *irqdomain;
 	struct mp_pin_info *pin_info;
+	struct resource *iomem_res;
 } ioapics[MAX_IO_APICS];
 
 #define mpc_ioapic_ver(ioapic_idx)	ioapics[ioapic_idx].mp_config.apicver
@@ -250,6 +251,12 @@ static void alloc_ioapic_saved_registers(int idx)
 		pr_err("IOAPIC %d: suspend/resume impossible!\n", idx);
 }
 
+static void free_ioapic_saved_registers(int idx)
+{
+	kfree(ioapics[idx].saved_registers);
+	ioapics[idx].saved_registers = NULL;
+}
+
 int __init arch_early_irq_init(void)
 {
 	struct irq_cfg *cfg;
@@ -2972,6 +2979,16 @@ static int mp_irqdomain_create(int ioapic)
 	return 0;
 }
 
+static void ioapic_destroy_irqdomain(int idx)
+{
+	if (ioapics[idx].irqdomain) {
+		irq_domain_remove(ioapics[idx].irqdomain);
+		ioapics[idx].irqdomain = NULL;
+	}
+	kfree(ioapics[idx].pin_info);
+	ioapics[idx].pin_info = NULL;
+}
+
 void __init setup_IO_APIC(void)
 {
 	int ioapic;
@@ -3733,6 +3750,7 @@ static struct resource * __init ioapic_setup_resources(void)
 		snprintf(mem, IOAPIC_RESOURCE_NAME_SIZE, "IOAPIC %u", i);
 		mem += IOAPIC_RESOURCE_NAME_SIZE;
 		num++;
+		ioapics[i].iomem_res = res;
 	}
 
 	ioapic_resources = res;
@@ -3955,6 +3973,43 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 	return 0;
 }
 
+int mp_unregister_ioapic(u32 gsi_base)
+{
+	int ioapic, pin;
+	int found = 0;
+	struct mp_pin_info *pin_info;
+
+	for_each_ioapic(ioapic)
+		if (ioapics[ioapic].gsi_config.gsi_base == gsi_base) {
+			found = 1;
+			break;
+		}
+	if (!found) {
+		pr_warn("can't find IOAPIC for GSI %d\n", gsi_base);
+		return -ENODEV;
+	}
+
+	for_each_pin(ioapic, pin) {
+		pin_info = mp_pin_info(ioapic, pin);
+		if (pin_info->count) {
+			pr_warn("pin%d on IOAPIC%d is still in use.\n",
+				pin, ioapic);
+			return -EBUSY;
+		}
+	}
+
+	/* Mark entry not present */
+	ioapics[ioapic].nr_registers  = 0;
+	ioapic_destroy_irqdomain(ioapic);
+	free_ioapic_saved_registers(ioapic);
+	if (ioapics[ioapic].iomem_res)
+		release_resource(ioapics[ioapic].iomem_res);
+	clear_fixmap(FIX_IO_APIC_BASE_0 + ioapic);
+	memset(&ioapics[ioapic], 0, sizeof(ioapics[ioapic]));
+
+	return 0;
+}
+
 int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq,
 		     irq_hw_number_t hwirq)
 {
-- 
1.7.10.4

[Patch v4 14/16] x86, irq: Introduce helper to check whether an IOAPIC has been registered

[Jiang Liu]

Introduce acpi_ioapic_registered() to check whether an IOAPIC has already
been registered, it will be used when enabling IOAPIC hotplug.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 arch/x86/include/asm/io_apic.h |    1 +
 arch/x86/kernel/acpi/boot.c    |   11 +++++++++++
 arch/x86/kernel/apic/io_apic.c |   11 +++++++++++
 include/linux/acpi.h           |    1 +
 4 files changed, 24 insertions(+)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index ce63cf34c93c..0db2b7037e4b 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -191,6 +191,7 @@ extern void mp_unmap_irq(int irq);
 extern int mp_register_ioapic(int id, u32 address, u32 gsi_base,
 			      struct ioapic_domain_cfg *cfg);
 extern int mp_unregister_ioapic(u32 gsi_base);
+extern int mp_ioapic_registered(u32 gsi_base);
 extern int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq,
 			    irq_hw_number_t hwirq);
 extern void mp_irqdomain_unmap(struct irq_domain *domain, unsigned int virq);
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 560a6d1cb0f4..6aa796f77b71 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -810,6 +810,17 @@ int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base)
 }
 EXPORT_SYMBOL(acpi_unregister_ioapic);
 
+int acpi_ioapic_registered(acpi_handle handle, u32 gsi_base)
+{
+	int ret;
+
+	down_write(&acpi_ioapic_rwsem);
+	ret  = mp_ioapic_registered(gsi_base);
+	up_write(&acpi_ioapic_rwsem);
+
+	return ret;
+}
+
 static int __init acpi_parse_sbf(struct acpi_table_header *table)
 {
 	struct acpi_table_boot *sb;
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index fc1e1f9567bc..e104993a2394 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -4010,6 +4010,17 @@ int mp_unregister_ioapic(u32 gsi_base)
 	return 0;
 }
 
+int mp_ioapic_registered(u32 gsi_base)
+{
+	int ioapic;
+
+	for_each_ioapic(ioapic)
+		if (ioapics[ioapic].gsi_config.gsi_base == gsi_base)
+			return 1;
+
+	return 0;
+}
+
 int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq,
 		     irq_hw_number_t hwirq)
 {
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index b41c5aef5336..754d99d5f258 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -155,6 +155,7 @@ int acpi_get_ioapic_id(acpi_handle handle, u32 gsi_base, u64 *phys_addr);
 
 int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base);
 int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base);
+int acpi_ioapic_registered(acpi_handle handle, u32 gsi_base);
 void acpi_irq_stats_init(void);
 extern u32 acpi_irq_handled;
 extern u32 acpi_irq_not_handled;
-- 
1.7.10.4

Re: [Patch v4 14/16] x86, irq: Introduce helper to check whether an IOAPIC has been registered

[Thomas Gleixner]

On Thu, 28 Aug 2014, Jiang Liu wrote:

> Introduce acpi_ioapic_registered() to check whether an IOAPIC has already
> been registered, it will be used when enabling IOAPIC hotplug.
> 
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  arch/x86/include/asm/io_apic.h |    1 +
>  arch/x86/kernel/acpi/boot.c    |   11 +++++++++++
>  arch/x86/kernel/apic/io_apic.c |   11 +++++++++++
>  include/linux/acpi.h           |    1 +
>  4 files changed, 24 insertions(+)
> 
> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> index ce63cf34c93c..0db2b7037e4b 100644
> --- a/arch/x86/include/asm/io_apic.h
> +++ b/arch/x86/include/asm/io_apic.h
> @@ -191,6 +191,7 @@ extern void mp_unmap_irq(int irq);
>  extern int mp_register_ioapic(int id, u32 address, u32 gsi_base,
>  			      struct ioapic_domain_cfg *cfg);
>  extern int mp_unregister_ioapic(u32 gsi_base);
> +extern int mp_ioapic_registered(u32 gsi_base);
>  extern int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq,
>  			    irq_hw_number_t hwirq);
>  extern void mp_irqdomain_unmap(struct irq_domain *domain, unsigned int virq);
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 560a6d1cb0f4..6aa796f77b71 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -810,6 +810,17 @@ int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base)
>  }
>  EXPORT_SYMBOL(acpi_unregister_ioapic);
>  
> +int acpi_ioapic_registered(acpi_handle handle, u32 gsi_base)
> +{
> +	int ret;
> +
> +	down_write(&acpi_ioapic_rwsem);

Why down_write? You are merily checking whether the thing is
registered already.

> +	ret  = mp_ioapic_registered(gsi_base);
> +	up_write(&acpi_ioapic_rwsem);
> +
> +	return ret;
> +}

So I assume that this is for a particular caller in the apci ioapic
hotplug code and that call site has its own serialization. Otherwise
the return value is not protected at all.

Please add a Docbook comment to that function, and document the
locking rules as thats pretty non obvious.

The such is missing in some other patches as well.

Thanks,

	tglx

Re: [Patch v4 14/16] x86, irq: Introduce helper to check whether an IOAPIC has been registered

[Jiang Liu]

On 2014/9/9 20:37, Thomas Gleixner wrote:
> On Thu, 28 Aug 2014, Jiang Liu wrote:
> 
>> Introduce acpi_ioapic_registered() to check whether an IOAPIC has already
>> been registered, it will be used when enabling IOAPIC hotplug.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>> ---
>>  arch/x86/include/asm/io_apic.h |    1 +
>>  arch/x86/kernel/acpi/boot.c    |   11 +++++++++++
>>  arch/x86/kernel/apic/io_apic.c |   11 +++++++++++
>>  include/linux/acpi.h           |    1 +
>>  4 files changed, 24 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
>> index ce63cf34c93c..0db2b7037e4b 100644
>> --- a/arch/x86/include/asm/io_apic.h
>> +++ b/arch/x86/include/asm/io_apic.h
>> @@ -191,6 +191,7 @@ extern void mp_unmap_irq(int irq);
>>  extern int mp_register_ioapic(int id, u32 address, u32 gsi_base,
>>  			      struct ioapic_domain_cfg *cfg);
>>  extern int mp_unregister_ioapic(u32 gsi_base);
>> +extern int mp_ioapic_registered(u32 gsi_base);
>>  extern int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq,
>>  			    irq_hw_number_t hwirq);
>>  extern void mp_irqdomain_unmap(struct irq_domain *domain, unsigned int virq);
>> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
>> index 560a6d1cb0f4..6aa796f77b71 100644
>> --- a/arch/x86/kernel/acpi/boot.c
>> +++ b/arch/x86/kernel/acpi/boot.c
>> @@ -810,6 +810,17 @@ int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base)
>>  }
>>  EXPORT_SYMBOL(acpi_unregister_ioapic);
>>  
>> +int acpi_ioapic_registered(acpi_handle handle, u32 gsi_base)
>> +{
>> +	int ret;
>> +
>> +	down_write(&acpi_ioapic_rwsem);
> 
> Why down_write? You are merily checking whether the thing is
> registered already.
Yeah, a down_read() should be enough:)

>> +	ret  = mp_ioapic_registered(gsi_base);
>> +	up_write(&acpi_ioapic_rwsem);
>> +
>> +	return ret;
>> +}
> 
> So I assume that this is for a particular caller in the apci ioapic
> hotplug code and that call site has its own serialization. Otherwise
> the return value is not protected at all.
> 
> Please add a Docbook comment to that function, and document the
> locking rules as thats pretty non obvious.
How about this comments about locking rules?
/*
 * Locks related to IOAPIC hotplug
 * Hotplug side:
 *      ->lock_device_hotplug() //device_hotplug_lock
 *              ->acpi_ioapic_rwsem
 *                      ->ioapic_lock
 * Interrupt mapping side:
 *      ->acpi_ioapic_rwsem
 *              ->ioapic_mutex
 *                      ->ioapic_lock
 */
Regards!
Gerry
> 
> The such is missing in some other patches as well.
> 
> Thanks,
> 
> 	tglx
> 

Re: [Patch v4 14/16] x86, irq: Introduce helper to check whether an IOAPIC has been registered

[Thomas Gleixner]

On Wed, 10 Sep 2014, Jiang Liu wrote:
> On 2014/9/9 20:37, Thomas Gleixner wrote:
> >> +	ret  = mp_ioapic_registered(gsi_base);
> >> +	up_write(&acpi_ioapic_rwsem);
> >> +
> >> +	return ret;
> >> +}
> > 
> > So I assume that this is for a particular caller in the apci ioapic
> > hotplug code and that call site has its own serialization. Otherwise
> > the return value is not protected at all.
> > 
> > Please add a Docbook comment to that function, and document the
> > locking rules as thats pretty non obvious.
> How about this comments about locking rules?
> /*
>  * Locks related to IOAPIC hotplug
>  * Hotplug side:
>  *      ->lock_device_hotplug() //device_hotplug_lock
>  *              ->acpi_ioapic_rwsem
>  *                      ->ioapic_lock
>  * Interrupt mapping side:
>  *      ->acpi_ioapic_rwsem
>  *              ->ioapic_mutex
>  *                      ->ioapic_lock
>  */

That looks asymetric. Why is the hotplug side not taking ioapic_mutex?

Thanks,

	tglx

Re: [Patch v4 14/16] x86, irq: Introduce helper to check whether an IOAPIC has been registered

[Jiang Liu]

On 2014/9/11 4:08, Thomas Gleixner wrote:
> On Wed, 10 Sep 2014, Jiang Liu wrote:
>> On 2014/9/9 20:37, Thomas Gleixner wrote:
>>>> +	ret  = mp_ioapic_registered(gsi_base);
>>>> +	up_write(&acpi_ioapic_rwsem);
>>>> +
>>>> +	return ret;
>>>> +}
>>>
>>> So I assume that this is for a particular caller in the apci ioapic
>>> hotplug code and that call site has its own serialization. Otherwise
>>> the return value is not protected at all.
>>>
>>> Please add a Docbook comment to that function, and document the
>>> locking rules as thats pretty non obvious.
>> How about this comments about locking rules?
>> /*
>>  * Locks related to IOAPIC hotplug
>>  * Hotplug side:
>>  *      ->lock_device_hotplug() //device_hotplug_lock
>>  *              ->acpi_ioapic_rwsem
>>  *                      ->ioapic_lock
>>  * Interrupt mapping side:
>>  *      ->acpi_ioapic_rwsem
>>  *              ->ioapic_mutex
>>  *                      ->ioapic_lock
>>  */
> 
> That looks asymetric. Why is the hotplug side not taking ioapic_mutex?
Hi Thomas,
	We decide to protect system from IOAPIC hotplug in the ACPI
layer. For ACPI enumerated IOAPICs, ioapic_mutex is redundant and
it's used to protect MPPARSE, SFI, OF enumerated IOAPICs.
Regards!
Gerry

> 
> Thanks,
> 
> 	tglx
>