Message-ID: <822a348f-46e1-44cb-b475-1b120bf96de4@redhat.com>
Date: Tue, 8 Apr 2025 14:55:52 +1000
From: Gavin Shan <gshan@...hat.com>
To: Steven Price <steven.price@....com>, kvm@...r.kernel.org,
 kvmarm@...ts.linux.dev
Cc: Catalin Marinas <catalin.marinas@....com>, Marc Zyngier <maz@...nel.org>,
 Will Deacon <will@...nel.org>, James Morse <james.morse@....com>,
 Oliver Upton <oliver.upton@...ux.dev>,
 Suzuki K Poulose <suzuki.poulose@....com>, Zenghui Yu
 <yuzenghui@...wei.com>, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, Joey Gouly <joey.gouly@....com>,
 Alexandru Elisei <alexandru.elisei@....com>,
 Christoffer Dall <christoffer.dall@....com>, Fuad Tabba <tabba@...gle.com>,
 linux-coco@...ts.linux.dev,
 Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>,
 Shanker Donthineni <sdonthineni@...dia.com>, Alper Gun
 <alpergun@...gle.com>, "Aneesh Kumar K . V" <aneesh.kumar@...nel.org>
Subject: Re: [PATCH v7 12/45] arm64: RME: Allocate/free RECs to match vCPUs

Hi Steven,

