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

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxx>, "
> @@ -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;
>  }
>  



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel