Re: [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear

[Marc Zyngier]

Hi Wei,

On 2019-10-23 09:38, liwei (GF) wrote:
> Hi Marc,
>
> On 2019/10/2 17:06, Marc Zyngier wrote:
> > The GICv3 architecture specification is incredibly misleading when 
> > it
> > comes to PMR and the requirement for a DSB. It turns out that this 
> > DSB
> > is only required if the CPU interface sends an Upstream Control
> > message to the redistributor in order to update the RD's view of 
> > PMR.
> >
> > This message is only sent when ICC_CTLR_EL1.PMHE is set, which isn't
> > the case in Linux. It can still be set from EL3, so some special 
> > care
> > is required. But the upshot is that in the (hopefuly large) majority
> > of the cases, we can drop the DSB altogether.
> >
> > This relies on a new static key being set if the boot CPU has PMHE
> > set. The drawback is that this static key has to be exported to
> > modules.
> >
> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > Cc: James Morse <james.morse@xxxxxxx>
> > Cc: Julien Thierry <julien.thierry.kdev@xxxxxxxxx>
> > Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > ---
> >  arch/arm64/include/asm/barrier.h   | 12 ++++++++++++
> >  arch/arm64/include/asm/daifflags.h |  3 ++-
> >  arch/arm64/include/asm/irqflags.h  | 19 ++++++++++---------
> >  arch/arm64/include/asm/kvm_host.h  |  3 +--
> >  arch/arm64/kernel/entry.S          |  6 ++++--
> >  arch/arm64/kvm/hyp/switch.c        |  4 ++--
> >  drivers/irqchip/irq-gic-v3.c       | 20 ++++++++++++++++++++
> >  include/linux/irqchip/arm-gic-v3.h |  2 ++
> >  8 files changed, 53 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/barrier.h 
> > b/arch/arm64/include/asm/barrier.h
> > index e0e2b1946f42..7d9cc5ec4971 100644
> > --- a/arch/arm64/include/asm/barrier.h
> > +++ b/arch/arm64/include/asm/barrier.h
> > @@ -29,6 +29,18 @@
> >  						 SB_BARRIER_INSN"nop\n",	\
> >  						 ARM64_HAS_SB))
> >
> > +#ifdef CONFIG_ARM64_PSEUDO_NMI
> > +#define pmr_sync()						\
> > +	do {							\
> > +		extern struct static_key_false gic_pmr_sync;	\
> > +								\
> > +		if (static_branch_unlikely(&gic_pmr_sync))	\
> > +			dsb(sy);				\
> > +	} while(0)
> > +#else
> > +#define pmr_sync()	do {} while (0)
> > +#endif
> > +
>
> Thank you for solving this problem, it helps a lot indeed.
>
> The pmr_sync() will call dsb(sy) when ARM64_PSEUDO_NMI=y and
> gic_pmr_sync=force,
> but if pseudo nmi is not enabled through boot option, it just take 
> one more
> redundant calling than before at the following two place.
>
> I think change dsb(sy) to
> +                       asm volatile(ALTERNATIVE("nop", "dsb sy",     
>  \
> +                               ARM64_HAS_IRQ_PRIO_MASKING)     \
> +                               : : : "memory");                \
> may be more appropriate.

I'm not sure I understand what you mean. The static key defaults to 
false,
so if pseudo_nmi is not enabled, this dsb(sy) is simply never executed.

Am I missing something obvious?

Thanks,

        M.

> Thanks,
> Wei
>
> > @@ -34,14 +35,14 @@ static inline void arch_local_irq_enable(void)
> >  	}
> >
> >  	asm volatile(ALTERNATIVE(
> > -		"msr	daifclr, #2		// arch_local_irq_enable\n"
> > -		"nop",
> > -		__msr_s(SYS_ICC_PMR_EL1, "%0")
> > -		"dsb	sy",
> > +		"msr	daifclr, #2		// arch_local_irq_enable",
> > +		__msr_s(SYS_ICC_PMR_EL1, "%0"),
> >  		ARM64_HAS_IRQ_PRIO_MASKING)
> >  		:
> >  		: "r" ((unsigned long) GIC_PRIO_IRQON)
> >  		: "memory");
> > +
> > +	pmr_sync();
> >  }
> >
> >  static inline void arch_local_irq_disable(void)
> > @@ -116,14 +117,14 @@ static inline unsigned long 
> > arch_local_irq_save(void)
> >  static inline void arch_local_irq_restore(unsigned long flags)
> >  {
> >  	asm volatile(ALTERNATIVE(
> > -			"msr	daif, %0\n"
> > -			"nop",
> > -			__msr_s(SYS_ICC_PMR_EL1, "%0")
> > -			"dsb	sy",
> > -			ARM64_HAS_IRQ_PRIO_MASKING)
> > +		"msr	daif, %0",
> > +		__msr_s(SYS_ICC_PMR_EL1, "%0"),
> > +		ARM64_HAS_IRQ_PRIO_MASKING)
> >  		:
> >  		: "r" (flags)
> >  		: "memory");
> > +
> > +	pmr_sync();
> >  }

--
Jazz is not dead. It just smells funny...