diff mbox

[RFC,XEN,v3,12/39] tools/xen-ndctl: add NVDIMM management util 'xen-ndctl'

Message ID 20170911043820.14617-13-haozhong.zhang@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Haozhong Zhang Sept. 11, 2017, 4:37 a.m. UTC
The kernel NVDIMM driver and the traditional NVDIMM management
utilities in Dom0 does not work now. 'xen-ndctl' is added as an
alternatively, which manages NVDIMM via Xen hypercalls.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 .gitignore             |   1 +
 tools/misc/Makefile    |   4 ++
 tools/misc/xen-ndctl.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 177 insertions(+)
 create mode 100644 tools/misc/xen-ndctl.c

Comments

Dan Williams Sept. 11, 2017, 5:10 a.m. UTC | #1
On Sun, Sep 10, 2017 at 9:37 PM, Haozhong Zhang
<haozhong.zhang@intel.com> wrote:
> The kernel NVDIMM driver and the traditional NVDIMM management
> utilities in Dom0 does not work now. 'xen-ndctl' is added as an
> alternatively, which manages NVDIMM via Xen hypercalls.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  .gitignore             |   1 +
>  tools/misc/Makefile    |   4 ++
>  tools/misc/xen-ndctl.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 177 insertions(+)
>  create mode 100644 tools/misc/xen-ndctl.c

What about my offer to move this functionality into the upstream ndctl
utility [1]? I think it is thoroughly confusing that you are reusing
the name 'ndctl' and avoiding integration with the upstream ndctl
utility.

[1]: https://patchwork.kernel.org/patch/9632865/
Haozhong Zhang Sept. 11, 2017, 5:39 a.m. UTC | #2
On 09/10/17 22:10 -0700, Dan Williams wrote:
> On Sun, Sep 10, 2017 at 9:37 PM, Haozhong Zhang
> <haozhong.zhang@intel.com> wrote:
> > The kernel NVDIMM driver and the traditional NVDIMM management
> > utilities in Dom0 does not work now. 'xen-ndctl' is added as an
> > alternatively, which manages NVDIMM via Xen hypercalls.
> >
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  .gitignore             |   1 +
> >  tools/misc/Makefile    |   4 ++
> >  tools/misc/xen-ndctl.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 177 insertions(+)
> >  create mode 100644 tools/misc/xen-ndctl.c
> 
> What about my offer to move this functionality into the upstream ndctl
> utility [1]? I think it is thoroughly confusing that you are reusing
> the name 'ndctl' and avoiding integration with the upstream ndctl
> utility.
> 
> [1]: https://patchwork.kernel.org/patch/9632865/

I'm not object to integrate it with ndctl.

My only concern is that the integration will introduces two types of
user interface. The upstream ndctl works with the kernel driver and
provides easily used *names* (e.g., namespace0.0, region0, nmem0,
etc.) for user input. However, this version patchset hides NFIT from
Dom0 (to simplify the first implementation), so the kernel driver does
not work in Dom0, neither does ndctl. Instead, xen-ndctl has to use
*the physical address* for users to specify their interested NVDIMM
region, which is different from upstream ndctl.


