linux-kernel - Re: [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ad164b94-06af-ffe7-b8ff-317b4078b1a5@huawei.com>
Date:   Wed, 23 Oct 2019 16:38:47 +0800
From:   "liwei (GF)" <liwei391@...wei.com>
To:     Marc Zyngier <maz@...nel.org>, Will Deacon <will@...nel.org>,
        "Catalin Marinas" <catalin.marinas@....com>
CC:     Suzuki K Poulose <suzuki.poulose@....com>,
        James Morse <james.morse@....com>,
        Julien Thierry <julien.thierry.kdev@...il.com>,
        <huawei.libin@...wei.com>, <guohanjun@...wei.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when
 ICC_CTLR_EL1.PMHE is clear

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@....com>
> Cc: Will Deacon <will@...nel.org>
> Cc: James Morse <james.morse@....com>
> Cc: Julien Thierry <julien.thierry.kdev@...il.com>
> Cc: Suzuki K Poulose <suzuki.poulose@....com>
> Signed-off-by: Marc Zyngier <maz@...nel.org>
> ---
>  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.

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();
>  }
>