Re: [Kgdb-bugreport] [PATCH v12 1/7] irqchip/gic-v3: Enable support for SGIs to act as NMIs
On Thu, Aug 31, 2023 at 08:31:37AM -0700, Doug Anderson wrote: > Hi, > > On Thu, Aug 31, 2023 at 1:53 AM Mark Rutland wrote: > > > > On Wed, Aug 30, 2023 at 12:11:22PM -0700, Douglas Anderson wrote: > > > As of commit 6abbd6988971 ("irqchip/gic, gic-v3: Make SGIs use > > > handle_percpu_devid_irq()") SGIs are treated the same as PPIs/EPPIs > > > and use handle_percpu_devid_irq() by default. Unfortunately, > > > handle_percpu_devid_irq() isn't NMI safe, and so to run in an NMI > > > context those should use handle_percpu_devid_fasteoi_nmi(). > > > > > > In order to accomplish this, we just have to make room for SGIs in the > > > array of refcounts that keeps track of which interrupts are set as > > > NMI. We also rename the array and create a new indexing scheme that > > > accounts for SGIs. > > > > > > Also, enable NMI support prior to gic_smp_init() as allocation of SGIs > > > as IRQs/NMIs happen as part of this routine. > > > > > > Co-developed-by: Sumit Garg > > > Signed-off-by: Sumit Garg > > > Signed-off-by: Douglas Anderson > > > --- > > > I'll note that this change is a little more black magic to me than > > > others in this series. I don't have a massive amounts of familiarity > > > with all the moving parts of gic-v3, so I mostly just followed Mark > > > Rutland's advice [1]. Please pay extra attention to make sure I didn't > > > do anything too terrible. > > > > > > Mark's advice wasn't a full patch and I ended up doing a bit of work > > > to translate it to reality, so I did not add him as "Co-developed-by" > > > here. Mark: if you would like this tag then please provide it and your > > > Signed-off-by. I certainly won't object. > > > > That's all reasonable, and I'm perfectly happy without a tag. > > > > I have one trivial nit below, but with or without that fixed up: > > > > Acked-by: Mark Rutland > > > > > > > > [1] > > > https://lore.kernel.org/r/znc-yrqopo0pa...@fvff77s0q05n.cambridge.arm.com > > > > > > Changes in v12: > > > - Added a comment about why we account for 16 SGIs when Linux uses 8. > > > > > > Changes in v10: > > > - Rewrite as needed for 5.11+ as per Mark Rutland and Sumit. > > > > > > drivers/irqchip/irq-gic-v3.c | 59 +--- > > > 1 file changed, 41 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > > index eedfa8e9f077..8d20122ba0a8 100644 > > > --- a/drivers/irqchip/irq-gic-v3.c > > > +++ b/drivers/irqchip/irq-gic-v3.c > > > @@ -78,6 +78,13 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key); > > > #define GIC_LINE_NR min(GICD_TYPER_SPIS(gic_data.rdists.gicd_typer), > > > 1020U) > > > #define GIC_ESPI_NR GICD_TYPER_ESPIS(gic_data.rdists.gicd_typer) > > > > > > +/* > > > + * There are 16 SGIs, though we only actually use 8 in Linux. The other > > > 8 SGIs > > > + * are potentially stolen by the secure side. Some code, especially code > > > dealing > > > + * with hwirq IDs, is simplified by accounting for all 16. > > > + */ > > > +#define SGI_NR 16 > > > + > > > /* > > > * The behaviours of RPR and PMR registers differ depending on the value > > > of > > > * SCR_EL3.FIQ, and the behaviour of non-secure priority registers of the > > > @@ -125,8 +132,8 @@ EXPORT_SYMBOL(gic_nonsecure_priorities); > > > __priority; \ > > > }) > > > > > > -/* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */ > > > -static refcount_t *ppi_nmi_refs; > > > +/* rdist_nmi_refs[n] == number of cpus having the rdist interrupt n set > > > as NMI */ > > > +static refcount_t *rdist_nmi_refs; > > > > > > static struct gic_kvm_info gic_v3_kvm_info __initdata; > > > static DEFINE_PER_CPU(bool, has_rss); > > > @@ -519,9 +526,22 @@ static u32 __gic_get_ppi_index(irq_hw_number_t hwirq) > > > } > > > } > > > > > > -static u32 gic_get_ppi_index(struct irq_data *d) > > > +static u32 __gic_get_rdist_idx(irq_hw_number_t hwirq) > > > +{ > > > + switch (__get_intid_range(hwirq)) { > > > + case SGI_RANGE: > > > + case PPI_RANGE: > > > + return hwirq; > > > + case EPPI_RANGE: > > > + return hwirq - EPPI_BASE_INTID + 32; > > > + default: > > > + unreachable(); > > > + } > > > +} > > > + > > > +static u32 gic_get_rdist_idx(struct irq_data *d) > > > { > > > - return __gic_get_ppi_index(d->hwirq); > > > + return __gic_get_rdist_idx(d->hwirq); > > > } > > > > Nit: It would be nicer to call this gic_get_rdist_index() to match > > gic_get_ppi_index(); likewise with __gic_get_rdist_index(). > > > > That's my fault given I suggested the gic_get_rdist_idx() name in: > > > > > > https://lore.kernel.org/linux-arm-kernel/znc-yrqopo0pa...@fvff77s0q05n.cambridge.arm.com/ > > > > ... so sorry about that! > > Yeah, I kept the name you suggested even though it seemed a little > inconsistent. I'll happily send a v13 with that fixed up,
Re: [Kgdb-bugreport] [PATCH v12 1/7] irqchip/gic-v3: Enable support for SGIs to act as NMIs
Hi, On Thu, Aug 31, 2023 at 1:53 AM Mark Rutland wrote: > > On Wed, Aug 30, 2023 at 12:11:22PM -0700, Douglas Anderson wrote: > > As of commit 6abbd6988971 ("irqchip/gic, gic-v3: Make SGIs use > > handle_percpu_devid_irq()") SGIs are treated the same as PPIs/EPPIs > > and use handle_percpu_devid_irq() by default. Unfortunately, > > handle_percpu_devid_irq() isn't NMI safe, and so to run in an NMI > > context those should use handle_percpu_devid_fasteoi_nmi(). > > > > In order to accomplish this, we just have to make room for SGIs in the > > array of refcounts that keeps track of which interrupts are set as > > NMI. We also rename the array and create a new indexing scheme that > > accounts for SGIs. > > > > Also, enable NMI support prior to gic_smp_init() as allocation of SGIs > > as IRQs/NMIs happen as part of this routine. > > > > Co-developed-by: Sumit Garg > > Signed-off-by: Sumit Garg > > Signed-off-by: Douglas Anderson > > --- > > I'll note that this change is a little more black magic to me than > > others in this series. I don't have a massive amounts of familiarity > > with all the moving parts of gic-v3, so I mostly just followed Mark > > Rutland's advice [1]. Please pay extra attention to make sure I didn't > > do anything too terrible. > > > > Mark's advice wasn't a full patch and I ended up doing a bit of work > > to translate it to reality, so I did not add him as "Co-developed-by" > > here. Mark: if you would like this tag then please provide it and your > > Signed-off-by. I certainly won't object. > > That's all reasonable, and I'm perfectly happy without a tag. > > I have one trivial nit below, but with or without that fixed up: > > Acked-by: Mark Rutland > > > > > [1] > > https://lore.kernel.org/r/znc-yrqopo0pa...@fvff77s0q05n.cambridge.arm.com > > > > Changes in v12: > > - Added a comment about why we account for 16 SGIs when Linux uses 8. > > > > Changes in v10: > > - Rewrite as needed for 5.11+ as per Mark Rutland and Sumit. > > > > drivers/irqchip/irq-gic-v3.c | 59 +--- > > 1 file changed, 41 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > index eedfa8e9f077..8d20122ba0a8 100644 > > --- a/drivers/irqchip/irq-gic-v3.c > > +++ b/drivers/irqchip/irq-gic-v3.c > > @@ -78,6 +78,13 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key); > > #define GIC_LINE_NR min(GICD_TYPER_SPIS(gic_data.rdists.gicd_typer), > > 1020U) > > #define GIC_ESPI_NR GICD_TYPER_ESPIS(gic_data.rdists.gicd_typer) > > > > +/* > > + * There are 16 SGIs, though we only actually use 8 in Linux. The other 8 > > SGIs > > + * are potentially stolen by the secure side. Some code, especially code > > dealing > > + * with hwirq IDs, is simplified by accounting for all 16. > > + */ > > +#define SGI_NR 16 > > + > > /* > > * The behaviours of RPR and PMR registers differ depending on the value of > > * SCR_EL3.FIQ, and the behaviour of non-secure priority registers of the > > @@ -125,8 +132,8 @@ EXPORT_SYMBOL(gic_nonsecure_priorities); > > __priority; \ > > }) > > > > -/* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */ > > -static refcount_t *ppi_nmi_refs; > > +/* rdist_nmi_refs[n] == number of cpus having the rdist interrupt n set as > > NMI */ > > +static refcount_t *rdist_nmi_refs; > > > > static struct gic_kvm_info gic_v3_kvm_info __initdata; > > static DEFINE_PER_CPU(bool, has_rss); > > @@ -519,9 +526,22 @@ static u32 __gic_get_ppi_index(irq_hw_number_t hwirq) > > } > > } > > > > -static u32 gic_get_ppi_index(struct irq_data *d) > > +static u32 __gic_get_rdist_idx(irq_hw_number_t hwirq) > > +{ > > + switch (__get_intid_range(hwirq)) { > > + case SGI_RANGE: > > + case PPI_RANGE: > > + return hwirq; > > + case EPPI_RANGE: > > + return hwirq - EPPI_BASE_INTID + 32; > > + default: > > + unreachable(); > > + } > > +} > > + > > +static u32 gic_get_rdist_idx(struct irq_data *d) > > { > > - return __gic_get_ppi_index(d->hwirq); > > + return __gic_get_rdist_idx(d->hwirq); > > } > > Nit: It would be nicer to call this gic_get_rdist_index() to match > gic_get_ppi_index(); likewise with __gic_get_rdist_index(). > > That's my fault given I suggested the gic_get_rdist_idx() name in: > > > https://lore.kernel.org/linux-arm-kernel/znc-yrqopo0pa...@fvff77s0q05n.cambridge.arm.com/ > > ... so sorry about that! Yeah, I kept the name you suggested even though it seemed a little inconsistent. I'll happily send a v13 with that fixed up, though I'll probably wait a little bit just to avoid spamming new versions too quickly. It's not like the patches can land in the middle of the merge window anyway. Unless someone says otherwise, I guess this series is in good shape to land then. Does anyone have any plans for the details of how to land it? I guess
Re: [Kgdb-bugreport] [PATCH v12 1/7] irqchip/gic-v3: Enable support for SGIs to act as NMIs
On Wed, Aug 30, 2023 at 12:11:22PM -0700, Douglas Anderson wrote: > As of commit 6abbd6988971 ("irqchip/gic, gic-v3: Make SGIs use > handle_percpu_devid_irq()") SGIs are treated the same as PPIs/EPPIs > and use handle_percpu_devid_irq() by default. Unfortunately, > handle_percpu_devid_irq() isn't NMI safe, and so to run in an NMI > context those should use handle_percpu_devid_fasteoi_nmi(). > > In order to accomplish this, we just have to make room for SGIs in the > array of refcounts that keeps track of which interrupts are set as > NMI. We also rename the array and create a new indexing scheme that > accounts for SGIs. > > Also, enable NMI support prior to gic_smp_init() as allocation of SGIs > as IRQs/NMIs happen as part of this routine. > > Co-developed-by: Sumit Garg > Signed-off-by: Sumit Garg > Signed-off-by: Douglas Anderson > --- > I'll note that this change is a little more black magic to me than > others in this series. I don't have a massive amounts of familiarity > with all the moving parts of gic-v3, so I mostly just followed Mark > Rutland's advice [1]. Please pay extra attention to make sure I didn't > do anything too terrible. > > Mark's advice wasn't a full patch and I ended up doing a bit of work > to translate it to reality, so I did not add him as "Co-developed-by" > here. Mark: if you would like this tag then please provide it and your > Signed-off-by. I certainly won't object. That's all reasonable, and I'm perfectly happy without a tag. I have one trivial nit below, but with or without that fixed up: Acked-by: Mark Rutland > > [1] https://lore.kernel.org/r/znc-yrqopo0pa...@fvff77s0q05n.cambridge.arm.com > > Changes in v12: > - Added a comment about why we account for 16 SGIs when Linux uses 8. > > Changes in v10: > - Rewrite as needed for 5.11+ as per Mark Rutland and Sumit. > > drivers/irqchip/irq-gic-v3.c | 59 +--- > 1 file changed, 41 insertions(+), 18 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index eedfa8e9f077..8d20122ba0a8 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -78,6 +78,13 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key); > #define GIC_LINE_NR min(GICD_TYPER_SPIS(gic_data.rdists.gicd_typer), 1020U) > #define GIC_ESPI_NR GICD_TYPER_ESPIS(gic_data.rdists.gicd_typer) > > +/* > + * There are 16 SGIs, though we only actually use 8 in Linux. The other 8 > SGIs > + * are potentially stolen by the secure side. Some code, especially code > dealing > + * with hwirq IDs, is simplified by accounting for all 16. > + */ > +#define SGI_NR 16 > + > /* > * The behaviours of RPR and PMR registers differ depending on the value of > * SCR_EL3.FIQ, and the behaviour of non-secure priority registers of the > @@ -125,8 +132,8 @@ EXPORT_SYMBOL(gic_nonsecure_priorities); > __priority; \ > }) > > -/* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */ > -static refcount_t *ppi_nmi_refs; > +/* rdist_nmi_refs[n] == number of cpus having the rdist interrupt n set as > NMI */ > +static refcount_t *rdist_nmi_refs; > > static struct gic_kvm_info gic_v3_kvm_info __initdata; > static DEFINE_PER_CPU(bool, has_rss); > @@ -519,9 +526,22 @@ static u32 __gic_get_ppi_index(irq_hw_number_t hwirq) > } > } > > -static u32 gic_get_ppi_index(struct irq_data *d) > +static u32 __gic_get_rdist_idx(irq_hw_number_t hwirq) > +{ > + switch (__get_intid_range(hwirq)) { > + case SGI_RANGE: > + case PPI_RANGE: > + return hwirq; > + case EPPI_RANGE: > + return hwirq - EPPI_BASE_INTID + 32; > + default: > + unreachable(); > + } > +} > + > +static u32 gic_get_rdist_idx(struct irq_data *d) > { > - return __gic_get_ppi_index(d->hwirq); > + return __gic_get_rdist_idx(d->hwirq); > } Nit: It would be nicer to call this gic_get_rdist_index() to match gic_get_ppi_index(); likewise with __gic_get_rdist_index(). That's my fault given I suggested the gic_get_rdist_idx() name in: https://lore.kernel.org/linux-arm-kernel/znc-yrqopo0pa...@fvff77s0q05n.cambridge.arm.com/ ... so sorry about that! Mark. > > static int gic_irq_nmi_setup(struct irq_data *d) > @@ -545,11 +565,14 @@ static int gic_irq_nmi_setup(struct irq_data *d) > > /* desc lock should already be held */ > if (gic_irq_in_rdist(d)) { > - u32 idx = gic_get_ppi_index(d); > + u32 idx = gic_get_rdist_idx(d); > > - /* Setting up PPI as NMI, only switch handler for first NMI */ > - if (!refcount_inc_not_zero(_nmi_refs[idx])) { > - refcount_set(_nmi_refs[idx], 1); > + /* > + * Setting up a percpu interrupt as NMI, only switch handler > + * for first NMI > + */ > + if