Haozhong
Dan Williams Sept. 11, 2017, 4:35 p.m. UTC | #3
On Sun, Sep 10, 2017 at 10:39 PM, Haozhong Zhang
<haozhong.zhang@intel.com> wrote:
> On 09/10/17 22:10 -0700, Dan Williams wrote:
>> On Sun, Sep 10, 2017 at 9:37 PM, Haozhong Zhang
>> <haozhong.zhang@intel.com> wrote:
>> > The kernel NVDIMM driver and the traditional NVDIMM management
>> > utilities in Dom0 does not work now. 'xen-ndctl' is added as an
>> > alternatively, which manages NVDIMM via Xen hypercalls.
>> >
>> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> > ---
>> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> > Cc: Wei Liu <wei.liu2@citrix.com>
>> > ---
>> >  .gitignore             |   1 +
>> >  tools/misc/Makefile    |   4 ++
>> >  tools/misc/xen-ndctl.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 177 insertions(+)
>> >  create mode 100644 tools/misc/xen-ndctl.c
>>
>> What about my offer to move this functionality into the upstream ndctl
>> utility [1]? I think it is thoroughly confusing that you are reusing
>> the name 'ndctl' and avoiding integration with the upstream ndctl
>> utility.
>>
>> [1]: https://patchwork.kernel.org/patch/9632865/
>
> I'm not object to integrate it with ndctl.
>
> My only concern is that the integration will introduces two types of
> user interface. The upstream ndctl works with the kernel driver and
> provides easily used *names* (e.g., namespace0.0, region0, nmem0,
> etc.) for user input. However, this version patchset hides NFIT from
> Dom0 (to simplify the first implementation), so the kernel driver does
> not work in Dom0, neither does ndctl. Instead, xen-ndctl has to use
> *the physical address* for users to specify their interested NVDIMM
> region, which is different from upstream ndctl.

Ok, I think this means that xen-ndctl should be renamed (xen-nvdimm?)
so that the distinction between the 2 tools is clear.
Konrad Rzeszutek Wilk Sept. 11, 2017, 9:24 p.m. UTC | #4
On Mon, Sep 11, 2017 at 09:35:08AM -0700, Dan Williams wrote:
> On Sun, Sep 10, 2017 at 10:39 PM, Haozhong Zhang
> <haozhong.zhang@intel.com> wrote:
> > On 09/10/17 22:10 -0700, Dan Williams wrote:
> >> On Sun, Sep 10, 2017 at 9:37 PM, Haozhong Zhang
> >> <haozhong.zhang@intel.com> wrote:
> >> > The kernel NVDIMM driver and the traditional NVDIMM management
> >> > utilities in Dom0 does not work now. 'xen-ndctl' is added as an
> >> > alternatively, which manages NVDIMM via Xen hypercalls.
> >> >
> >> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >> > ---
> >> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> > Cc: Wei Liu <wei.liu2@citrix.com>
> >> > ---
> >> >  .gitignore             |   1 +
> >> >  tools/misc/Makefile    |   4 ++
> >> >  tools/misc/xen-ndctl.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  3 files changed, 177 insertions(+)
> >> >  create mode 100644 tools/misc/xen-ndctl.c
> >>
> >> What about my offer to move this functionality into the upstream ndctl
> >> utility [1]? I think it is thoroughly confusing that you are reusing
> >> the name 'ndctl' and avoiding integration with the upstream ndctl
> >> utility.
> >>
> >> [1]: https://patchwork.kernel.org/patch/9632865/
> >
> > I'm not object to integrate it with ndctl.
> >
> > My only concern is that the integration will introduces two types of
> > user interface. The upstream ndctl works with the kernel driver and
> > provides easily used *names* (e.g., namespace0.0, region0, nmem0,
> > etc.) for user input. However, this version patchset hides NFIT from
> > Dom0 (to simplify the first implementation), so the kernel driver does
> > not work in Dom0, neither does ndctl. Instead, xen-ndctl has to use
> > *the physical address* for users to specify their interested NVDIMM
> > region, which is different from upstream ndctl.
> 
> Ok, I think this means that xen-ndctl should be renamed (xen-nvdimm?)
> so that the distinction between the 2 tools is clear.

