[v1] genirq/irqdomain: Don't call ops->select for DOMAIN_BUS_ANY tokens

[PATCH] genirq/irqdomain: Don't call ops->select for DOMAIN_BUS_ANY tokens

Posted by Marc Zyngier 1 year, 11 months ago

Users of the IRQCHIP_PLATFORM_DRIVER_{BEGIN,END} helpers rely
on a fwspec containing only the fwnode (and crucially a number
of parameters set to 0) together with a DOMAIN_BUS_ANY token
to check whether a parent irqchip has probed and registered
a domain.

Since de1ff306dcf4 ("genirq/irqdomain: Remove the param count
restriction from select()"), we call ops->select unconditionally,
meaning that irqchips implementing select now need to handle
ANY as a match.

Instead of adding more esoteric checks to the individual drivers,
add that condition to irq_find_matching_fwspec(), and let it
handle the corner case, as per the comment in the function.

This restores the functionnality of the above helpers.

Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reported-by: Biju Das <biju.das.jz@bp.renesas.com>
Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20240219-gic-fix-child-domain-v1-1-09f8fd2d9a8f@linaro.org
---
 kernel/irq/irqdomain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index aeb41655d6de..3dd1c871e091 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -449,7 +449,7 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec,
 	 */
 	mutex_lock(&irq_domain_mutex);
 	list_for_each_entry(h, &irq_domain_list, link) {
-		if (h->ops->select)
+		if (h->ops->select && bus_token != DOMAIN_BUS_ANY)
 			rc = h->ops->select(h, fwspec, bus_token);
 		else if (h->ops->match)
 			rc = h->ops->match(h, to_of_node(fwnode), bus_token);
-- 
2.39.2

Re: [PATCH] genirq/irqdomain: Don't call ops->select for DOMAIN_BUS_ANY tokens

Posted by Thomas Gleixner 1 year, 11 months ago

On Tue, Feb 20 2024 at 11:47, Marc Zyngier wrote:
> Users of the IRQCHIP_PLATFORM_DRIVER_{BEGIN,END} helpers rely
> on a fwspec containing only the fwnode (and crucially a number
> of parameters set to 0) together with a DOMAIN_BUS_ANY token
> to check whether a parent irqchip has probed and registered
> a domain.
>
> Since de1ff306dcf4 ("genirq/irqdomain: Remove the param count
> restriction from select()"), we call ops->select unconditionally,
> meaning that irqchips implementing select now need to handle
> ANY as a match.
>
> Instead of adding more esoteric checks to the individual drivers,
> add that condition to irq_find_matching_fwspec(), and let it
> handle the corner case, as per the comment in the function.
>
> This restores the functionnality of the above helpers.
>
> Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reported-by: Biju Das <biju.das.jz@bp.renesas.com>
> Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()")
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Link: https://lore.kernel.org/r/20240219-gic-fix-child-domain-v1-1-09f8fd2d9a8f@linaro.org

Bah. That breaks x86 because it uses DOMAIN_BUS_ANY to find the MSI
parent for a fwspec (IOAPIC and HPET) which gets either picked up by the
interrupt remapping or by the root vector domain.

Fix below.

Thanks,

        tglx
---
Subject: x86/apic/msi: Use DOMAIN_BUS_GENERIC_MSI for HPET/IO-APIC domain search
From: Thomas Gleixner <tglx@linutronix.de>
Date: Sun, 25 Feb 2024 16:56:12 +0100

The recent restriction to invoke irqdomain_ops::select() only when the
domain bus toke is DOMAIN_BUS_ANY breaks the search for the parent MSI
domain of HPET and IO-APIC. The latter causes a full boot fail.

The restriction itself makes sense to avoid adding DOMAIN_BUS_ANY matches
into the various ARM specific select() callbacks. Reverting this change
would obviously break ARM platforms again and require DOMAIN_BUS_ANY
matches added to various places.

A simpler solution is to use the DOMAIN_BUS_GENERIC_MSI token for the HPET
and IO-APIC parent domain search. This works out of the box because the
affected parent domains check only for the firmware specification content
and not for the bus token.

Fixes: 5aa3c0cf5bba ("genirq/irqdomain: Don't call ops->select for DOMAIN_BUS_ANY tokens")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/apic/io_apic.c |    2 +-
 arch/x86/kernel/hpet.c         |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2354,7 +2354,7 @@ static int mp_irqdomain_create(int ioapi
 	fwspec.param_count = 1;
 	fwspec.param[0] = mpc_ioapic_id(ioapic);
 
-	parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_ANY);
+	parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_GENERIC_MSI);
 	if (!parent) {
 		if (!cfg->dev)
 			irq_domain_free_fwnode(fn);
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -568,7 +568,7 @@ static struct irq_domain *hpet_create_ir
 	fwspec.param_count = 1;
 	fwspec.param[0] = hpet_id;
 
-	parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_ANY);
+	parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_GENERIC_MSI);
 	if (!parent) {
 		irq_domain_free_fwnode(fn);
 		kfree(domain_info);

Re: [PATCH] genirq/irqdomain: Don't call ops->select for DOMAIN_BUS_ANY tokens

Posted by Marc Zyngier 1 year, 11 months ago

On 2024-02-25 16:19, Thomas Gleixner wrote:
> On Tue, Feb 20 2024 at 11:47, Marc Zyngier wrote:
>> Users of the IRQCHIP_PLATFORM_DRIVER_{BEGIN,END} helpers rely
>> on a fwspec containing only the fwnode (and crucially a number
>> of parameters set to 0) together with a DOMAIN_BUS_ANY token
>> to check whether a parent irqchip has probed and registered
>> a domain.
>> 
>> Since de1ff306dcf4 ("genirq/irqdomain: Remove the param count
>> restriction from select()"), we call ops->select unconditionally,
>> meaning that irqchips implementing select now need to handle
>> ANY as a match.
>> 
>> Instead of adding more esoteric checks to the individual drivers,
>> add that condition to irq_find_matching_fwspec(), and let it
>> handle the corner case, as per the comment in the function.
>> 
>> This restores the functionnality of the above helpers.
>> 
>> Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Reported-by: Biju Das <biju.das.jz@bp.renesas.com>
>> Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count 
>> restriction from select()")
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> Link: 
>> https://lore.kernel.org/r/20240219-gic-fix-child-domain-v1-1-09f8fd2d9a8f@linaro.org
> 
> Bah. That breaks x86 because it uses DOMAIN_BUS_ANY to find the MSI
> parent for a fwspec (IOAPIC and HPET) which gets either picked up by 
> the
> interrupt remapping or by the root vector domain.
> 
> Fix below.
> 
> Thanks,
> 
>         tglx
> ---
> Subject: x86/apic/msi: Use DOMAIN_BUS_GENERIC_MSI for HPET/IO-APIC 
> domain search
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Sun, 25 Feb 2024 16:56:12 +0100
> 
> The recent restriction to invoke irqdomain_ops::select() only when the
> domain bus toke is DOMAIN_BUS_ANY breaks the search for the parent MSI
> domain of HPET and IO-APIC. The latter causes a full boot fail.
> 
> The restriction itself makes sense to avoid adding DOMAIN_BUS_ANY 
> matches
> into the various ARM specific select() callbacks. Reverting this change
> would obviously break ARM platforms again and require DOMAIN_BUS_ANY
> matches added to various places.
> 
> A simpler solution is to use the DOMAIN_BUS_GENERIC_MSI token for the 
> HPET
> and IO-APIC parent domain search. This works out of the box because the
> affected parent domains check only for the firmware specification 
> content
> and not for the bus token.
> 
> Fixes: 5aa3c0cf5bba ("genirq/irqdomain: Don't call ops->select for
> DOMAIN_BUS_ANY tokens")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Looks good to me.

Reviewed-by: Marc Zyngier <maz@kernel.org>

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] genirq/irqdomain: Don't call ops->select for DOMAIN_BUS_ANY tokens

Posted by Borislav Petkov 1 year, 11 months ago

On Sun, Feb 25, 2024 at 05:19:52PM +0100, Thomas Gleixner wrote:
> Bah. That breaks x86 because it uses DOMAIN_BUS_ANY to find the MSI
> parent for a fwspec (IOAPIC and HPET) which gets either picked up by the
> interrupt remapping or by the root vector domain.
> 
> Fix below.

The guest boots fine now, thx.

Tested-by: Borislav Petkov (AMD) <bp@alien8.de>

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[tip: irq/msi] x86/apic/msi: Use DOMAIN_BUS_GENERIC_MSI for HPET/IO-APIC domain search

Posted by tip-bot2 for Thomas Gleixner 1 year, 11 months ago

The following commit has been merged into the irq/msi branch of tip:

Commit-ID:     c147e1ef59d4751a60687074e4268ecc0ef31b5c
Gitweb:        https://git.kernel.org/tip/c147e1ef59d4751a60687074e4268ecc0ef31b5c
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Sun, 25 Feb 2024 16:56:12 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sun, 25 Feb 2024 18:53:08 +01:00

x86/apic/msi: Use DOMAIN_BUS_GENERIC_MSI for HPET/IO-APIC domain search

The recent restriction to invoke irqdomain_ops::select() only when the
domain bus token is not DOMAIN_BUS_ANY breaks the search for the parent MSI
domain of HPET and IO-APIC. The latter causes a full boot fail.

The restriction itself makes sense to avoid adding DOMAIN_BUS_ANY matches
into the various ARM specific select() callbacks. Reverting this change
would obviously break ARM platforms again and require DOMAIN_BUS_ANY
matches added to various places.

A simpler solution is to use the DOMAIN_BUS_GENERIC_MSI token for the HPET
and IO-APIC parent domain search. This works out of the box because the
affected parent domains check only for the firmware specification content
and not for the bus token.

Fixes: 5aa3c0cf5bba ("genirq/irqdomain: Don't call ops->select for DOMAIN_BUS_ANY tokens")
Reported-by: Borislav Petkov (AMD) <bp@alien8.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/878r38cy8n.ffs@tglx
---
 arch/x86/kernel/apic/io_apic.c | 2 +-
 arch/x86/kernel/hpet.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 40c7cf1..e66c775 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2354,7 +2354,7 @@ static int mp_irqdomain_create(int ioapic)
 	fwspec.param_count = 1;
 	fwspec.param[0] = mpc_ioapic_id(ioapic);
 
-	parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_ANY);
+	parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_GENERIC_MSI);
 	if (!parent) {
 		if (!cfg->dev)
 			irq_domain_free_fwnode(fn);
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index a38d0c9..c96ae8f 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -568,7 +568,7 @@ static struct irq_domain *hpet_create_irq_domain(int hpet_id)
 	fwspec.param_count = 1;
 	fwspec.param[0] = hpet_id;
 
-	parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_ANY);
+	parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_GENERIC_MSI);
 	if (!parent) {
 		irq_domain_free_fwnode(fn);
 		kfree(domain_info);

[tip: irq/msi] genirq/irqdomain: Don't call ops->select for DOMAIN_BUS_ANY tokens

Posted by tip-bot2 for Marc Zyngier 1 year, 11 months ago

The following commit has been merged into the irq/msi branch of tip:

Commit-ID:     5aa3c0cf5bba6437c9e63a56f684f61de8b503d6
Gitweb:        https://git.kernel.org/tip/5aa3c0cf5bba6437c9e63a56f684f61de8b503d6
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Tue, 20 Feb 2024 11:47:31 
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 20 Feb 2024 17:30:57 +01:00

genirq/irqdomain: Don't call ops->select for DOMAIN_BUS_ANY tokens

Users of the IRQCHIP_PLATFORM_DRIVER_{BEGIN,END} helpers rely on a fwspec
containing only the fwnode (and crucially a number of parameters set to 0)
together with a DOMAIN_BUS_ANY token to check whether a parent irqchip has
probed and registered a domain.

Since de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction
from select()"), ops->select() is called unconditionally, meaning that
irqchips implementing select() now need to handle ANY as a match.

Instead of adding more esoteric checks to the individual drivers, add that
condition to irq_find_matching_fwspec(), and let it handle the corner case,
as per the comment in the function.

This restores the functionality of the above helpers.

Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()")
Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reported-by: Biju Das <biju.das.jz@bp.renesas.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Biju Das <biju.das.jz@bp.renesas.com>
Link: https://lore.kernel.org/r/20240220114731.1898534-1-maz@kernel.org
Link: https://lore.kernel.org/r/20240219-gic-fix-child-domain-v1-1-09f8fd2d9a8f@linaro.org
---
 kernel/irq/irqdomain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index aeb4165..3dd1c87 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -449,7 +449,7 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec,
 	 */
 	mutex_lock(&irq_domain_mutex);
 	list_for_each_entry(h, &irq_domain_list, link) {
-		if (h->ops->select)
+		if (h->ops->select && bus_token != DOMAIN_BUS_ANY)
 			rc = h->ops->select(h, fwspec, bus_token);
 		else if (h->ops->match)
 			rc = h->ops->match(h, to_of_node(fwnode), bus_token);