Re: [PATCH 2/5] libxc: Split off xc_minios_privcmd.c

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

On Thu, 2015-02-26 at 11:56 +0000, Wei Liu wrote:
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> We are going to want to use some but not all of the machinery
> previously in xc_minios.c.  Split the privcmd and gnttab code into its
> own file.

Does it make sense to put gnttab code in a file called privcmd.c?

I don't think there is much of a link between the two, is there?

I'm not sure which half of this rump libxc is going to want but perhaps
either _common or _standalone would be a better suffix, depending which
way it goes?

>   This part is pure code motion.
> 
> But we also have to:
> 
>  - Alter the Makefile to build and link xc_minios_privcmd.c too.
> 
>  - Rename some of the minios_*_ops symbols to have proper namespaceing
>    and make them have external linkage, so that the init code (which
>    remains in xc_minios.c) can reference them.
> 
>  - Call these *_ops symbols xc_*_ops so that we can mix and match in
>    the future.  This does not impede the existing mechanisms for
>    run-time overriding.  (But leave a comment next to the new
>    declarations in xc_private.h saying not to use these.)
> 
>  - Change map_frames_ex to minios_map_frames_ex if compiling on rump
>    kernel.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> [ wei: wrap long lines, use __RUMPRUN__ and define macro for map_frames_ex ]
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxc/Makefile            |   2 +-
>  tools/libxc/xc_minios.c         | 243 +--------------------------------
>  tools/libxc/xc_minios_privcmd.c | 291 ++++++++++++++++++++++++++++++++++++++++
>  tools/libxc/xc_private.h        |   3 +
>  4 files changed, 299 insertions(+), 240 deletions(-)
>  create mode 100644 tools/libxc/xc_minios_privcmd.c
> 
> diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
> index 6fa88c7..4ace2b6 100644
> --- a/tools/libxc/Makefile
> +++ b/tools/libxc/Makefile
> @@ -47,7 +47,7 @@ CTRL_SRCS-$(CONFIG_Linux) += xc_linux.c xc_linux_osdep.c
>  CTRL_SRCS-$(CONFIG_FreeBSD) += xc_freebsd.c xc_freebsd_osdep.c
>  CTRL_SRCS-$(CONFIG_SunOS) += xc_solaris.c
>  CTRL_SRCS-$(CONFIG_NetBSD) += xc_netbsd.c
> -CTRL_SRCS-$(CONFIG_MiniOS) += xc_minios.c
> +CTRL_SRCS-$(CONFIG_MiniOS) += xc_minios.c xc_minios_privcmd.c
>  
>  GUEST_SRCS-y :=
>  GUEST_SRCS-y += xg_private.c xc_suspend.c
> diff --git a/tools/libxc/xc_minios.c b/tools/libxc/xc_minios.c
> index e703684..90e3363 100644
> --- a/tools/libxc/xc_minios.c
> +++ b/tools/libxc/xc_minios.c
> @@ -41,164 +41,10 @@
>  
>  #include "xc_private.h"
>  
> -void minios_interface_close_fd(int fd);
>  void minios_evtchn_close_fd(int fd);
> -void minios_gnttab_close_fd(int fd);
> -
> -extern void minios_interface_close_fd(int fd);
> -extern void minios_evtchn_close_fd(int fd);
>  
>  extern struct wait_queue_head event_queue;
>  
> -static xc_osdep_handle minios_privcmd_open(xc_interface *xch)
> -{
> -    int fd = alloc_fd(FTYPE_XC);
> -
> -    if ( fd == -1)
> -        return XC_OSDEP_OPEN_ERROR;
> -
> -    return (xc_osdep_handle)fd;
> -}
> -
> -static int minios_privcmd_close(xc_interface *xch, xc_osdep_handle h)
> -{
> -    int fd = (int)h;
> -    return close(fd);
> -}
> -
> -void minios_interface_close_fd(int fd)
> -{
> -    files[fd].type = FTYPE_NONE;
> -}
> -
> -static void *minios_privcmd_alloc_hypercall_buffer(xc_interface *xch, xc_osdep_handle h, int npages)
> -{
> -    return xc_memalign(xch, PAGE_SIZE, npages * PAGE_SIZE);
> -}
> -
> -static void minios_privcmd_free_hypercall_buffer(xc_interface *xch, xc_osdep_handle h, void *ptr, int npages)
> -{
> -    free(ptr);
> -}
> -
> -static int minios_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcmd_hypercall_t *hypercall)
> -{
> -    multicall_entry_t call;
> -    int i, ret;
> -
> -    call.op = hypercall->op;
> -    for (i = 0; i < ARRAY_SIZE(hypercall->arg); i++)
> -	call.args[i] = hypercall->arg[i];
> -
> -    ret = HYPERVISOR_multicall(&call, 1);
> -
> -    if (ret < 0) {
> -	errno = -ret;
> -	return -1;
> -    }
> -    if ((long) call.result < 0) {
> -        errno = - (long) call.result;
> -        return -1;
> -    }
> -    return call.result;
> -}
> -
> -static void *minios_privcmd_map_foreign_bulk(xc_interface *xch, xc_osdep_handle h,
> -                                             uint32_t dom, int prot,
> -                                             const xen_pfn_t *arr, int *err, unsigned int num)
> -{
> -    unsigned long pt_prot = 0;
> -    if (prot & PROT_READ)
> -	pt_prot = L1_PROT_RO;
> -    if (prot & PROT_WRITE)
> -	pt_prot = L1_PROT;
> -    return map_frames_ex(arr, num, 1, 0, 1, dom, err, pt_prot);    
> -}
> -
> -static void *minios_privcmd_map_foreign_batch(xc_interface *xch,  xc_osdep_handle h,
> -                                              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;
> -}
> -
> -static void *minios_privcmd_map_foreign_range(xc_interface *xch, xc_osdep_handle h,
> -                                              uint32_t dom,
> -                                              int size, int prot,
> -                                              unsigned long mfn)
> -{
> -    unsigned long pt_prot = 0;
> -
> -    if (prot & PROT_READ)
> -	pt_prot = L1_PROT_RO;
> -    if (prot & PROT_WRITE)
> -	pt_prot = L1_PROT;
> -
> -    assert(!(size % getpagesize()));
> -    return map_frames_ex(&mfn, size / getpagesize(), 0, 1, 1, dom, NULL, pt_prot);
> -}
> -
> -static void *minios_privcmd_map_foreign_ranges(xc_interface *xch, xc_osdep_handle h,
> -                                               uint32_t dom,
> -                                               size_t size, int prot, size_t chunksize,
> -                                               privcmd_mmap_entry_t entries[], int nentries)
> -{
> -    unsigned long *mfns;
> -    int i, j, n;
> -    unsigned long pt_prot = 0;
> -    void *ret;
> -
> -    if (prot & PROT_READ)
> -	pt_prot = L1_PROT_RO;
> -    if (prot & PROT_WRITE)
> -	pt_prot = L1_PROT;
> -
> -    mfns = malloc((size / XC_PAGE_SIZE) * sizeof(*mfns));
> -
> -    n = 0;
> -    for (i = 0; i < nentries; i++)
> -        for (j = 0; j < chunksize / XC_PAGE_SIZE; j++)
> -            mfns[n++] = entries[i].mfn + j;
> -
> -    ret = map_frames_ex(mfns, n, 1, 0, 1, dom, NULL, pt_prot);
> -    free(mfns);
> -    return ret;
> -}
> -
> -
> -static struct xc_osdep_ops minios_privcmd_ops = {
> -    .open = &minios_privcmd_open,
> -    .close = &minios_privcmd_close,
> -
> -    .u.privcmd = {
> -        .alloc_hypercall_buffer = &minios_privcmd_alloc_hypercall_buffer,
> -        .free_hypercall_buffer = &minios_privcmd_free_hypercall_buffer,
> -
> -        .hypercall = &minios_privcmd_hypercall,
> -
> -        .map_foreign_batch = &minios_privcmd_map_foreign_batch,
> -        .map_foreign_bulk = &minios_privcmd_map_foreign_bulk,
> -        .map_foreign_range = &minios_privcmd_map_foreign_range,
> -        .map_foreign_ranges = &minios_privcmd_map_foreign_ranges,
> -    },
> -};
> -
> -
>  /* XXX Note: This is not threadsafe */
>  static struct evtchn_port_info* port_alloc(int fd) {
>      struct evtchn_port_info *port_info;
> @@ -409,7 +255,7 @@ static int minios_evtchn_unmask(xc_evtchn *xce, xc_osdep_handle h, evtchn_port_t
>      return 0;
>  }
>  
> -static struct xc_osdep_ops minios_evtchn_ops = {
> +struct xc_osdep_ops xc_evtchn_ops = {
>      .open = &minios_evtchn_open,
>      .close = &minios_evtchn_close,
>  
> @@ -437,97 +283,16 @@ void *xc_memalign(xc_interface *xch, size_t alignment, size_t size)
>      return memalign(alignment, size);
>  }
>  
> -static xc_osdep_handle minios_gnttab_open(xc_gnttab *xcg)
> -{
> -    int fd = alloc_fd(FTYPE_GNTMAP);
> -    if ( fd == -1 )
> -        return XC_OSDEP_OPEN_ERROR;
> -    gntmap_init(&files[fd].gntmap);
> -    return (xc_osdep_handle)fd;
> -}
> -
> -static int minios_gnttab_close(xc_gnttab *xcg, xc_osdep_handle h)
> -{
> -    int fd = (int)h;
> -    return close(fd);
> -}
> -
> -void minios_gnttab_close_fd(int fd)
> -{
> -    gntmap_fini(&files[fd].gntmap);
> -    files[fd].type = FTYPE_NONE;
> -}
> -
> -static void *minios_gnttab_grant_map(xc_gnttab *xcg, xc_osdep_handle h,
> -                                     uint32_t count, int flags, int prot,
> -                                     uint32_t *domids, uint32_t *refs,
> -                                     uint32_t notify_offset,
> -                                     evtchn_port_t notify_port)
> -{
> -    int fd = (int)h;
> -    int stride = 1;
> -    if (flags & XC_GRANT_MAP_SINGLE_DOMAIN)
> -        stride = 0;
> -    if (notify_offset != -1 || notify_port != -1) {
> -        errno = ENOSYS;
> -        return NULL;
> -    }
> -    return gntmap_map_grant_refs(&files[fd].gntmap,
> -                                 count, domids, stride,
> -                                 refs, prot & PROT_WRITE);
> -}
> -
> -static int minios_gnttab_munmap(xc_gnttab *xcg, xc_osdep_handle h,
> -                                void *start_address,
> -                                uint32_t count)
> -{
> -    int fd = (int)h;
> -    int ret;
> -    ret = gntmap_munmap(&files[fd].gntmap,
> -                        (unsigned long) start_address,
> -                        count);
> -    if (ret < 0) {
> -        errno = -ret;
> -        return -1;
> -    }
> -    return ret;
> -}
> -
> -static int minios_gnttab_set_max_grants(xc_gnttab *xcg, xc_osdep_handle h,
> -                             uint32_t count)
> -{
> -    int fd = (int)h;
> -    int ret;
> -    ret = gntmap_set_max_grants(&files[fd].gntmap,
> -                                count);
> -    if (ret < 0) {
> -        errno = -ret;
> -        return -1;
> -    }
> -    return ret;
> -}
> -
> -static struct xc_osdep_ops minios_gnttab_ops = {
> -    .open = &minios_gnttab_open,
> -    .close = &minios_gnttab_close,
> -
> -    .u.gnttab = {
> -        .grant_map = &minios_gnttab_grant_map,
> -        .munmap = &minios_gnttab_munmap,
> -        .set_max_grants = &minios_gnttab_set_max_grants,
> -    },
> -};
> -
>  static struct xc_osdep_ops *minios_osdep_init(xc_interface *xch, enum xc_osdep_type type)
>  {
>      switch ( type )
>      {
>      case XC_OSDEP_PRIVCMD:
> -        return &minios_privcmd_ops;
> +        return &xc_privcmd_ops;
>      case XC_OSDEP_EVTCHN:
> -        return &minios_evtchn_ops;
> +        return &xc_evtchn_ops;
>      case XC_OSDEP_GNTTAB:
> -        return &minios_gnttab_ops;
> +        return &xc_gnttab_ops;
>      default:
>          return NULL;
>      }
> diff --git a/tools/libxc/xc_minios_privcmd.c b/tools/libxc/xc_minios_privcmd.c
> new file mode 100644
> index 0000000..7766b86
> --- /dev/null
> +++ b/tools/libxc/xc_minios_privcmd.c
> @@ -0,0 +1,291 @@
> +/******************************************************************************
> + *
> + * Copyright 2007-2008 Samuel Thibault <samuel.thibault@eu.citrix.com>.
> + * All rights reserved.
> + * Use is subject to license terms.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#undef NDEBUG
> +#include "xen-external/bsd-sys-queue.h"
> +#include <mini-os/types.h>
> +#include <mini-os/os.h>
> +#include <mini-os/mm.h>
> +#include <mini-os/lib.h>
> +#include <mini-os/gntmap.h>
> +#include <mini-os/events.h>
> +#include <mini-os/wait.h>
> +#include <sys/mman.h>
> +
> +#include <xen/memory.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <assert.h>
> +#include <stdint.h>
> +#include <inttypes.h>
> +#include <malloc.h>
> +
> +#include "xc_private.h"
> +
> +#ifdef __RUMPRUN___
> +# define map_frames_ex minios_map_frames_ex
> +#endif /* __RUMPRUN__ */
> +
> +void minios_interface_close_fd(int fd);
> +void minios_gnttab_close_fd(int fd);
> +
> +static xc_osdep_handle minios_privcmd_open(xc_interface *xch)
> +{
> +    int fd = alloc_fd(FTYPE_XC);
> +
> +    if ( fd == -1)
> +        return XC_OSDEP_OPEN_ERROR;
> +
> +    return (xc_osdep_handle)fd;
> +}
> +
> +static int minios_privcmd_close(xc_interface *xch, xc_osdep_handle h)
> +{
> +    int fd = (int)h;
> +    return close(fd);
> +}
> +
> +void minios_interface_close_fd(int fd)
> +{
> +    files[fd].type = FTYPE_NONE;
> +}
> +
> +static void *minios_privcmd_alloc_hypercall_buffer(xc_interface *xch,
> +						   xc_osdep_handle h,
> +						   int npages)
> +{
> +    return xc_memalign(xch, PAGE_SIZE, npages * PAGE_SIZE);
> +}
> +
> +static void minios_privcmd_free_hypercall_buffer(xc_interface *xch,
> +						 xc_osdep_handle h,
> +						 void *ptr, int npages)
> +{
> +    free(ptr);
> +}
> +
> +static int minios_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h,
> +				    privcmd_hypercall_t *hypercall)
> +{
> +    multicall_entry_t call;
> +    int i, ret;
> +
> +    call.op = hypercall->op;
> +    for (i = 0; i < ARRAY_SIZE(hypercall->arg); i++)
> +	call.args[i] = hypercall->arg[i];
> +
> +    ret = HYPERVISOR_multicall(&call, 1);
> +
> +    if (ret < 0) {
> +	errno = -ret;
> +	return -1;
> +    }
> +    if ((long) call.result < 0) {
> +        errno = - (long) call.result;
> +        return -1;
> +    }
> +    return call.result;
> +}
> +
> +static void *minios_privcmd_map_foreign_bulk(xc_interface *xch,
> +					     xc_osdep_handle h,
> +                                             uint32_t dom, int prot,
> +                                             const xen_pfn_t *arr,
> +					     int *err, unsigned int num)
> +{
> +    unsigned long pt_prot = 0;
> +    if (prot & PROT_READ)
> +	pt_prot = L1_PROT_RO;
> +    if (prot & PROT_WRITE)
> +	pt_prot = L1_PROT;
> +    return map_frames_ex(arr, num, 1, 0, 1, dom, err, pt_prot);    
> +}
> +
> +static void *minios_privcmd_map_foreign_batch(xc_interface *xch,
> +					      xc_osdep_handle h,
> +                                              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;
> +}
> +
> +static void *minios_privcmd_map_foreign_range(xc_interface *xch,
> +					      xc_osdep_handle h,
> +                                              uint32_t dom,
> +                                              int size, int prot,
> +                                              unsigned long mfn)
> +{
> +    unsigned long pt_prot = 0;
> +
> +    if (prot & PROT_READ)
> +	pt_prot = L1_PROT_RO;
> +    if (prot & PROT_WRITE)
> +	pt_prot = L1_PROT;
> +
> +    assert(!(size % getpagesize()));
> +    return map_frames_ex(&mfn, size / getpagesize(), 0, 1, 1, dom,
> +			 NULL, pt_prot);
> +}
> +
> +static void *minios_privcmd_map_foreign_ranges(xc_interface *xch,
> +					       xc_osdep_handle h,
> +                                               uint32_t dom,
> +                                               size_t size, int prot,
> +					       size_t chunksize,
> +                                               privcmd_mmap_entry_t entries[],
> +					       int nentries)
> +{
> +    unsigned long *mfns;
> +    int i, j, n;
> +    unsigned long pt_prot = 0;
> +    void *ret;
> +
> +    if (prot & PROT_READ)
> +	pt_prot = L1_PROT_RO;
> +    if (prot & PROT_WRITE)
> +	pt_prot = L1_PROT;
> +
> +    mfns = malloc((size / XC_PAGE_SIZE) * sizeof(*mfns));
> +
> +    n = 0;
> +    for (i = 0; i < nentries; i++)
> +        for (j = 0; j < chunksize / XC_PAGE_SIZE; j++)
> +            mfns[n++] = entries[i].mfn + j;
> +
> +    ret = map_frames_ex(mfns, n, 1, 0, 1, dom, NULL, pt_prot);
> +    free(mfns);
> +    return ret;
> +}
> +
> +struct xc_osdep_ops xc_privcmd_ops = {
> +    .open = &minios_privcmd_open,
> +    .close = &minios_privcmd_close,
> +
> +    .u.privcmd = {
> +        .alloc_hypercall_buffer = &minios_privcmd_alloc_hypercall_buffer,
> +        .free_hypercall_buffer = &minios_privcmd_free_hypercall_buffer,
> +
> +        .hypercall = &minios_privcmd_hypercall,
> +
> +        .map_foreign_batch = &minios_privcmd_map_foreign_batch,
> +        .map_foreign_bulk = &minios_privcmd_map_foreign_bulk,
> +        .map_foreign_range = &minios_privcmd_map_foreign_range,
> +        .map_foreign_ranges = &minios_privcmd_map_foreign_ranges,
> +    },
> +};
> +
> +static xc_osdep_handle minios_gnttab_open(xc_gnttab *xcg)
> +{
> +    int fd = alloc_fd(FTYPE_GNTMAP);
> +    if ( fd == -1 )
> +        return XC_OSDEP_OPEN_ERROR;
> +    gntmap_init(&files[fd].gntmap);
> +    return (xc_osdep_handle)fd;
> +}
> +
> +static int minios_gnttab_close(xc_gnttab *xcg, xc_osdep_handle h)
> +{
> +    int fd = (int)h;
> +    return close(fd);
> +}
> +
> +void minios_gnttab_close_fd(int fd)
> +{
> +    gntmap_fini(&files[fd].gntmap);
> +    files[fd].type = FTYPE_NONE;
> +}
> +
> +static void *minios_gnttab_grant_map(xc_gnttab *xcg, xc_osdep_handle h,
> +                                     uint32_t count, int flags, int prot,
> +                                     uint32_t *domids, uint32_t *refs,
> +                                     uint32_t notify_offset,
> +                                     evtchn_port_t notify_port)
> +{
> +    int fd = (int)h;
> +    int stride = 1;
> +    if (flags & XC_GRANT_MAP_SINGLE_DOMAIN)
> +        stride = 0;
> +    if (notify_offset != -1 || notify_port != -1) {
> +        errno = ENOSYS;
> +        return NULL;
> +    }
> +    return gntmap_map_grant_refs(&files[fd].gntmap,
> +                                 count, domids, stride,
> +                                 refs, prot & PROT_WRITE);
> +}
> +
> +static int minios_gnttab_munmap(xc_gnttab *xcg, xc_osdep_handle h,
> +                                void *start_address,
> +                                uint32_t count)
> +{
> +    int fd = (int)h;
> +    int ret;
> +    ret = gntmap_munmap(&files[fd].gntmap,
> +                        (unsigned long) start_address,
> +                        count);
> +    if (ret < 0) {
> +        errno = -ret;
> +        return -1;
> +    }
> +    return ret;
> +}
> +
> +static int minios_gnttab_set_max_grants(xc_gnttab *xcg, xc_osdep_handle h,
> +                             uint32_t count)
> +{
> +    int fd = (int)h;
> +    int ret;
> +    ret = gntmap_set_max_grants(&files[fd].gntmap,
> +                                count);
> +    if (ret < 0) {
> +        errno = -ret;
> +        return -1;
> +    }
> +    return ret;
> +}
> +
> +struct xc_osdep_ops xc_gnttab_ops = {
> +    .open = &minios_gnttab_open,
> +    .close = &minios_gnttab_close,
> +
> +    .u.gnttab = {
> +        .grant_map = &minios_gnttab_grant_map,
> +        .munmap = &minios_gnttab_munmap,
> +        .set_max_grants = &minios_gnttab_set_max_grants,
> +    },
> +};
> +
> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
> index 45b8644..152465f 100644
> --- a/tools/libxc/xc_private.h
> +++ b/tools/libxc/xc_private.h
> @@ -111,6 +111,9 @@ struct xc_interface_core {
>      xc_osdep_handle  ops_handle; /* opaque data for xc_osdep_ops */
>  };
>  
> +/* Do not use these directly; go via the handle you already have. */
> +extern struct xc_osdep_ops xc_privcmd_ops, xc_evtchn_ops, xc_gnttab_ops;
> +
>  void xc_report_error(xc_interface *xch, int code, const char *fmt, ...)
>      __attribute__((format(printf,3,4)));
>  void xc_reportv(xc_interface *xch, xentoollog_logger *lg, xentoollog_level,