Re: [PATCH] arm64: Independently update HDFGRTR_EL2 and HDFGWTR_EL2

From: Marc Zyngier <maz@kernel.org>
To: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] arm64: Independently update HDFGRTR_EL2 and HDFGWTR_EL2
Date: Wed, 18 Oct 2023 13:40:37 +0100	[thread overview]
Message-ID: <86r0lsm7cq.wl-maz@kernel.org> (raw)
In-Reply-To: <20231018030007.1968317-1-anshuman.khandual@arm.com>

On Wed, 18 Oct 2023 04:00:07 +0100,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> Currently PMSNEVFR_EL1 system register read, and write access EL2 traps are
> disabled, via setting the same bit (i.e 62) in HDFGRTR_EL2, and HDFGWTR_EL2
> respectively. Although very similar, bit fields are not exact same in these
> two EL2 trap configure registers particularly when it comes to read-only or
> write-only accesses such as ready-only 'HDFGRTR_EL2.nBRBIDR' which needs to
> be set while enabling BRBE on NVHE platforms. Using the exact same bit mask
> fields for both these trap register risk writing into their RESERVED areas,
> which is undesirable.

Sorry, I don't understand at all what you are describing. You seem to
imply that the read and write effects of the FGT doesn't apply the
same way. But my reading of the ARM ARM is that  behave completely
symmetrically.

Also, what is nBRBIDR doing here? It is still set to 0. What
'RESERVED' state are you talking about?

> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> This applies on v6.6-rc6.
> 
> I guess it should be okay to use 'x2' as it is in the clobbered register
> list for init_el2_state() function. But please do let me know otherwise.
> 
>  arch/arm64/include/asm/el2_setup.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> index 899b5c10f84c..c534afb1a30d 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h
> @@ -206,16 +206,19 @@
>  	cbz	x1, .Lskip_fgt_\@
>  
>  	mov	x0, xzr
> +	mov	x2, xzr
>  	mrs	x1, id_aa64dfr0_el1
>  	ubfx	x1, x1, #ID_AA64DFR0_EL1_PMSVer_SHIFT, #4
>  	cmp	x1, #3
>  	b.lt	.Lset_debug_fgt_\@
> +
>  	/* Disable PMSNEVFR_EL1 read and write traps */
> -	orr	x0, x0, #(1 << 62)
> +	orr	x0, x0, #HDFGRTR_EL2_nPMSNEVFR_EL1_MASK
> +	orr	x2, x2, #HDFGWTR_EL2_nPMSNEVFR_EL1_MASK
>  
>  .Lset_debug_fgt_\@:
>  	msr_s	SYS_HDFGRTR_EL2, x0
> -	msr_s	SYS_HDFGWTR_EL2, x0
> +	msr_s	SYS_HDFGWTR_EL2, x2

So what has changed here, aside from clobbering an extra register? The
masks are the same, the initial values are the same... Is it in
preparation for some other work?

/me puzzled.

	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