I think it makes much more sense to integrate this in the upstream
version of ndctl. As surely in the future the ndctl will need to work
with other OSes too? Such as FreeBSD?
Dan Williams Sept. 13, 2017, 5:45 p.m. UTC | #5
On Mon, Sep 11, 2017 at 2:24 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Mon, Sep 11, 2017 at 09:35:08AM -0700, Dan Williams wrote:
>> On Sun, Sep 10, 2017 at 10:39 PM, Haozhong Zhang
>> <haozhong.zhang@intel.com> wrote:
>> > On 09/10/17 22:10 -0700, Dan Williams wrote:
>> >> On Sun, Sep 10, 2017 at 9:37 PM, Haozhong Zhang
>> >> <haozhong.zhang@intel.com> wrote:
>> >> > The kernel NVDIMM driver and the traditional NVDIMM management
>> >> > utilities in Dom0 does not work now. 'xen-ndctl' is added as an
>> >> > alternatively, which manages NVDIMM via Xen hypercalls.
>> >> >
>> >> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> >> > ---
>> >> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> >> > Cc: Wei Liu <wei.liu2@citrix.com>
>> >> > ---
>> >> >  .gitignore             |   1 +
>> >> >  tools/misc/Makefile    |   4 ++
>> >> >  tools/misc/xen-ndctl.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >  3 files changed, 177 insertions(+)
>> >> >  create mode 100644 tools/misc/xen-ndctl.c
>> >>
>> >> What about my offer to move this functionality into the upstream ndctl
>> >> utility [1]? I think it is thoroughly confusing that you are reusing
>> >> the name 'ndctl' and avoiding integration with the upstream ndctl
>> >> utility.
>> >>
>> >> [1]: https://patchwork.kernel.org/patch/9632865/
>> >
>> > I'm not object to integrate it with ndctl.
>> >
>> > My only concern is that the integration will introduces two types of
>> > user interface. The upstream ndctl works with the kernel driver and
>> > provides easily used *names* (e.g., namespace0.0, region0, nmem0,
>> > etc.) for user input. However, this version patchset hides NFIT from
>> > Dom0 (to simplify the first implementation), so the kernel driver does
>> > not work in Dom0, neither does ndctl. Instead, xen-ndctl has to use
>> > *the physical address* for users to specify their interested NVDIMM
>> > region, which is different from upstream ndctl.
>>
>> Ok, I think this means that xen-ndctl should be renamed (xen-nvdimm?)
>> so that the distinction between the 2 tools is clear.
>
> I think it makes much more sense to integrate this in the upstream
> version of ndctl. As surely in the future the ndctl will need to work
> with other OSes too? Such as FreeBSD?

I'm receptive to carrying Xen-specific enabling and / or a FreeBSD
compat layer in ndctl.
diff mbox

Patch

diff --git a/.gitignore b/.gitignore
index ecb198f914..30655673f7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -216,6 +216,7 @@  tools/misc/xen-hvmctx
 tools/misc/xenlockprof
 tools/misc/lowmemd
 tools/misc/xencov
