Message-ID: <20250618133355.2af1dcc4@donnerap.manchester.arm.com>
Date: Wed, 18 Jun 2025 13:33:55 +0100
From: Andre Przywara <andre.przywara@....com>
To: Steven Price <steven.price@....com>
Cc: kvm@...r.kernel.org, kvmarm@...ts.linux.dev, 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>, Gavin Shan <gshan@...hat.com>,
 Shanker Donthineni <sdonthineni@...dia.com>, Alper Gun
 <alpergun@...gle.com>, "Aneesh Kumar K . V" <aneesh.kumar@...nel.org>, Emi
 Kisanuki <fj0570is@...itsu.com>
Subject: Re: [PATCH v9 15/43] arm64: RME: Allow VMM to set RIPAS

On Wed, 11 Jun 2025 11:48:12 +0100
Steven Price <steven.price@....com> wrote:

Hi Steven,

one build error below, on my machine (with GCC10):

> 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@....com>
> ---
> Changes from v8:
>  * Propagate the 'may_block' flag to allow conditional calls to
>    cond_resched_rwlock_write().
>  * Introduce alloc_rtt() to wrap alloc_delegated_granule() and
>    kvm_account_pgtable_pages() and use when allocating RTTs.
>  * Code reorganisation to allow init_ipa_state and set_ipa_state to
>    share a common ripas_change() function,
>  * Other minor changes following review.
> Changes from v7:
>  * Replace use of "only_shared" with the upstream "attr_filter" field
>    of struct kvm_gfn_range.
>  * Clean up the logic in alloc_delegated_granule() for when to call
>    kvm_account_pgtable_pages().
>  * Rename realm_destroy_protected_granule() to
>    realm_destroy_private_granule() to match the naming elsewhere. Also
>    fix the return codes in the function to be descriptive.
>  * Several other minor changes to names/return codes.
> Changes from v6:
>  * Split the code dealing with the guest triggering a RIPAS change into
>    a separate patch, so this patch is purely for the VMM setting up the
>    RIPAS before the guest first runs.
>  * Drop the useless flags argument from alloc_delegated_granule().
>  * Account RTTs allocated for a guest using kvm_account_pgtable_pages().
>  * Deal with the RMM granule size potentially being smaller than the
>    host's PAGE_SIZE. Although note alloc_delegated_granule() currently
>    still allocates an entire host page for every RMM granule (so wasting
>    memory when PAGE_SIZE>4k).
> 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 |   6 +
>  arch/arm64/kvm/mmu.c             |   8 +-
>  arch/arm64/kvm/rme.c             | 447 +++++++++++++++++++++++++++++++
>  3 files changed, 458 insertions(+), 3 deletions(-)
> 

[ ... ]

> diff --git a/arch/arm64/kvm/rme.c b/arch/arm64/kvm/rme.c
> index 25705da6f153..fe75c41d6ac3 100644
> --- a/arch/arm64/kvm/rme.c
> +++ b/arch/arm64/kvm/rme.c

[ ... ]

