Message-ID: <0a3a60082d13b27388a0893abce8be47b045c107.camel@arm.com>
Date: Thu, 9 Oct 2025 09:48:36 +0000
From: Sascha Bischoff <Sascha.Bischoff@....com>
To: "broonie@...nel.org" <broonie@...nel.org>
CC: "yuzenghui@...wei.com" <yuzenghui@...wei.com>, Joey Gouly
	<Joey.Gouly@....com>, Suzuki Poulose <Suzuki.Poulose@....com>, nd
	<nd@....com>, Mark Rutland <Mark.Rutland@....com>, "lpieralisi@...nel.org"
	<lpieralisi@...nel.org>, "kvmarm@...ts.linux.dev" <kvmarm@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, Catalin Marinas <Catalin.Marinas@....com>,
	"maz@...nel.org" <maz@...nel.org>, "oliver.upton@...ux.dev"
	<oliver.upton@...ux.dev>, "will@...nel.org" <will@...nel.org>
Subject: Re: [PATCH 1/3] arm64/sysreg: Support feature-specific fields with
 'Feat' descriptor

On Tue, 2025-10-07 at 17:28 +0100, Mark Brown wrote:
> On Tue, Oct 07, 2025 at 03:35:14PM +0000, Sascha Bischoff wrote:
> 
> > Some system register field encodings change based on the available
> > and
> > in-use architecture features. In order to support these different
> > field encodings, introduce the Feat descriptor (Feat, ElseFeat,
> > EndFeat) for describing such sysregs.
> 
> We have other system registers which change layouts based on things
> other than features, ESR_ELx is probably the most entertaining of
> these
> but there's others like PAR_EL1.  Ideally we should probably do the
> logic for generating the conditionals in a manner that's not tied to
> features with a layer on top that generates standard naming for
> common
> patterns like FEAT, but OTOH part of why that's not been done because
> it's got a bunch of nasty issues so perhaps just doing the simpler
> case
> is fine.

Hi Mark - thanks for your input!

I agree that there are plenty of cases where sysreg fields change based
on more than just architectural features. Whilst my intent for this
change wasn't strictly to cover all of those cases, it could arguably
be used for that. Effectively, all this change is doing is to add a
prefix to the field definitions so it can be used however works best.

It does feel as if Feat was potentially the wrong name to choose.
Something like Fieldset, Prefix, AltLayout, etc might be better. I'm
rather open to suggestions here - naming is hard!

> 
> > The Feat descriptor can be used in the following way (Feat acts as
> > both an if and an else-if):
> 
> >         Sysreg  EXAMPLE 0    1    2    3    4
> >         Feat    FEAT_A
> >         Field   63:0    Foo
> >         Feat    FEAT_B
> 
> This assumes that there will never be nesting of these conditions in
> the architecture.

My thinking was that for something like that one could do:

        Feat    FEAT_A_B

Or similar. Yes, it means that anything that is nested needs to be
flattened, but it does allow one to describe that situation. In the
end, all this effectively does is to add a prefix, and said prefix can
be anything. Double so if we move away from calling it Feat, and use
something more generic instead.

> I'm not sure I would want to assume that, even for plain
> features though I can't think of any examples where it's an issue.
> There are more serious issues with the implementation for practical
> patterns with nesting (see below) which mean we might not want to
> deal
> with that now but we should define the syntax for the file in a way
> that
> will cope so I'd prefer not to have the implicit elses.

I think we could do something more like this (sticking with calling it
Feat for now):

Sysreg EXAMPLE ...
Feat A
...
EndFeat
Feat B
..
EndFeat
Field ...
EndSysreg

This way each set of field layouts is explicitly wrapped in a block,
except for the "default" ones (if present/desired). It is a bit more
verbose, but removes as much of the implicit nature as possible, and
removes the need for the Else which is more in keeping with the rest of
the blocks. I'll do this for v2.

> 
> I'd also be inclined to say that something that's specificly for
> features shouldn't repeat the FEAT_ so:
> 
>           Feat    A
> 
> instead, but that's purely a taste question.

Yup. I think the correct thing here does somewhat depend on how generic
this becomes.

> 
> > -	define("REG_" reg, "S" op0 "_" op1 "_C" crn "_C" crm "_"
> > op2)
> > -	define("SYS_" reg, "sys_reg(" op0 ", " op1 ", " crn ", "
> > crm ", " op2 ")")
> > +	feat = null
> > +
> > +	define(feat, "REG_" reg, "S" op0 "_" op1 "_C" crn "_C" crm
> > "_" op2)
> > +	define(feat, "SYS_" reg, "sys_reg(" op0 ", " op1 ", " crn
> > ", " crm ", " op2 ")")
> >  
> > -	define("SYS_" reg "_Op0", op0)
> > -	define("SYS_" reg "_Op1", op1)
> > -	define("SYS_" reg "_CRn", crn)
> > -	define("SYS_" reg "_CRm", crm)
> > -	define("SYS_" reg "_Op2", op2)
> > +	define(feat, "SYS_" reg "_Op0", op0)
> > +	define(feat, "SYS_" reg "_Op1", op1)
> > +	define(feat, "SYS_" reg "_CRn", crn)
> > +	define(feat, "SYS_" reg "_CRm", crm)
> > +	define(feat, "SYS_" reg "_Op2", op2)
> 
> Possibly it's worth having a reg_define() or something given the
> number of things needing updating here?
> 
> > @@ -201,6 +205,7 @@ $1 == "EndSysreg" && block_current() ==
> > "Sysreg" {
> >  	res0 = null
> >  	res1 = null
> >  	unkn = null
> > +	feat = null
> >  
> >  	block_pop()
> >  	next
> 
> Probably worth complaining if we end a sysreg with a feature/prefix
> defined.

This is already be caught as we hit the EndSysreg directive while still
in the Feat block:

Error at 4690: unhandled statement
Error at 4690: Missing terminator for Feat block

I can be more specific in catching that, if you like.

I have missed a check to catch someone defining fewer than 64-bits in a
Feat block, though - will be fixed in v2.

> 
> > +$1 == "Feat" && (block_current() == "Sysreg" || block_current() ==
> > "SysregFields" || block_current() == "Feat") {
> 
> ...
> 
> > +	next_bit = 63
> > +
> > +	res0 = "UL(0)"
> > +	res1 = "UL(0)"
> > +	unkn = "UL(0)"
> 
> This is only going to work if the whole register is in a FEAT_ block,
> so
> you coudn't have:
> 
>         Sysreg  EXAMPLE 0    1    2    3    4
> 	Res0	63
>         Feat    FEAT_A
>         Field   62:1    Foo
>         Feat    FEAT_B
>         Field   62:32    Foo
>         Field   31:1     Bar
> 	EndFeat
> 	Res0	0
> 	EndSysreg
> 
> but then supporting partial registers does have entertaining
> consequences for handling Res0 and Res1.  If we're OK with that
> restriction then the program should complain if someone tries to 
> define a smaller FEAT block.

It was my intent to only support whole registers here. Anything else
gets insanely complex rather quickly, and much of the benefit gets lost
IMO. By only operating on whole Sysregs (or SysregFields) we are able
to re-use the existing logic to ensure that definitions are complete,
and the generator's state tracking remains simple. And, as you say, it
would break the ResX, Unkn side of things if we did anything else.

When I add the missing check I mentioned above, then it will complain
if fewer than 64 bits are in a defined block.

Thanks,
Sascha

Powered by blists - more mailing lists