Re: [PATCH 3/3] KVM: arm64: Skip break phase when we have FEAT_BBM level 2

[Oliver Upton]

On Sun, Jun 04, 2023 at 09:23:39AM +0100, Marc Zyngier wrote:
> On Fri, 02 Jun 2023 18:01:47 +0100, Colton Lewis <coltonlewis@xxxxxxxxxx> wrote:
> > +static bool stage2_try_make_pte(const struct kvm_pgtable_visit_ctx *ctx, struct kvm_s2_mmu *mmu, kvm_pte_t new)
> >  {
> >  	struct kvm_pgtable_mm_ops *mm_ops = ctx->mm_ops;
> > 
> > -	WARN_ON(!stage2_pte_is_locked(*ctx->ptep));
> > +	if (!stage2_has_bbm_level2())
> > +		WARN_ON(!stage2_pte_is_locked(*ctx->ptep));
> > +
> > +	if (!stage2_try_set_pte(ctx, new))
> > +		return false;
> > +
> > +	if (kvm_pte_table(ctx->old, ctx->level))
> > +		kvm_call_hyp(__kvm_tlb_flush_vmid, mmu);
> > +	else if (kvm_pte_valid(ctx->old) && !stage2_pte_perms_equal(ctx->old, new))
> > +		kvm_call_hyp(__kvm_tlb_flush_vmid_ipa_nsh, mmu, ctx->addr, ctx->level);
> 
> Why a non-shareable invalidation? Nothing in this code captures the
> rationale for it. What if the permission change was a *restriction* of
> the permission? It should absolutely be global, and not local.

IIRC, Colton was testing largely with permission relaxation, and had
forward progress issues b.c. the stale TLB entry was never invalidated
in response to a permission fault.

Nonetheless, I very much agree with your suggestion. Non-Shareable
invalidations should only be applied after exhausting all other
invalidation requirements for a particular manipulation to the stage-2
tables.

> >
> >  	if (stage2_pte_is_counted(new))
> >  		mm_ops->get_page(ctx->ptep);
> > 
> > -	smp_store_release(ctx->ptep, new);
> > +	return true;
> >  }
> > 
> >  static void stage2_put_pte(const struct kvm_pgtable_visit_ctx *ctx, struct kvm_s2_mmu *mmu,
> > @@ -879,7 +917,8 @@ static int stage2_map_walker_try_leaf(const struct kvm_pgtable_visit_ctx *ctx,
> >  	    stage2_pte_executable(new))
> >  		mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), granule);
> > 
> > -	stage2_make_pte(ctx, new);
> > +	if (!stage2_try_make_pte(ctx, data->mmu, new))
> > +		return -EAGAIN;
> 
> So we don't have forward-progress guarantees anymore? I'm not sure
> this is a change I'm overly fond of.

I'll take the blame for the clunky wording here, though I do not believe
there are any real changes to our forward progress guarantees relative to
the existing code.

Previously, we did the CAS on the break side of things to have a fault
handler 'take ownership' of a PTE. The CAS now needs to move onto the
make end when doing a BBM=2 style manipulation.

Would you rather see something explicitly keyed on the BBM capability
here? Then we could use a helper that implies unconditional success for
BBM!=2 systems.

--
Thanks,
Oliver