Re: [PATCH 3/4] arm64: add ARM64_HAS_GIC_PRIO_NO_PMHE cpucap

[Marc Zyngier <maz@kernel.org>]

On Mon, 23 Jan 2023 14:30:17 +0000,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Mon, Jan 23, 2023 at 01:23:31PM +0000, Marc Zyngier wrote:
> > On Mon, 23 Jan 2023 12:40:41 +0000,
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > > 
> > > When Priority Mask Hint Enable (PMHE) == 0b1, the GIC may use the PMR
> > > value to determine whether to signal an IRQ to a PE, and consequently
> > > after a change to the PMR value, a DSB SY may be required to ensure that
> > > interrupts are signalled to a CPU in finite time. When PMHE == 0b0,
> > > interrupts are always signalled to the relevant PE, and all masking
> > > occurs locally, without requiring a DSB SY.
> > > 
> > > Since commit:
> > > 
> > >   f226650494c6aa87 ("arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear")
> > > 
> > > ... we handle this dynamically: in most cases a static key is used to
> > > determine whether to issue a DSB SY, but the entry code must read from
> > > ICC_CTLR_EL1 as static keys aren't accessible from plain assembly.
> > > 
> > > It would be much nicer to use an alternative instruction sequence for
> > > the DSB, as this would avoid the need to read from ICC_CTLR_EL1 in the
> > > entry code, and for most other code this will result in simpler code
> > > generation with fewer instructions and fewer branches.
> > > 
> > > This patch adds a new ARM64_HAS_GIC_PRIO_NO_PMHE cpucap which is only
> > > set when ICC_CTLR_EL1.PMHE == 0b0 (and GIC priority masking is in use).
> > > This allows us to replace the existing users of the `gic_pmr_sync`
> > > static key with alternative sequences which default to a DSB SY and are
> > > relaxed to a NOP when PMHE is not in use.
> > 
> > I personally find the "negative capability" pretty annoying, specially
> > considering that hardly anyone uses PMHE. The way the code reads with
> > this patch, it is always some sort of double negation.
> 
> For the polarity and double-negation, I could rename this to
> ARM64_HAS_GIC_PRIO_RELAXED_SYNC, if that helps?

It certainly reads much better.

> 
> > Can't the DSB be patched-in instead, making the PMHE cap a "positive"
> > one? 
> 
> We could; my rationale for doing it this way is that we can use the common NOP
> patching helper, and avoid generating a copy of the `DSB SY` instruction per
> pmr_sync() call (which gets generated near to the call and never gets free,
> unlike the alt_instr entries), which adds up quickly when using pseudo-NMIs.

Having an equivalent to alt_cb_patch_nops to patch in "DSB SY" would
result in similar gains, only less reusable...

>
> > It shouldn't affect interrupt distribution as long as the
> > patching occurs before we take interrupts. For modules, the patching always
> > occurs before we can run the module, so this should be equally safe.
> 
> I agree it shouldn't matter either way -- until we've patched in
> ARM64_HAS_GIC_PRIO_MASKING alternatives it's not going to matter.
> 
> > The patch otherwise looks OK to me.
> 
> Thanks!
> 
> Do you have a preference between the ARM64_HAS_GIC_PRIO_RELAXED_SYNC or
> ARM64_HAS_GIC_PRIO_PMHE options above?

ARM64_HAS_GIC_PRIO_PMHE would have my preference (it spells out the
feature that drives the property), but this requires a bit more work
(a new patching callback), and probably results in more limited gains
memory wise.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel