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

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Jan 24, 2023 at 09:31:04AM +0000, Marc Zyngier wrote:
> 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.

FWIW, I'll go with that for now; as below it's more painful to go from `NOP` to
`DSB SY`, and either we end up needing a alt_patch_dsb_sy() callback, or
generating the alternative sequences this patch was trying to avoid.

I'll go clear up all the related naming and comments to talk in terms of
"relaxed synchronization" rather than "no PMHE".

Thanks,
Mark.

> > > 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