Re: [PATCH 0/1] arm64/tools/sysreg: Add Sysreg128/SysregFields128

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Aug 05, 2024 at 08:30:48AM +0530, Anshuman Khandual wrote:
> On 8/3/24 16:37, Mark Rutland wrote:
> > On Thu, Aug 01, 2024 at 11:14:35AM +0530, Anshuman Khandual wrote:
> >> FEAT_SYSREG128 enables 128 bit wide system registers which also need to be
> >> defined in (arch/arm64/toos/sysreg) for auto mask generation. This adds two
> >> new field types i.e Sysreg128 and SysregFields128 for that same purpose. It
> >> utilizes recently added macro GENMASK_U128() while also adding some helpers
> >> such as define_field_128() and parse_bitdef_128().
> >>
> >> This patch applies after the following series which adds GENMASK_U128()
> >>
> >> https://lore.kernel.org/all/20240725054808.286708-1-anshuman.khandual@arm.com/
> > 
> > Is that patch merged or not? It wouod make a lot more sense to send them
> > together as one series.
> 
> The latest series [1] here has been reviewed and merged in bitmap-for-next
> tree for testing purpose. Also there has been an additional patch [2] just
> to keep the GENMASK_U128() helpers inside !__ASSEMBLY__ guard.
> 
> [1] https://lore.kernel.org/all/20240801071646.682731-1-anshuman.khandual@arm.com/
> [2] https://lore.kernel.org/all/20240803133753.1598137-1-yury.norov@gmail.com/
> 
> GENMASK_U128() series could have been part of this series, although 128 bit
> mask creation seems generic enough to stand on its own.
> 
> >> A. Example for SysregFields128
> >>
> >> ------------------------------
> >> SysregFields128 TTBRx_D128_EL1
> >> Res0   127:88
> >> Field  87:80   BADDR_HIGH
> >> Res0   79:64
> >> Field  63:48   ASID
> >> Field  47:5    BADDR_LOW
> >> Res0   4:3
> >> Field  2:1     SKL
> >> Field  0       CnP
> >> EndSysregFields128
> >> ------------------------------
> > 
> > Ok, so we get the definitions, but do we have all the other helpers we'd
> > need to make that useable, i.e.
> 
> The first objective was to get the definitions right, so that they could
> be stored in the new gcc __unit128 data type.

I can understand that being the first thing you do while prototyping,
but upstream we don't even know that we *want* to use __unit128, becuase
whether that is desireable depends on how we can *consume* the values.

> > * read_sysreg() and write_sysreg() variants that can use MRRS/MSRR
> > 
> > * Macros for assembly to use these?
> > 
> > * Other bitfield manipulation helpers that can operate on U128, e.g.
> >   FIELD_GET() and FIELD_PREP() ?
> 
> These are still work in progress, but will share when available.
> 
> > Without end-to-end usage this is a bit academic. If the U128 definitions
> > are oainful to use from asm we might want separate hi64/lo64
> > definitions.
> 
> Right, U128 definitions are difficult to use in asm code because they way
> gcc compiler deals with 128 bit data types.

I meant that this is likely to be painful even for plain where the
compiler is not involved, because the *assembler* cannot handle bits
127:64, e.g.

| [mark@gravadlaks:~]% cat test.S
| .if ((1 << 32) >> 32) != 1
| .error "Oh no! absolute expressions are evaluated as 32-bits or less"
| .endif
| 
| .if ((((1 << 32) << 32) >> 32) >> 32) != 1
| .error "Oh no! absolute expressions are evaluated as 64-bits or less"
| .endif
| [mark@gravadlaks:~]% as test.S -o test.o
| test.S: Assembler messages:
| test.S:6: Error: Oh no! absolute expressions are evaluated as 64-bits or less

... and so either we have to handle the hi64/lo64 halves explicitly when
generating definitions, or we need the assembler to gain support for
128-bit absolute expressions.

... and if we go for the former, we don't need U128 at all; hence why it
would have made more sense to do this in one go.

> Should the separate hi64/lo64 definitions be generic or platform
> specific without adding them to kernel bitops ?

I don't understand that question here. I was describing the problem of
the bit definitions, not operations operating upon those definitions.

Mark.