Re: [PATCH v16 22/45] KVM: arm64: CCA: Handle RMI_EXIT_RIPAS_CHANGE

[Kohei Enju]

On 09/07 16:58, Gavin Shan wrote:
> Hi Kohei,
> 
> On 9/7/26 3:05 PM, Kohei Enju wrote:
> > Hi Steven,
> > 
> > On 08/03 14:43, Steven Price wrote:
> > > The guest can request that a region of its protected address space is
> > > switched between RIPAS_RAM and RIPAS_EMPTY (and back) using
> > > RSI_IPA_STATE_SET. This causes a guest exit with the
> > > RMI_EXIT_RIPAS_CHANGE code. We treat this as a request to convert a
> > > protected region to unprotected (or back), exiting to the VMM to make
> > > the necessary changes to the guest_memfd and memslot mappings. On the
> > > next entry the RIPAS changes are committed by making RMI_RTT_SET_RIPAS
> > > calls.
> > > 
> > > The VMM may wish to reject the RIPAS change requested by the guest. For
> > > now it can only do this by no longer scheduling the VCPU as we don't
> > > currently have a usecase for returning that rejection to the guest, but
> > > by postponing the RMI_RTT_SET_RIPAS changes to entry we leave the door
> > > open for adding a new ioctl in the future for this purpose.
> > > 
> > > Signed-off-by: Steven Price <steven.price@xxxxxxx>
> > > ---
> > > Changes since v15:
> > >   * Propagate negative error returns.
> > > Changes since v14:
> > >   * Use addition rather than bitwise OR for adding the shared_bit in
> > >     realm_unmap_shared_range(), this handles the case where the region
> > >     includes the last address (which means 'end' already has the bit
> > >     set).
> > > Changes since v13:
> > >   * Switch to the new RMI_RTT_UNPROT_UNMAP range-based API.
> > >   * Drop ugly hack for RMM bug which errored when the RIPAS was already
> > >     set to the desired value.
> > > Changes since v12:
> > >   * Switch to the new RMM v2.0 RMI_RTT_DATA_UNMAP which can unmap an
> > >     address range.
> > > Changes since v11:
> > >   * Combine the "Allow VMM to set RIPAS" patch into this one to avoid
> > >     adding functions before they are used.
> > >   * Drop the CAP for setting RIPAS and adapt to changes from previous
> > >     patches.
> > > Changes since v10:
> > >   * Add comment explaining the assignment of rec->run->exit.ripas_base in
> > >     kvm_complete_ripas_change().
> > > Changes since v8:
> > >   * Make use of ripas_change() from a previous patch to implement
> > >     realm_set_ipa_state().
> > >   * Update exit.ripas_base after a RIPAS change so that, if instead of
> > >     entering the guest we exit to user space, we don't attempt to repeat
> > >     the RIPAS change (triggering an error from the RMM).
> > > Changes since v7:
> > >   * Rework the loop in realm_set_ipa_state() to make it clear when the
> > >     'next' output value of rmi_rtt_set_ripas() is used.
> > > New patch for v7: The code was previously split awkwardly between two
> > > other patches.
> > > ---
> > >   arch/arm64/include/asm/kvm_rmi.h |   6 +
> > >   arch/arm64/kvm/mmu.c             |   8 +-
> > >   arch/arm64/kvm/rmi.c             | 473 +++++++++++++++++++++++++++++++
> > >   3 files changed, 484 insertions(+), 3 deletions(-)
> > > 
> > > [...]
> > > +
> > > +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;
> > > +	long ret = 0;
> > > +
> > > +	if (vcpu) {
> > > +		rec_phys = vcpu->arch.rec.rec_phys;
> > > +		memcache = &vcpu->arch.mmu_page_cache;
> > > +
> > > +		WARN_ON(action != RIPAS_SET);
> > > +	} else {
> > > +		WARN_ON(action != RIPAS_INIT);
> > > +	}
> > > +
> > > +	while (ipa < end) {
> > > +		unsigned long next = ~0;
> > > +
> > > +		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;
> > > +		}
> > > +
> > > +		if (ret < 0)
> > > +			goto out;
> > > +
> > > +		switch (RMI_RETURN_STATUS(ret)) {
> > > +		case RMI_SUCCESS:
> > > +			ipa = next;
> > > +			break;
> > > +		case RMI_ERROR_RTT: {
> > > +			int err_level = RMI_RETURN_INDEX(ret);
> > > +			int level = find_map_level(realm, ipa, end);
> > > +
> > 
> > I have been testing this series using TF-RMM from:
> >    https://git.trustedfirmware.org/TF-RMM/tf-rmm.git topics/rmm-v2.0-poc_3
> > 
> > I found that Linux repeatedly issues the same SMC_RMI_RTT_SET_RIPAS call
> > [0], eventually causing a soft lockup [1].
> > 
> > The root cause appears to be an RMM bug fixed by:
> >    https://git.trustedfirmware.org/plugins/gitiles/TF-RMM/tf-rmm.git/+/38c1de0adf66
> > 
> > Without this fix, RMM returns RMI_ERROR_RTT at level 3. In this case,
> > both err_level and level are 3. Since this is RIPAS_SET, the check below
> > is skipped, realm_create_rtt_levels(realm, ipa, 3, 3, ...) is a no-op,
> > and ipa does not advance. The same RMI is therefore retried
> > indefinitely.
> > 
> 
> FYI. This is a known issue, reported previously [1]. Following the discussions
> in the thread will lead to the TF-RMM commit you mentioned.
> 
> [1] https://lore.kernel.org/linux-coco/d0ffbcb5-0cca-4d1e-9788-6b430345fa9b@xxxxxxxxxx/

Hi Gavin,

Thank you for the FYI.
I wasn't aware of that discussions, so I appreciate you pointing it out.

Thanks,
Kohei

> 
> Thanks,
> Gavin
> 
> > > +			/*
> > > +			 * If the operation failed at deeper level than
> > > +			 * what is required for the address range, this
> > > +			 * implies encountering an unexpected entry,
> > > +			 * (e.g., RIPAS_DESTROYED), which the RMM prevents
> > > +			 * us from modifying. This is only applicable for
> > > +			 * RMI_RTT_INIT_RIPAS. All the other requests
> > > +			 * are generated by the Realm and thus RMM should
> > > +			 * be able to allow the transition.
> > > +			 */
> > > +			if (action == RIPAS_INIT && WARN_ON_ONCE(err_level >= level))
> > > +				return -ENXIO;
> > 
> > Although the root cause is an RMM bug, should we also guard RIPAS_SET
> > against this no-progress case?
> > 
> > Thanks,
> > Kohei
> > 
> > [0]
> >    SMC_RSI_VERSION                   10000 > RSI_SUCCESS 10000 10001
> >    SMC_RSI_REALM_CONFIG              830e5000 > RSI_SUCCESS
> >    SMC_RSI_IPA_STATE_SET             80000000 c0000000 1 0
> >    SMC_RMI_RTT_SET_RIPAS             10060182000 10056e1c000 8000c000 c0000000 > RMI_ERROR_RTT 3
> >    SMC_RMI_RTT_SET_RIPAS             10060182000 10056e1c000 8000c000 c0000000 > RMI_ERROR_RTT 3
> >    SMC_RMI_RTT_SET_RIPAS             10060182000 10056e1c000 8000c000 c0000000 > RMI_ERROR_RTT 3
> >    ... (the same SMC_RMI_RTT_SET_RIPAS call is logged repeatedly)
> > 
> > [1]
> >    [  201.945663] CPU: 113 UID: 0 PID: 8093 Comm: kvm-vcpu-0
> >    [...]
> >    [  201.946291] pstate: 61402009 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> >    [  201.946416] pc : arm_smccc_1_2_smc+0x34/0x70
> >    [  201.946430] lr : rmi_smccc_invoke+0xc0/0x108
> >    [  201.946442] sp : ffff8000b59ab5e0
> >    [  201.946448] x29: ffff8000b59ab690 x28: 000000008313e000 x27: 0000000000000003
> >    [  201.946510] x26: 000008241cb3c000 x25: ffffffffffffffff x24: ffff8000b59ab830
> >    [  201.946569] x23: ffff8000b59ab7c0 x22: 0000082419daa000 x21: 000008241cb3c000
> >    [  201.946701] x20: 00000000000000fb x19: ffff8000b59ab5f8 x18: 0000000000000000
> >    [  201.946740] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
> >    [  201.947097] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
> >    [  201.947127] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
> >    [  201.947550] x8 : 0000000000000000 x7 : 0000000000000000 x6 : 0000000000000000
> >    [  201.947783] x5 : 0000000000000000 x4 : 00000000c0000000 x3 : 0000000000000000
> >    [  201.947802] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000000304
> >    [  201.948305] Call trace:
> >    [  201.948438]  arm_smccc_1_2_smc+0x34/0x70 (P)
> >    [  201.948484]  rmi_sro_execute+0x24/0xd0
> >    [  201.948571]  rmi_rtt_set_ripas.constprop.0+0x6c/0xb0
> >    [  201.948698]  ripas_change+0xc4/0x1e8
> >    [  201.948889]  kvm_rec_handle_request+0x190/0x308
> >    [  201.948943]  check_vcpu_requests+0xcc/0x4f8
> >    [  201.948960]  kvm_arch_vcpu_ioctl_run+0x208/0x7c0
> >    [  201.948986]  kvm_vcpu_ioctl+0x174/0xac8
> >    [  201.949062]  __arm64_sys_ioctl+0xb4/0x118
> >    [  201.949089]  invoke_syscall.constprop.0+0xa8/0x100
> >    [  201.949147]  do_el0_svc+0xb8/0xc8
> >    [  201.949159]  el0_svc+0x48/0x1f8
> >    [  201.949169]  el0t_64_sync_handler+0xa0/0xe8
> >    [  201.949194]  el0t_64_sync+0x1ac/0x1b0
> > 
> > > +
> > > +			ret = realm_create_rtt_levels(realm, ipa, err_level,
> > > +						      level, memcache);
> > > +			if (ret)
> > > +				goto out;
> > > +			/* Retry with the RTT levels in place */
> > > +			break;
> > > +		}
> > > +		default:
> > > +			WARN_ON(1);
> > > +			ret = -ENXIO;
> > > +			goto out;
> > > +		}
> > > +	}
> > > +
> > > +out:
> > > +	if (top_ipa)
> > > +		*top_ipa = ipa;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int realm_set_ipa_state(struct kvm_vcpu *vcpu,
> > > +			       unsigned long start,
> > > +			       unsigned long end,
> > > +			       unsigned long ripas,
> > > +			       unsigned long *top_ipa)
> > > +{
> > > +	struct kvm *kvm = vcpu->kvm;
> > > +	int ret = ripas_change(kvm, vcpu, start, end, RIPAS_SET, top_ipa);
> > > +
> > > +	if (!ret && ripas == RMI_EMPTY && *top_ipa != start)
> > > +		realm_unmap_private_range(kvm, start, *top_ipa, false);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int kvm_complete_ripas_change(struct kvm_vcpu *vcpu)
> > > +{
> > > +	struct kvm *kvm = vcpu->kvm;
> > > +	struct realm_rec *rec = &vcpu->arch.rec;
> > > +	unsigned long base = rec->run->exit.ripas_base;
> > > +	unsigned long top = rec->run->exit.ripas_top;
> > > +	unsigned long ripas = rec->run->exit.ripas_value;
> > > +	unsigned long top_ipa = base;
> > > +	int ret;
> > > +
> > > +	do {
> > > +		kvm_mmu_topup_memory_cache(&vcpu->arch.mmu_page_cache,
> > > +					   kvm_mmu_cache_min_pages(vcpu->arch.hw_mmu));
> > > +		write_lock(&kvm->mmu_lock);
> > > +		ret = realm_set_ipa_state(vcpu, base, top, ripas, &top_ipa);
> > > +		write_unlock(&kvm->mmu_lock);
> > > +
> > > +		if (ret == -ENOMEM) {
> > > +			/* If no progress, then stop */
> > > +			if (top_ipa == base)
> > > +				break;
> > > +			base = top_ipa;
> > > +			continue;
> > > +		}
> > > +
> > > +		if (WARN_RATELIMIT(ret,
> > > +				   "Unable to satisfy RIPAS_CHANGE for %#lx - %#lx, ripas: %#lx\n",
> > > +				   base, top, ripas))
> > > +			break;
> > > +
> > > +		base = top_ipa;
> > > +	} while (base < top);
> > > +
> > > +	/*
> > > +	 * If this function is called again before the REC_ENTER call then
> > > +	 * avoid calling realm_set_ipa_state() again by changing to the value
> > > +	 * of ripas_base for the part that has already been covered. The RMM
> > > +	 * ignores the contains of the rec_exit structure so this doesn't
> > > +	 * affect the RMM.
> > > +	 */
> > > +	rec->run->exit.ripas_base = base;
> > > +
> > > +	return 1;
> > > +}
> > > +
> > >   int kvm_rec_handle_request(struct kvm_vcpu *vcpu)
> > >   {
> > >   	struct realm_rec *rec = &vcpu->arch.rec;
> > > @@ -224,6 +695,8 @@ int kvm_rec_handle_request(struct kvm_vcpu *vcpu)
> > >   					vcpu_get_reg(vcpu, rt);
> > >   		}
> > >   		break;
> > > +	case RMI_EXIT_RIPAS_CHANGE:
> > > +		return kvm_complete_ripas_change(vcpu);
> > >   	default:
> > >   		KVM_BUG(1, vcpu->kvm, "Unhandled realm exit_reason");
> > >   		return -ENXIO;
> > > -- 
> > > 2.43.0
> > > 
> > 
>