+tools/misc/xen-ndctl
 tools/pkg-config/*
 tools/qemu-xen-build
 tools/xentrace/xenalyze
diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index eaa28793ef..124775b7f4 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -32,6 +32,7 @@  INSTALL_SBIN                   += xenpm
 INSTALL_SBIN                   += xenwatchdogd
 INSTALL_SBIN                   += xen-livepatch
 INSTALL_SBIN                   += xen-diag
+INSTALL_SBIN                   += xen-ndctl
 INSTALL_SBIN += $(INSTALL_SBIN-y)
 
 # Everything to be installed in a private bin/
@@ -118,4 +119,7 @@  xen-lowmemd: xen-lowmemd.o
 xencov: xencov.o
 	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
 
+xen-ndctl: xen-ndctl.o
+	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+
 -include $(DEPS_INCLUDE)
diff --git a/tools/misc/xen-ndctl.c b/tools/misc/xen-ndctl.c
new file mode 100644
index 0000000000..de40e29ff6
--- /dev/null
+++ b/tools/misc/xen-ndctl.c
@@ -0,0 +1,172 @@ 
+/*
+ * xen-ndctl.c
+ *
+ * Xen NVDIMM management tool
+ *
+ * Copyright (C) 2017,  Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without restriction,
+ * including without limitation the rights to use, copy, modify, merge,
+ * publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
+ * IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
+ * CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
+ * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
+ * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <errno.h>
+#include <stdio.h>
+#include <string.h>
+#include <xenctrl.h>
+
+static xc_interface *xch;
+
+static int handle_help(int argc, char *argv[]);
+static int handle_list_cmds(int argc, char *argv[]);
+
+static const struct xen_ndctl_cmd
+{
+    const char *name;
+    const char *syntax;
+    const char *help;
+    int (*handler)(int argc, char **argv);
+    bool need_xc;
+} cmds[] =
+{
+    {
+        .name    = "help",
+        .syntax  = "[command]",
+        .help    = "Show this message or the help message of 'command'.\n"
+                   "Use command 'list-cmds' to list all supported commands.\n",
+        .handler = handle_help,
+    },
+
+    {
+        .name    = "list-cmds",
+        .syntax  = "",
+        .help    = "List all supported commands.\n",
+        .handler = handle_list_cmds,
+    },
+};
+
+static const unsigned int nr_cmds = sizeof(cmds) / sizeof(cmds[0]);
+
+static void show_help(const char *cmd)
+{
+    unsigned int i;
+
+    if ( !cmd )
+    {
+        fprintf(stderr,
+                "Usage: xen-ndctl <command> [args]\n\n"
+                "List all supported commands by 'xen-ndctl list-cmds'.\n"
+                "Get help of a command by 'xen-ndctl help <command>'.\n");
+        return;
+    }
+
+    for ( i = 0; i < nr_cmds; i++ )
+        if ( !strcmp(cmd, cmds[i].name) )
+        {
+            fprintf(stderr, "Usage: xen-ndctl %s %s\n\n%s",
+                    cmds[i].name, cmds[i].syntax, cmds[i].help);
+            break;
+        }
+
+    if ( i == nr_cmds )
+        fprintf(stderr, "Unsupported command '%s'.\n"
+                "List all supported commands by 'xen-ndctl list-cmds'.\n",
+                cmd);
+}
+
+static int handle_unrecognized_argument(const char *cmd, const char *argv)
+{
+    fprintf(stderr, "Unrecognized argument: %s.\n\n", argv);
+    show_help(cmd);
+
+    return -EINVAL;
+}
+
+static int handle_help(int argc, char *argv[])
+{
+    if ( argc == 1 )
+        show_help(NULL);
+    else if ( argc == 2 )
+        show_help(argv[1]);
+    else
+        return handle_unrecognized_argument(argv[0], argv[2]);
+
+    return 0;
+}
+
+static int handle_list_cmds(int argc, char *argv[])
+{
+    unsigned int i;
+
+    if ( argc > 1 )
+        return handle_unrecognized_argument(argv[0], argv[1]);
+
+    for ( i = 0; i < nr_cmds; i++ )
+        fprintf(stderr, "%s\n", cmds[i].name);
+
+    return 0;
+}
+
+int main(int argc, char *argv[])
+{
+    unsigned int i;
+    int rc = 0;
+    const char *cmd;
+
+    if ( argc <= 1 )
+    {
+        show_help(NULL);
+        return 0;
+    }
+
+    cmd = argv[1];
+
+    for ( i = 0; i < nr_cmds; i++ )
+        if ( !strcmp(cmd, cmds[i].name) )
+        {
+            if ( cmds[i].need_xc )
+            {
+                xch = xc_interface_open(0, 0, 0);
+                if ( !xch )
+                {
+                    rc = -errno;
+                    fprintf(stderr, "Cannot get xc handler: %s\n",
+                            strerror(errno));
+                    break;
+                }
+            }
+            rc = cmds[i].handler(argc - 1, &argv[1]);
+            if ( rc )
+                fprintf(stderr, "\n'%s' failed: %s\n",
+                        cmds[i].name, strerror(-rc));
+            break;
+        }
+
+    if ( i == nr_cmds )
+    {
+        fprintf(stderr, "Unsupported command '%s'. "
+                "List all supported commands by 'xen-ndctl list-cmds'.\n",
+                cmd);
+        rc = -ENOSYS;
+    }
+
+    if ( xch )
+        xc_interface_close(xch);
+
+    return rc;
+}