Re: [RFC PATCH 10/16] xen/balloon: support ballooning in xenhost_t

[Juergen Gross]

On 09.05.19 19:25, Ankur Arora wrote:
> Xen ballooning uses hollow struct pages (with the underlying GFNs being
> populated/unpopulated via hypercalls) which are used by the grant logic
> to map grants from other domains.
>
> This patch allows the default xenhost to provide an alternate ballooning
> allocation mechanism. This is expected to be useful for local xenhosts
> (type xenhost_r0) because unlike Xen, where there is an external
> hypervisor which can change the memory underneath a GFN, that is not
> possible when the hypervisor is running in the same address space
> as the entity doing the ballooning.
>
> Co-developed-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx>
> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
> Signed-off-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx>
> ---
>   arch/x86/xen/enlighten_hvm.c       |  7 +++++++
>   arch/x86/xen/enlighten_pv.c        |  8 ++++++++
>   drivers/xen/balloon.c              | 19 ++++++++++++++++---
>   drivers/xen/grant-table.c          |  4 ++--
>   drivers/xen/privcmd.c              |  4 ++--
>   drivers/xen/xen-selfballoon.c      |  2 ++
>   drivers/xen/xenbus/xenbus_client.c |  6 +++---
>   drivers/xen/xlate_mmu.c            |  4 ++--
>   include/xen/balloon.h              |  4 ++--
>   include/xen/xenhost.h              | 19 +++++++++++++++++++
>   10 files changed, 63 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 5ef4d6ad920d..08becf574743 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -63,6 +63,7 @@
>   #include <asm/tlb.h>
>   
>   #include <xen/interface/xen.h>
> +#include <xen/xenhost.h>
>   #include <asm/xen/hypervisor.h>
>   #include <asm/xen/hypercall.h>
>   
> @@ -583,12 +584,21 @@ static int add_ballooned_pages(int nr_pages)
>    * @pages: pages returned
>    * @return 0 on success, error otherwise
>    */
> -int alloc_xenballooned_pages(int nr_pages, struct page **pages)
> +int alloc_xenballooned_pages(xenhost_t *xh, int nr_pages, struct page **pages)
>   {
>   	int pgno = 0;
>   	struct page *page;
>   	int ret;
>   
> +	/*
> +	 * xenmem transactions for remote xenhost are disallowed.
> +	 */
> +	if (xh->type == xenhost_r2)
> +		return -EINVAL;

Why don't you set a dummy function returning -EINVAL into the xenhost_r2
structure instead?

> +
> +	if (xh->ops->alloc_ballooned_pages)
> +		return xh->ops->alloc_ballooned_pages(xh, nr_pages, pages);
> +

Please make alloc_xenballooned_pages() an inline wrapper and use the
current implmentaion as the default. This avoids another if ().

>   	mutex_lock(&balloon_mutex);
>   
>   	balloon_stats.target_unpopulated += nr_pages;
> @@ -620,7 +630,7 @@ int alloc_xenballooned_pages(int nr_pages, struct page **pages)
>   	return 0;
>    out_undo:
>   	mutex_unlock(&balloon_mutex);
> -	free_xenballooned_pages(pgno, pages);
> +	free_xenballooned_pages(xh, pgno, pages);
>   	return ret;
>   }
>   EXPORT_SYMBOL(alloc_xenballooned_pages);
> @@ -630,10 +640,13 @@ EXPORT_SYMBOL(alloc_xenballooned_pages);
>    * @nr_pages: Number of pages
>    * @pages: pages to return
>    */
> -void free_xenballooned_pages(int nr_pages, struct page **pages)
> +void free_xenballooned_pages(xenhost_t *xh, int nr_pages, struct page **pages)
>   {
>   	int i;
>   
> +	if (xh->ops->free_ballooned_pages)
> +		return xh->ops->free_ballooned_pages(xh, nr_pages, pages);
> +

Same again: please use an inline wrapper.


Juergen