On 3/8/25 1:43 AM, Steven Price wrote:
> On 03/03/2025 07:08, Gavin Shan wrote:
>> On 2/14/25 2:13 AM, Steven Price wrote:
>>> The RMM maintains a data structure known as the Realm Execution Context
>>> (or REC). It is similar to struct kvm_vcpu and tracks the state of the
>>> virtual CPUs. KVM must delegate memory and request the structures are
>>> created when vCPUs are created, and suitably tear down on destruction.
>>>
>>> RECs must also be supplied with addition pages - auxiliary (or AUX)
>>> granules - for storing the larger registers state (e.g. for SVE). The
>>> number of AUX granules for a REC depends on the parameters with which
>>> the Realm was created - the RMM makes this information available via the
>>> RMI_REC_AUX_COUNT call performed after creating the Realm Descriptor
>>> (RD).
>>>
>>> Note that only some of register state for the REC can be set by KVM, the
>>> rest is defined by the RMM (zeroed). The register state then cannot be
>>> changed by KVM after the REC is created (except when the guest
>>> explicitly requests this e.g. by performing a PSCI call). The RMM also
>>> requires that the VMM creates RECs in ascending order of the MPIDR.
>>>
>>> See Realm Management Monitor specification (DEN0137) for more
>>> information:
>>> https://developer.arm.com/documentation/den0137/
>>>
>>> Signed-off-by: Steven Price <steven.price@....com>
>>> ---
>>> Changes since v6:
>>>    * Avoid reporting the KVM_ARM_VCPU_REC feature if the guest isn't a
>>>      realm guest.
>>>    * Support host page size being larger than RMM's granule size when
>>>      allocating/freeing aux granules.
>>> Changes since v5:
>>>    * Separate the concept of vcpu_is_rec() and
>>>      kvm_arm_vcpu_rec_finalized() by using the KVM_ARM_VCPU_REC feature as
>>>      the indication that the VCPU is a REC.
>>> Changes since v2:
>>>    * Free rec->run earlier in kvm_destroy_realm() and adapt to previous
>>> patches.
>>> ---
>>>    arch/arm64/include/asm/kvm_emulate.h |   7 ++
>>>    arch/arm64/include/asm/kvm_host.h    |   3 +
>>>    arch/arm64/include/asm/kvm_rme.h     |  18 +++
>>>    arch/arm64/kvm/arm.c                 |  13 +-
>>>    arch/arm64/kvm/reset.c               |  11 ++
>>>    arch/arm64/kvm/rme.c                 | 179 +++++++++++++++++++++++++++
>>>    6 files changed, 229 insertions(+), 2 deletions(-)
>>>
>>
>> With the following one comment addressed:
>>
>> Reviewed-by: Gavin Shan <gshan@...hat.com>
>>
>> [...]
>>
>>>      /*
>>> diff --git a/arch/arm64/include/asm/kvm_rme.h b/arch/arm64/include/
>>> asm/kvm_rme.h
>>> index 698bb48a8ae1..5db377943db4 100644
>>> --- a/arch/arm64/include/asm/kvm_rme.h
>>> +++ b/arch/arm64/include/asm/kvm_rme.h
>>> @@ -6,6 +6,7 @@
>>>    #ifndef __ASM_KVM_RME_H
>>>    #define __ASM_KVM_RME_H
>>>    +#include <asm/rmi_smc.h>
>>>    #include <uapi/linux/kvm.h>
>>>      /**
>>> @@ -65,6 +66,21 @@ struct realm {
>>>        unsigned int ia_bits;
>>>    };
>>>    +/**
>>> + * struct realm_rec - Additional per VCPU data for a Realm
>>> + *
>>> + * @mpidr: MPIDR (Multiprocessor Affinity Register) value to identify
>>> this VCPU
>>> + * @rec_page: Kernel VA of the RMM's private page for this REC
>>> + * @aux_pages: Additional pages private to the RMM for this REC
>>> + * @run: Kernel VA of the RmiRecRun structure shared with the RMM
>>> + */
>>> +struct realm_rec {
>>> +    unsigned long mpidr;
>>> +    void *rec_page;
>>> +    struct page *aux_pages[REC_PARAMS_AUX_GRANULES];
>>> +    struct rec_run *run;
>>> +};
>>> +
>>
>> REC_PARAMS_AUX_GRANULES represents the maximal number of the auxiliary
>> granules.
>> Since the base page size is always larger than or equal to granule size
>> (4KB).
>> The capacity of array @aux_pages[] needs to be REC_PARAMS_AUX_GRANULES.
>> Ideally,
>> the array's size can be computed dynamically and it's allocated in
>> kvm_create_rec().
> 
> This is indeed another example of where pages and granules have got
> mixed. The RMM specification provides a maximum number of granules (and
> there's a similar array in struct rec_params). Here the use of
> REC_PARAMS_AUX_GRANULES is just to keep the structure more simple (no
> dynamic allocation) with the cost of ~128bytes. Obviously if
> PAGE_SIZE>4k then this array could be smaller.
> 

Exactly. The confusion is caused by the mismatched size between page and granule.
We're declaring an array of pages and the array's capacity is REC_PARAMS_AUX_GRANULES.
A comment is needed to explain for now. In long run, I think it's nice to allocate
them dynamically. The buddy allocator allocates 2^N pages and memory waste will be
existing without perfect alignment.

>> Alternatively, to keep the code simple, a comment is needed here to
>> explain why
>> the array's size has been set to REC_PARAMS_AUX_GRANULES.
> 
> Definitely a valid point - this could do with a comment explaining the
> situation.
> 

Ack.

>> An relevant question: Do we plan to support differentiated sizes between
>> page
>> and granule? I had the assumption this feature will be supported in the
>> future
>> after the base model (equal page and granule size) gets merged first.
> 
> Yes I do plan to support it. This series actually has the basic support
> in it already, with an experimental patch at the end enabling that
> support. It "works" but I haven't tested it well and I think some of the
> error handling isn't quite right yet.
> 
> But there's also a bunch more work to be done (like here) to avoid
> over-allocating memory when PAGE_SIZE>4k. E.g. RTTs need an
> sub-allocator so that we don't waste an entire (larger) page on each RTT.
> 

Ok. Frankly I didn't expect it when I reviewed the first patches of this
series. I thought it would be an incremental feature, meaning a separate
series to support it after the base series is accepted. Anyway, it's something
I need to review on the next respin (v8) if it's going to be supported
together with the base functionalities :-)

Thanks,
Gavin

Powered by blists - more mailing lists