Re: [Xen-Devel] [PATCH] [GSOC14] refactored mempaging code from xenpaging to libxc.

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6912 bytes --]

On 12/06/14 14:59, Andres Lagar Cavilla wrote:
> On Thu, Jun 12, 2014 at 6:22 AM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote:
>
>     On 12/06/14 10:23, Dushyant Behl wrote:
>     > This patch is part of the work done under the gsoc project -
>     > Lazy Restore Using Memory Paging.
>     >
>     > This patch moves the code to initialize mempaging from xenpaging
>     to libxc.
>
>     The source code is named libxc, but technically you are moving it into
>     libxenguest which is a separate library from libxenctrl.
>
>     > The code refractored from xenpaging is the code which sets up
>     paging,
>     > initializes a shared ring and event channel to communicate with
>     xen. This
>     > communication is done between the hypervisor and the dom0 tool
>     which performs
>     > the activity of pager. The xenpaging code is changed to use the
>     newly created
>     > routines and is tested to properly build and work with this code.
>     >
>     > The refractoring is done so that any tool which will act as pager in
>     > lazy restore or use memory paging can use a same routines to
>     initialize mempaging.
>     > This refactoring will also allow any future (in-tree) tools to
>     use mempaging.
>     >
>     > Signed-off-by: Dushyant Behl <myselfdushyantbehl@gmail.com
>     <mailto:myselfdushyantbehl@gmail.com>>
>     > Reviewed-by: Andres Lagar-Cavilla <andres@lagarcavilla.org
>     <mailto:andres@lagarcavilla.org>> 
>
>     > ---
>     >  tools/libxc/Makefile                |   3 +
>     >  tools/libxc/xc_mem_paging_setup.c   | 132
>     ++++++++++++++++++++++++++++++++++++
>     >  tools/libxc/xenctrl.h               |  12 ++++
>     >  tools/ocaml/libs/xc/xenctrl_stubs.c |  11 +--
>     >  tools/xenpaging/Makefile            |   4 +-
>     >  tools/xenpaging/xenpaging.c         |  93 +++----------------------
>     >  6 files changed, 167 insertions(+), 88 deletions(-)
>     >  create mode 100644 tools/libxc/xc_mem_paging_setup.c
>     >
>     > diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
>     > index a74b19e..2439853 100644
>     > --- a/tools/libxc/Makefile
>     > +++ b/tools/libxc/Makefile
>     > @@ -69,6 +69,9 @@ GUEST_SRCS-$(CONFIG_ARM)     +=
>     xc_dom_armzimageloader.c
>     >  GUEST_SRCS-y                 += xc_dom_binloader.c
>     >  GUEST_SRCS-y                 += xc_dom_compat_linux.c
>     >
>     > +# mem paging setup
>     > +GUEST_SRCS-y                 += xc_mem_paging_setup.c
>     > +
>
>     I don't think you need the comment here.  It is obvious from the
>     name of
>     the file, although I would suggest you name it "xc_dom_paging.c" or
>     something a tad less specific.
>
>     >  GUEST_SRCS-$(CONFIG_X86)     += xc_dom_x86.c
>     >  GUEST_SRCS-$(CONFIG_X86)     += xc_cpuid_x86.c
>     >  GUEST_SRCS-$(CONFIG_X86)     += xc_hvm_build_x86.c
>     > diff --git a/tools/libxc/xc_mem_paging_setup.c
>     b/tools/libxc/xc_mem_paging_setup.c
>     > new file mode 100644
>     > index 0000000..125d203
>     > --- /dev/null
>     > +++ b/tools/libxc/xc_mem_paging_setup.c
>     > @@ -0,0 +1,132 @@
>     > +/*
>     > + * tools/libxc/xc_mem_paging_setup.c
>     > + *
>     > + * Routines to initialize memory paging. Create shared ring
>     > + * and event channels to communicate with the hypervisor.
>     > + *
>     > + * Copyright (c) 2014 Dushyant Behl
>     > + * Copyright (c) 2009 by Citrix Systems, Inc. (Patrick Colp)
>     > + *
>     > + * 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; either
>     > + * version 2.1 of the License, or (at your option) any later
>     version.
>     > + *
>     > + * 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
>     > + */
>     > +
>     > +#include "xc_private.h"
>     > +#include <xen/event_channel.h>
>     > +#include <xen/mem_event.h>
>     > +
>     > +/*
>     > + * Mem paging ring and event channel setup routine.
>     > + * Setup a shared ring and an event channel to communicate between
>     > + * hypervisor and the tool performing mem paging operations.
>
>     Please document here, or in the header file, what the error
>     semantics of
>     the function are.  Libxc is a complete mess so these things need
>     spelling out.
>
>     > + */
>     > +int xc_mem_paging_ring_setup(xc_interface *xch, domid_t domain_id,
>     > +                                void *ring_page, int *port,
>     > +                                uint32_t *evtchn_port,
>     > +                                xc_evtchn **xce_handle,
>     > +                                mem_event_back_ring_t *back_ring)
>
>     The parameters should be aligned with the xc_interface.
>
>     > +{
>     > +    int rc;
>     > +    unsigned long ring_pfn, mmap_pfn;
>
>     uint64_t's as these are hvmparams
>
>     David Vrabel's GenID series fixes the prototypes of
>     xc_[gs]et_hvm_param() to be sane.
>
> Andrew, we were thinking this should be rebased onto your git branch
> for save restore rework, which I assume contains these GenID changes
> as well. Can you point to the right git coordinates? Thanks

As David and I are both working on our series at the same time, my
save/restore is a revision out of date and doesn't yet contain the new
prototypes.  If you can wait for Davids work to get accepted, I shall
rebase and provide a v5.5 or v6.

>
>     > +
>     > +    /* Map the ring page */
>     > +    xc_get_hvm_param(xch, domain_id, HVM_PARAM_PAGING_RING_PFN,
>     &ring_pfn);
>     > +    mmap_pfn = ring_pfn;
>     > +    ring_page = xc_map_foreign_batch(xch, domain_id,
>     > +                                        PROT_READ | PROT_WRITE,
>     &mmap_pfn, 1);
>
>     You are stuffing a pointer into an unsigned long?
>
> ring_page is void * ... mmap_pfn is ulong ....

So it is.  I got ring_pfn and ring_page confused.

>
>     > +    if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
>
>     What is this check for?
>
> foreign_batch has the old semantics of setting the MSB of the pfn in
> error. Maybe use map_foreign_bulk with cleaner error reporting?

Eww.  That is a nasty interface, and will completely break 32bit
toolstacks on machines with hotplug regions above the 8TB boundary.

Please do use map_foreign_bulk().

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 12808 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel