diff mbox

[RFC,QEMU,v4,03/10] hostmem-xen: add a host memory backend for Xen

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

Commit Message

Haozhong Zhang Dec. 7, 2017, 10:18 a.m. UTC 
vNVDIMM requires a host memory backend to allocate its backend
resources to the guest. When QEMU is used as Xen device model, the
backend resource allocation of vNVDIMM is managed out of QEMU. A new
host memory backend 'memory-backend-xen' is introduced to represent
the backend resource allocated by Xen. It simply creates a memory
region of the specified size as a placeholder in the guest address
space, which will be mapped by Xen to the actual backend resource.

Following example QEMU options create a vNVDIMM device backed by a 4GB
host PMEM region at host physical address 0x100000000:
   -object memory-backend-xen,id=mem1,host-addr=0x100000000,size=4G
   -device nvdimm,id=nvdimm1,memdev=mem1

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
---
 backends/Makefile.objs |   1 +
 backends/hostmem-xen.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++
 backends/hostmem.c     |   9 +++++
 hw/mem/pc-dimm.c       |   6 ++-
 4 files changed, 123 insertions(+), 1 deletion(-)
 create mode 100644 backends/hostmem-xen.c

Comments

Anthony PERARD Feb. 27, 2018, 4:41 p.m. UTC | #1 
On Thu, Dec 07, 2017 at 06:18:05PM +0800, Haozhong Zhang wrote:
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index ee2c2d5bfd..ba13a52994 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -12,6 +12,7 @@
>  #include "qemu/osdep.h"
>  #include "sysemu/hostmem.h"
>  #include "hw/boards.h"
> +#include "hw/xen/xen.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
>  #include "qapi-types.h"
> @@ -277,6 +278,14 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
>              goto out;
>          }
>  
> +        /*
> +         * The backend storage of MEMORY_BACKEND_XEN is managed by Xen,
> +         * so no further work in this function is needed.
> +         */
> +        if (xen_enabled() && !backend->mr.ram_block) {
> +            goto out;
> +        }
> +
>          ptr = memory_region_get_ram_ptr(&backend->mr);
>          sz = memory_region_size(&backend->mr);
>  
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 66eace5a5c..dcbfce33d5 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -28,6 +28,7 @@
>  #include "sysemu/kvm.h"
>  #include "trace.h"
>  #include "hw/virtio/vhost.h"
> +#include "hw/xen/xen.h"
>  
>  typedef struct pc_dimms_capacity {
>       uint64_t size;
> @@ -108,7 +109,10 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
>      }
>  
>      memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> -    vmstate_register_ram(vmstate_mr, dev);
> +    /* memory-backend-xen is not backed by RAM. */
> +    if (!xen_enabled()) {

Is it possible to have the same condition as the one used in
host_memory_backend_memory_complete? i.e. base on whether the memory
region is mapped or not (backend->mr.ram_block).

> +        vmstate_register_ram(vmstate_mr, dev);
> +    }
>      numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
>  
>  out:
> -- 
> 2.15.1
>
Haozhong Zhang Feb. 28, 2018, 7:56 a.m. UTC | #2 
On 02/27/18 16:41 +0000, Anthony PERARD wrote:
> On Thu, Dec 07, 2017 at 06:18:05PM +0800, Haozhong Zhang wrote:
> > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > index ee2c2d5bfd..ba13a52994 100644
> > --- a/backends/hostmem.c
> > +++ b/backends/hostmem.c
> > @@ -12,6 +12,7 @@
> >  #include "qemu/osdep.h"
> >  #include "sysemu/hostmem.h"
> >  #include "hw/boards.h"
> > +#include "hw/xen/xen.h"
> >  #include "qapi/error.h"
> >  #include "qapi/visitor.h"
> >  #include "qapi-types.h"
> > @@ -277,6 +278,14 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
> >              goto out;
> >          }
> >  
> > +        /*
> > +         * The backend storage of MEMORY_BACKEND_XEN is managed by Xen,
> > +         * so no further work in this function is needed.
> > +         */
> > +        if (xen_enabled() && !backend->mr.ram_block) {
> > +            goto out;
> > +        }
> > +
> >          ptr = memory_region_get_ram_ptr(&backend->mr);
> >          sz = memory_region_size(&backend->mr);
> >  
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index 66eace5a5c..dcbfce33d5 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -28,6 +28,7 @@
> >  #include "sysemu/kvm.h"
> >  #include "trace.h"
> >  #include "hw/virtio/vhost.h"
> > +#include "hw/xen/xen.h"
> >  
> >  typedef struct pc_dimms_capacity {
> >       uint64_t size;
> > @@ -108,7 +109,10 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> >      }
> >  
> >      memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> > -    vmstate_register_ram(vmstate_mr, dev);
> > +    /* memory-backend-xen is not backed by RAM. */
> > +    if (!xen_enabled()) {
> 
> Is it possible to have the same condition as the one used in
> host_memory_backend_memory_complete? i.e. base on whether the memory
> region is mapped or not (backend->mr.ram_block).

Like "if (!xen_enabled() || backend->mr.ram_block))"? No, it will mute
the abortion (vmstate_register_ram --> qemu_ram_set_idstr ) caused by
the case that !backend->mr.ram_block in the non-xen environment.

Haozhong

> 
> > +        vmstate_register_ram(vmstate_mr, dev);
> > +    }
> >      numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
> >  
> >  out:
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Anthony PERARD
Anthony PERARD March 2, 2018, 11:50 a.m. UTC | #3 
On Wed, Feb 28, 2018 at 03:56:54PM +0800, Haozhong Zhang wrote:
> On 02/27/18 16:41 +0000, Anthony PERARD wrote:
> > On Thu, Dec 07, 2017 at 06:18:05PM +0800, Haozhong Zhang wrote:
> > > @@ -108,7 +109,10 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> > >      }
> > >  
> > >      memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> > > -    vmstate_register_ram(vmstate_mr, dev);
> > > +    /* memory-backend-xen is not backed by RAM. */
> > > +    if (!xen_enabled()) {
> > 
> > Is it possible to have the same condition as the one used in
> > host_memory_backend_memory_complete? i.e. base on whether the memory
> > region is mapped or not (backend->mr.ram_block).
> 
> Like "if (!xen_enabled() || backend->mr.ram_block))"? No, it will mute
> the abortion (vmstate_register_ram --> qemu_ram_set_idstr ) caused by
> the case that !backend->mr.ram_block in the non-xen environment.

In non-xen environment, vmstate_register_ram() will be called, because
!xen_enabled() is true, it would not matter if there is a ram_block or
not.

But if there is a memory-backend that can run in a xen environment that
have a ram_block, vmstate_register_ram would not be called in the
origial patch, but if we use (!xen_enabled() || vmstate_mr->ram_block)
as condition then vmstate_register_ram will be called.

Is this make sense?

> > > +        vmstate_register_ram(vmstate_mr, dev);
> > > +    }
> > >      numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
> > >  
> > >  out:
Haozhong Zhang March 5, 2018, 7:53 a.m. UTC | #4 
On 03/02/18 11:50 +0000, Anthony PERARD wrote:
> On Wed, Feb 28, 2018 at 03:56:54PM +0800, Haozhong Zhang wrote:
> > On 02/27/18 16:41 +0000, Anthony PERARD wrote:
> > > On Thu, Dec 07, 2017 at 06:18:05PM +0800, Haozhong Zhang wrote:
> > > > @@ -108,7 +109,10 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> > > >      }
> > > >  
> > > >      memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> > > > -    vmstate_register_ram(vmstate_mr, dev);
> > > > +    /* memory-backend-xen is not backed by RAM. */
> > > > +    if (!xen_enabled()) {
> > > 
> > > Is it possible to have the same condition as the one used in
> > > host_memory_backend_memory_complete? i.e. base on whether the memory
> > > region is mapped or not (backend->mr.ram_block).
> > 
> > Like "if (!xen_enabled() || backend->mr.ram_block))"? No, it will mute
> > the abortion (vmstate_register_ram --> qemu_ram_set_idstr ) caused by
> > the case that !backend->mr.ram_block in the non-xen environment.
> 
> In non-xen environment, vmstate_register_ram() will be called, because
> !xen_enabled() is true, it would not matter if there is a ram_block or
> not.

Sorry, I really meant 'if (backend->mr.ram_block)', which may mute the
abortion in non-xen environment. 'if (!xen_enabled())' keeps the
original semantics in non-xen environment, so it's unlikely to break
the non-xen usage.

Haozhong

> 
> But if there is a memory-backend that can run in a xen environment that
> have a ram_block, vmstate_register_ram would not be called in the
> origial patch, but if we use (!xen_enabled() || vmstate_mr->ram_block)
> as condition then vmstate_register_ram will be called.
> 
> Is this make sense?
> 
> > > > +        vmstate_register_ram(vmstate_mr, dev);
> > > > +    }
> > > >      numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
> > > >  
> > > >  out:
> 
> -- 
> Anthony PERARD
>
diff mbox