> @@ -318,6 +619,140 @@ static int realm_create_rd(struct kvm *kvm)
>  	return r;
>  }
>  
> +static void realm_unmap_private_range(struct kvm *kvm,
> +				      unsigned long start,
> +				      unsigned long end,
> +				      bool may_block)
> +{
> +	struct realm *realm = &kvm->arch.realm;
> +	unsigned long next_addr, addr;
> +	int ret;
> +
> +	for (addr = start; addr < end; addr = next_addr) {
> +		ret = realm_unmap_private_page(realm, addr, &next_addr);
> +
> +		if (ret)
> +			break;
> +
> +		if (may_block)
> +			cond_resched_rwlock_write(&kvm->mmu_lock);
> +	}
> +
> +	realm_fold_rtt_level(realm, get_start_level(realm) + 1,
> +			     start, end);
> +}
> +
> +void kvm_realm_unmap_range(struct kvm *kvm, unsigned long start,
> +			   unsigned long size, bool unmap_private,
> +			   bool may_block)
> +{
> +	unsigned long end = start + size;
> +	struct realm *realm = &kvm->arch.realm;
> +
> +	end = min(BIT(realm->ia_bits - 1), end);
> +
> +	if (!kvm_realm_is_created(kvm))
> +		return;
> +
> +	realm_unmap_shared_range(kvm, find_map_level(realm, start, end),
> +				 start, end, may_block);
> +	if (unmap_private)
> +		realm_unmap_private_range(kvm, start, end, may_block);
> +}
> +
> +enum ripas_action {
> +	RIPAS_INIT,
> +	RIPAS_SET,
> +};
> +
> +static int ripas_change(struct kvm *kvm,
> +			struct kvm_vcpu *vcpu,
> +			unsigned long ipa,
> +			unsigned long end,
> +			enum ripas_action action,
> +			unsigned long *top_ipa)
> +{
> +	struct realm *realm = &kvm->arch.realm;
> +	phys_addr_t rd_phys = virt_to_phys(realm->rd);
> +	phys_addr_t rec_phys;
> +	struct kvm_mmu_memory_cache *memcache = NULL;
> +	int ret = 0;
> +
> +	if (vcpu) {
> +		rec_phys = virt_to_phys(vcpu->arch.rec.rec_page);
> +		memcache = &vcpu->arch.mmu_page_cache;
> +
> +		WARN_ON(action != RIPAS_SET);
> +	} else {
> +		WARN_ON(action != RIPAS_INIT);
> +	}
> +
> +	while (ipa < end) {
> +		unsigned long next;
> +
> +		switch (action) {
> +		case RIPAS_INIT:
> +			ret = rmi_rtt_init_ripas(rd_phys, ipa, end, &next);
> +			break;
> +		case RIPAS_SET:
> +			ret = rmi_rtt_set_ripas(rd_phys, rec_phys, ipa, end,
> +						&next);
> +			break;
> +		}
> +
> +		switch (RMI_RETURN_STATUS(ret)) {
> +		case RMI_SUCCESS:
> +			ipa = next;
> +			break;
> +		case RMI_ERROR_RTT:
> +			int err_level = RMI_RETURN_INDEX(ret);

This breaks the build on GCC <= v10:

/src/linux/arch/arm64/kvm/rme.c: In function ‘ripas_change’:
/src/linux/arch/arm64/kvm/rme.c:1190:4: error: a label can only be part of a statement and a declaration is not a statement
 1190 |    int err_level = RMI_RETURN_INDEX(ret);
      |    ^~~
/src/linux/arch/arm64/kvm/rme.c:1191:4: error: expected expression before int’
 1191 |    int level = find_map_level(realm, ipa, end);
      |    ^~~
/src/linux/arch/arm64/kvm/rme.c:1193:21: error: ‘level’ undeclared (first use in this function)
 1193 |    if (err_level >= level)
      |                     ^~~~~

With GCC 11 and later I see this still as a warning when using -Wpedantic,
but it vanishes when also paired with -std=gnu2x.

So either hoist the variable declaration up, or use brackets, this worked
for me as well, and I see it in other places:

		case RMI_ERROR_RTT: {
			....
		}
		default:

Cheers,
Andre


> +			int level = find_map_level(realm, ipa, end);
> +
> +			if (err_level >= level)
> +				return -EINVAL;
> +
> +			ret = realm_create_rtt_levels(realm, ipa,
> err_level,
> +						      level, memcache);
> +			if (ret)
> +				return ret;
> +			/* Retry with the RTT levels in place */
> +			break;
> +		default:
> +			WARN_ON(1);
> +			return -ENXIO;
> +		}
> +	}
> +
> +	if (top_ipa)
> +		*top_ipa = ipa;
> +
> +	return 0;
> +}
> +
> +static int realm_init_ipa_state(struct kvm *kvm,
> +				unsigned long ipa,
> +				unsigned long end)
> +{
> +	return ripas_change(kvm, NULL, ipa, end, RIPAS_INIT, NULL);
> +}
> +
> +static int kvm_init_ipa_range_realm(struct kvm *kvm,
> +				    struct arm_rme_init_ripas *args)
> +{
> +	gpa_t addr, end;
> +
> +	addr = args->base;
> +	end = addr + args->size;
> +
> +	if (end < addr)
> +		return -EINVAL;
> +
> +	if (kvm_realm_state(kvm) != REALM_STATE_NEW)
> +		return -EPERM;
> +
> +	return realm_init_ipa_state(kvm, addr, end);
> +}
> +
>  /* Protects access to rme_vmid_bitmap */
>  static DEFINE_SPINLOCK(rme_vmid_lock);
>  static unsigned long *rme_vmid_bitmap;
> @@ -441,6 +876,18 @@ int kvm_realm_enable_cap(struct kvm *kvm, struct
> kvm_enable_cap *cap) case KVM_CAP_ARM_RME_CREATE_REALM:
>  		r = kvm_create_realm(kvm);
>  		break;
> +	case KVM_CAP_ARM_RME_INIT_RIPAS_REALM: {
> +		struct arm_rme_init_ripas args;
> +		void __user *argp = u64_to_user_ptr(cap->args[1]);
> +
> +		if (copy_from_user(&args, argp, sizeof(args))) {
> +			r = -EFAULT;
> +			break;
> +		}
> +
> +		r = kvm_init_ipa_range_realm(kvm, &args);
> +		break;
> +	}
>  	default:
>  		r = -EINVAL;
>  		break;

Powered by blists - more mailing lists