Re: [PATCH XEN v2 12/15] tools: Remove xc_map_foreign_batch

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

On 07/15/2015 04:47 PM, Ian Campbell wrote:
> It can trivially be replaced by xc_map_foreign_bulk which is the
> interface I want to move to going forward. All in tree users are
> trivially converted by supplying the appropriate error array and
> adjusting the what error handling exists (which in many cases is not
> much).
> 
> 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.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>

xentrace and xenbaked:

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

(Note that I'm going to be switching to using @citrix.com in tags post-4.6)

> ---
>  docs/misc/toolstack-library-abis.pandoc | 17 +++-------
>  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               | 25 ++++++++++----
>  tools/xenmon/xenbaked.c                 | 19 ++++++++---
>  tools/xenpaging/pagein.c                |  4 ++-
>  tools/xenpaging/xenpaging.c             | 12 ++++---
>  tools/xentrace/xentrace.c               | 10 +++---
>  12 files changed, 68 insertions(+), 130 deletions(-)
> 
> diff --git a/docs/misc/toolstack-library-abis.pandoc b/docs/misc/toolstack-library-abis.pandoc
> index 271e4b5..4ad6c7d 100644
> --- a/docs/misc/toolstack-library-abis.pandoc
> +++ b/docs/misc/toolstack-library-abis.pandoc
> @@ -274,7 +274,6 @@ Interface                         Underlying interface           Known external
>  `xc_map_foreign_pages`            `IOCTL_PRIVCMD_MMAP...`        qemu-pv
>  `xc_map_foreign_range`            `IOCTL_PRIVCMD_MMAP...`        qemu-dm, and qemu-pv
>  `xc_map_foreign_ranges`
> -`xc_map_foreign_batch`
>  `xc_map_foreign_bulk`             `IOCTL_PRIVCMD_MMAPBATCH_V2`   qemu-dm
>  
>  ### Managing guest memory map
> @@ -883,7 +882,6 @@ osdep level interfaces:
>  
>  Interface            Linux Minios Netbsd Freebsd Solaris
>  -------------------- ----- ------ ------ ------- -------
> -`map_foreign_batch`  Y     Y      Y              Y
>  `map_foreign_bulk`   Y     Y      compat Y       compat
>  `map_foreign_range`  Y     Y      Y      Y       Y
>  `map_foreign_ranges` Y     Y      Y      Y       Y
> @@ -897,7 +895,6 @@ Some osdep call other `xc_map_foreign_*` recursivley:
>  
>  Interface            Linux                        FreeBSD
>  -------------------- ---------------------------- -------------------------
> -`map_foreign_batch`  `IOCTL_PRIVCMD_MMAPBATCH`    N/A
>  `map_foreign_bulk`   `IOCTL_PRIVCMD_MMAPBATCH_V2` `IOCTL_PRIVCMD_MMAPBATCH`
>  `map_foreign_range`  `xc_map_foreign_pages`       `xc_map_foreign_pages`
>  `map_foreign_ranges` `xc_map_foreign_pages`       `xc_map_foreign_pages`
> @@ -909,17 +906,14 @@ Interface               Underlying interface
>  `xc_map_foreign_pages`  `xc_map_foreign_bulk`
>  `xc_map_foreign_range`  `osdep: map_foreign_range`
>  `xc_map_foreign_ranges` `osdep: map_foreign_ranges`
> -`xc_map_foreign_batch`  `osdep: map_foreign_batch`
>  `xc_map_foreign_bulk`   `osdep: map_foreign_bulk`
>  
>  `osdep: *` have been collapsed since osdep layer was removed.
>  
> -`xc_map_foreign_bulk_compat` is an internal helper used by osdep
> -backends which have not implemented there own version. It uses
> -`xc_map_foreign_batch`.
> -
> -`xc_map_foreign_batch_single` is part of linux osdep, but name has
> -leaked.
> +`xc_map_foreign_bulk` can be an internal compat helper used by oses
> +which have not implemented their own version. It used to use
> +`xc_map_foreign_batch` which is now an osdep for those OSes which need
> +it.
>  
>  Interface               Internal users               External users
>  ----------------------  --------------               --------------
> @@ -928,11 +922,10 @@ Interface               Internal users               External users
>                          mfndump, memshrtool, xenmon,
>                          xenstored, xentrace
>  `xc_map_foreign_ranges` NONE
> -`xc_map_foreign_batch`  xenmon, xenpaging, xentrace
>  `xc_map_foreign_bulk`   used recursively             qemu-dm
>  
>  Only one interface should become a stable API. Something based on
>  `xc_map_foreign_bulk` seems most appropriate (it is used by
>  `xc_map_foreign_pages` as well as by xc_map_foreign_range` and
>  `xc_map_foreign_ranges` on Linux + FreeBSD. A compat version exists in
> -terms for `xc_map_foreign_batch` for other OSes.
> +terms of `xc_map_foreign_batch` for other OSes.
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 61f68bb..201fd70 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1367,16 +1367,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 da77e9d..ae133a1 100644
> --- a/tools/libxc/xc_foreign_memory.c
> +++ b/tools/libxc/xc_foreign_memory.c
> @@ -56,6 +56,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)
> @@ -76,7 +78,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 488e8df..6641bd1 100644
> --- a/tools/libxc/xc_linux_osdep.c
> +++ b/tools/libxc/xc_linux_osdep.c
> @@ -86,8 +86,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;
> @@ -108,59 +108,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
> @@ -300,7 +247,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 27a4c4a..ed20652 100644
> --- a/tools/libxc/xc_minios.c
> +++ b/tools/libxc/xc_minios.c
> @@ -74,28 +74,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 a61e52f..f02015c 100644
> --- a/tools/libxc/xc_netbsd.c
> +++ b/tools/libxc/xc_netbsd.c
> @@ -68,16 +68,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;
>      }
>  
> @@ -88,7 +88,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 1e1a0c4..0a0fd14 100644
> --- a/tools/libxc/xc_solaris.c
> +++ b/tools/libxc/xc_solaris.c
> @@ -68,9 +68,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 a5b3277..4d7e7cc 100644
> --- a/tools/libxc/xc_vm_event.c
> +++ b/tools/libxc/xc_vm_event.c
> @@ -46,6 +46,7 @@ void *xc_vm_event_enable(xc_interface *xch, domid_t domain_id, int param,
>      void *ring_page = NULL;
>      uint64_t pfn;
>      xen_pfn_t ring_pfn, mmap_pfn;
> +    int err;
>      unsigned int op, mode;
>      int rc1, rc2, saved_errno;
>  
> @@ -73,9 +74,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,
> -                                     &mmap_pfn, 1);
> -    if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
> +    ring_page = xc_map_foreign_bulk(xch, domain_id, PROT_READ | PROT_WRITE,
> +				    &mmap_pfn, &err, 1);
> +    if ( !ring_page )
>      {
>          /* Map failed, populate ring page */
>          rc1 = xc_domain_populate_physmap_exact(xch, domain_id, 1, 0, 0,
> @@ -87,11 +88,11 @@ 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,
> -                                         &mmap_pfn, 1);
> -        if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
> +        ring_page = xc_map_foreign_bulk(xch, domain_id, PROT_READ | PROT_WRITE,
> +					&mmap_pfn, &err, 1);
> +        if ( !ring_page )
>          {
> -            PERROR("Could not map the ring page\n");
> +            PERROR("Could not map the ring page: %d\n", err);
>              goto out;
>          }
>      }
> @@ -157,3 +158,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 f092aec..9e2a49b 100644
> --- a/tools/xenmon/xenbaked.c
> +++ b/tools/xenmon/xenbaked.c
> @@ -408,14 +408,15 @@ static struct t_struct *map_tbufs(unsigned long tbufs_mfn, unsigned int num,
>                                     + tbufs.t_info->mfn_offset[i];
>          int j;
>          xen_pfn_t pfn_list[tbufs.t_info->tbuf_size];
> +        int err_list[tbufs.t_info->tbuf_size];
>  
>          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,
> -                                             PROT_READ | PROT_WRITE,
> -                                             pfn_list,
> -                                             tbufs.t_info->tbuf_size);
> +        tbufs.meta[i] = xc_map_foreign_bulk(xc_handle, DOMID_XEN,
> +					    PROT_READ | PROT_WRITE,
> +					    pfn_list, err_list,
> +					    tbufs.t_info->tbuf_size);
>          if ( tbufs.meta[i] == NULL )
>          {
>              PERROR("Failed to map cpu buffer!");
> @@ -1176,3 +1177,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/pagein.c b/tools/xenpaging/pagein.c
> index 7cb0f33..18699ec 100644
> --- a/tools/xenpaging/pagein.c
> +++ b/tools/xenpaging/pagein.c
> @@ -23,6 +23,7 @@ static void *page_in(void *arg)
>      void *page;
>      int i, num;
>      xen_pfn_t gfns[XENPAGING_PAGEIN_QUEUE_SIZE];
> +    int errs[XENPAGING_PAGEIN_QUEUE_SIZE];
>  
>      while (1)
>      {
> @@ -42,7 +43,8 @@ static void *page_in(void *arg)
>          pthread_mutex_unlock(&page_in_mutex);
>  
>          /* Ignore errors */
> -        page = xc_map_foreign_pages(pia->xch, pia->dom, PROT_READ, gfns, num);
> +        page = xc_map_foreign_bulk(pia->xch, pia->dom, PROT_READ,
> +                                   gfns, errs, num);
>          if (page)
>              munmap(page, PAGE_SIZE * num);
>      }
> diff --git a/tools/xenpaging/xenpaging.c b/tools/xenpaging/xenpaging.c
> index 6776896..a819a5b 100644
> --- a/tools/xenpaging/xenpaging.c
> +++ b/tools/xenpaging/xenpaging.c
> @@ -282,6 +282,7 @@ static struct xenpaging *xenpaging_init(int argc, char *argv[])
>      char *p;
>      int rc;
>      unsigned long ring_pfn, mmap_pfn;
> +    int mmap_err;
>  
>      /* Allocate memory */
>      paging = calloc(1, sizeof(struct xenpaging));
> @@ -343,8 +344,8 @@ 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);
> +        xc_map_foreign_bulk(xch, paging->vm_event.domain_id,
> +                            PROT_READ | PROT_WRITE, &mmap_pfn, &mmap_err, 1);
>      if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
>      {
>          /* Map failed, populate ring page */
> @@ -359,11 +360,12 @@ static struct xenpaging *xenpaging_init(int argc, char *argv[])
>  
>          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);
> +            xc_map_foreign_bulk(xch, paging->vm_event.domain_id,
> +                                PROT_READ | PROT_WRITE,
> +                                &mmap_pfn, &mmap_err, 1);
>          if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
>          {
> -            PERROR("Could not map the ring page\n");
> +            PERROR("Could not map the ring page: %d\n", mmap_err);
>              goto err;
>          }
>      }
> diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
> index c970d42..f66a77c 100644
> --- a/tools/xentrace/xentrace.c
> +++ b/tools/xentrace/xentrace.c
> @@ -504,14 +504,15 @@ static struct t_struct *map_tbufs(unsigned long tbufs_mfn, unsigned int num,
>                                     + tbufs.t_info->mfn_offset[i];
>          int j;
>          xen_pfn_t pfn_list[tbufs.t_info->tbuf_size];
> +        int pfn_err[tbufs.t_info->tbuf_size];
>  
>          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,
> -                                             PROT_READ | PROT_WRITE,
> -                                             pfn_list,
> -                                             tbufs.t_info->tbuf_size);
> +        tbufs.meta[i] = xc_map_foreign_bulk(xc_handle, DOMID_XEN,
> +                                            PROT_READ | PROT_WRITE,
> +                                            pfn_list, pfn_err,
> +                                            tbufs.t_info->tbuf_size);
>          if ( tbufs.meta[i] == NULL )
>          {
>              PERROR("Failed to map cpu buffer!");
> @@ -1221,6 +1222,7 @@ int main(int argc, char **argv)
>  
>      return ret;
>  }
> +
>  /*
>   * Local variables:
>   * mode: C
>