On 29/01/2025 23:25, Gavin Shan wrote:
> On 12/13/24 1:55 AM, Steven Price wrote:
>> Each page within the protected region of the realm guest can be marked
>> as either RAM or EMPTY. Allow the VMM to control this before the guest
>> has started and provide the equivalent functions to change this (with
>> the guest's approval) at runtime.
>>
>> When transitioning from RIPAS RAM (1) to RIPAS EMPTY (0) the memory is
>> unmapped from the guest and undelegated allowing the memory to be reused
>> by the host. When transitioning to RIPAS RAM the actual population of
>> the leaf RTTs is done later on stage 2 fault, however it may be
>> necessary to allocate additional RTTs to allow the RMM track the RIPAS
>> for the requested range.
>>
>> When freeing a block mapping it is necessary to temporarily unfold the
>> RTT which requires delegating an extra page to the RMM, this page can
>> then be recovered once the contents of the block mapping have been
>> freed.
>>
>> Signed-off-by: Steven Price <steven.price@xxxxxxx>
>> ---
>> Changes from v5:
>> * Adapt to rebasing.
>> * Introduce find_map_level()
>> * Rename some functions to be clearer.
>> * Drop the "spare page" functionality.
>> Changes from v2:
>> * {alloc,free}_delegated_page() moved from previous patch to this one.
>> * alloc_delegated_page() now takes a gfp_t flags parameter.
>> * Fix the reference counting of guestmem pages to avoid leaking memory.
>> * Several misc code improvements and extra comments.
>> ---
>> arch/arm64/include/asm/kvm_rme.h | 17 ++
>> arch/arm64/kvm/mmu.c | 8 +-
>> arch/arm64/kvm/rme.c | 411 +++++++++++++++++++++++++++++++
>> 3 files changed, 433 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_rme.h b/arch/arm64/include/
>> asm/kvm_rme.h
>> index be64b749fcac..4e7758f0e4b5 100644
>> --- a/arch/arm64/include/asm/kvm_rme.h
>> +++ b/arch/arm64/include/asm/kvm_rme.h
>> @@ -92,6 +92,15 @@ void kvm_realm_destroy_rtts(struct kvm *kvm, u32
>> ia_bits);
>> int kvm_create_rec(struct kvm_vcpu *vcpu);
>> void kvm_destroy_rec(struct kvm_vcpu *vcpu);
>> +void kvm_realm_unmap_range(struct kvm *kvm,
>> + unsigned long ipa,
>> + u64 size,
>> + bool unmap_private);
>> +int realm_set_ipa_state(struct kvm_vcpu *vcpu,
>> + unsigned long addr, unsigned long end,
>> + unsigned long ripas,
>> + unsigned long *top_ipa);
>> +
>
> The declaration of realm_set_ipa_state() is unnecessary since its scope has
> been limited to rme.c
Ack, the function can be static too.
>> #define RMM_RTT_BLOCK_LEVEL 2
>> #define RMM_RTT_MAX_LEVEL 3
>> @@ -110,4 +119,12 @@ static inline unsigned long
>> rme_rtt_level_mapsize(int level)
>> return (1UL << RMM_RTT_LEVEL_SHIFT(level));
>> }
>> +static inline bool realm_is_addr_protected(struct realm *realm,
>> + unsigned long addr)
>> +{
>> + unsigned int ia_bits = realm->ia_bits;
>> +
>> + return !(addr & ~(BIT(ia_bits - 1) - 1));
>> +}
>> +
>> #endif
>
> The check on the specified address to determine its range seems a bit
> complicated
> to me, it can be simplified like below. Besides, it may be a good idea
> to rename
> it to have the prefix "kvm_realm_".
>
> static inline bool kvm_realm_is_{private | protected}_address(struct
> realm *realm,
> unsigned long addr)
> {
> return !(addr & BIT(realm->ia_bits - 1));
> }
Ack
> A question related to the terms used in this series to describe a
> granule's state:
> "protected" or "private", "unprotected" or "shared". Those terms are all
> used in
> the function names of this series. I guess it would be nice to unify so
> that
> "private" and "shared" to be used, which is consistent to the terms used by
> guest-memfd. For example, kvm_realm_is_protected_address() can be
> renamed to
> kvm_realm_is_private_address().
Happy with the rename here. More generally it's a little awkward because
the RMM spec does refer to protected/unprotected (e.g.
RMI_RTT_MAP_UNPROTECTED). So there's always a choice between aligning
with the RMM spec or aligning with guest-memfd.
Thanks,
Steve
