From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [Xen-Devel] [PATCH] [GSOC14] refactored mempaging code from xenpaging to libxc. Date: Thu, 12 Jun 2014 15:11:53 +0100 Message-ID: <5399B529.7030608@citrix.com> References: <1402565030-22004-1-git-send-email-myselfdushyantbehl@gmail.com> <5399A984.6010300@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0204335090964203978==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andres Lagar Cavilla Cc: stefano.stabellini@eu.citrix.com, Dushyant Behl , dave.scott@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --===============0204335090964203978== Content-Type: multipart/alternative; boundary="------------060006030703080304090200" --------------060006030703080304090200 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit On 12/06/14 14:59, Andres Lagar Cavilla wrote: > On Thu, Jun 12, 2014 at 6:22 AM, Andrew Cooper > > 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 > > > Reviewed-by: Andres Lagar-Cavilla > > > > --- > > 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 > > +#include > > + > > +/* > > + * 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 --------------060006030703080304090200 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: 8bit
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> 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>
> Reviewed-by: Andres Lagar-Cavilla <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
--------------060006030703080304090200-- --===============0204335090964203978== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============0204335090964203978==--