Re: [PATCH v2 09/47] KVM: arm64: Avoid BBM when changing only s/w bits in Stage-2 PTE

[Oliver Upton <oliver.upton@linux.dev>]

Hi Fuad,

On Tue, Apr 16, 2024 at 10:56:00AM +0100, Fuad Tabba wrote:
> From: Will Deacon <will@kernel.org>
> 
> Break-before-make (BBM) can be expensive, as transitioning via an
> invalid mapping (i.e. the "break" step) requires the completion of TLB
> invalidation and can also cause other agents to fault concurrently on
> the invalid mapping.
> 
> Since BBM is not required when changing only the software bits of a PTE,
> avoid the sequence in this case and just update the PTE directly.
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  arch/arm64/kvm/hyp/pgtable.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 67647b853c9b..30a9a503c477 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -979,6 +979,21 @@ static int stage2_map_walker_try_leaf(const struct kvm_pgtable_visit_ctx *ctx,
>  	if (!stage2_pte_needs_update(ctx->old, new))
>  		return -EAGAIN;
>  
> +	/* If we're only changing software bits, then store them and go! */
> +	if (!kvm_pgtable_walk_shared(ctx) &&
> +	    !((ctx->old ^ new) & ~KVM_PTE_LEAF_ATTR_HI_SW)) {
> +		bool old_is_counted = stage2_pte_is_counted(ctx->old);
> +
> +		if (old_is_counted != stage2_pte_is_counted(new)) {
> +			if (old_is_counted)
> +				mm_ops->put_page(ctx->ptep);
> +			else
> +				mm_ops->get_page(ctx->ptep);
> +		}
> +		WRITE_ONCE(*ctx->ptep, new);

IIUC, you're using a naked WRITE_ONCE() to avoid a error path that
cleans up the reference count. That's fine, but can you change this to a
WARN_ON_ONCE(stage2_try_set_pte())? Should this ever be changed to
become a shared walk w/o gracefully handling a cleanup here I'd hope the
warning would catch someone's attention.

-- 
Thanks,
Oliver