Re: [PATCH v6 16/43] arm64: RME: Allow VMM to set RIPAS

[Steven Price <steven.price@arm.com>]

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@arm.com>
>> ---
>> 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