Re: [PATCH v8 13/14] arm/cpu: switch to a generated cpu-sysregs.h.inc

[Cornelia Huck <cohuck@redhat.com>]

On Tue, Jul 01 2025, Peter Maydell <peter.maydell@linaro.org> wrote:

> On Tue, 1 Jul 2025 at 17:07, Cornelia Huck <cohuck@redhat.com> wrote:
>>
>> On Mon, Jun 30 2025, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> > On Tue, 17 Jun 2025 at 16:41, Cornelia Huck <cohuck@redhat.com> wrote:
>> >>
>> >> Generated against Linux 6.15.
>> >>
>> >> Reviewed-by: Sebastian Ott <sebott@redhat.com>
>> >> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> >> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> >
>> > Stripping out all the lines that just moved around,
>> > here are the additions:
>> >
>> >> +DEF(ID_AFR0_EL1, 3, 0, 0, 1, 3)
>> >
>> > This is ARMCPU::id_afr0. If we want to store this in
>> > the idregs[] array that's fine but we ought to move it.
>> >
>>
>> I guess the *afr* regs fell into the cracks because they were not in the
>> isar struct -- makes sense to move them.
>>
>> (This reg "must be interpreted with" MIDR_EL1; I'm wondering if that has
>> any implications once we enlighten guests with possible midr/revidr/aidr
>> values.)
>
> It just means that the contents are entirely implementation
> defined, so you can't tell what they mean unless you first
> look at MIDR_EL1 to find out what your implementation is.

Would we need to provide multiple afr options then once we allow to
specify multiple values of MIDR and friends?

>
> (Linux doesn't appear to ever look at the AFR0 registers.)

Or ignore it for now...

>
>> >> +DEF(ID_AA64PFR2_EL1, 3, 0, 0, 4, 2)
>> >
>> >> +DEF(ID_AA64FPFR0_EL1, 3, 0, 0, 4, 7)
>> >
>> >> +DEF(ID_AA64DFR2_EL1, 3, 0, 0, 5, 2)
>> >
>> > These are all ID registers we don't implement yet
>> > because we don't implement any features that are
>> > described in them (i.e. our implementation is RAZ/WI).
>> > I guess it's OK to list them here, though we won't
>> > do anything with the array entry.
>>
>> I don't think it hurts to include them, it makes things easier when we
>> want to deal with non-zero values in there via kvm.
>>
>> >
>> >> +DEF(ID_AA64AFR0_EL1, 3, 0, 0, 5, 4)
>> >
>> > ARMCPU::id_aa64afr0
>> >
>> >> +DEF(ID_AA64AFR1_EL1, 3, 0, 0, 5, 5)
>> >
>> > ARMCPU::id_aa64afr1
>> >
>> >> +DEF(ID_AA64ISAR3_EL1, 3, 0, 0, 6, 3)
>> >
>> >> +DEF(ID_AA64MMFR4_EL1, 3, 0, 0, 7, 4)
>> >
>> > More ID regs for features we don't implement yet.
>> >
>> >> +DEF(CCSIDR_EL1, 3, 1, 0, 0, 0)
>> >
>> > CCSIDR_EL1 isn't a single ID register, it's an array
>> > of them (indexed by CCSELR_EL1). We keep them in
>> > ARMCPU::ccsidr[]. I don't think it makes sense to
>> > have an entry in isar.idregs[] for this.
>>
>> Hm, IIUC, kvm gets the correct CCSIDR_EL1 under the covers already
>> (i.e. no array).
>
> There's support in the KVM GET_ONE_REG API for reading
> and writing the full array (see KVM_REG_ARM_DEMUX_ID_CCSIDR).
> We just don't bother in QEMU at the moment because we never
> need the values in userspace.

We could manually exclude things like CCSIDR_EL1, but is it worth the
effort if it's simply present?

>
>> >> +DEF(CLIDR_EL1, 3, 1, 0, 0, 1)
>> >
>> > ARMCPU::clidr
>> >
>> >> +DEF(CCSIDR2_EL1, 3, 1, 0, 0, 2)
>> >
>> > This is an array too.
>>
>> Currently, kvm handles this as undef.
>
> That seems like a missing feature in KVM -- it ought to be
> implemented on anything where ID_AA64MMFR2.CCIDX says
> we have FEAT_CCIDX (which means that the whole cache ID
> format changes and the info is split between CCSIDR
> and CCSIDR2).
>
>> >> +DEF(GMID_EL1, 3, 1, 0, 0, 4)
>> >
>> > Another ID register for a feature we don't yet implement
>> > (and a slightly oddball one in that it should UNDEF
>> > until we do implement FEAT_MTE2).
>> >
>> >> +DEF(SMIDR_EL1, 3, 1, 0, 0, 6)
>> >
>> > We implement this as a fixed zero in helper.c.
>>
>> Undef in kvm.
>
> Probably because KVM doesn't have support for SME in
> guests yet ?

Once we do get support for it in kvm, it would make sense to have it in
place already, and it probably won't conflict with the fixed definition
in tcg code, right?

>
>> >
>> >> +DEF(DCZID_EL0, 3, 3, 0, 0, 7)
>> >
>> > We construct the value of this one in aa64_dczid_read()
>> > based on ARMCPU::dcz_blocksize plus some runtime info.
>>
>> That one's another bit of a headache (I didn't manage to spot kvm code
>> dealing with it.)
>
> The hardware handles it for you -- inside the guest, the
> value of the DZP bit depends on HCR_EL2.TDZ (and maybe also
> SCTLR_EL1.DZE). The value of DCZID_EL0.BS has to match the
> real host CPU anyway because you can't change the block
> size that the DC ZVA insn is going to use. (You could
> trap DCZID_EL0 accesses via HFGRTR_EL2.DCZID_EL0 if you really
> wanted to, but you'd also need to trap-and-emulate the
> DC ZVA insns, which is a lot of effort for something
> no guest is really going to notice the difference on.)

So I guess it wouldn't hurt to include this in the array?

My basic question is: Do we need to exclude certain registers from being
included in the autogenerated list and derived code? (On the flip side,
once we add the code that discovers registers writable via kvm, we also
found that we need some registers not in the source file...)