| Message ID | 20240529172116.1313498-1-mark.rutland@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | irqchip/gic-v3: use compiletime constant PMR values | expand |
On Wed, 29 May 2024 18:21:12 +0100, Mark Rutland <mark.rutland@arm.com> wrote: > > The GIC distributor and PMR/RPR can present different views of the > interrupt priority space dependent upon the values of GICD_CTLR.DS and > SCR_EL3.FIQ. Currently we treat the distributor's view of the priority > space as canonical, and when the two differ we change the way we handle > values in the PMR/RPR, using the `gic_nonsecure_priorities` static key > to decide what to do. > > This approach works, but it's sub-optimal. When using pseudo-NMI we > manipulate the distributor rarely, and we manipulate the PMR/RPR > registers very frequently in code spread out throughout the kernel (e.g. > local_irq_{save,restore}()). It would be nicer if we could use fixed > values for the PMR/RPR, and dynamically choose the values programmed > into the distributor. > > This series reworks the GIC code (and a small part of arm64 architecture > code) to allow the use of compiletime-constant PMR values. This > simplifies the logic for PMR management, and when using pseudo-NMI this > results in smaller and better generates code for saving/restoring the > irqflags, saving ~4K of text for defconfig + CONFIG_PSEUDO_NMI=y. > > The first patch is a preparatory cleanup which I think makes sense > regardlesss of the rest of the series. The second and third patches > rework the GICv3 code to be able to choose priorities at boot time, and > the final patch makes the actual switch. > > I've given this some light testing atop v6.10-rc1 with pseudo-NMI > enabled (with priority debugging), along with lockdep on the following > systems: > > * M1SDP Morello board, bare metal > Where GICD_CTRL.DS=0, SCR_EL3.FIQ=0 > Using shifted (NS) values in the distributor > > * M1SDP Morello board, KVM guest > Where GICD_CTRL.DS=1, SCR_EL3.FIQ=0 > Using unshifted values in the distributor > > * ThunderX2, KVM guest > Where GICD_CTRL.DS=1, SCR_EL3.FIQ=0 > Using unshifted values in the distributor > > On ThunderX2 bare-metal there is an existing boot-time hang when using > pseudo-NMI which is not solved by this series. With this seires applied, > the logging added in patch 3 reports that GICD_CTRL.DS=1, SCR_EL3.FIQ=0, > and so this should be using the same priorities which are seem to work > in a guest. It is pretty odd that bare metal reports DS=1+FIQ=0, as if it didn't have a secure side (and I'm pretty sure TX2 does). I wonder if that's only the distributor lying about security being disabled. Note that a another issue exists with the original ThunderX, where while the HW reports: GICv3: GICD_CTRL.DS=0, SCR_EL3.FIQ=1 the machine locks up with RCU stalls. Similarly, this is independent of this series being applied or not. But ThunderX has so many bugs and warts that I'm definitely not losing any sleep over it. However, it works well on my Synquacer, which is reassuring. > > Mark. > > Mark Rutland (4): > irqchip/gic-common: remove sync_access callback > irqchip/gic-v3: make distributor priorities variables > irqchip/gic-v3: detect GICD_CTRL.DS and SCR_EL3.FIQ earlier > irqchip/gic-v3: select priorities at boot time > > arch/arm64/include/asm/arch_gicv3.h | 15 -- > arch/arm64/include/asm/ptrace.h | 35 +--- > arch/arm64/kernel/image-vars.h | 5 - > drivers/irqchip/irq-gic-common.c | 22 +-- > drivers/irqchip/irq-gic-common.h | 7 +- > drivers/irqchip/irq-gic-v3-its.c | 11 +- > drivers/irqchip/irq-gic-v3.c | 225 ++++++++++++------------ > drivers/irqchip/irq-gic.c | 10 +- > drivers/irqchip/irq-hip04.c | 6 +- > include/linux/irqchip/arm-gic-common.h | 4 - > include/linux/irqchip/arm-gic-v3-prio.h | 52 ++++++ > include/linux/irqchip/arm-gic-v3.h | 2 +- > 12 files changed, 193 insertions(+), 201 deletions(-) > create mode 100644 include/linux/irqchip/arm-gic-v3-prio.h For the series: Reviewed-by: Marc Zyngier <maz@kernel.org> Tested-by: Marc Zyngier <maz@kernel.org> Given that this touches some pretty deep aspects of the arm64 code, I think this should go via the arm64 tree, but I'll let you work that one out (you may want to Cc LKML and tglx though). Thanks, M.
The GIC distributor and PMR/RPR can present different views of the interrupt priority space dependent upon the values of GICD_CTLR.DS and SCR_EL3.FIQ. Currently we treat the distributor's view of the priority space as canonical, and when the two differ we change the way we handle values in the PMR/RPR, using the `gic_nonsecure_priorities` static key to decide what to do. This approach works, but it's sub-optimal. When using pseudo-NMI we manipulate the distributor rarely, and we manipulate the PMR/RPR registers very frequently in code spread out throughout the kernel (e.g. local_irq_{save,restore}()). It would be nicer if we could use fixed values for the PMR/RPR, and dynamically choose the values programmed into the distributor. This series reworks the GIC code (and a small part of arm64 architecture code) to allow the use of compiletime-constant PMR values. This simplifies the logic for PMR management, and when using pseudo-NMI this results in smaller and better generates code for saving/restoring the irqflags, saving ~4K of text for defconfig + CONFIG_PSEUDO_NMI=y. The first patch is a preparatory cleanup which I think makes sense regardlesss of the rest of the series. The second and third patches rework the GICv3 code to be able to choose priorities at boot time, and the final patch makes the actual switch. I've given this some light testing atop v6.10-rc1 with pseudo-NMI enabled (with priority debugging), along with lockdep on the following systems: * M1SDP Morello board, bare metal Where GICD_CTRL.DS=0, SCR_EL3.FIQ=0 Using shifted (NS) values in the distributor * M1SDP Morello board, KVM guest Where GICD_CTRL.DS=1, SCR_EL3.FIQ=0 Using unshifted values in the distributor * ThunderX2, KVM guest Where GICD_CTRL.DS=1, SCR_EL3.FIQ=0 Using unshifted values in the distributor On ThunderX2 bare-metal there is an existing boot-time hang when using pseudo-NMI which is not solved by this series. With this seires applied, the logging added in patch 3 reports that GICD_CTRL.DS=1, SCR_EL3.FIQ=0, and so this should be using the same priorities which are seem to work in a guest. Mark. Mark Rutland (4): irqchip/gic-common: remove sync_access callback irqchip/gic-v3: make distributor priorities variables irqchip/gic-v3: detect GICD_CTRL.DS and SCR_EL3.FIQ earlier irqchip/gic-v3: select priorities at boot time arch/arm64/include/asm/arch_gicv3.h | 15 -- arch/arm64/include/asm/ptrace.h | 35 +--- arch/arm64/kernel/image-vars.h | 5 - drivers/irqchip/irq-gic-common.c | 22 +-- drivers/irqchip/irq-gic-common.h | 7 +- drivers/irqchip/irq-gic-v3-its.c | 11 +- drivers/irqchip/irq-gic-v3.c | 225 ++++++++++++------------ drivers/irqchip/irq-gic.c | 10 +- drivers/irqchip/irq-hip04.c | 6 +- include/linux/irqchip/arm-gic-common.h | 4 - include/linux/irqchip/arm-gic-v3-prio.h | 52 ++++++ include/linux/irqchip/arm-gic-v3.h | 2 +- 12 files changed, 193 insertions(+), 201 deletions(-) create mode 100644 include/linux/irqchip/arm-gic-v3-prio.h