Re: [PATCH v15 15/37] KVM: arm64: CCA: Handle RMI_EXIT_RIPAS_CHANGE

Suzuki K Poulose <[email protected]> Mon, 3 Aug 2026 16:30:02 +0100
Newsgroups dev.linux.lists.kvmarm,dev.linux.lists.linux-coco,org.infradead.lists.linux-arm-kernel,org.kernel.vger.kvm,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
On 03/08/2026 15:48, Marc Zyngier wrote:
> On Mon, 03 Aug 2026 15:04:32 +0100,
> Steven Price <[email protected]> wrote:
> 
> [...]
> 
>>>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>>>> index cd06881c1497..dcc2ab08d0e4 100644
>>>> --- a/arch/arm64/kvm/mmu.c
>>>> +++ b/arch/arm64/kvm/mmu.c
>>>> @@ -319,6 +319,7 @@ static void invalidate_icache_guest_page(void *va, size_t size)
>>>>    * @start: The intermediate physical base address of the range to unmap
>>>>    * @size:  The size of the area to unmap
>>>>    * @may_block: Whether or not we are permitted to block
>>>> + * @only_shared: If true then protected mappings should not be unmapped
>>>>    *
>>>
>>> I don't understand the need for this additional argument. Given that
>>> CCA imposes that shared and private are in non-overlapping ranges, why
>>> is it necessary to introduce this at the core of the S2 management
>>> code?
>>>
>>> I'd expect that the CCA code could simply work out what range it needs
>>> to run on and keep the API intact.
>>>
>>>>    * Clear a range of stage-2 mappings, lowering the various ref-counts.  Must
>>>>    * be called while holding mmu_lock (unless for freeing the stage2 pgd before
>>>> @@ -326,7 +327,7 @@ static void invalidate_icache_guest_page(void *va, size_t size)
>>>>    * with things behind our backs.
>>>>    */
>>>>   static void __unmap_stage2_range(struct kvm_s2_mmu *mmu, phys_addr_t start, u64 size,
>>>> -				 bool may_block)
>>>> +				 bool may_block, bool only_shared)
>>>>   {
>>>>   	struct kvm *kvm = kvm_s2_mmu_to_kvm(mmu);
>>>>   	phys_addr_t end = start + size;
>>>
>>> So what is the *actual* change?
>>
>> It looks like I've screwed up what goes in which patch. The real change
>> is in patch 22 where this 'only_shared' property gets passed down to
>> kvm_realm_unmap_range(). There it's used to decide whether the private
>> range should be unmapped or not.
>>
>> The reason for this is kvm_unmap_gfn_range() which has a 'attr_filter'
>> member of 'kvm_gfn_range' which can specify KVM_FILTER_PRIVATE. Which is
>> used in __kvm_gmem_set_attributes() so choose whether to invalidate the
>> private or shared part of a gmem range.
> 
> But you can readily distinguish the private/shared part of the address
> range by looking at the top IPA bit, right? And from there, you should
> be in a position to decide whether to call __unmap_stage2_range() on a
> per IPA range, rather than making this information trickle all over
> the place.

Yes, we can for an "IPA". However there are two different things
at play here. There is KVM generic "GPA" (as manitained by the slot)
and where the backend maps it in stage2 "IPA". For Arm CCA, for shared
GPAs, the "IPA" is an alias of the GPA. But, here we get the GPA from
the core layer.

There is a shared/private state associated with each "GPA" (GFN) as
maintained by the Guest_memfd for each GPA in a memslot backed by it.
When a change occurs in the backing store (e.g. set GPA to private,
punch a hole in GPA, destroy a given range of GPAs), an invalidation
needs to be triggered in the Stage2. Now, given different architectures
have different ways to deal with private vs shared GPAs, (e.g.,
on CCA, shared GPAs are mapped in an aliased IPA) and the private GPAs
are special, Guest_memfd tells you which ones need invalidation, using
the filter. This filter then gets handled by the backends (CCA unmaps
aliased address for Shared filter). Ideally this filter should be passed
down to the core routine, so that it can be handled by the backends
appropriately (similar to the range->mayblock).

> 
> I.e don't hack into the API. Use the information you already have to
> decide whether you need to use the existing API at all.
> 
> If that cannot be done for whatever reason, then you need to indirect
> the call with a set of operations that leave the existing code
> untouched.

Please let us know if you think we need to explore this ^ ?

Cheers
Suzuki

> 
> 	M.
>