Re: [PATCH 2/3] xen/grant_table: use the new GNTTABOP_unmap_and_replace hypercall

[Ian Campbell <ian.campbell@citrix.com>]

On Sun, 2013-07-21 at 18:32 +0100, Stefano Stabellini wrote:
> The current GNTTABOP_unmap_and_replace (7) is deprecated, rename
> it to GNTTABOP_unmap_and_replace_legacy.

We would normally call this foo_compat.

>  Introduce the new
> GNTTABOP_unmap_and_replace (12) and detect at boot time which one is
> available.

I don't think this exists yet, is there a h/v patch under separate
cover?

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: alex@alex.org.uk
> CC: dcrisan@flexiant.com
> ---
>  drivers/xen/grant-table.c           |    8 ++++++++
>  include/xen/grant_table.h           |    1 +
>  include/xen/interface/grant_table.h |    8 +++++---
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 04c1b2d..0f7d00b 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -65,6 +65,8 @@ static grant_ref_t gnttab_free_head;
>  static DEFINE_SPINLOCK(gnttab_list_lock);
>  unsigned long xen_hvm_resume_frames;
>  EXPORT_SYMBOL_GPL(xen_hvm_resume_frames);
> +int __read_mostly gnttabop_unmap_and_replace = GNTTABOP_unmap_and_replace; 
> +EXPORT_SYMBOL_GPL(gnttabop_unmap_and_replace);

I can see this getting set but not used, does it really need to be
exported?

>  
>  static union {
>  	struct grant_entry_v1 *v1;
> @@ -1238,6 +1240,12 @@ int gnttab_init(void)
>  	gnttab_free_count = nr_init_grefs - NR_RESERVED_ENTRIES;
>  	gnttab_free_head  = NR_RESERVED_ENTRIES;
>  
> +	ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_and_replace, NULL, 1);
> +	if (ret == -ENOSYS) {
> +		gnttabop_unmap_and_replace = GNTTABOP_unmap_and_replace_legacy;
> +		pr_warn("GNTTABOP_unmap_and_replace unavailable, falling back to the racy legacy version\n");

Perhaps WARN_ONCE?

> +	}
> +
>  	printk("Grant table initialized\n");
>  	return 0;
>  
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index 694dcaf..867d1a7 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -179,6 +179,7 @@ int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
>  void arch_gnttab_unmap(void *shared, unsigned long nr_gframes);
>  
>  extern unsigned long xen_hvm_resume_frames;
> +extern int gnttabop_unmap_and_replace;
>  unsigned int gnttab_max_grant_frames(void);
>  
>  #define gnttab_map_vaddr(map) ((void *)(map.host_virt_addr))
> diff --git a/include/xen/interface/grant_table.h b/include/xen/interface/grant_table.h
> index e40fae9..2a2b74f 100644
> --- a/include/xen/interface/grant_table.h
> +++ b/include/xen/interface/grant_table.h
> @@ -408,15 +408,17 @@ DEFINE_GUEST_HANDLE_STRUCT(gnttab_query_size);
>  /*
>   * GNTTABOP_unmap_and_replace: Destroy one or more grant-reference mappings
>   * tracked by <handle> but atomically replace the page table entry with one
> - * pointing to the machine address under <new_addr>.  <new_addr> will be
> - * redirected to the null entry.
> + * pointing to the machine address under <new_addr>.
> + * GNTTABOP_unmap_and_replace_legacy also redirects <new_addr> to the
> + * null entry

I wonder if rather than _legacy (or _compat) if the old behaviour might
be well described as "unmap_and_swap", since I think that is what
clearing new_addr means?

>
>   * NOTES:
>   *  1. The call may fail in an undefined manner if either mapping is not
>   *     tracked by <handle>.
>   *  2. After executing a batch of unmaps, it is guaranteed that no stale
>   *     mappings will remain in the device or host TLBs.
>   */
> -#define GNTTABOP_unmap_and_replace    7
> +#define GNTTABOP_unmap_and_replace_legacy    7
> +#define GNTTABOP_unmap_and_replace    12
>  struct gnttab_unmap_and_replace {
>      /* IN parameters. */
>      uint64_t host_addr;