Patch

diff --git a/backends/Makefile.objs b/backends/Makefile.objs
index 0400799efd..3096fde21f 100644
--- a/backends/Makefile.objs
+++ b/backends/Makefile.objs
@@ -5,6 +5,7 @@  common-obj-$(CONFIG_TPM) += tpm.o
 
 common-obj-y += hostmem.o hostmem-ram.o
 common-obj-$(CONFIG_LINUX) += hostmem-file.o
+common-obj-${CONFIG_XEN_BACKEND} += hostmem-xen.o
 
 common-obj-y += cryptodev.o
 common-obj-y += cryptodev-builtin.o
diff --git a/backends/hostmem-xen.c b/backends/hostmem-xen.c
new file mode 100644
index 0000000000..99211efd81
--- /dev/null
+++ b/backends/hostmem-xen.c
@@ -0,0 +1,108 @@ 
+/*
+ * QEMU Host Memory Backend for Xen
+ *
+ * Copyright(C) 2017 Intel Corporation.
+ *
+ * Author:
+ *   Haozhong Zhang <haozhong.zhang@intel.com>
+ *
+ * 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 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, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+#include "sysemu/hostmem.h"
+#include "qapi/error.h"
+#include "qom/object_interfaces.h"
+
+#define TYPE_MEMORY_BACKEND_XEN "memory-backend-xen"
+
+#define MEMORY_BACKEND_XEN(obj) \
+    OBJECT_CHECK(HostMemoryBackendXen, (obj), TYPE_MEMORY_BACKEND_XEN)
+
+typedef struct HostMemoryBackendXen HostMemoryBackendXen;
+
+struct HostMemoryBackendXen {
+    HostMemoryBackend parent_obj;
+
+    uint64_t host_addr;
+};
+
+static void xen_backend_get_host_addr(Object *obj, Visitor *v, const char *name,
+                                      void *opaque, Error **errp)
+{
+    HostMemoryBackendXen *backend = MEMORY_BACKEND_XEN(obj);
+    uint64_t value = backend->host_addr;
+
+    visit_type_size(v, name, &value, errp);
+}
+
+static void xen_backend_set_host_addr(Object *obj, Visitor *v, const char *name,
+                                      void *opaque, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+    HostMemoryBackendXen *xb = MEMORY_BACKEND_XEN(obj);
+    Error *local_err = NULL;
+    uint64_t value;
+
+    if (memory_region_size(&backend->mr)) {
+        error_setg(&local_err, "cannot change property value");
+        goto out;
+    }
+
+    visit_type_size(v, name, &value, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    xb->host_addr = value;
+
+ out:
+    error_propagate(errp, local_err);
+}
+
+static void xen_backend_alloc(HostMemoryBackend *backend, Error **errp)
+{
+    if (!backend->size) {
+        error_setg(errp, "can't create backend with size 0");
+        return;
+    }
+    memory_region_init(&backend->mr, OBJECT(backend), "hostmem-xen",
+                       backend->size);
+    backend->mr.align = getpagesize();
+}
+
+static void xen_backend_class_init(ObjectClass *oc, void *data)
+{
+    HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
+
+    bc->alloc = xen_backend_alloc;
+
+    object_class_property_add(oc, "host-addr", "int",
+                              xen_backend_get_host_addr,
+                              xen_backend_set_host_addr,
+                              NULL, NULL, &error_abort);
+}
+
+static const TypeInfo xen_backend_info = {
+    .name = TYPE_MEMORY_BACKEND_XEN,
+    .parent = TYPE_MEMORY_BACKEND,
+    .class_init = xen_backend_class_init,
+    .instance_size = sizeof(HostMemoryBackendXen),
+};
+
+static void register_types(void)
+{
+    type_register_static(&xen_backend_info);
+}
+
+type_init(register_types);
diff --git a/backends/hostmem.c b/backends/hostmem.c
index ee2c2d5bfd..ba13a52994 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -12,6 +12,7 @@ 
 #include "qemu/osdep.h"
 #include "sysemu/hostmem.h"
 #include "hw/boards.h"
+#include "hw/xen/xen.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "qapi-types.h"
@@ -277,6 +278,14 @@  host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
             goto out;
         }
 
+        /*
+         * The backend storage of MEMORY_BACKEND_XEN is managed by Xen,
+         * so no further work in this function is needed.
+         */
+        if (xen_enabled() && !backend->mr.ram_block) {
+            goto out;
+        }
+
         ptr = memory_region_get_ram_ptr(&backend->mr);
         sz = memory_region_size(&backend->mr);
 
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 66eace5a5c..dcbfce33d5 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -28,6 +28,7 @@ 
 #include "sysemu/kvm.h"
 #include "trace.h"
 #include "hw/virtio/vhost.h"
+#include "hw/xen/xen.h"
 
 typedef struct pc_dimms_capacity {
      uint64_t size;
@@ -108,7 +109,10 @@  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
     }
 
     memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
-    vmstate_register_ram(vmstate_mr, dev);
+    /* memory-backend-xen is not backed by RAM. */
+    if (!xen_enabled()) {
+        vmstate_register_ram(vmstate_mr, dev);
+    }
     numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
 
 out: