Re: [PATCH v3 27/31] KVM: arm64: Use atomic refcount helpers for 'struct hyp_page::refcount'

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

On Tue, Apr 23, 2024 at 12:44:32AM +0100, Will Deacon wrote:
> Hi Oliver,
> 
> On Mon, Apr 22, 2024 at 01:46:14PM -0700, Oliver Upton wrote:
> > On Mon, Apr 22, 2024 at 02:08:17PM +0100, Fuad Tabba wrote:
> > 
> > [...]
> > 
> > > > > Adding a BUG_ON() for taking a reference on a non-refcounted page (i.e.
> > > > > p->refcount was 0) would be nice, especially since we're past the point of
> > > > > serializing everything and you can theoretically have a zero count page
> > > > > outside of the free list.
> > > > >
> > > > > Seems like otherwise we'd get actually hit the BUG_ON() in an unrelated
> > > > > allocation path.
> > > 
> > > Actually, the refcount can be 0 without it being an error. For
> > > example, when hyp pins memory shared with it by the host
> > > (mem_protect.c:hyp_pin_shared_mem()).
> > 
> > Are those not by their very definition non-refcounted pages?
> 
> Right, we're using the refcount for two things here: (1) so that the
> allocator knows when to return the page to the pool and (2) so that the
> hypervisor can transiently prevent a page which has been shared by the
> host from being unshared. That second part is needed to e.g. prevent a
> page holding a host vCPU structure being donated to a guest as normal
> memory and then having the hypervisor write to it as a result of a host
> hypercall. We use the refcount for this because the same page can be
> shared with the hypervisor multiple times and we need to know when the
> last host sharer has dropped its pin.

Ah, right. I don't have the muscle memory for the pKVM bits upstream
(even though I should), I missed the very obvious refcount test in
hyp_ack_unshare().

So it is a refcount on the page state, be it allocated or shared. And
the 0 -> 1 transition on a shared page happens through an increment
rather than an initializer like pages from the hyp pool.

> > I can't imagine we'd want pages in a shared state with the host to ever
> > get returned to the hyp allocator. Seems an erroneous hyp_put_page() would
> > get you there, though.
> 
> Given the dual-use above, I don't think a BUG_ON() on the refcount is
> the right fix. Instead, we'd probably want a (cheap) mechanism to
> differentiate pages in states (1) and (2). This could be a new flag in
> 'struct hyp_page' or perhaps we could be creative and set the 'order' to
> HYP_NO_ORDER for pinned pages and then have a BUG() to check 'p->order'
> against 'pool->max_order' in hyp_put_page().
> 
> What do you think?

Excellent idea. Having a way to disambiguate page states would be great,
all the better if it can (ab)use an existing field.

-- 
Thanks,
Oliver