Re: [PATCH XEN v6 14/32] tools: Remove xc_map_foreign_batch

[George Dunlap <george.dunlap@citrix.com>]

On 03/12/15 11:22, Ian Campbell wrote:
> It can trivially be replaced by xc_map_foreign_pages which is the
> interface I want to move to going forward (by standardising on _bulk
> but handling err=NULL as _pages does).
> 
> The callers of _batch are checking a mixture of a NULL return or
> looking to see if the top nibble of the (usually sole) mfn they pass
> has been modified to be non-zero to detect errors. _pages never
> modifies the mfn it was given (it's const) and returns NULL on
> failure, so adjust the error handling where necessary. Some callers
> use a copy of the mfn array, for reuse on failure with _batch, which
> is no longer necessary as _pages doesn't modify the array, however I
> haven't cleaned that up here.
> 
> This reduces the twist maze of xc_map_foreign_* by one, which will be
> useful when trying to come up with an underlying stable interface.
> 
> NetBSD and Solaris implemented xc_map_foreign_bulk in terms of
> xc_map_foreign_batch via a compat layer, so xc_map_foreign_batch
> becomes an internal osdep for them. New OS ports should always
> implement xc_map_foreign_bulk instead.

"New OS ports should always implement xc_map_foreign_pages instead"?

Other than that:

Acked-by: George Dunlap <george.dunlap@citrix.com>

> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> v6: Switch to xc_map_foreign_pages not xc_map_foreign_bulk, since the
>     former has more similar error handling semantics to the current
>     usage.
> 
>     Dropped acks.
> ---
>  tools/libxc/include/xenctrl.h   | 10 -------
>  tools/libxc/xc_foreign_memory.c |  4 ++-
>  tools/libxc/xc_linux_osdep.c    | 59 +++--------------------------------------
>  tools/libxc/xc_minios.c         | 22 ---------------
>  tools/libxc/xc_netbsd.c         | 10 +++----
>  tools/libxc/xc_solaris.c        |  6 ++---
>  tools/libxc/xc_vm_event.c       | 18 ++++++++++---
>  tools/xenmon/xenbaked.c         | 12 ++++++++-
>  tools/xenpaging/xenpaging.c     | 14 +++++-----
>  tools/xentrace/xentrace.c       |  3 ++-
>  10 files changed, 48 insertions(+), 110 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 78400d3..cb41c07 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1387,16 +1387,6 @@ void *xc_map_foreign_pages(xc_interface *xch, uint32_t dom, int prot,
>                             const xen_pfn_t *arr, int num );
>  
>  /**
> - * DEPRECATED - use xc_map_foreign_bulk() instead.
> - *
> - * Like xc_map_foreign_pages(), except it can succeeed partially.
> - * When a page cannot be mapped, its PFN in @arr is or'ed with
> - * 0xF0000000 to indicate the error.
> - */
> -void *xc_map_foreign_batch(xc_interface *xch, uint32_t dom, int prot,
> -                           xen_pfn_t *arr, int num );
> -
> -/**
>   * Like xc_map_foreign_pages(), except it can succeed partially.
>   * When a page cannot be mapped, its respective field in @err is
>   * set to the corresponding errno value.
> diff --git a/tools/libxc/xc_foreign_memory.c b/tools/libxc/xc_foreign_memory.c
> index b205bca..2413e75 100644
> --- a/tools/libxc/xc_foreign_memory.c
> +++ b/tools/libxc/xc_foreign_memory.c
> @@ -55,6 +55,8 @@ void *xc_map_foreign_pages(xc_interface *xch, uint32_t dom, int prot,
>   * just implement xc_map_foreign_bulk.
>   */
>  #if defined(__NetBSD__) || defined(__sun__)
> +void *osdep_map_foreign_batch(xc_interface *xch, uint32_t dom, int prot,
> +                              xen_pfn_t *arr, int num );
>  void *xc_map_foreign_bulk(xc_interface *xch,
>                            uint32_t dom, int prot,
>                            const xen_pfn_t *arr, int *err, unsigned int num)
> @@ -75,7 +77,7 @@ void *xc_map_foreign_bulk(xc_interface *xch,
>      }
>  
>      memcpy(pfn, arr, num * sizeof(*arr));
> -    ret = xc_map_foreign_batch(xch, dom, prot, pfn, num);
> +    ret = osdep_map_foreign_batch(xch, dom, prot, pfn, num);
>  
>      if (ret) {
>          for (i = 0; i < num; ++i)
> diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
> index e68c495..39c88ce 100644
> --- a/tools/libxc/xc_linux_osdep.c
> +++ b/tools/libxc/xc_linux_osdep.c
> @@ -91,8 +91,8 @@ int osdep_privcmd_close(xc_interface *xch)
>      return close(fd);
>  }
>  
> -static int xc_map_foreign_batch_single(int fd, uint32_t dom,
> -                                       xen_pfn_t *mfn, unsigned long addr)
> +static int map_foreign_batch_single(int fd, uint32_t dom,
> +                                    xen_pfn_t *mfn, unsigned long addr)
>  {
>      privcmd_mmapbatch_t ioctlx;
>      int rc;
> @@ -113,59 +113,6 @@ static int xc_map_foreign_batch_single(int fd, uint32_t dom,
>      return rc;
>  }
>  
> -void *xc_map_foreign_batch(xc_interface *xch,
> -                           uint32_t dom, int prot,
> -                           xen_pfn_t *arr, int num)
> -{
> -    int fd = xch->privcmdfd;
> -    privcmd_mmapbatch_t ioctlx;
> -    void *addr;
> -    int rc;
> -
> -    addr = mmap(NULL, num << XC_PAGE_SHIFT, prot, MAP_SHARED, fd, 0);
> -    if ( addr == MAP_FAILED )
> -    {
> -        PERROR("xc_map_foreign_batch: mmap failed");
> -        return NULL;
> -    }
> -
> -    ioctlx.num = num;
> -    ioctlx.dom = dom;
> -    ioctlx.addr = (unsigned long)addr;
> -    ioctlx.arr = arr;
> -
> -    rc = ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, &ioctlx);
> -    if ( (rc < 0) && (errno == ENOENT) )
> -    {
> -        int i;
> -
> -        for ( i = 0; i < num; i++ )
> -        {
> -            if ( (arr[i] & PRIVCMD_MMAPBATCH_MFN_ERROR) ==
> -                           PRIVCMD_MMAPBATCH_PAGED_ERROR )
> -            {
> -                unsigned long paged_addr = (unsigned long)addr + (i << XC_PAGE_SHIFT);
> -                rc = xc_map_foreign_batch_single(fd, dom, &arr[i],
> -                                                 paged_addr);
> -                if ( rc < 0 )
> -                    goto out;
> -            }
> -        }
> -    }
> -
> - out:
> -    if ( rc < 0 )
> -    {
> -        int saved_errno = errno;
> -        PERROR("xc_map_foreign_batch: ioctl failed");
> -        (void)munmap(addr, num << XC_PAGE_SHIFT);
> -        errno = saved_errno;
> -        return NULL;
> -    }
> -
> -    return addr;
> -}
> -
>  /*
>   * Retry mmap of all paged gfns in batches
>   * retuns < 0 on fatal error
> @@ -305,7 +252,7 @@ void *xc_map_foreign_bulk(xc_interface *xch,
>                      err[i] = rc ?: -EINVAL;
>                      continue;
>                  }
> -                rc = xc_map_foreign_batch_single(fd, dom, pfn + i,
> +                rc = map_foreign_batch_single(fd, dom, pfn + i,
>                          (unsigned long)addr + ((unsigned long)i<<XC_PAGE_SHIFT));
>                  if ( rc < 0 )
>                  {
> diff --git a/tools/libxc/xc_minios.c b/tools/libxc/xc_minios.c
> index e3c8241..3ea3124 100644
> --- a/tools/libxc/xc_minios.c
> +++ b/tools/libxc/xc_minios.c
> @@ -73,28 +73,6 @@ void *xc_map_foreign_bulk(xc_interface *xch,
>      return map_frames_ex(arr, num, 1, 0, 1, dom, err, pt_prot);
>  }
>  
> -void *xc_map_foreign_batch(xc_interface *xch,
> -                           uint32_t dom, int prot,
> -                           xen_pfn_t *arr, int num)
> -{
> -    unsigned long pt_prot = 0;
> -    int err[num];
> -    int i;
> -    unsigned long addr;
> -
> -    if (prot & PROT_READ)
> -	pt_prot = L1_PROT_RO;
> -    if (prot & PROT_WRITE)
> -	pt_prot = L1_PROT;
> -
> -    addr = (unsigned long) map_frames_ex(arr, num, 1, 0, 1, dom, err, pt_prot);
> -    for (i = 0; i < num; i++) {
> -        if (err[i])
> -            arr[i] |= 0xF0000000;
> -    }
> -    return (void *) addr;
> -}
> -
>  void *xc_map_foreign_range(xc_interface *xch,
>                             uint32_t dom,
>                             int size, int prot,
> diff --git a/tools/libxc/xc_netbsd.c b/tools/libxc/xc_netbsd.c
> index d7f7f31..6a7ff9f 100644
> --- a/tools/libxc/xc_netbsd.c
> +++ b/tools/libxc/xc_netbsd.c
> @@ -67,16 +67,16 @@ int osdep_privcmd_close(xc_interface *xch)
>      return close(fd);
>  }
>  
> -void *xc_map_foreign_batch(xc_interface *xch,
> -                           uint32_t dom, int prot,
> -                           xen_pfn_t *arr, int num)
> +void *osdep_map_foreign_batch(xc_interface *xch,
> +                              uint32_t dom, int prot,
> +                              xen_pfn_t *arr, int num)
>  {
>      int fd = xch->privcmdfd;
>      privcmd_mmapbatch_t ioctlx;
>      void *addr;
>      addr = mmap(NULL, num*XC_PAGE_SIZE, prot, MAP_ANON | MAP_SHARED, -1, 0);
>      if ( addr == MAP_FAILED ) {
> -        PERROR("xc_map_foreign_batch: mmap failed");
> +        PERROR("osdep_map_foreign_batch: mmap failed");
>          return NULL;
>      }
>  
> @@ -87,7 +87,7 @@ void *xc_map_foreign_batch(xc_interface *xch,
>      if ( ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, &ioctlx) < 0 )
>      {
>          int saved_errno = errno;
> -        PERROR("xc_map_foreign_batch: ioctl failed");
> +        PERROR("osdep_map_foreign_batch: ioctl failed");
>          (void)munmap(addr, num*XC_PAGE_SIZE);
>          errno = saved_errno;
>          return NULL;
> diff --git a/tools/libxc/xc_solaris.c b/tools/libxc/xc_solaris.c
> index 5e72dee..2df7a06 100644
> --- a/tools/libxc/xc_solaris.c
> +++ b/tools/libxc/xc_solaris.c
> @@ -67,9 +67,9 @@ int osdep_privcmd_close(xc_interface *xch)
>      return close(fd);
>  }
>  
> -void *xc_map_foreign_batch(xc_interface *xch,
> -                          uint32_t dom, int prot,
> -                          xen_pfn_t *arr, int num)
> +void *osdep_map_foreign_batch(xc_interface *xch,
> +                              uint32_t dom, int prot,
> +                              xen_pfn_t *arr, int num)
>  {
>      int fd = xch->privcmdfd;
>      privcmd_mmapbatch_t ioctlx;
> diff --git a/tools/libxc/xc_vm_event.c b/tools/libxc/xc_vm_event.c
> index 2fef96a..d2d99e4 100644
> --- a/tools/libxc/xc_vm_event.c
> +++ b/tools/libxc/xc_vm_event.c
> @@ -72,9 +72,9 @@ void *xc_vm_event_enable(xc_interface *xch, domid_t domain_id, int param,
>  
>      ring_pfn = pfn;
>      mmap_pfn = pfn;
> -    ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ | PROT_WRITE,
> +    ring_page = xc_map_foreign_pages(xch, domain_id, PROT_READ | PROT_WRITE,
>                                       &mmap_pfn, 1);
> -    if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
> +    if ( !ring_page )
>      {
>          /* Map failed, populate ring page */
>          rc1 = xc_domain_populate_physmap_exact(xch, domain_id, 1, 0, 0,
> @@ -86,9 +86,9 @@ void *xc_vm_event_enable(xc_interface *xch, domid_t domain_id, int param,
>          }
>  
>          mmap_pfn = ring_pfn;
> -        ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ | PROT_WRITE,
> +        ring_page = xc_map_foreign_pages(xch, domain_id, PROT_READ | PROT_WRITE,
>                                           &mmap_pfn, 1);
> -        if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
> +        if ( !ring_page )
>          {
>              PERROR("Could not map the ring page\n");
>              goto out;
> @@ -156,3 +156,13 @@ void *xc_vm_event_enable(xc_interface *xch, domid_t domain_id, int param,
>  
>      return ring_page;
>  }
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/xenmon/xenbaked.c b/tools/xenmon/xenbaked.c
> index e4602ef..da46126 100644
> --- a/tools/xenmon/xenbaked.c
> +++ b/tools/xenmon/xenbaked.c
> @@ -411,7 +411,7 @@ static struct t_struct *map_tbufs(unsigned long tbufs_mfn, unsigned int num,
>          for ( j=0; j<tbufs.t_info->tbuf_size; j++)
>              pfn_list[j] = (xen_pfn_t)mfn_list[j];
>  
> -        tbufs.meta[i] = xc_map_foreign_batch(xc_handle, DOMID_XEN,
> +        tbufs.meta[i] = xc_map_foreign_pages(xc_handle, DOMID_XEN,
>                                               PROT_READ | PROT_WRITE,
>                                               pfn_list,
>                                               tbufs.t_info->tbuf_size);
> @@ -1175,3 +1175,13 @@ static int process_record(int cpu, struct t_rec *r)
>  
>      return 4 + (r->cycles_included ? 8 : 0) + (r->extra_u32 * 4);
>  }
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/xenpaging/xenpaging.c b/tools/xenpaging/xenpaging.c
> index df99c6a..c4bc713 100644
> --- a/tools/xenpaging/xenpaging.c
> +++ b/tools/xenpaging/xenpaging.c
> @@ -342,9 +342,9 @@ static struct xenpaging *xenpaging_init(int argc, char *argv[])
>                          HVM_PARAM_PAGING_RING_PFN, &ring_pfn);
>      mmap_pfn = ring_pfn;
>      paging->vm_event.ring_page = 
> -        xc_map_foreign_batch(xch, paging->vm_event.domain_id, 
> -                                PROT_READ | PROT_WRITE, &mmap_pfn, 1);
> -    if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
> +        xc_map_foreign_pages(xch, paging->vm_event.domain_id,
> +                             PROT_READ | PROT_WRITE, &mmap_pfn, 1);
> +    if ( !paging->vm_event.ring_page )
>      {
>          /* Map failed, populate ring page */
>          rc = xc_domain_populate_physmap_exact(paging->xc_handle, 
> @@ -356,11 +356,11 @@ static struct xenpaging *xenpaging_init(int argc, char *argv[])
>              goto err;
>          }
>  
> -        mmap_pfn = ring_pfn;
>          paging->vm_event.ring_page = 
> -            xc_map_foreign_batch(xch, paging->vm_event.domain_id, 
> -                                    PROT_READ | PROT_WRITE, &mmap_pfn, 1);
> -        if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
> +            xc_map_foreign_pages(xch, paging->vm_event.domain_id,
> +                                 PROT_READ | PROT_WRITE,
> +                                 &mmap_pfn, 1);
> +        if ( !paging->vm_event.ring_page )
>          {
>              PERROR("Could not map the ring page\n");
>              goto err;
> diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
> index c970d42..6cbe0ac 100644
> --- a/tools/xentrace/xentrace.c
> +++ b/tools/xentrace/xentrace.c
> @@ -508,7 +508,7 @@ static struct t_struct *map_tbufs(unsigned long tbufs_mfn, unsigned int num,
>          for ( j=0; j<tbufs.t_info->tbuf_size; j++)
>              pfn_list[j] = (xen_pfn_t)mfn_list[j];
>  
> -        tbufs.meta[i] = xc_map_foreign_batch(xc_handle, DOMID_XEN,
> +        tbufs.meta[i] = xc_map_foreign_pages(xc_handle, DOMID_XEN,
>                                               PROT_READ | PROT_WRITE,
>                                               pfn_list,
>                                               tbufs.t_info->tbuf_size);
> @@ -1221,6 +1221,7 @@ int main(int argc, char **argv)
>  
>      return ret;
>  }
> +
>  /*
>   * Local variables:
>   * mode: C
>