Malta defconfig compiles with GIC on. Hence when compiling for SMP it causes the
new IPI code to be activated. But on qemu malta there's no GIC causing a
BUG_ON(!ipidomain) to be hit in mips_smp_ipi_init().
Since in that configuration one can only run a single core SMP (!), skip IPI
initialisation if we detect that this is the case. It is a sensible behaviour
to introduce and should keep such possible configuration to run rather than die
hard unnecessarily.
Signed-off-by: Qais Yousef <[email protected]>
Reported-by: Guenter Roeck <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/mips/kernel/smp.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index 37708d9..27cb638 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -243,6 +243,18 @@ static int __init mips_smp_ipi_init(void)
struct irq_domain *ipidomain;
struct device_node *node;
+ /*
+ * In some cases like qemu-malta, it is desired to try SMP with
+ * a single core. Qemu-malta has no GIC, so an attempt to set any IPIs
+ * would cause a BUG_ON() to be triggered since there's no ipidomain.
+ *
+ * Since for a single core system IPIs aren't required really, skip the
+ * initialisation which should generally keep any such configurations
+ * happy and only fail hard when trying to truely run SMP.
+ */
+ if (cpumask_weight(cpu_possible_mask) == 1)
+ return 0;
+
node = of_irq_find_parent(of_root);
ipidomain = irq_find_matching_host(node, DOMAIN_BUS_IPI);
--
2.7.3
On Thu, Mar 17, 2016 at 09:08:09PM +0000, Qais Yousef wrote:
> Malta defconfig compiles with GIC on. Hence when compiling for SMP it causes the
> new IPI code to be activated. But on qemu malta there's no GIC causing a
> BUG_ON(!ipidomain) to be hit in mips_smp_ipi_init().
>
> Since in that configuration one can only run a single core SMP (!), skip IPI
> initialisation if we detect that this is the case. It is a sensible behaviour
> to introduce and should keep such possible configuration to run rather than die
> hard unnecessarily.
>
> Signed-off-by: Qais Yousef <[email protected]>
> Reported-by: Guenter Roeck <[email protected]>
Tested-by: Guenter Roeck <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> arch/mips/kernel/smp.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> index 37708d9..27cb638 100644
> --- a/arch/mips/kernel/smp.c
> +++ b/arch/mips/kernel/smp.c
> @@ -243,6 +243,18 @@ static int __init mips_smp_ipi_init(void)
> struct irq_domain *ipidomain;
> struct device_node *node;
>
> + /*
> + * In some cases like qemu-malta, it is desired to try SMP with
> + * a single core. Qemu-malta has no GIC, so an attempt to set any IPIs
> + * would cause a BUG_ON() to be triggered since there's no ipidomain.
> + *
> + * Since for a single core system IPIs aren't required really, skip the
> + * initialisation which should generally keep any such configurations
> + * happy and only fail hard when trying to truely run SMP.
> + */
> + if (cpumask_weight(cpu_possible_mask) == 1)
> + return 0;
> +
> node = of_irq_find_parent(of_root);
> ipidomain = irq_find_matching_host(node, DOMAIN_BUS_IPI);
>
> --
> 2.7.3
>
On Thu, Mar 17, 2016 at 09:08:09PM +0000, Qais Yousef wrote:
> Malta defconfig compiles with GIC on. Hence when compiling for SMP it causes the
> new IPI code to be activated. But on qemu malta there's no GIC causing a
> BUG_ON(!ipidomain) to be hit in mips_smp_ipi_init().
>
> Since in that configuration one can only run a single core SMP (!), skip IPI
> initialisation if we detect that this is the case. It is a sensible behaviour
> to introduce and should keep such possible configuration to run rather than die
> hard unnecessarily.
Hi Qais/Ralf,
This patch is insufficient I'm afraid. It's entirely possible to use SMP
with multiple VPEs in a single core on Malta boards that don't have a
GIC - we have code handling IPIs in that case guarded by #ifdef
CONFIG_MIPS_MT_SMP in arch/mips/mti-malta/malta-int.c. I think the
BUG_ON needs to be removed entirely, unless that single-core multi-VPE
IPI code is also converted to use an IPI irqdomain.
Thanks,
Paul
>
> Signed-off-by: Qais Yousef <[email protected]>
> Reported-by: Guenter Roeck <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> arch/mips/kernel/smp.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> index 37708d9..27cb638 100644
> --- a/arch/mips/kernel/smp.c
> +++ b/arch/mips/kernel/smp.c
> @@ -243,6 +243,18 @@ static int __init mips_smp_ipi_init(void)
> struct irq_domain *ipidomain;
> struct device_node *node;
>
> + /*
> + * In some cases like qemu-malta, it is desired to try SMP with
> + * a single core. Qemu-malta has no GIC, so an attempt to set any IPIs
> + * would cause a BUG_ON() to be triggered since there's no ipidomain.
> + *
> + * Since for a single core system IPIs aren't required really, skip the
> + * initialisation which should generally keep any such configurations
> + * happy and only fail hard when trying to truely run SMP.
> + */
> + if (cpumask_weight(cpu_possible_mask) == 1)
> + return 0;
> +
> node = of_irq_find_parent(of_root);
> ipidomain = irq_find_matching_host(node, DOMAIN_BUS_IPI);
>
> --
> 2.7.3
>
>
Hi Paul,
On 01/04/2016 13:48, Paul Burton wrote:
> On Thu, Mar 17, 2016 at 09:08:09PM +0000, Qais Yousef wrote:
>> Malta defconfig compiles with GIC on. Hence when compiling for SMP it causes the
>> new IPI code to be activated. But on qemu malta there's no GIC causing a
>> BUG_ON(!ipidomain) to be hit in mips_smp_ipi_init().
>>
>> Since in that configuration one can only run a single core SMP (!), skip IPI
>> initialisation if we detect that this is the case. It is a sensible behaviour
>> to introduce and should keep such possible configuration to run rather than die
>> hard unnecessarily.
> Hi Qais/Ralf,
>
> This patch is insufficient I'm afraid. It's entirely possible to use SMP
> with multiple VPEs in a single core on Malta boards that don't have a
> GIC - we have code handling IPIs in that case guarded by #ifdef
> CONFIG_MIPS_MT_SMP in arch/mips/mti-malta/malta-int.c. I think the
> BUG_ON needs to be removed entirely, unless that single-core multi-VPE
> IPI code is also converted to use an IPI irqdomain.
>
I was under the impression that SMP is only supported under GIC and
older forms of SMP are deprecated.
I think the problem you're describing is different to the one this is
trying to fix. The right fix for your issue is to make
CONFIG_GENERIC_IRQ_IPI selected when CONFIG_MIPS_GIC && !CONFIG_MIPS_MT_SMP.
Would it be easy for you to create such a patch and test it?
Thanks,
Qais
On 04/02/2016 05:19 AM, Qais Yousef wrote:
> Hi Paul,
>
> On 01/04/2016 13:48, Paul Burton wrote:
>> On Thu, Mar 17, 2016 at 09:08:09PM +0000, Qais Yousef wrote:
>>> Malta defconfig compiles with GIC on. Hence when compiling for SMP it causes the
>>> new IPI code to be activated. But on qemu malta there's no GIC causing a
>>> BUG_ON(!ipidomain) to be hit in mips_smp_ipi_init().
>>>
>>> Since in that configuration one can only run a single core SMP (!), skip IPI
>>> initialisation if we detect that this is the case. It is a sensible behaviour
>>> to introduce and should keep such possible configuration to run rather than die
>>> hard unnecessarily.
>> Hi Qais/Ralf,
>>
>> This patch is insufficient I'm afraid. It's entirely possible to use SMP
>> with multiple VPEs in a single core on Malta boards that don't have a
>> GIC - we have code handling IPIs in that case guarded by #ifdef
>> CONFIG_MIPS_MT_SMP in arch/mips/mti-malta/malta-int.c. I think the
>> BUG_ON needs to be removed entirely, unless that single-core multi-VPE
>> IPI code is also converted to use an IPI irqdomain.
>>
>
> I was under the impression that SMP is only supported under GIC and older forms of SMP are deprecated.
>
> I think the problem you're describing is different to the one this is trying to fix. The right fix for your issue is to make CONFIG_GENERIC_IRQ_IPI selected when CONFIG_MIPS_GIC && !CONFIG_MIPS_MT_SMP.
>
Didn't Paul say that his system doesn't have a GIC ? Are you saying that he can not
(or no longer) run an image built for SMP on his system ?
Guenter
On Sat, Apr 02, 2016 at 01:19:03PM +0100, Qais Yousef wrote:
> Hi Paul,
>
> On 01/04/2016 13:48, Paul Burton wrote:
> >On Thu, Mar 17, 2016 at 09:08:09PM +0000, Qais Yousef wrote:
> >>Malta defconfig compiles with GIC on. Hence when compiling for SMP it causes the
> >>new IPI code to be activated. But on qemu malta there's no GIC causing a
> >>BUG_ON(!ipidomain) to be hit in mips_smp_ipi_init().
> >>
> >>Since in that configuration one can only run a single core SMP (!), skip IPI
> >>initialisation if we detect that this is the case. It is a sensible behaviour
> >>to introduce and should keep such possible configuration to run rather than die
> >>hard unnecessarily.
> >Hi Qais/Ralf,
> >
> >This patch is insufficient I'm afraid. It's entirely possible to use SMP
> >with multiple VPEs in a single core on Malta boards that don't have a
> >GIC - we have code handling IPIs in that case guarded by #ifdef
> >CONFIG_MIPS_MT_SMP in arch/mips/mti-malta/malta-int.c. I think the
> >BUG_ON needs to be removed entirely, unless that single-core multi-VPE
> >IPI code is also converted to use an IPI irqdomain.
> >
>
> I was under the impression that SMP is only supported under GIC and older
> forms of SMP are deprecated.
Hi Qais,
That's incorrect. We're talking about systems that don't have any GIC -
there are Malta configurations that don't. QEMU is the obvious one, and
you can break it (at least v2.5.0 which has no GIC) with this patch just
by using "-cpu 34Kf -smp 2" to show up 2 VPEs. I believe there are also
real bitfiles (though probably unused these days) that would have the
same problem.
> I think the problem you're describing is different to the one this is trying
> to fix. The right fix for your issue is to make CONFIG_GENERIC_IRQ_IPI
> selected when CONFIG_MIPS_GIC && !CONFIG_MIPS_MT_SMP.
>
> Would it be easy for you to create such a patch and test it?
That would be insufficient, since we can have a kernel that includes GIC
support & CONFIG_MIPS_MT_SMP (SMVP) support and not know whether
there'll actually be a GIC or multiple VPEs until runtime. So Kconfig
cannot solve this at compile time.
I believe that the best thing to do would be to convert the single-core
MT IPI code to use the IRQ domain stuff you added (which I don't see any
documentation for & am currently struggling to decipher). But given that
we're post-merge-window on the way to v4.6 I think the best we can do is
to just return when there's no IPI domain, rather than BUG_ON. I'll
submit a patch to do that later this morning.
Thanks,
Paul
Commit fbde2d7d8290 ("MIPS: Add generic SMP IPI support") introduced
code that BUG_ON's in the case of a kernel that supports IPI domains but
does not have one at runtime. This case is possible on Malta where for
IPIs we may use either the GIC (which has an IPI IRQ domain
implementation) or core-local software interrupts between VPEs (which do
not currently have an IPI IRQ domain implementation). We can not know
which will be used until runtime when we know whether a GIC is actually
present, and if we run on a system with multiple VPEs and no GIC then
the BUG_ON is hit.
A simple way to reproduce this bug is by using QEMU, which partially
implements the MT ASE but does not implement the GIC as of version 2.5.
Using "-cpu 34Kf -smp 2" will present a system with 2 VPEs in one core &
no GIC, hitting the BUG_ON.
Given that we're post-merge-window on the way to v4.6, avoid this by
just returning from mips_smp_ipi_init when no IPI IRQ domain is found.
Ideally at some point all IPI implementations would be converted to the
same system & we'd be able to restore the check.
Signed-off-by: Paul Burton <[email protected]>
Fixes: fbde2d7d8290 ("MIPS: Add generic SMP IPI support")
---
arch/mips/kernel/smp.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index 8b687fe..630bcfb 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -254,7 +254,17 @@ static int __init mips_smp_ipi_init(void)
if (node && !ipidomain)
ipidomain = irq_find_matching_host(NULL, DOMAIN_BUS_IPI);
- BUG_ON(!ipidomain);
+ /*
+ * There are systems which only use IPI domains some of the time,
+ * depending upon configuration we don't know until runtime. An
+ * example is Malta where we may compile in support for GIC & the
+ * MT ASE, but run on a system which has multiple VPEs in a single
+ * core and doesn't include a GIC. Until all IPI implementations
+ * have been converted to use IPI domains the best we can do here
+ * is to return & hope some other code sets up the IPIs.
+ */
+ if (!ipidomain)
+ return 0;
call_virq = irq_reserve_ipi(ipidomain, cpu_possible_mask);
BUG_ON(!call_virq);
--
2.8.0
On Mon, Apr 04, 2016 at 10:02:23AM +0200, Ralf Baechle wrote:
> FYI, Qais' initial fix is in the pull request I sent to Linus yesterday so
> any fixes please relative to that patch.
Hi Ralf,
To late - I already submitted:
https://patchwork.linux-mips.org/patch/13003/
I can rebase, but it'll be a revert of Qais' workaround followed by
mine & squashed.
Thanks,
Paul
On Mon, Apr 04, 2016 at 07:41:40AM +0100, Paul Burton wrote:
> > On 01/04/2016 13:48, Paul Burton wrote:
> > >On Thu, Mar 17, 2016 at 09:08:09PM +0000, Qais Yousef wrote:
> > >>Malta defconfig compiles with GIC on. Hence when compiling for SMP it causes the
> > >>new IPI code to be activated. But on qemu malta there's no GIC causing a
> > >>BUG_ON(!ipidomain) to be hit in mips_smp_ipi_init().
> > >>
> > >>Since in that configuration one can only run a single core SMP (!), skip IPI
> > >>initialisation if we detect that this is the case. It is a sensible behaviour
> > >>to introduce and should keep such possible configuration to run rather than die
> > >>hard unnecessarily.
> > >Hi Qais/Ralf,
> > >
> > >This patch is insufficient I'm afraid. It's entirely possible to use SMP
> > >with multiple VPEs in a single core on Malta boards that don't have a
> > >GIC - we have code handling IPIs in that case guarded by #ifdef
> > >CONFIG_MIPS_MT_SMP in arch/mips/mti-malta/malta-int.c. I think the
> > >BUG_ON needs to be removed entirely, unless that single-core multi-VPE
> > >IPI code is also converted to use an IPI irqdomain.
> > >
> >
> > I was under the impression that SMP is only supported under GIC and older
> > forms of SMP are deprecated.
>
> Hi Qais,
>
> That's incorrect. We're talking about systems that don't have any GIC -
> there are Malta configurations that don't. QEMU is the obvious one, and
> you can break it (at least v2.5.0 which has no GIC) with this patch just
> by using "-cpu 34Kf -smp 2" to show up 2 VPEs. I believe there are also
> real bitfiles (though probably unused these days) that would have the
> same problem.
>
> > I think the problem you're describing is different to the one this is trying
> > to fix. The right fix for your issue is to make CONFIG_GENERIC_IRQ_IPI
> > selected when CONFIG_MIPS_GIC && !CONFIG_MIPS_MT_SMP.
> >
> > Would it be easy for you to create such a patch and test it?
>
> That would be insufficient, since we can have a kernel that includes GIC
> support & CONFIG_MIPS_MT_SMP (SMVP) support and not know whether
> there'll actually be a GIC or multiple VPEs until runtime. So Kconfig
> cannot solve this at compile time.
>
> I believe that the best thing to do would be to convert the single-core
> MT IPI code to use the IRQ domain stuff you added (which I don't see any
> documentation for & am currently struggling to decipher). But given that
> we're post-merge-window on the way to v4.6 I think the best we can do is
> to just return when there's no IPI domain, rather than BUG_ON. I'll
> submit a patch to do that later this morning.
FYI, Qais' initial fix is in the pull request I sent to Linus yesterday so
any fixes please relative to that patch.
Ralf
On Mon, Apr 04, 2016 at 09:06:54AM +0100, Paul Burton wrote:
> On Mon, Apr 04, 2016 at 10:02:23AM +0200, Ralf Baechle wrote:
> > FYI, Qais' initial fix is in the pull request I sent to Linus yesterday so
> > any fixes please relative to that patch.
>
> Hi Ralf,
>
> To late - I already submitted:
>
> https://patchwork.linux-mips.org/patch/13003/
>
> I can rebase, but it'll be a revert of Qais' workaround followed by
> mine & squashed.
Yeah, rebase and something on top of that will be fine.
Ralf
On Mon, Apr 04, 2016 at 09:28:16AM +0100, Qais Yousef wrote:
> On 4 Apr 2016 09:06, "Paul Burton" <[email protected]> wrote:
> >
> > On Mon, Apr 04, 2016 at 10:02:23AM +0200, Ralf Baechle wrote:
> > > FYI, Qais' initial fix is in the pull request I sent to Linus yesterday
> so
> > > any fixes please relative to that patch.
> >
> > Hi Ralf,
> >
> > To late - I already submitted:
> >
> > https://patchwork.linux-mips.org/patch/13003/
> >
> > I can rebase, but it'll be a revert of Qais' workaround followed by
> > mine & squashed.
> >
> > Thanks,
> > Paul
>
> Removing BUG_ON () is the real workaround.
Hi Qais,
They're both workarounds. I think given the point in the cycle that
we're at now it's the best option for now.
> The problem with Malta is that it always selects GIC even when it does
> not have it.
No, that's by design. GIC support can be enabled in Kconfig & the actual
presence of a GIC is detected at runtime. A kernel with GIC support can
run on a system with or without a GIC.
> If you add 2 ipi domains then you need to add a way to tell the platform
> which one to use.
Then we may need some way to do that, or perhaps it'll be OK if we only
register one of them on any given system. As mentioned I haven't really
deciphered this IPI IRQ domain code yet.
> All of the patches I sent were sent for review and were around for a long
> time. There's no need for the passive aggressiveness please because you
> found a problem now. Everything went through the formal process and you and
> everyone had a chance to comment and raise issues out.
Passive agressiveness? Really?
I'm not complaining about your patches having being merged. I haven't
said you failed to follow any process. I didn't have time to review your
patches, and clearly nobody else spotted this either, and a regression
has snuck into mainline. I simply want to fix it. I would also have
liked to find some documentation since it's difficult to figure out how
to add support for the single-core multi-VPE software interrupt case,
but that's the only criticism I've made of the patches & I think it's
just. Please don't be so personal...
> My idea of a fix is to get config dependencies sorted but I'll leave it
> with you and Ralf.
As I tried to explain before and above, we don't have all of the
required information until runtime, so Kconfig cannot solve this. That
won't work.
Thanks,
Paul
Commit fbde2d7d8290 ("MIPS: Add generic SMP IPI support") introduced
code that BUG_ON's in the case of a kernel that supports IPI domains but
does not have one at runtime. This case is possible on Malta where for
IPIs we may use either the GIC (which has an IPI IRQ domain
implementation) or core-local software interrupts between VPEs (which do
not currently have an IPI IRQ domain implementation). We can not know
which will be used until runtime when we know whether a GIC is actually
present, and if we run on a system with multiple VPEs and no GIC then
the BUG_ON is hit.
Commit 19fb5818ed60 ("IPS: Fix broken malta qemu") worked around this
for the single-core single-VPE case typically seen using QEMU, but does
not catch the multi-VPE case. This patch removes the insufficient CPU
presence check that was added and works around the bug differently,
effectively reverting that commit.
A simple way to reproduce this bug is by using QEMU, which partially
implements the MT ASE but does not implement the GIC as of version 2.5.
Using "-cpu 34Kf -smp 2" will present a system with 2 VPEs in one core &
no GIC, hitting the BUG_ON.
Given that we're post-merge-window on the way to v4.6, avoid this by
just returning from mips_smp_ipi_init when no IPI IRQ domain is found.
Ideally at some point all IPI implementations would be converted to the
same IPI IRQ domain interface & we'd be able to restore the check.
Signed-off-by: Paul Burton <[email protected]>
Cc: Qais Yousef <[email protected]>
Fixes: fbde2d7d8290 ("MIPS: Add generic SMP IPI support")
Fixes: 19fb5818ed60 ("IPS: Fix broken malta qemu")
Reverts: 19fb5818ed60 ("IPS: Fix broken malta qemu")
---
Changes in v2:
- Rebase this workaround atop Qais' workaround at Ralf's request.
- Update the commit message to reflect that.
arch/mips/kernel/smp.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index c87c0da..630bcfb 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -243,18 +243,6 @@ static int __init mips_smp_ipi_init(void)
struct irq_domain *ipidomain;
struct device_node *node;
- /*
- * In some cases like qemu-malta, it is desired to try SMP with
- * a single core. Qemu-malta has no GIC, so an attempt to set any IPIs
- * would cause a BUG_ON() to be triggered since there's no ipidomain.
- *
- * Since for a single core system IPIs aren't required really, skip the
- * initialisation which should generally keep any such configurations
- * happy and only fail hard when trying to truely run SMP.
- */
- if (cpumask_weight(cpu_possible_mask) == 1)
- return 0;
-
node = of_irq_find_parent(of_root);
ipidomain = irq_find_matching_host(node, DOMAIN_BUS_IPI);
@@ -266,7 +254,17 @@ static int __init mips_smp_ipi_init(void)
if (node && !ipidomain)
ipidomain = irq_find_matching_host(NULL, DOMAIN_BUS_IPI);
- BUG_ON(!ipidomain);
+ /*
+ * There are systems which only use IPI domains some of the time,
+ * depending upon configuration we don't know until runtime. An
+ * example is Malta where we may compile in support for GIC & the
+ * MT ASE, but run on a system which has multiple VPEs in a single
+ * core and doesn't include a GIC. Until all IPI implementations
+ * have been converted to use IPI domains the best we can do here
+ * is to return & hope some other code sets up the IPIs.
+ */
+ if (!ipidomain)
+ return 0;
call_virq = irq_reserve_ipi(ipidomain, cpu_possible_mask);
BUG_ON(!call_virq);
--
2.8.0