Re: [PATCH v3 01/10] arm/cpu: Add infra to handle generated ID register definitions

[Eric Auger <eric.auger@redhat.com>]

On 5/13/25 4:05 PM, Cornelia Huck wrote:
> On Tue, May 13 2025, Eric Auger <eric.auger@redhat.com> wrote:
>
>> Hi Connie,
>>
>> On 4/14/25 6:38 PM, Cornelia Huck wrote:
>>> From: Eric Auger <eric.auger@redhat.com>
>>>
>>> The known ID regs are described in a new initialization function
>>> dubbed initialize_cpu_sysreg_properties(). That code will be
>>> automatically generated from linux arch/arm64/tools/sysreg. For the
>>> time being let's just describe a single id reg, CTR_EL0. In this
>>> description we only care about non RES/RAZ fields, ie. named fields.
>>>
>>> The registers are populated in an array indexed by ARMIDRegisterIdx
>>> and their fields are added in a sorted list.
>>>
>>> [CH: adapted to reworked register storage]
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>  target/arm/cpu-custom.h            | 60 ++++++++++++++++++++++++++++++
>>>  target/arm/cpu-sysreg-properties.c | 41 ++++++++++++++++++++
>>>  target/arm/cpu64.c                 |  2 +
>>>  target/arm/meson.build             |  1 +
>>>  4 files changed, 104 insertions(+)
>>>  create mode 100644 target/arm/cpu-custom.h
>> do we still want reference to the "custom" terminology, following
>> initial comments?
> Hm, maybe 'cpu-idregs.h'?
OK for me

Eric
>
>>>  create mode 100644 target/arm/cpu-sysreg-properties.c
>>>
>>> diff --git a/target/arm/cpu-custom.h b/target/arm/cpu-custom.h
>>> new file mode 100644
>>> index 000000000000..615347376e56
>>> --- /dev/null
>>> +++ b/target/arm/cpu-custom.h
>>> @@ -0,0 +1,60 @@
>>> +/*
>>> + * handle ID registers and their fields
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +#ifndef ARM_CPU_CUSTOM_H
>>> +#define ARM_CPU_CUSTOM_H
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/error-report.h"
>>> +#include "cpu.h"
>>> +#include "cpu-sysregs.h"
>>> +
>>> +typedef struct ARM64SysRegField {
>>> +    const char *name; /* name of the field, for instance CTR_EL0_IDC */
>>> +    int index;
>> worth to add a comment saying this is the ARMIDRegisterIdx of the parent
>> sysreg.
> ok
>
>>> +    int lower;
>>> +    int upper;
>>> +} ARM64SysRegField;
>>> +
>>> +typedef struct ARM64SysReg {
>>> +    const char *name;   /* name of the sysreg, for instance CTR_EL0 */
>>> +    ARMSysRegs sysreg;
>>> +    int index;
>> now that we have different kinds of indexing, may be worth adding a
>> comment to explain which one is being used.
>> I guess here it is ARMIDRegisterIdx. So you could even change the data type.
> Yeah, comments are good, I'll add some.
>
>>> +    GList *fields; /* list of named fields, excluding RES* */
>>> +} ARM64SysReg;
>>> +
>>> +void initialize_cpu_sysreg_properties(void);
>>> +
>>> +/*
>>> + * List of exposed ID regs (automatically populated from linux
>>> + * arch/arm64/tools/sysreg)
>>> + */
>>> +extern ARM64SysReg arm64_id_regs[NUM_ID_IDX];
>>> +
>>> +/* Allocate a new field and insert it at the head of the @reg list */
>>> +static inline GList *arm64_sysreg_add_field(ARM64SysReg *reg, const char *name,
>>> +                                     uint8_t min, uint8_t max) {
>>> +
>>> +     ARM64SysRegField *field = g_new0(ARM64SysRegField, 1);
>>> +
>>> +     field->name = name;
>>> +     field->lower = min;
>>> +     field->upper = max;
>>> +     field->index = reg->index;
>>> +
>>> +     reg->fields = g_list_append(reg->fields, field);
>>> +     return reg->fields;
>>> +}
>>> +
>>> +static inline ARM64SysReg *arm64_sysreg_get(ARMIDRegisterIdx index)
>>> +{
>>> +        ARM64SysReg *reg = &arm64_id_regs[index];
>>> +
>>> +        reg->index = index;
>>> +        reg->sysreg = id_register_sysreg[index];
>>> +        return reg;
>>> +}
>>> +
>>> +#endif
>>> diff --git a/target/arm/cpu-sysreg-properties.c b/target/arm/cpu-sysreg-properties.c
>>> new file mode 100644
>>> index 000000000000..8b7ef5badfb9
>>> --- /dev/null
>>> +++ b/target/arm/cpu-sysreg-properties.c
>>> @@ -0,0 +1,41 @@
>>> +/*
>>> + * QEMU ARM CPU SYSREG PROPERTIES
>>> + * to be generated from linux sysreg
>>> + *
>>> + * Copyright (c) Red Hat, Inc. 2024
>> maybe increment the year now ;-)
> Wait, it is 2025 already? :)
>