Re: [PATCH v5 02/25] KVM: arm64: Allow attaching of non-coalescable pages to a hyp pool

From: Oliver Upton <oliver.upton@linux.dev>
To: Will Deacon <will@kernel.org>
Cc: kvmarm@lists.linux.dev, "Sean Christopherson" <seanjc@google.com>,
	"Vincent Donnefort" <vdonnefort@google.com>,
	"Alexandru Elisei" <alexandru.elisei@arm.com>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"James Morse" <james.morse@arm.com>,
	"Chao Peng" <chao.p.peng@linux.intel.com>,
	"Quentin Perret" <qperret@google.com>,
	"Suzuki K Poulose" <suzuki.poulose@arm.com>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Fuad Tabba" <tabba@google.com>, "Marc Zyngier" <maz@kernel.org>,
	kernel-team@android.com, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 02/25] KVM: arm64: Allow attaching of non-coalescable pages to a hyp pool
Date: Fri, 28 Oct 2022 00:17:40 +0000	[thread overview]
Message-ID: <Y1sfpM3IjNvr8ckf@google.com> (raw)
In-Reply-To: <20221020133827.5541-3-will@kernel.org>

On Thu, Oct 20, 2022 at 02:38:04PM +0100, Will Deacon wrote:
> From: Quentin Perret <qperret@google.com>
> 
> All the contiguous pages used to initialize a 'struct hyp_pool' are
> considered coalescable, which means that the hyp page allocator will
> actively try to merge them with their buddies on the hyp_put_page() path.
> However, using hyp_put_page() on a page that is not part of the inital
> memory range given to a hyp_pool() is currently unsupported.
> 
> In order to allow dynamically extending hyp pools at run-time, add a
> check to __hyp_attach_page() to allow inserting 'external' pages into
> the free-list of order 0. This will be necessary to allow lazy donation
> of pages from the host to the hypervisor when allocating guest stage-2
> page-table pages at EL2.

Is it ever going to be the case that we wind up mixing static and
dynamic memory within the same buddy allocator? Reading ahead a bit it
would seem pKVM uses separate allocators (i.e. pkvm_hyp_vm::pool for
donated memory) but just wanted to make sure.

I suppose what I'm getting at is the fact that the pool range makes
little sense in this case. Adding a field to hyp_pool describing the
type of pool that it is would make this more readable, such that we know
a pool contains only donated memory, and thus zero order pages should
never be coalesced.

> Tested-by: Vincent Donnefort <vdonnefort@google.com>
> Signed-off-by: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kvm/hyp/nvhe/page_alloc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c
> index 1ded09fc9b10..0d15227aced8 100644
> --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c
> +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c
> @@ -93,11 +93,15 @@ static inline struct hyp_page *node_to_page(struct list_head *node)
>  static void __hyp_attach_page(struct hyp_pool *pool,
>  			      struct hyp_page *p)
>  {
> +	phys_addr_t phys = hyp_page_to_phys(p);
>  	unsigned short order = p->order;
>  	struct hyp_page *buddy;
>  
>  	memset(hyp_page_to_virt(p), 0, PAGE_SIZE << p->order);
>  
> +	if (phys < pool->range_start || phys >= pool->range_end)
> +		goto insert;
> +

Assuming this is kept as-is...

This check reads really odd to me, but I understand how it applies to
the use case here. Perhaps create a helper (to be shared with
__find_buddy_nocheck()) and add a nice comment atop it describing the
significance of pages that exist outside the boundaries of the buddy
allocator.

--
Thanks,
Oliver

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