On 29/01/2025 23:25, Gavin Shan wrote:

On 12/13/24 1:55 AM, Steven Price wrote:

quoted

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.

quoted

  #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