Re: [PATCH v4 06/21] KVM: arm64: Add support for stage-2 map()/unmap() in generic page-table

From: Will Deacon <will@kernel.org>
To: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Gavin Shan <gshan@redhat.com>,
	Suzuki Poulose <suzuki.poulose@arm.com>,
	Marc Zyngier <maz@kernel.org>,
	Quentin Perret <qperret@google.com>,
	James Morse <james.morse@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	kernel-team@android.com, kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 06/21] KVM: arm64: Add support for stage-2 map()/unmap() in generic page-table
Date: Thu, 10 Sep 2020 13:34:05 +0100	[thread overview]
Message-ID: <20200910123404.GA18100@willie-the-truck> (raw)
In-Reply-To: <f5939f12-56e8-794c-8d9b-9ae348bba3c0@arm.com>

On Thu, Sep 10, 2020 at 12:20:42PM +0100, Alexandru Elisei wrote:
> On 9/7/20 4:23 PM, Will Deacon wrote:
> > +static int stage2_map_walk_table_post(u64 addr, u64 end, u32 level,
> > +				      kvm_pte_t *ptep,
> > +				      struct stage2_map_data *data)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!data->anchor)
> > +		return 0;
> > +
> > +	free_page((unsigned long)kvm_pte_follow(*ptep));
> > +	put_page(virt_to_page(ptep));
> > +
> > +	if (data->anchor == ptep) {
> > +		data->anchor = NULL;
> > +		ret = stage2_map_walk_leaf(addr, end, level, ptep, data);
> > +	}
> 
> I had another look at this function. If we're back to the anchor entry, then that
> means that we know from the pre-order visitor that 1. the mapping is supported at
> this level and 2. that the pte was invalidated. This means that
> kvm_set_valid_leaf_pte() will succeed in changing the entry. How about instead of
> calling stage2_map_walk_leaf() -> stage2_map_walker_try_leaf() ->
> kvm_set_valid_leaf_pte() we call kvm_set_valid_leaf_pte() directly, followed by
> get_page(virt_to_page(ptep)? It would make the code a lot easier to follow
> (stage2_map_walk_leaf() is pretty complicated, imo, but that can't really be
> avoided), and also slightly faster.

I'm not sure I agree. I would consider kvm_set_valid_leaf_pte() to be lower
level, and not the right function to be calling here. Rather,
stage2_map_walk_leaf() is what would have been called if we hadn't spotted
the potential for a block mapping anyway, so I much prefer that symmetry. It
also means that if stage2_map_walk_leaf() grows some extra logic in future
that we need to take into account here, we won't miss anything.

> > +/*
> > + * This is a little fiddly, as we use all three of the walk flags. The idea
> > + * is that the TABLE_PRE callback runs for table entries on the way down,
> > + * looking for table entries which we could conceivably replace with a
> > + * block entry for this mapping. If it finds one, then it sets the 'anchor'
> > + * field in 'struct stage2_map_data' to point at the table entry, before
> > + * clearing the entry to zero and descending into the now detached table.
> > + *
> > + * The behaviour of the LEAF callback then depends on whether or not the
> > + * anchor has been set. If not, then we're not using a block mapping higher
> > + * up the table and we perform the mapping at the existing leaves instead.
> > + * If, on the other hand, the anchor _is_ set, then we drop references to
> > + * all valid leaves so that the pages beneath the anchor can be freed.
> > + *
> > + * Finally, the TABLE_POST callback does nothing if the anchor has not
> > + * been set, but otherwise frees the page-table pages while walking back up
> > + * the page-table, installing the block entry when it revisits the anchor
> > + * pointer and clearing the anchor to NULL.
> > + */
> 
> The comment does wonders at explaining what is going on, thank you.

Glad it helps!

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel