Re: [PATCH 16/20] KVM: arm64: Add __pkvm_host_split_guest HVC

[Vincent Donnefort <vdonnefort@google.com>]

On Mon, Sep 07, 2026 at 06:39:49PM +0100, Wei-Lin Chang wrote:
> On Mon, Aug 03, 2026 at 11:09:00AM +0100, Vincent Donnefort wrote:
> 
> [...]
> 
> > diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> > index 41a8687938eb..355573d53fed 100644
> > --- a/arch/arm64/include/asm/kvm_pgtable.h
> > +++ b/arch/arm64/include/asm/kvm_pgtable.h
> > @@ -824,8 +824,7 @@ int kvm_pgtable_stage2_flush(struct kvm_pgtable *pgt, u64 addr, u64 size);
> >   * kvm_pgtable_stage2_split() is best effort: it tries to break as many
> >   * blocks in the input range as allowed by @mc_capacity.
> 
> Maybe also adjust the comment and remove @mc_capacity while changing
> this, looks like it was included by mistake.
> 
> >   */
> > -int kvm_pgtable_stage2_split(struct kvm_pgtable *pgt, u64 addr, u64 size,
> > -			     struct kvm_mmu_memory_cache *mc);
> > +int kvm_pgtable_stage2_split(struct kvm_pgtable *pgt, u64 addr, u64 size, void *mc);
> >  
> 
> [...]
> 
> > +static int host_stage2_split_gfn_meta(phys_addr_t phys, u64 ipa, u64 size, struct pkvm_hyp_vm *vm)
> > +{
> > +	pkvm_handle_t handle;
> > +	kvm_pte_t pte;
> > +	u64 gfn, end;
> > +	s8 level;
> > +	int ret;
> > +
> > +	ret = kvm_pgtable_get_leaf(&host_mmu.pgt, phys, &pte, &level);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (kvm_granule_size(level) != size)
> > +		return -EINVAL;
> > +
> > +	ret = host_stage2_decode_gfn_meta(pte, &handle, &gfn);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (handle != vm->kvm.arch.pkvm.handle || gfn != (ipa >> PAGE_SHIFT))
> > +		return -EINVAL;
> > +
> > +	end = phys + size;
> > +	while (phys < end) {
> > +		u64 meta = host_stage2_encode_gfn_meta(vm, gfn);
> > +		kvm_pte_t annotation = FIELD_PREP(KVM_HOST_DONATION_PTE_OWNER_MASK, PKVM_ID_GUEST) |
> > +				       FIELD_PREP(KVM_HOST_DONATION_PTE_EXTRA_MASK, meta);
> > +
> > +		ret = host_stage2_try(kvm_pgtable_stage2_annotate, &host_mmu.pgt,
> > +				      phys, PAGE_SIZE, &host_s2_pool,
> > +				      KVM_HOST_INVALID_PTE_TYPE_DONATION, annotation);
> > +		if (WARN_ON(ret))
> > +			return ret;
> > +
> > +		phys += PAGE_SIZE;
> > +		gfn++;
> 
> Annotating every PTE is required here because patch 2's
> stage2_map_prefault_idmap() copies the same annotation to each PTE, so
> the resulting PTEs all contain the same gfn.
> 
> In other words patch 2 simply leaves wrong annotations in this case, and
> we make up for it here.
> 
> We can't just remove patch 2 because a HYP owned block needs to be
> broken down with HYP owned annotations when a single page is donated
> back to the host.
> 
> With this, would it be a better design to have an annotation-aware
> helper that splits a host block properly including the annotations, and
> call it whenever pKVM annotates or maps host stage-2?

I ended-up with that solution because it is difficult to draw the line between
pulling pKVM specific knowledge in pgtable.c and reusing pgtable.c private
functions in mem_protect.c. So I wanted a mem_protect.c function that can split
the annotations, but without having to touch pgtable.c. Indeed, that isn't the
most optimal.

Looking again at stage2_split_walker(), perhaps pgtable.c could export only a
single additional function to allow implementing a pkvm version of stage2_split.
This stage2 split can then be used for both guest and host.

Let me see if I can optimise that

Eventually part of the "pkvm_stage2_split()" could be reused by
stage2_map_prefault_idmap() I'll see if that's worth it.

> 
> >  	}
> >  
> > -	*gfn = FIELD_GET(KVM_HOST_PTE_OWNER_GUEST_GFN_MASK, meta);
> >  	return 0;
> >  }
> >  
> 
> [...]
> 
> > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > index c4ebae0544d4..986b5a19b07a 100644
> > --- a/arch/arm64/kvm/hyp/pgtable.c
> > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > @@ -1537,9 +1537,10 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
> >  			       enum kvm_pgtable_walk_flags visit)
> >  {
> >  	struct kvm_pgtable_mm_ops *mm_ops = ctx->mm_ops;
> > -	struct kvm_mmu_memory_cache *mc = ctx->arg;
> > -	struct kvm_s2_mmu *mmu;
> > +	struct stage2_map_data *data = ctx->arg;
> >  	kvm_pte_t pte = ctx->old, new, *childp;
> > +	struct kvm_s2_mmu *mmu = data->mmu;
> > +	void *mc = data->memcache;
> >  	enum kvm_pgtable_prot prot;
> >  	s8 level = ctx->level;
> >  	bool force_pte;
> > @@ -1554,30 +1555,37 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
> >  	if (!kvm_pte_valid(pte))
> >  		return 0;
> >  
> > -	nr_pages = stage2_block_get_nr_page_tables(level);
> > -	if (nr_pages < 0)
> > -		return nr_pages;
> > -
> > -	if (mc->nobjs >= nr_pages) {
> > -		/* Build a tree mapped down to the PTE granularity. */
> > +	if (unlikely(is_protected_kvm_enabled())) {
> > +		/* pKVM only supports splitting PMD-level blocks */
> > +		if (level != KVM_PGTABLE_LAST_LEVEL - 1)
> > +			return -EINVAL;
> >  		force_pte = true;
> >  	} else {
> > -		/*
> > -		 * Don't force PTEs, so create_unlinked() below does
> > -		 * not populate the tree up to the PTE level. The
> > -		 * consequence is that the call will require a single
> > -		 * page of level 2 entries at level 1, or a single
> > -		 * page of PTEs at level 2. If we are at level 1, the
> > -		 * PTEs will be created recursively.
> > -		 */
> > -		force_pte = false;
> > -		nr_pages = 1;
> > +		struct kvm_mmu_memory_cache *host_mc = mc;
> > +
> > +		nr_pages = stage2_block_get_nr_page_tables(level);
> > +		if (nr_pages < 0)
> > +			return nr_pages;
> > +
> > +		if (host_mc->nobjs >= nr_pages) {
> > +			/* Build a tree mapped down to the PTE granularity. */
> > +			force_pte = true;
> > +		} else if (host_mc->nobjs) {
> > +			/*
> > +			 * Don't force PTEs, so create_unlinked() below does
> > +			 * not populate the tree up to the PTE level. The
> > +			 * consequence is that the call will require a single
> > +			 * page of level 2 entries at level 1, or a single
> > +			 * page of PTEs at level 2. If we are at level 1, the
> > +			 * PTEs will be created recursively.
> > +			 */
> > +			force_pte = false;
> > +			nr_pages = 1;
> > +		} else {
> > +			return -ENOMEM;
> > +		}
> >  	}
> >  
> > -	if (mc->nobjs < nr_pages)
> > -		return -ENOMEM;
> > -
> > -	mmu = container_of(mc, struct kvm_s2_mmu, split_page_cache);
> >  	phys = kvm_pte_to_phys(pte);
> >  	prot = kvm_pgtable_stage2_pte_prot(pte);
> 
> I feel like this change is trying too hard to make the pKVM case fit
> in stage2_split_walker(). The assumptions differ by much: the original
> usage assumes kvm_mmu_memory_cache, can potentially split more than one
> level, and makes decisions base on the memcache's reserve.
> 
> Perhaps create another function for pKVM?

Looking at this version, the pKVM case is rather small, hence reusing the same
function made sense to me.

A completely separate function is actually what we have in Android kernels at
the moment.

Anyway, as per the discussion above, I'll probably try to create a pKVM specific
version of the split.

> 
> Thanks,
> Wei-Lin Chang
> 
> [...]