| Message ID | 1607419304-26140-1-git-send-email-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/irq: report bug if NR_IPI greater than max SGI during compile time | expand |
On 2020-12-08 09:21, Pingfan Liu wrote: > Although there is a runtime WARN_ON() when NR_IPR > max SGI, it had > better > do the check during built time, and associate these related code > together. > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > To: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > --- > arch/arm64/kernel/smp.c | 2 ++ > drivers/irqchip/irq-gic-v3.c | 2 +- > drivers/irqchip/irq-gic.c | 2 +- > include/linux/irqchip/arm-gic-common.h | 2 ++ > 4 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index 18e9727..9fc383c 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -33,6 +33,7 @@ > #include <linux/kernel_stat.h> > #include <linux/kexec.h> > #include <linux/kvm_host.h> > +#include <linux/irqchip/arm-gic-common.h> > > #include <asm/alternative.h> > #include <asm/atomic.h> > @@ -76,6 +77,7 @@ enum ipi_msg_type { > IPI_WAKEUP, > NR_IPI > }; > +static_assert(NR_IPI <= MAX_SGI_NUM); I am trying *very hard* to remove dependencies between the architecture code and random drivers, so this kind of check really is counter-productive. Driver code should not have to know the number of IPIs, because there is no requirement that all IPIs should map 1:1 to SGIs. Conflating the two is already wrong, and I really don't want to add more of that. Thanks, M.
On Tue, Dec 8, 2020 at 5:31 PM Marc Zyngier <maz@kernel.org> wrote: > > On 2020-12-08 09:21, Pingfan Liu wrote: > > Although there is a runtime WARN_ON() when NR_IPR > max SGI, it had > > better > > do the check during built time, and associate these related code > > together. > > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Jason Cooper <jason@lakedaemon.net> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > To: linux-arm-kernel@lists.infradead.org > > Cc: linux-kernel@vger.kernel.org > > --- > > arch/arm64/kernel/smp.c | 2 ++ > > drivers/irqchip/irq-gic-v3.c | 2 +- > > drivers/irqchip/irq-gic.c | 2 +- > > include/linux/irqchip/arm-gic-common.h | 2 ++ > > 4 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > > index 18e9727..9fc383c 100644 > > --- a/arch/arm64/kernel/smp.c > > +++ b/arch/arm64/kernel/smp.c > > @@ -33,6 +33,7 @@ > > #include <linux/kernel_stat.h> > > #include <linux/kexec.h> > > #include <linux/kvm_host.h> > > +#include <linux/irqchip/arm-gic-common.h> > > > > #include <asm/alternative.h> > > #include <asm/atomic.h> > > @@ -76,6 +77,7 @@ enum ipi_msg_type { > > IPI_WAKEUP, > > NR_IPI > > }; > > +static_assert(NR_IPI <= MAX_SGI_NUM); > > I am trying *very hard* to remove dependencies between the architecture > code and random drivers, so this kind of check really is > counter-productive. > > Driver code should not have to know the number of IPIs, because there is > no requirement that all IPIs should map 1:1 to SGIs. Conflating the two Just curious about this. Is there an IPI which is not implemented by SGI? Or mapping several IPIs to a single SGI, and scatter out due to a global variable value? Thanks, Pingfan > is already wrong, and I really don't want to add more of that. > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
On 2020-12-08 09:43, Pingfan Liu wrote: > On Tue, Dec 8, 2020 at 5:31 PM Marc Zyngier <maz@kernel.org> wrote: >> >> On 2020-12-08 09:21, Pingfan Liu wrote: >> > Although there is a runtime WARN_ON() when NR_IPR > max SGI, it had >> > better >> > do the check during built time, and associate these related code >> > together. >> > >> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> >> > Cc: Catalin Marinas <catalin.marinas@arm.com> >> > Cc: Will Deacon <will@kernel.org> >> > Cc: Thomas Gleixner <tglx@linutronix.de> >> > Cc: Jason Cooper <jason@lakedaemon.net> >> > Cc: Marc Zyngier <maz@kernel.org> >> > Cc: Mark Rutland <mark.rutland@arm.com> >> > To: linux-arm-kernel@lists.infradead.org >> > Cc: linux-kernel@vger.kernel.org >> > --- >> > arch/arm64/kernel/smp.c | 2 ++ >> > drivers/irqchip/irq-gic-v3.c | 2 +- >> > drivers/irqchip/irq-gic.c | 2 +- >> > include/linux/irqchip/arm-gic-common.h | 2 ++ >> > 4 files changed, 6 insertions(+), 2 deletions(-) >> > >> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c >> > index 18e9727..9fc383c 100644 >> > --- a/arch/arm64/kernel/smp.c >> > +++ b/arch/arm64/kernel/smp.c >> > @@ -33,6 +33,7 @@ >> > #include <linux/kernel_stat.h> >> > #include <linux/kexec.h> >> > #include <linux/kvm_host.h> >> > +#include <linux/irqchip/arm-gic-common.h> >> > >> > #include <asm/alternative.h> >> > #include <asm/atomic.h> >> > @@ -76,6 +77,7 @@ enum ipi_msg_type { >> > IPI_WAKEUP, >> > NR_IPI >> > }; >> > +static_assert(NR_IPI <= MAX_SGI_NUM); >> >> I am trying *very hard* to remove dependencies between the >> architecture >> code and random drivers, so this kind of check really is >> counter-productive. >> >> Driver code should not have to know the number of IPIs, because there >> is >> no requirement that all IPIs should map 1:1 to SGIs. Conflating the >> two > > Just curious about this. Is there an IPI which is not implemented by > SGI? Or mapping several IPIs to a single SGI, and scatter out due to a > global variable value? We currently have a single NS SGI left, and I'd like to move some of the non-critical IPIs over to dispatching mechanism (the two "CPU stop" IPIs definitely are candidate for merging). That's not implemented yet, but I don't see a need to add checks that would otherwise violate this IPI/SGI distinction. Thanks, M.
On Tue, Dec 8, 2020 at 5:51 PM Marc Zyngier <maz@kernel.org> wrote: > > On 2020-12-08 09:43, Pingfan Liu wrote: > > On Tue, Dec 8, 2020 at 5:31 PM Marc Zyngier <maz@kernel.org> wrote: > >> > >> On 2020-12-08 09:21, Pingfan Liu wrote: > >> > Although there is a runtime WARN_ON() when NR_IPR > max SGI, it had > >> > better > >> > do the check during built time, and associate these related code > >> > together. > >> > > >> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > >> > Cc: Catalin Marinas <catalin.marinas@arm.com> > >> > Cc: Will Deacon <will@kernel.org> > >> > Cc: Thomas Gleixner <tglx@linutronix.de> > >> > Cc: Jason Cooper <jason@lakedaemon.net> > >> > Cc: Marc Zyngier <maz@kernel.org> > >> > Cc: Mark Rutland <mark.rutland@arm.com> > >> > To: linux-arm-kernel@lists.infradead.org > >> > Cc: linux-kernel@vger.kernel.org > >> > --- > >> > arch/arm64/kernel/smp.c | 2 ++ > >> > drivers/irqchip/irq-gic-v3.c | 2 +- > >> > drivers/irqchip/irq-gic.c | 2 +- > >> > include/linux/irqchip/arm-gic-common.h | 2 ++ > >> > 4 files changed, 6 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > >> > index 18e9727..9fc383c 100644 > >> > --- a/arch/arm64/kernel/smp.c > >> > +++ b/arch/arm64/kernel/smp.c > >> > @@ -33,6 +33,7 @@ > >> > #include <linux/kernel_stat.h> > >> > #include <linux/kexec.h> > >> > #include <linux/kvm_host.h> > >> > +#include <linux/irqchip/arm-gic-common.h> > >> > > >> > #include <asm/alternative.h> > >> > #include <asm/atomic.h> > >> > @@ -76,6 +77,7 @@ enum ipi_msg_type { > >> > IPI_WAKEUP, > >> > NR_IPI > >> > }; > >> > +static_assert(NR_IPI <= MAX_SGI_NUM); > >> > >> I am trying *very hard* to remove dependencies between the > >> architecture > >> code and random drivers, so this kind of check really is > >> counter-productive. > >> > >> Driver code should not have to know the number of IPIs, because there > >> is > >> no requirement that all IPIs should map 1:1 to SGIs. Conflating the > >> two > > > > Just curious about this. Is there an IPI which is not implemented by > > SGI? Or mapping several IPIs to a single SGI, and scatter out due to a > > global variable value? > > We currently have a single NS SGI left, and I'd like to move some of the > non-critical IPIs over to dispatching mechanism (the two "CPU stop" IPIs > definitely are candidate for merging). That's not implemented yet, but > I don't see a need to add checks that would otherwise violate this > IPI/SGI distinction. Got it. Thanks for your detailed explanation. Regards, Pingfan
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 18e9727..9fc383c 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -33,6 +33,7 @@ #include <linux/kernel_stat.h> #include <linux/kexec.h> #include <linux/kvm_host.h> +#include <linux/irqchip/arm-gic-common.h> #include <asm/alternative.h> #include <asm/atomic.h> @@ -76,6 +77,7 @@ enum ipi_msg_type { IPI_WAKEUP, NR_IPI }; +static_assert(NR_IPI <= MAX_SGI_NUM); static int ipi_irq_base __read_mostly; static int nr_ipi __read_mostly = NR_IPI; diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 16fecc0..ee13f85 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -1162,7 +1162,7 @@ static void __init gic_smp_init(void) gic_starting_cpu, NULL); /* Register all 8 non-secure SGIs */ - base_sgi = __irq_domain_alloc_irqs(gic_data.domain, -1, 8, + base_sgi = __irq_domain_alloc_irqs(gic_data.domain, -1, MAX_SGI_NUM, NUMA_NO_NODE, &sgi_fwspec, false, NULL); if (WARN_ON(base_sgi <= 0)) diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 6053245..07d36de 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -845,7 +845,7 @@ static __init void gic_smp_init(void) "irqchip/arm/gic:starting", gic_starting_cpu, NULL); - base_sgi = __irq_domain_alloc_irqs(gic_data[0].domain, -1, 8, + base_sgi = __irq_domain_alloc_irqs(gic_data[0].domain, -1, MAX_SGI_NUM, NUMA_NO_NODE, &sgi_fwspec, false, NULL); if (WARN_ON(base_sgi <= 0)) diff --git a/include/linux/irqchip/arm-gic-common.h b/include/linux/irqchip/arm-gic-common.h index fa8c045..7e45a9f 100644 --- a/include/linux/irqchip/arm-gic-common.h +++ b/include/linux/irqchip/arm-gic-common.h @@ -16,6 +16,8 @@ (GICD_INT_DEF_PRI << 8) |\ GICD_INT_DEF_PRI) +#define MAX_SGI_NUM 8 + enum gic_type { GIC_V2, GIC_V3,
Although there is a runtime WARN_ON() when NR_IPR > max SGI, it had better do the check during built time, and associate these related code together. Signed-off-by: Pingfan Liu <kernelfans@gmail.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Marc Zyngier <maz@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> To: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- arch/arm64/kernel/smp.c | 2 ++ drivers/irqchip/irq-gic-v3.c | 2 +- drivers/irqchip/irq-gic.c | 2 +- include/linux/irqchip/arm-gic-common.h | 2 ++ 4 files changed, 6 insertions(+), 2 deletions(-)