Re: [PATCH v3 04/10] kvm: kvm_get_writable_id_regs

[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:
>> Add an helper to retrieve the writable id reg bitmask. The
>> status of the query is stored in the CPU struct so that an
>> an error, if any, can be reported on vcpu realize().
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  target/arm/cpu.h     | 26 ++++++++++++++++++++++++++
>>  target/arm/kvm.c     | 32 ++++++++++++++++++++++++++++++++
>>  target/arm/kvm_arm.h |  7 +++++++
>>  3 files changed, 65 insertions(+)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index d27134f4a025..bbee7ff2414a 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -856,6 +856,26 @@ typedef struct {
>>      uint32_t map, init, supported;
>>  } ARMVQMap;
>>  
>> +typedef enum ARMIdRegsState {
>> +    WRITABLE_ID_REGS_UNKNOWN,
>> +    WRITABLE_ID_REGS_NOT_DISCOVERABLE,
>> +    WRITABLE_ID_REGS_FAILED,
>> +    WRITABLE_ID_REGS_AVAIL,
>> +} ARMIdRegsState;
>> +
>> +/*
>> + * The following structures are for the purpose of mapping the output of
>> + * KVM_ARM_GET_REG_WRITABLE_MASKS that also may cover id registers we do
>> + * not support in QEMU
>> + * ID registers in op0==3, op1=={0,1,3}, crn=0, crm=={0-7}, op2=={0-7},
>> + * as used by the KVM_ARM_GET_REG_WRITABLE_MASKS ioctl call.
>> + */
>> +#define NR_ID_REGS (3 * 8 * 8)
> We may rename this define to better associate to the KVM API. I tend to
> mix it with NUM_ID_IDX now ;-)
> maybe something like KVM_NR_EXPOSED_ID_REGS

The kernel calls it KVM_FEATURE_ID_RANGE_SIZE, but I'd like to avoid
adding 'KVM' in the name, as it is basically a range of registers and
nothing really KVM specific... maybe ID_REG_RANGE_SIZE?

>> +
>> +typedef struct IdRegMap {
>> +    uint64_t regs[NR_ID_REGS];
>> +} IdRegMap;
> I would add a comment saying this is the mask array, just to prevent the
> reading from thinking it is the actual reg content.

"More comments" seems to be a theme :) I'll go ahead and add them where
it makes sense.

>> +
>>  /* REG is ID_XXX */
>>  #define FIELD_DP64_IDREG(ISAR, REG, FIELD, VALUE)                       \
>>      ({                                                                  \
>> @@ -1044,6 +1064,12 @@ struct ArchCPU {
>>       */
>>      bool host_cpu_probe_failed;
>>  
>> +    /*
>> +     * state of writable id regs query used to report an error, if any,
>> +     * on KVM custom vcpu model realize
>> +     */
>> +    ARMIdRegsState writable_id_regs;
> maybe rename into writable_id_reg_status that would better reflect what
> it is.

Indeed.