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

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

On 5/13/25 4:42 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:
>>> 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?
what about NR_ID_REG_MASKS
because it is masks actually.
>
>>> +
>>> +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.
yeah. I am sorry. previously we had a single indexing and on top of that
we have several kinds of definitions for id regs and writable masks.
Even for me who contributed to that mess it can be difficult to parse so
I imagine it can be painful to read for other readers too ;-)

Eric
>
>>> +
>>>  /* 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.
>