Re: [Xen-devel] [RFC XEN PATCH 13/16] tools/libxl: add support to map host pmem device to guests

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On 01/27/17 17:06 -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Oct 10, 2016 at 08:32:32AM +0800, Haozhong Zhang wrote:
> > We can map host pmem devices or files on pmem devices to guests. This
> > patch adds support to map pmem devices. The implementation relies on the
> > Linux pmem driver, so it currently functions only when libxl is compiled
> Perhaps say when the pmem driver was introduced and also what CONFIG
> option to use to enable it?
Yes, I'll add the kernel version and CONFIG_*.

> > for Linux.
> >
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@xxxxxxxxx>
> > ---
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> >  tools/libxl/Makefile       |   2 +-
> >  tools/libxl/libxl_nvdimm.c | 210 +++++++++++++++++++++++++++++++++++++++++++++
> >  tools/libxl/libxl_nvdimm.h |  45 ++++++++++
> >  3 files changed, 256 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/libxl/libxl_nvdimm.c
> >  create mode 100644 tools/libxl/libxl_nvdimm.h
> >
> > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> > index a904927..ecc9ae1 100644
> > --- a/tools/libxl/Makefile
> > +++ b/tools/libxl/Makefile
> > @@ -106,7 +106,7 @@ ifeq ($(CONFIG_NetBSD),y)
> >  LIBXL_OBJS-y += libxl_netbsd.o
> >  else
> >  ifeq ($(CONFIG_Linux),y)
> > -LIBXL_OBJS-y += libxl_linux.o
> > +LIBXL_OBJS-y += libxl_linux.o libxl_nvdimm.o
> >  else
> >  ifeq ($(CONFIG_FreeBSD),y)
> >  LIBXL_OBJS-y += libxl_freebsd.o
> > diff --git a/tools/libxl/libxl_nvdimm.c b/tools/libxl/libxl_nvdimm.c
> > new file mode 100644
> > index 0000000..7bcbaaf
> > --- /dev/null
> > +++ b/tools/libxl/libxl_nvdimm.c
> > @@ -0,0 +1,210 @@
> > +/*
> > + * tools/libxl/libxl_nvdimm.c
> > + *
> > + * Copyright (c) 2016, Intel Corporation.
> 2017 now.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> LGPL please. libxl uses that license.
I'll fix the license.

> > + * (at your option) any later version.
> > + *
> > + * This program 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 General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> > + *
> > + * Author: Haozhong Zhang <haozhong.zhang@xxxxxxxxx>
> > + */
> > +
> > +#include <stdlib.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <unistd.h>
> > +#include <errno.h>
> > +#include <stdint.h>
> > +
> > +#include "libxl_internal.h"
> > +#include "libxl_arch.h"
> > +#include "libxl_nvdimm.h"
> > +
> > +#include <xc_dom.h>
> > +
> > +#define BLK_DEVICE_ROOT "/sys/dev/block"
> > +
> > +static int nvdimm_sysfs_read(libxl__gc *gc,
> > +                             unsigned int major, unsigned int minor,
> > +                             const char *name, void **data_r)
> > +{
> > +    char *path = libxl__sprintf(gc, BLK_DEVICE_ROOT"/%u:%u/device/%s",
> > +                                major, minor, name);
> > +    return libxl__read_sysfs_file_contents(gc, path, data_r, NULL);
> > +}
> > +
> > +static int nvdimm_get_spa(libxl__gc *gc, unsigned int major, unsigned int 
> > minor,
> > +                          uint64_t *spa_r)
> > +{
> > +    void *data;
> > +    int ret = nvdimm_sysfs_read(gc, major, minor, "resource", &data);
> > +
> > +    if ( ret )
> > +        return ret;
> > +
> > +    *spa_r = strtoll(data, NULL, 0);
> > +    return 0;
> > +}
> > +
> > +static int nvdimm_get_size(libxl__gc *gc, unsigned int major, unsigned int 
> > minor,
> > +                           uint64_t *size_r)
> > +{
> > +    void *data;
> > +    int ret = nvdimm_sysfs_read(gc, major, minor, "size", &data);
> > +
> > +    if ( ret )
> > +        return ret;
> > +
> > +    *size_r = strtoll(data, NULL, 0);
> > +
> > +    return 0;
> > +}
> > +
> > +static int add_pages(libxl__gc *gc, uint32_t domid,
> > +                     xen_pfn_t mfn, xen_pfn_t gpfn, unsigned long nr_mfns)
> > +{
> > +    unsigned int nr;
> > +    int ret = 0;
> > +
> > +    while ( nr_mfns )
> > +    {
> > +        nr = min(nr_mfns, (unsigned long) UINT_MAX);
> No need for space                           ^- here.
will remove spaces here and in other patches

> > +
> > +        ret = xc_domain_populate_pmemmap(CTX->xch, domid, mfn, gpfn, nr);
> > +        if ( ret )
> > +        {
> > +            LOG(ERROR, "failed to map pmem pages, "
> > +                "mfn 0x%" PRIx64", gpfn 0x%" PRIx64 ", nr_mfns %u, err %d",
> > +                mfn, gpfn, nr, ret);
> > +            break;
> > +        }
> > +
> > +        nr_mfns -= nr;
> > +        mfn += nr;
> > +        gpfn += nr;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static int add_file(libxl__gc *gc, uint32_t domid, int fd,
> > +                    xen_pfn_t mfn, xen_pfn_t gpfn, unsigned long nr_mfns)
> > +{
> > +    return -EINVAL;
> Hehehehe..
After consulting the driver developers, I'm going to remove the file
support in the version, because there is current no stable way to fix
the mapping between file extents and their physical locations (some of
my previous understanding about file mapping was wrong).

> > +}
> > +
> > +int libxl_nvdimm_add_device(libxl__gc *gc,
> > +                            uint32_t domid, const char *path,
> > +                            uint64_t guest_spa, uint64_t guest_size)
> > +{
> > +    int fd;
> > +    struct stat st;
> > +    unsigned int major, minor;
> > +    uint64_t host_spa, host_size;
> > +    xen_pfn_t mfn, gpfn;
> > +    unsigned long nr_gpfns;
> > +    int ret;
> > +
> > +    if ( (guest_spa & ~XC_PAGE_MASK) || (guest_size & ~XC_PAGE_MASK) )
> > +        return -EINVAL;
> That is the wrong return value. The libxl functions return enum
> libxl_error
>
> Please use those throughout the code.
Thanks for indicating this, I totally didn't notice it.

> > +
> > +    fd = open(path, O_RDONLY);
> > +    if ( fd < 0 )
> > +    {
> > +        LOG(ERROR, "failed to open file %s (err: %d)", path, errno);
> > +        return -EIO;
> > +    }
> > +
> > +    ret = fstat(fd, &st);
> > +    if ( ret )
> > +    {
> > +        LOG(ERROR, "failed to get status of file %s (err: %d)",
> > +            path, errno);
> > +        goto out;
> > +    }
> > +
> > +    switch ( st.st_mode & S_IFMT )
> > +    {
> > +    case S_IFBLK:
> > +        major = major(st.st_rdev);
> > +        minor = minor(st.st_rdev);
> > +        break;
> > +
> > +    case S_IFREG:
> > +        major = major(st.st_dev);
> > +        minor = minor(st.st_dev);
> > +        break;
> > +
> > +    default:
> > +        LOG(ERROR, "%s is neither a block device nor a regular file", path);
> > +        ret = -EINVAL;
> > +        goto out;
> > +    }
> > +
> > +    ret = nvdimm_get_spa(gc, major, minor, &host_spa);
> > +    if ( ret )
> > +    {
> > +        LOG(ERROR, "failed to get SPA of device %u:%u", major, minor);
> > +        goto out;
> > +    }
> > +    else if ( host_spa & ~XC_PAGE_MASK )
> > +    {
> > +        ret = -EINVAL;
> > +        goto out;
> > +    }
> > +
> > +    ret = nvdimm_get_size(gc, major, minor, &host_size);
> > +    if ( ret )
> > +    {
> > +        LOG(ERROR, "failed to get size of device %u:%u", major, minor);
> > +        goto out;
> > +    }
> > +    else if ( guest_size > host_size )
> > +    {
> > +        LOG(ERROR, "vNVDIMM size %" PRIu64 " expires NVDIMM size %" PRIu64,
> expires? larger?
larger

> > +            guest_size, host_size);
> > +        ret = -EINVAL;
> > +        goto out;
> > +    }
> > +
> > +    mfn = host_spa >> XC_PAGE_SHIFT;
> > +    gpfn = guest_spa >> XC_PAGE_SHIFT;
> > +    nr_gpfns = guest_size >> XC_PAGE_SHIFT;
> > +
> > +    switch ( st.st_mode & S_IFMT )
> > +    {
> > +    case S_IFBLK:
> > +        ret = add_pages(gc, domid, mfn, gpfn, nr_gpfns);
> You will need to change the return value.
yes

> > +        break;
> > +
> > +    case S_IFREG:
> > +        ret = add_file(gc, domid, fd, mfn, gpfn, nr_gpfns);
> Ditto here.
> > +        break;
> > +
> > +    default:
> > +        LOG(ERROR, "%s is neither a block device nor a regular file", path);
> > +        ret = -EINVAL;
> > +    }
> > +
> > + out:
> > +    close(fd);
> > +    return ret;
> > +}
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/tools/libxl/libxl_nvdimm.h b/tools/libxl/libxl_nvdimm.h
> > new file mode 100644
> > index 0000000..4de2fb2
> > --- /dev/null
> > +++ b/tools/libxl/libxl_nvdimm.h
> Why not add it in libxl.h?
>
> (Along with the LIBXL_HAVE_NVDIMM and such?)
will move them to libxl.h

Thanks,
Haozhong

> > @@ -0,0 +1,45 @@
> > +/*
> > + * tools/libxl/libxl_nvdimm.h
> > + *
> > + * Copyright (c) 2016, Intel Corporation.
> Now 2017
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> And please relicense it under LGPL
>
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program 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 General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> > + *
> > + * Author: Haozhong Zhang <haozhong.zhang@xxxxxxxxx>
> > + */
> > +
> > +#ifndef LIBXL_NVDIMM_H
> > +#define LIBXL_NVDIMM_H
> > +
> > +#include <stdint.h>
> > +#include "libxl_internal.h"
> > +
> > +#if defined(__linux__)
> > +
> > +int libxl_nvdimm_add_device(libxl__gc *gc,
> > +                            uint32_t domid, const char *path,
> > +                            uint64_t spa, uint64_t length);
> > +
> > +#else
> > +
> > +int libxl_nvdimm_add_device(libxl__gc *gc,
> > +                            uint32_t domid, const char *path,
> > +                            uint64_t spa, uint64_t length)
> > +{
> > +    return -EINVAL;
> > +}
> > +
> > +#endif /* __linux__ */
> > +
> > +#endif /* !LIBXL_NVDIMM_H */
> > --
> > 2.10.1
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx
> > https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel