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

[Cornelia Huck <cohuck@redhat.com>]

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'?

>>  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? :)