Re: [PATCH v3 04/42] arm64: sysreg: Replace HGFxTR_EL2 with HFG{R,W}TR_EL2

[Marc Zyngier <maz@kernel.org>]

On Tue, 29 Apr 2025 15:26:56 +0100,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> On Sat, Apr 26, 2025 at 01:27:58PM +0100, Marc Zyngier wrote:

> > @@ -240,8 +240,8 @@
> >  	cbz	x1, .Lset_fgt_\@
> >  
> >  	/* Disable traps of access to GCS registers at EL0 and EL1 */
> > -	orr	x0, x0, #HFGxTR_EL2_nGCS_EL1_MASK
> > -	orr	x0, x0, #HFGxTR_EL2_nGCS_EL0_MASK
> > +	orr	x0, x0, #HFGRTR_EL2_nGCS_EL1_MASK
> > +	orr	x0, x0, #HFGRTR_EL2_nGCS_EL0_MASK
> >  
> >  .Lset_fgt_\@:
> >  	msr_s	SYS_HFGRTR_EL2, x0
> 
> We still treat them as the same here, funny that the diff cut off the next line:
> 
> 	msr_s   SYS_HFGWTR_EL2, x0
> 
> Not saying you should do anything about it, I think it's fine.

Yeah, I had spotted these, but pointlessly duplicating these for R/W
did seem over the top.

Overall, What I am trying to achieve is to prevent that someone
accidentally uses something such as HFGxTR_EL2.AIDR_EL1 to HFGWTR_EL2.
I want to be able to catch those early (compile time) when they are
used in macros that compose register and bit names.

> 
> > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > index f36d067967c33..43a630b940bfb 100644
> > --- a/arch/arm64/include/asm/kvm_arm.h
> > +++ b/arch/arm64/include/asm/kvm_arm.h
> > @@ -325,7 +325,7 @@
> >   * Once we get to a point where the two describe the same thing, we'll
> >   * merge the definitions. One day.
> >   */
> > -#define __HFGRTR_EL2_RES0	HFGxTR_EL2_RES0
> > +#define __HFGRTR_EL2_RES0	HFGRTR_EL2_RES0
> >  #define __HFGRTR_EL2_MASK	GENMASK(49, 0)
> >  #define __HFGRTR_EL2_nMASK	~(__HFGRTR_EL2_RES0 | __HFGRTR_EL2_MASK)
> >  
> > @@ -336,7 +336,7 @@
> >  #define __HFGRTR_ONLY_MASK	(BIT(46) | BIT(42) | BIT(40) | BIT(28) | \
> >  				 GENMASK(26, 25) | BIT(21) | BIT(18) | \
> >  				 GENMASK(15, 14) | GENMASK(10, 9) | BIT(2))
> > -#define __HFGWTR_EL2_RES0	(__HFGRTR_EL2_RES0 | __HFGRTR_ONLY_MASK)
> > +#define __HFGWTR_EL2_RES0	HFGWTR_EL2_RES0
> >  #define __HFGWTR_EL2_MASK	(__HFGRTR_EL2_MASK & ~__HFGRTR_ONLY_MASK)
> >  #define __HFGWTR_EL2_nMASK	~(__HFGWTR_EL2_RES0 | __HFGWTR_EL2_MASK)
> >  
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index e98cfe7855a62..7a1ef5be7efb2 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -273,7 +273,8 @@ struct kvm_sysreg_masks;
> >  
> >  enum fgt_group_id {
> >  	__NO_FGT_GROUP__,
> > -	HFGxTR_GROUP,
> > +	HFGRTR_GROUP,
> > +	HFGWTR_GROUP = HFGRTR_GROUP,
> 
> I think this change makes most of the diffs using this enum more confusing, but
> it also seems to algin the code more closely with HDFGWTR_EL2 and HDFGWTR_EL2.

Indeed. And once you add FEAT_FGT2 to the mix, HFGxTR becomes really
out of place. As for the confusing aspect, I agree that the notion of
group is a bit jarring, and maybe some documentation would help. The
idea is actually simple:

A sysreg trap always tells us whether this is for read or write. The
data stored for each sysreg tells us which FGT register is controlling
that trap. But since we can have one FGT register for read, and
another for write, we would have to store both. Trouble is, we only
have 63 bits in that descriptor. To save some space, we encode only
the group (covering both read and write), and use the WnR bit to pick
the correct guy.

This means we can encode 11 possible registers in 3 bits, with
restrictions. We still have plenty of bits left, but I'm pretty sure
the architecture will force us to eat into it pretty quickly.

[...]

> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 005ad28f73068..6e01b06bedcae 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -5147,12 +5147,12 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu)
> >  	if (test_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags))
> >  		goto out;
> >  
> > -	kvm->arch.fgu[HFGxTR_GROUP] = (HFGxTR_EL2_nAMAIR2_EL1		|
> > -				       HFGxTR_EL2_nMAIR2_EL1		|
> > -				       HFGxTR_EL2_nS2POR_EL1		|
> > -				       HFGxTR_EL2_nACCDATA_EL1		|
> > -				       HFGxTR_EL2_nSMPRI_EL1_MASK	|
> > -				       HFGxTR_EL2_nTPIDR2_EL0_MASK);
> > +	kvm->arch.fgu[HFGRTR_GROUP] = (HFGRTR_EL2_nAMAIR2_EL1		|
> > +				       HFGRTR_EL2_nMAIR2_EL1		|
> > +				       HFGRTR_EL2_nS2POR_EL1		|
> > +				       HFGRTR_EL2_nACCDATA_EL1		|
> > +				       HFGRTR_EL2_nSMPRI_EL1_MASK	|
> > +				       HFGRTR_EL2_nTPIDR2_EL0_MASK);
> 
> For example here you see HFGRTR_GROUP but it actually also applies to HFGWTR_GROUP.

Because we use the same encoding trick. I don't see a good way to express
that in a clean way, unfortunately. If you have an idea, I'm all ears!

Thanks,

	M.

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