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

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

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?
> 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.

> + * (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.
> +
> +        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..
> +}
> +
> +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.
> +
> +    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?
> +            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.
> +        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?)

> @@ -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