Re: [RFC/PATCH v1] xen-gntdev: Use XENMEM_populate_physmap on unmapped pages in HVM

[Ian Campbell <Ian.Campbell@eu.citrix.com>]

On Fri, 2011-03-04 at 20:53 +0000, Daniel De Graaf wrote:
> >> Is there some reason the gntdev driver can't just balloon down
> >> (XENMEM_decrease_reservation) to make itself a space to map and then
> >> balloon back up (XENMEM_increase_reservation) after unmap when running
> >> HVM?
> > 
> > I recall having problems with doing this last time, but I think other changes
> > to make the balloon driver work in HVM may have fixed the issue. I'll try it
> > again; I think this would be the best solution.
> 
> Using the balloon hypercalls does work in my test. Below is a patch that
> uses them directly in gntdev, which is simpler but doesn't handle errors
> in re-ballooning and doesn't take advantage of already-ballooned pages
> from the balloon device. Taking advantage of the balloon device is also
> possible and would be more efficient, but adds another dependency to
> gntdev. Any opinions on which method is preferred? If not, I'll try
> making a patch to implement the balloon-assisted version.

The balloon driver in the 2.6.32 pvops (and all old-style Xen kernels
too) has interfaces which are used by the backend drivers for exactly
this purpose.

It was always a bit of a wart that the backends depended on the balloon
driver in this way. IMHO the core ballooning functionality and kernel
interfaces should be moved into the core kernel under CONFIG_XEN. All
that should remain under CONFIG_XEN_BALLOON is the sysfs and xenbus
integration e.g. the user/admin visible interfaces.

This is somewhat analogous to drivers/xen/{events,grant-table}.c being
core kernel and drivers/xen/{gnt,evt}dev.c being configurable optional
modules.

It's possible that what remains under CONFIG_XEN_BALLOON is such a tiny
amount of code that it's not worth keeping it as a separate option,
although the "depends SYSFS" aspect may make it more worthwhile.

Ian.

> 
> 8<-------------------------------------------------------------------------
> 
> Grant mappings cause the PFN<->MFN mapping to be lost on the pages
> used for the mapping. Instead of leaking memory, explicitly free it
> to the hypervisor and request the memory back after unmapping. This
> removes the need for the bad-page leak workaround.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/xen/gntdev.c |   58 +++++++++++++++++++++++++++++++++----------------
>  1 files changed, 39 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index d43ff30..104098a 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -41,6 +41,7 @@
>  #include <asm/xen/hypervisor.h>
>  #include <asm/xen/hypercall.h>
>  #include <asm/xen/page.h>
> +#include <xen/interface/memory.h>
>  
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Derek G. Murray <Derek.Murray@cl.cam.ac.uk>, "
> @@ -203,27 +204,9 @@ static void gntdev_put_map(struct grant_map *map)
>  			unmap_grant_pages(map, 0, map->count);
>  
>  		for (i = 0; i < map->count; i++) {
> -			uint32_t check, *tmp;
>  			if (!map->pages[i])
>  				continue;
> -			/* XXX When unmapping in an HVM domain, Xen will
> -			 * sometimes end up mapping the GFN to an invalid MFN.
> -			 * In this case, writes will be discarded and reads will
> -			 * return all 0xFF bytes.  Leak these unusable GFNs
> -			 * until Xen supports fixing their p2m mapping.
> -			 *
> -			 * Confirmed present in Xen 4.1-RC3 with HVM source
> -			 */
> -			tmp = kmap(map->pages[i]);
> -			*tmp = 0xdeaddead;
> -			mb();
> -			check = *tmp;
> -			kunmap(map->pages[i]);
> -			if (check == 0xdeaddead)
> -				__free_page(map->pages[i]);
> -			else
> -				pr_debug("Discard page %d=%ld\n", i,
> -					page_to_pfn(map->pages[i]));
> +			__free_page(map->pages[i]);
>  		}
>  	}
>  	kfree(map->pages);
> @@ -258,11 +241,19 @@ static int map_grant_pages(struct grant_map *map)
>  {
>  	int i, err = 0;
>  	phys_addr_t addr;
> +	unsigned long frame;
>  
>  	if (!use_ptemod) {
> +		struct xen_memory_reservation reservation = {
> +			.address_bits = 0,
> +			.extent_order = 0,
> +			.nr_extents   = 1,
> +			.domid        = DOMID_SELF
> +		};
>  		/* Note: it could already be mapped */
>  		if (map->map_ops[0].handle != -1)
>  			return 0;
> +		set_xen_guest_handle(reservation.extent_start, &frame);
>  		for (i = 0; i < map->count; i++) {
>  			addr = (phys_addr_t)
>  				pfn_to_kaddr(page_to_pfn(map->pages[i]));
> @@ -271,6 +262,12 @@ static int map_grant_pages(struct grant_map *map)
>  				map->grants[i].domid);
>  			gnttab_set_unmap_op(&map->unmap_ops[i], addr,
>  				map->flags, -1 /* handle */);
> +			/* TODO batch hypercall, needs buffer */
> +			frame = page_to_pfn(map->pages[i]);
> +			err = HYPERVISOR_memory_op(XENMEM_decrease_reservation,
> +				&reservation);
> +			if (WARN_ON(err <= 0))
> +				printk(KERN_INFO "memop returned %d\n", err);
>  		}
>  	}
>  
> @@ -324,6 +321,29 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
>  			map->unmap_ops[offset+i].status);
>  		map->unmap_ops[offset+i].handle = -1;
>  	}
> +
> +	if (!use_ptemod) {
> +		struct xen_memory_reservation reservation = {
> +			.address_bits = 0,
> +			.extent_order = 0,
> +			.nr_extents   = 1,
> +			.domid        = DOMID_SELF
> +		};
> +		int rc;
> +		unsigned long frame;
> +		set_xen_guest_handle(reservation.extent_start, &frame);
> +		/* TODO batch hypercall, needs buffer */
> +		for (i = offset; i < pages + offset; i++) {
> +			frame = page_to_pfn(map->pages[i]);
> +			rc = HYPERVISOR_memory_op(XENMEM_populate_physmap,
> +				&reservation);
> +			if (WARN_ON(rc <= 0)) {
> +				map->pages[i] = NULL;
> +				continue;
> +			}
> +		}
> +	}
> +
>  	return err;
>  }
>