[Qemu-devel] [PATCH RFCv2 0/9] qdev: Hotplug handler chaining + virtio-pmem

[Qemu-devel] [PATCH RFCv2 0/9] qdev: Hotplug handler chaining + virtio-pmem

[David Hildenbrand]

This series implements supprt for hotplug handler chaining (proposed
by Igor), something that is necessary to turn selected virtio devices into
memory devices. Planned devices inlude virtio-mem and virtio-pmem. The
current prototype of virtio-pmem is included.

The machine hotplug handler can intercept hotplug handler calls
to properly prepare/teardown the memory device part of a device. Control
is then passed on to the actual bus hotplug handler. So the default hotplug
handler is effectively overwritten to make interception possible.

It is based on the following patches/series
- [PATCH v1] pc: Use hotplug_handler_(plug|unplug|unplug_request)
-- Queued by Paolo
- [PATCH v3 0/2] s390x/pci: hotplug handler fixes and reworks

Patch 1-3 are the preparations for hotplug handler chaining. The remaining
patches are a modified prototype of virtio-pmem.

TODO:
- More testing for patch #1 + eventually write some unplug tests
- Pankaj has to fixup some things in "virtio-pmem: Prototype"
-- See patch description for details

I modified Pankajs work to work with this series. virtio-pmem is included
as it was requested during review of previous preparations to showcase a
real user, so we can discuss if this is good enough for us or if we have
to do further changes.

More details about virtio-pmem (including the Linux guest driver side)
can be found at:
    https://lkml.org/lkml/2018/7/13/102
    https://lkml.org/lkml/2019/1/9/756

Example: defining a simple virtio-pmem device (on /dev/zero for simplicity):

    qemu-system-x86_64 \
     -machine pc \
     -monitor stdio \
     -m 8G,maxmem=20G \
     -object memory-backend-file,id=mem1,mem-path=/dev/zero,size=4G \
     -device virtio-pmem-pci,id=vp1,memdev=mem1

QEMU 3.0.50 monitor - type 'help' for more information
(qemu) info memory-devices
Memory device [virtio-pmem]: "vp1"
  memaddr: 0x240000000
  size: 4294967296
  memdev: /objects/mem1

(qemu) info memory_size_summary
base memory: 8589934592
plugged memory: 4294967296

RFC -> RFCv2
- "virtio-pmem: Prototype"
- Minor documentation/style changes
- "virtio-pci: Proxy for virtio-pmem"
-- Separate header file virtio-pmem-pci.h
- "pc: Support for virtio-pmem-pci"
-- Only handle virtio-pmem-pci specially


David Hildenbrand (6):
  qdev: Let the hotplug_handler_unplug() caller delete the device
  qdev: Provide qdev_get_bus_hotplug_handler()
  virtio-pci: Allow to specify additional interfaces for the base type
  hmp: Handle virtio-pmem when printing memory device infos
  numa: Handle virtio-pmem in NUMA stats
  pc: Support for virtio-pmem-pci

Igor Mammedov (1):
  qdev: Let machine hotplug handler to override bus hotplug handler

Pankaj Gupta (2):
  virtio-pmem: Prototype
  virtio-pci: Proxy for virtio-pmem

 default-configs/i386-softmmu.mak            |   1 +
 hmp.c                                       |  27 +--
 hw/acpi/cpu.c                               |   1 +
 hw/acpi/memory_hotplug.c                    |   1 +
 hw/acpi/pcihp.c                             |   3 +-
 hw/core/qdev.c                              |  19 +-
 hw/i386/pc.c                                |  75 +++++++-
 hw/pci/pcie.c                               |   3 +-
 hw/pci/shpc.c                               |   3 +-
 hw/ppc/spapr.c                              |   4 +-
 hw/ppc/spapr_pci.c                          |   3 +-
 hw/s390x/css-bridge.c                       |   2 +-
 hw/s390x/s390-pci-bus.c                     |  13 +-
 hw/virtio/Makefile.objs                     |   3 +
 hw/virtio/virtio-pci.c                      |   1 +
 hw/virtio/virtio-pci.h                      |   1 +
 hw/virtio/virtio-pmem-pci.c                 | 144 +++++++++++++++
 hw/virtio/virtio-pmem-pci.h                 |  39 ++++
 hw/virtio/virtio-pmem.c                     | 191 ++++++++++++++++++++
 include/hw/qdev-core.h                      |  12 ++
 include/hw/virtio/virtio-pmem.h             |  54 ++++++
 include/standard-headers/linux/virtio_ids.h |   1 +
 numa.c                                      |  24 +--
 qapi/misc.json                              |  28 ++-
 qdev-monitor.c                              |   9 +-
 25 files changed, 612 insertions(+), 50 deletions(-)
 create mode 100644 hw/virtio/virtio-pmem-pci.c
 create mode 100644 hw/virtio/virtio-pmem-pci.h
 create mode 100644 hw/virtio/virtio-pmem.c
 create mode 100644 include/hw/virtio/virtio-pmem.h

-- 
2.17.2

[Qemu-devel] [PATCH RFCv2 1/9] qdev: Let the hotplug_handler_unplug() caller delete the device

[David Hildenbrand]

When unplugging a device, at one point the device will be destroyed
via object_unparent(). This will, one the one hand, unrealize the
removed device hierarchy, and on the other hand, destroy/free the
device hierarchy.

When chaining hotplug handlers, we want to overwrite a bus hotplug
handler by the machine hotplug handler, to be able to perform
some part of the plug/unplug and to forward the calls to the bus hotplug
handler.

For now, the bus hotplug handler would trigger an object_unparent(), not
allowing us to perform some unplug action on a device after we forwarded
the call to the bus hotplug handler. The device would be gone at that
point.

machine_unplug_handler(dev)
    /* eventually do unplug stuff */
    bus_unplug_handler(dev)
    /* dev is gone, we can't do more unplug stuff */

So move the object_unparent() to the original caller of the unplug. For
now, keep the unrealize() at the original places of the
object_unparent(). For implicitly chained hotplug handlers (e.g. pc
code calling acpi hotplug handlers), the object_unparent() has to be
done by the outermost caller. So when calling hotplug_handler_unplug()
from inside an unplug handler, nothing is to be done.

hotplug_handler_unplug(dev) -> calls machine_unplug_handler()
    machine_unplug_handler(dev) {
        /* eventually do unplug stuff */
        bus_unplug_handler(dev) -> calls unrealize(dev)
        /* we can do more unplug stuff but device already unrealized */
    }
object_unparent(dev)

In the long run, every unplug action should be factored out of the
unrealize() function into the unplug handler (especially for PCI). Then
we can get rid of the additonal unrealize() calls and object_unparent()
will properly unrealize the device hierarchy after the device has been
unplugged.

hotplug_handler_unplug(dev) -> calls machine_unplug_handler()
    machine_unplug_handler(dev) {
        /* eventually do unplug stuff */
        bus_unplug_handler(dev) -> only unplugs, does not unrealize
        /* we can do more unplug stuff */
    }
object_unparent(dev) -> will unrealize

The original approach was suggested by Igor Mammedov for the PCI
part, but I extended it to all hotplug handlers. I consider this one
step into the right direction.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/acpi/cpu.c            |  1 +
 hw/acpi/memory_hotplug.c |  1 +
 hw/acpi/pcihp.c          |  3 ++-
 hw/core/qdev.c           |  3 +--
 hw/i386/pc.c             |  5 ++---
 hw/pci/pcie.c            |  3 ++-
 hw/pci/shpc.c            |  3 ++-
 hw/ppc/spapr.c           |  4 ++--
 hw/ppc/spapr_pci.c       |  3 ++-
 hw/s390x/css-bridge.c    |  2 +-
 hw/s390x/s390-pci-bus.c  | 13 ++++++++-----
 qdev-monitor.c           |  9 +++++++--
 12 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index f10b190019..37703a8806 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -126,6 +126,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data,
             dev = DEVICE(cdev->cpu);
             hotplug_ctrl = qdev_get_hotplug_handler(dev);
             hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
+            object_unparent(OBJECT(dev));
         }
         break;
     case ACPI_CPU_CMD_OFFSET_WR:
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 921cad2c5e..297812d5f7 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -189,6 +189,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
                 error_free(local_err);
                 break;
             }
+            object_unparent(OBJECT(dev));
             trace_mhp_acpi_pc_dimm_deleted(mem_st->selector);
         }
         break;
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 7bc7a72340..31b1a8fe58 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -174,6 +174,7 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
             if (!acpi_pcihp_pc_no_hotplug(s, dev)) {
                 hotplug_ctrl = qdev_get_hotplug_handler(qdev);
                 hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort);
+                object_unparent(OBJECT(qdev));
             }
         }
     }
@@ -269,7 +270,7 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
 void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
                                  DeviceState *dev, Error **errp)
 {
-    object_unparent(OBJECT(dev));
+    object_property_set_bool(OBJECT(dev), false, "realized", NULL);
 }
 
 void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d59071b8ed..278cc094ec 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -286,8 +286,7 @@ void qbus_reset_all_fn(void *opaque)
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
                                   DeviceState *dev, Error **errp)
 {
-    /* just zap it */
-    object_unparent(OBJECT(dev));
+    object_property_set_bool(OBJECT(dev), false, "realized", NULL);
 }
 
 /*
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 450a144e3f..fd0cb29ba9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1998,8 +1998,7 @@ static void pc_memory_unplug(HotplugHandler *hotplug_dev,
     }
 
     pc_dimm_unplug(PC_DIMM(dev), MACHINE(pcms));
-    object_unparent(OBJECT(dev));
-
+    object_property_set_bool(OBJECT(dev), false, "realized", NULL);
  out:
     error_propagate(errp, local_err);
 }
@@ -2105,7 +2104,7 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
 
     found_cpu = pc_find_cpu_slot(MACHINE(pcms), cpu->apic_id, NULL);
     found_cpu->cpu = NULL;
-    object_unparent(OBJECT(dev));
+    object_property_set_bool(OBJECT(dev), false, "realized", NULL);
 
     /* decrement the number of CPUs */
     pcms->boot_cpus--;
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 230478faab..9b829c8434 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -450,7 +450,7 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
 void pcie_cap_slot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                              Error **errp)
 {
-    object_unparent(OBJECT(dev));
+    object_property_set_bool(OBJECT(dev), false, "realized", NULL);
 }
 
 static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
@@ -458,6 +458,7 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
     HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(dev));
 
     hotplug_handler_unplug(hotplug_ctrl, DEVICE(dev), &error_abort);
+    object_unparent(OBJECT(dev));
 }
 
 void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 45053b39b9..44620ad845 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -249,6 +249,7 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
             hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(affected_dev));
             hotplug_handler_unplug(hotplug_ctrl, DEVICE(affected_dev),
                                    &error_abort);
+            object_unparent(OBJECT(affected_dev));
         }
     }
 }
@@ -546,7 +547,7 @@ void shpc_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
 void shpc_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                            Error **errp)
 {
-    object_unparent(OBJECT(dev));
+    object_property_set_bool(OBJECT(dev), false, "realized", NULL);
 }
 
 void shpc_device_unplug_request_cb(HotplugHandler *hotplug_dev,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0942f35bf8..9675024c8b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3450,7 +3450,7 @@ static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
-    object_unparent(OBJECT(dev));
+    object_property_set_bool(OBJECT(dev), false, "realized", NULL);
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }
 
@@ -3557,7 +3557,7 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
 
     assert(core_slot);
     core_slot->cpu = NULL;
-    object_unparent(OBJECT(dev));
+    object_property_set_bool(OBJECT(dev), false, "realized", NULL);
 }
 
 static
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 5cdc98513d..a2babde3da 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1373,6 +1373,7 @@ void spapr_phb_remove_pci_device_cb(DeviceState *dev)
     HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
 
     hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+    object_unparent(OBJECT(dev));
 }
 
 static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb,
@@ -1495,7 +1496,7 @@ static void spapr_pci_unplug(HotplugHandler *plug_handler,
      * an 'idle' state, as the device cleanup code expects.
      */
     pci_device_reset(PCI_DEVICE(plugged_dev));
-    object_unparent(OBJECT(plugged_dev));
+    object_property_set_bool(OBJECT(plugged_dev), false, "realized", NULL);
 }
 
 static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
index 1bd6c8b458..614c2a3b8b 100644
--- a/hw/s390x/css-bridge.c
+++ b/hw/s390x/css-bridge.c
@@ -51,7 +51,7 @@ static void ccw_device_unplug(HotplugHandler *hotplug_dev,
 
     css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0);
 
-    object_unparent(OBJECT(dev));
+    object_property_set_bool(OBJECT(dev), false, "realized", NULL);
 }
 
 static void virtual_css_bus_reset(BusState *qbus)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 6be4364fce..61a9fe4596 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -154,14 +154,17 @@ static void s390_pci_perform_unplug(S390PCIBusDevice *pbdev)
 
     /* Unplug the PCI device */
     if (pbdev->pdev) {
-        hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(pbdev->pdev));
-        hotplug_handler_unplug(hotplug_ctrl, DEVICE(pbdev->pdev),
-                               &error_abort);
+        DeviceState *pdev = DEVICE(pbdev->pdev);
+
+        hotplug_ctrl = qdev_get_hotplug_handler(pdev);
+        hotplug_handler_unplug(hotplug_ctrl, pdev, &error_abort);
+        object_unparent(OBJECT(pdev));
     }
 
     /* Unplug the zPCI device */
     hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(pbdev));
     hotplug_handler_unplug(hotplug_ctrl, DEVICE(pbdev), &error_abort);
+    object_unparent(OBJECT(pbdev));
 }
 
 void s390_pci_sclp_deconfigure(SCCB *sccb)
@@ -1019,7 +1022,7 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                      pbdev->fh, pbdev->fid);
         bus = pci_get_bus(pci_dev);
         devfn = pci_dev->devfn;
-        object_unparent(OBJECT(pci_dev));
+        object_property_set_bool(OBJECT(dev), false, "realized", NULL);
 
         s390_pci_msix_free(pbdev);
         s390_pci_iommu_free(s, bus, devfn);
@@ -1036,7 +1039,7 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
         pbdev->fid = 0;
         QTAILQ_REMOVE(&s->zpci_devs, pbdev, link);
         g_hash_table_remove(s->zpci_table, &pbdev->idx);
-        object_unparent(OBJECT(pbdev));
+        object_property_set_bool(OBJECT(dev), false, "realized", NULL);
     }
 }
 
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 07147c63bf..7705acd6c7 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -862,6 +862,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
     HotplugHandler *hotplug_ctrl;
     HotplugHandlerClass *hdc;
+    Error *local_err = NULL;
 
     if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
         error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
@@ -890,10 +891,14 @@ void qdev_unplug(DeviceState *dev, Error **errp)
      * otherwise just remove it synchronously */
     hdc = HOTPLUG_HANDLER_GET_CLASS(hotplug_ctrl);
     if (hdc->unplug_request) {
-        hotplug_handler_unplug_request(hotplug_ctrl, dev, errp);
+        hotplug_handler_unplug_request(hotplug_ctrl, dev, &local_err);
     } else {
-        hotplug_handler_unplug(hotplug_ctrl, dev, errp);
+        hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
+        if (!local_err) {
+            object_unparent(OBJECT(dev));
+        }
     }
+    error_propagate(errp, local_err);
 }
 
 void qmp_device_del(const char *id, Error **errp)
-- 
2.17.2

[Qemu-devel] [PATCH RFCv2 2/9] qdev: Let machine hotplug handler to override bus hotplug handler

[David Hildenbrand]

From: Igor Mammedov <imammedo@redhat.com>

it will allow to return another hotplug handler than the default
one for a specific bus based device type. Which is needed to handle
non trivial plug/unplug sequences that need the access to resources
configured outside of bus where device is attached.

That will allow for returned hotplug handler to orchestrate wiring
in arbitrary order, by chaining other hotplug handlers when
it's needed.

PS:
It could be used for hybrid virtio-mem and virtio-pmem devices
where it will return machine as hotplug handler which will do
necessary wiring at machine level and then pass control down
the chain to bus specific hotplug handler.

Example of top level hotplug handler override and custom plug sequence:

  some_machine_get_hotplug_handler(machine){
      if (object_dynamic_cast(OBJECT(dev), TYPE_SOME_BUS_DEVICE)) {
          return HOTPLUG_HANDLER(machine);
      }
      return NULL;
  }

  some_machine_device_plug(hotplug_dev, dev) {
      if (object_dynamic_cast(OBJECT(dev), TYPE_SOME_BUS_DEVICE)) {
          /* do machine specific initialization */
          some_machine_init_special_device(dev)

          /* pass control to bus specific handler */
          hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev)
      }
  }

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/qdev.c         |  6 ++----
 include/hw/qdev-core.h | 11 +++++++++++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 278cc094ec..7ad45c0bd6 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -235,12 +235,10 @@ HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
 
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
 {
-    HotplugHandler *hotplug_ctrl;
+    HotplugHandler *hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
 
-    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+    if (hotplug_ctrl == NULL && dev->parent_bus) {
         hotplug_ctrl = dev->parent_bus->hotplug_handler;
-    } else {
-        hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
     }
     return hotplug_ctrl;
 }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 0a84c42756..9081fb0030 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -281,6 +281,17 @@ void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
+/**
+ * qdev_get_hotplug_handler: Get handler responsible for device wiring
+ *
+ * Find HOTPLUG_HANDLER for @dev that provides [pre|un]plug callbacks for it.
+ *
+ * Note: in case @dev has a parent bus, it will be returned as handler unless
+ * machine handler overrides it.
+ *
+ * Returns: pointer to object that implements TYPE_HOTPLUG_HANDLER interface
+ *          or NULL if there aren't any.
+ */
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
 void qdev_unplug(DeviceState *dev, Error **errp);
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
-- 
2.17.2

[Qemu-devel] [PATCH RFCv2 3/9] qdev: Provide qdev_get_bus_hotplug_handler()

[David Hildenbrand]

Let's use a wrapper instead of looking it up manually. This function can
than be reused when we explicitly want to have the bus hotplug handler
(e.g. when the bus hotplug handler was overwritten by the machine
hotplug handler).

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/qdev.c         | 10 +++++++++-
 include/hw/qdev-core.h |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 7ad45c0bd6..e2207d77a4 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -233,12 +233,20 @@ HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
     return NULL;
 }
 
+HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev)
+{
+    if (dev->parent_bus) {
+        return dev->parent_bus->hotplug_handler;
+    }
+    return NULL;
+}
+
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
 {
     HotplugHandler *hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
 
     if (hotplug_ctrl == NULL && dev->parent_bus) {
-        hotplug_ctrl = dev->parent_bus->hotplug_handler;
+        hotplug_ctrl = qdev_get_bus_hotplug_handler(dev);
     }
     return hotplug_ctrl;
 }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 9081fb0030..cb0ec63c2d 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -280,6 +280,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
 void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
+HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
 HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
 /**
  * qdev_get_hotplug_handler: Get handler responsible for device wiring
-- 
2.17.2

[Qemu-devel] [PATCH RFCv2 4/9] virtio-pmem: Prototype

[David Hildenbrand]

From: Pankaj Gupta <pagupta@redhat.com>

This is the current protoype of virtio-pmem. Support will require
machine changes for the architectures that will support it, so it will
not yet be compiled.

TODO:
- Use separate struct for tracking requests internally
- Move request/response structs to linux headers
- Factor out linux header sync
- Drop debug printfs

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
[ MemoryDevice/MemoryRegion changes, cleanups, addr property "memaddr",
  split up patches, unplug handler ]
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/Makefile.objs                     |   2 +
 hw/virtio/virtio-pmem.c                     | 191 ++++++++++++++++++++
 include/hw/virtio/virtio-pmem.h             |  54 ++++++
 include/standard-headers/linux/virtio_ids.h |   1 +
 qapi/misc.json                              |  28 ++-
 5 files changed, 275 insertions(+), 1 deletion(-)
 create mode 100644 hw/virtio/virtio-pmem.c
 create mode 100644 include/hw/virtio/virtio-pmem.h

diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index ea7913d532..565667096e 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -9,6 +9,8 @@ obj-$(CONFIG_VIRTIO_BALLOON) += virtio-balloon.o
 obj-$(CONFIG_VIRTIO_CRYPTO) += virtio-crypto.o
 obj-$(call land,$(CONFIG_VIRTIO_CRYPTO),$(CONFIG_VIRTIO_PCI)) += virtio-crypto-pci.o
 
+obj-$(CONFIG_VIRTIO_PMEM) += virtio-pmem.o
+
 obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
 obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock.o
 ifeq ($(CONFIG_PCI),y)
diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
new file mode 100644
index 0000000000..6ec2ea5d1d
--- /dev/null
+++ b/hw/virtio/virtio-pmem.c
@@ -0,0 +1,191 @@
+/*
+ * Virtio PMEM device
+ *
+ * Copyright (C) 2018-2019 Red Hat, Inc.
+ *
+ * Authors:
+ *  Pankaj Gupta <pagupta@redhat.com>
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "qemu/error-report.h"
+#include "hw/virtio/virtio-pmem.h"
+#include "hw/virtio/virtio-access.h"
+#include "standard-headers/linux/virtio_ids.h"
+#include "block/aio.h"
+#include "block/thread-pool.h"
+
+typedef struct VirtIOPMEMresp {
+    int ret;
+} VirtIOPMEMResp;
+
+typedef struct VirtIODeviceRequest {
+    VirtQueueElement elem;
+    int fd;
+    VirtIOPMEM *pmem;
+    VirtIOPMEMResp resp;
+} VirtIODeviceRequest;
+
+static int worker_cb(void *opaque)
+{
+    VirtIODeviceRequest *req = opaque;
+    int err = 0;
+
+    printf("\n performing flush ...");
+    /* flush raw backing image */
+    err = fsync(req->fd);
+    printf("\n performed flush ...:errcode::%d", err);
+    if (err != 0) {
+        err = EIO;
+    }
+    req->resp.ret = err;
+
+    return 0;
+}
+
+static void done_cb(void *opaque, int ret)
+{
+    VirtIODeviceRequest *req = opaque;
+    int len = iov_from_buf(req->elem.in_sg, req->elem.in_num, 0,
+                              &req->resp, sizeof(VirtIOPMEMResp));
+
+    /* Callbacks are serialized, so no need to use atomic ops. */
+    virtqueue_push(req->pmem->rq_vq, &req->elem, len);
+    virtio_notify((VirtIODevice *)req->pmem, req->pmem->rq_vq);
+    g_free(req);
+}
+
+static void virtio_pmem_flush(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIODeviceRequest *req;
+    VirtIOPMEM *pmem = VIRTIO_PMEM(vdev);
+    HostMemoryBackend *backend = MEMORY_BACKEND(pmem->memdev);
+    ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
+
+    req = virtqueue_pop(vq, sizeof(VirtIODeviceRequest));
+    if (!req) {
+        virtio_error(vdev, "virtio-pmem missing request data");
+        return;
+    }
+
+    if (req->elem.out_num < 1 || req->elem.in_num < 1) {
+        virtio_error(vdev, "virtio-pmem request not proper");
+        g_free(req);
+        return;
+    }
+    req->fd = memory_region_get_fd(&backend->mr);
+    req->pmem = pmem;
+    thread_pool_submit_aio(pool, worker_cb, req, done_cb, req);
+}
+
+static void virtio_pmem_get_config(VirtIODevice *vdev, uint8_t *config)
+{
+    VirtIOPMEM *pmem = VIRTIO_PMEM(vdev);
+    struct virtio_pmem_config *pmemcfg = (struct virtio_pmem_config *) config;
+
+    virtio_stq_p(vdev, &pmemcfg->start, pmem->start);
+    virtio_stq_p(vdev, &pmemcfg->size, memory_region_size(&pmem->memdev->mr));
+}
+
+static uint64_t virtio_pmem_get_features(VirtIODevice *vdev, uint64_t features,
+                                        Error **errp)
+{
+    return features;
+}
+
+static void virtio_pmem_realize(DeviceState *dev, Error **errp)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VirtIOPMEM *pmem = VIRTIO_PMEM(dev);
+
+    if (!pmem->memdev) {
+        error_setg(errp, "virtio-pmem memdev not set");
+        return;
+    }
+
+    if (host_memory_backend_is_mapped(pmem->memdev)) {
+        char *path = object_get_canonical_path_component(OBJECT(pmem->memdev));
+        error_setg(errp, "can't use already busy memdev: %s", path);
+        g_free(path);
+        return;
+    }
+
+    host_memory_backend_set_mapped(pmem->memdev, true);
+    virtio_init(vdev, TYPE_VIRTIO_PMEM, VIRTIO_ID_PMEM,
+                sizeof(struct virtio_pmem_config));
+    pmem->rq_vq = virtio_add_queue(vdev, 128, virtio_pmem_flush);
+}
+
+static void virtio_pmem_unrealize(DeviceState *dev, Error **errp)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VirtIOPMEM *pmem = VIRTIO_PMEM(dev);
+
+    host_memory_backend_set_mapped(pmem->memdev, false);
+    pmem->rq_vq = virtio_add_queue(vdev, 128, virtio_pmem_flush);
+    virtio_cleanup(vdev);
+}
+
+static void virtio_pmem_fill_device_info(const VirtIOPMEM *pmem,
+                                         VirtioPMEMDeviceInfo *vi)
+{
+    vi->memaddr = pmem->start;
+    vi->size = pmem->memdev ? memory_region_size(&pmem->memdev->mr) : 0;
+    vi->memdev = object_get_canonical_path(OBJECT(pmem->memdev));
+}
+
+static MemoryRegion *virtio_pmem_get_memory_region(VirtIOPMEM *pmem,
+                                                   Error **errp)
+{
+    if (!pmem->memdev) {
+        error_setg(errp, "'%s' property must be set", VIRTIO_PMEM_MEMDEV_PROP);
+        return NULL;
+    }
+
+    return &pmem->memdev->mr;
+}
+
+static Property virtio_pmem_properties[] = {
+    DEFINE_PROP_UINT64(VIRTIO_PMEM_ADDR_PROP, VirtIOPMEM, start, 0),
+    DEFINE_PROP_LINK(VIRTIO_PMEM_MEMDEV_PROP, VirtIOPMEM, memdev,
+                     TYPE_MEMORY_BACKEND, HostMemoryBackend *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_pmem_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+    VirtIOPMEMClass *vpc = VIRTIO_PMEM_CLASS(klass);
+
+    dc->props = virtio_pmem_properties;
+
+    vdc->realize = virtio_pmem_realize;
+    vdc->unrealize = virtio_pmem_unrealize;
+    vdc->get_config = virtio_pmem_get_config;
+    vdc->get_features = virtio_pmem_get_features;
+
+    vpc->fill_device_info = virtio_pmem_fill_device_info;
+    vpc->get_memory_region = virtio_pmem_get_memory_region;
+}
+
+static TypeInfo virtio_pmem_info = {
+    .name          = TYPE_VIRTIO_PMEM,
+    .parent        = TYPE_VIRTIO_DEVICE,
+    .class_size    = sizeof(VirtIOPMEMClass),
+    .class_init    = virtio_pmem_class_init,
+    .instance_size = sizeof(VirtIOPMEM),
+};
+
+static void virtio_register_types(void)
+{
+    type_register_static(&virtio_pmem_info);
+}
+
+type_init(virtio_register_types)
diff --git a/include/hw/virtio/virtio-pmem.h b/include/hw/virtio/virtio-pmem.h
new file mode 100644
index 0000000000..c9917b9a31
--- /dev/null
+++ b/include/hw/virtio/virtio-pmem.h
@@ -0,0 +1,54 @@
+/*
+ * Virtio PMEM device
+ *
+ * Copyright (C) 2018-2019 Red Hat, Inc.
+ *
+ * Authors:
+ *  Pankaj Gupta <pagupta@redhat.com>
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_VIRTIO_PMEM_H
+#define HW_VIRTIO_PMEM_H
+
+#include "hw/virtio/virtio.h"
+#include "sysemu/hostmem.h"
+
+#define TYPE_VIRTIO_PMEM "virtio-pmem"
+
+#define VIRTIO_PMEM(obj) \
+        OBJECT_CHECK(VirtIOPMEM, (obj), TYPE_VIRTIO_PMEM)
+#define VIRTIO_PMEM_CLASS(oc) \
+        OBJECT_CLASS_CHECK(VirtIOPMEMClass, (oc), TYPE_VIRTIO_PMEM)
+#define VIRTIO_PMEM_GET_CLASS(obj) \
+        OBJECT_GET_CLASS(VirtIOPMEMClass, (obj), TYPE_VIRTIO_PMEM)
+
+#define VIRTIO_PMEM_ADDR_PROP "memaddr"
+#define VIRTIO_PMEM_MEMDEV_PROP "memdev"
+
+typedef struct VirtIOPMEM {
+    VirtIODevice parent_obj;
+
+    VirtQueue *rq_vq;
+    uint64_t start;
+    HostMemoryBackend *memdev;
+} VirtIOPMEM;
+
+typedef struct VirtIOPMEMClass {
+    /* private */
+    VirtIODevice parent;
+
+    /* public */
+    void (*fill_device_info)(const VirtIOPMEM *pmem, VirtioPMEMDeviceInfo *vi);
+    MemoryRegion *(*get_memory_region)(VirtIOPMEM *pmem, Error **errp);
+} VirtIOPMEMClass;
+
+struct virtio_pmem_config {
+    uint64_t start;
+    uint64_t size;
+};
+
+#endif
diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
index 6d5c3b2d4f..346389565a 100644
--- a/include/standard-headers/linux/virtio_ids.h
+++ b/include/standard-headers/linux/virtio_ids.h
@@ -43,5 +43,6 @@
 #define VIRTIO_ID_INPUT        18 /* virtio input */
 #define VIRTIO_ID_VSOCK        19 /* virtio vsock transport */
 #define VIRTIO_ID_CRYPTO       20 /* virtio crypto */
+#define VIRTIO_ID_PMEM         25 /* virtio pmem */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
diff --git a/qapi/misc.json b/qapi/misc.json
index 24d20a880a..b71eca2666 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -2949,16 +2949,42 @@
           }
 }
 
+##
+# @VirtioPMEMDeviceInfo:
+#
+# VirtioPMEM state information
+#
+# @id: device's ID
+#
+# @memaddr: physical address in memory, where device is mapped
+#
+# @size: size of memory that the device provides
+#
+# @memdev: memory backend linked with device
+#
+# Since: 4.0
+##
+{ 'struct': 'VirtioPMEMDeviceInfo',
+  'data': { '*id': 'str',
+            'memaddr': 'size',
+            'size': 'size',
+            'memdev': 'str'
+          }
+}
+
 ##
 # @MemoryDeviceInfo:
 #
 # Union containing information about a memory device
 #
+# nvdimm is included since 2.12. virtio-pmem is included since 4.0.
+#
 # Since: 2.1
 ##
 { 'union': 'MemoryDeviceInfo',
   'data': { 'dimm': 'PCDIMMDeviceInfo',
-            'nvdimm': 'PCDIMMDeviceInfo'
+            'nvdimm': 'PCDIMMDeviceInfo',
+            'virtio-pmem': 'VirtioPMEMDeviceInfo'
           }
 }
 
-- 
2.17.2

[Qemu-devel] [PATCH RFCv2 5/9] virtio-pci: Allow to specify additional interfaces for the base type

[David Hildenbrand]

Let's allow to specify additional interfaces for the base type (e.g.
later TYPE_MEMORY_DEVICE), something that was possible before the
rework of virtio PCI device instantiation.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-pci.c | 1 +
 hw/virtio/virtio-pci.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index b282109343..8f53192f4f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1938,6 +1938,7 @@ void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t)
         .class_init    = virtio_pci_base_class_init,
         .class_data    = (void *)t,
         .abstract      = true,
+        .interfaces    = t->interfaces,
     };
     TypeInfo generic_type_info = {
         .name = t->generic_name,
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index bd223a6e3b..5d303b1641 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -232,6 +232,7 @@ typedef struct VirtioPCIDeviceTypeInfo {
     size_t instance_size;
     void (*instance_init)(Object *obj);
     void (*class_init)(ObjectClass *klass, void *data);
+    InterfaceInfo *interfaces;
 } VirtioPCIDeviceTypeInfo;
 
 /* Register virtio-pci type(s).  @t must be static. */
-- 
2.17.2

[Qemu-devel] [PATCH RFCv2 6/9] virtio-pci: Proxy for virtio-pmem

[David Hildenbrand]

From: Pankaj Gupta <pagupta@redhat.com>

We need a proxy device for virtio-pmem, and this device has to be the
actual memory device so we can cleanly hotplug it.

Forward memory device class functions either to the actual device or use
properties of the virtio-pmem device to implement these in the proxy.

virtio-pmem will only be compiled for selected, supported architectures
(that can deal with virtio/pci devices being memory devices). An
architecture that is prepared for that can simply enable
CONFIG_VIRTIO_PMEM to make it work.

As not all architectures support memory devices (and CONFIG_VIRTIO_PMEM
will be enabled per supported architecture), we have to move the PCI proxy
to a separate file.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
[ split up patches, memory-device changes, move pci proxy, drop legacy
  pci ID ]
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/Makefile.objs     |   1 +
 hw/virtio/virtio-pmem-pci.c | 144 ++++++++++++++++++++++++++++++++++++
 hw/virtio/virtio-pmem-pci.h |  39 ++++++++++
 3 files changed, 184 insertions(+)
 create mode 100644 hw/virtio/virtio-pmem-pci.c
 create mode 100644 hw/virtio/virtio-pmem-pci.h

diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 565667096e..12f965081d 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -10,6 +10,7 @@ obj-$(CONFIG_VIRTIO_CRYPTO) += virtio-crypto.o
 obj-$(call land,$(CONFIG_VIRTIO_CRYPTO),$(CONFIG_VIRTIO_PCI)) += virtio-crypto-pci.o
 
 obj-$(CONFIG_VIRTIO_PMEM) += virtio-pmem.o
+common-obj-$(call land,$(CONFIG_VIRTIO_PMEM),$(CONFIG_VIRTIO_PCI)) += virtio-pmem-pci.o
 
 obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
 obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock.o
diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
new file mode 100644
index 0000000000..ddaf2662c7
--- /dev/null
+++ b/hw/virtio/virtio-pmem-pci.c
@@ -0,0 +1,144 @@
+/*
+ * Virtio PMEM PCI device
+ *
+ * Copyright (C) 2018-2019 Red Hat, Inc.
+ *
+ * Authors:
+ *  Pankaj Gupta <pagupta@redhat.com>
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "virtio-pmem-pci.h"
+#include "hw/mem/memory-device.h"
+#include "qapi/error.h"
+
+static void virtio_pmem_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+    VirtIOPMEMPCI *pmem_pci = VIRTIO_PMEM_PCI(vpci_dev);
+    DeviceState *vdev = DEVICE(&pmem_pci->vdev);
+
+    qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
+    object_property_set_bool(OBJECT(vdev), true, "realized", errp);
+}
+
+static void virtio_pmem_pci_set_addr(MemoryDeviceState *md, uint64_t addr,
+                                     Error **errp)
+{
+    object_property_set_uint(OBJECT(md), addr, VIRTIO_PMEM_ADDR_PROP, errp);
+}
+
+static uint64_t virtio_pmem_pci_get_addr(const MemoryDeviceState *md)
+{
+    return object_property_get_uint(OBJECT(md), VIRTIO_PMEM_ADDR_PROP,
+                                    &error_abort);
+}
+
+static MemoryRegion *virtio_pmem_pci_get_memory_region(MemoryDeviceState *md,
+                                                       Error **errp)
+{
+    VirtIOPMEMPCI *pci_pmem = VIRTIO_PMEM_PCI(md);
+    VirtIOPMEM *pmem = VIRTIO_PMEM(&pci_pmem->vdev);
+    VirtIOPMEMClass *vpc = VIRTIO_PMEM_GET_CLASS(pmem);
+
+    return vpc->get_memory_region(pmem, errp);
+}
+
+static uint64_t virtio_pmem_pci_get_plugged_size(const MemoryDeviceState *md,
+                                                 Error **errp)
+{
+    VirtIOPMEMPCI *pci_pmem = VIRTIO_PMEM_PCI(md);
+    VirtIOPMEM *pmem = VIRTIO_PMEM(&pci_pmem->vdev);
+    VirtIOPMEMClass *vpc = VIRTIO_PMEM_GET_CLASS(pmem);
+    MemoryRegion *mr = vpc->get_memory_region(pmem, errp);
+
+    /* the plugged size corresponds to the region size */
+    return mr ? 0 : memory_region_size(mr);
+}
+
+static void virtio_pmem_pci_fill_device_info(const MemoryDeviceState *md,
+                                             MemoryDeviceInfo *info)
+{
+    VirtioPMEMDeviceInfo *vi = g_new0(VirtioPMEMDeviceInfo, 1);
+    VirtIOPMEMPCI *pci_pmem = VIRTIO_PMEM_PCI(md);
+    VirtIOPMEM *pmem = VIRTIO_PMEM(&pci_pmem->vdev);
+    VirtIOPMEMClass *vpc = VIRTIO_PMEM_GET_CLASS(pmem);
+    DeviceState *dev = DEVICE(md);
+
+    if (dev->id) {
+        vi->has_id = true;
+        vi->id = g_strdup(dev->id);
+    }
+
+    /* let the real device handle everything else */
+    vpc->fill_device_info(pmem, vi);
+
+    info->u.virtio_pmem.data = vi;
+    info->type = MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM;
+}
+
+static void virtio_pmem_pci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+    PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+    MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(klass);
+
+    k->realize = virtio_pmem_pci_realize;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+    pcidev_k->class_id = PCI_CLASS_OTHERS;
+
+    mdc->get_addr = virtio_pmem_pci_get_addr;
+    mdc->set_addr = virtio_pmem_pci_set_addr;
+    mdc->get_plugged_size = virtio_pmem_pci_get_plugged_size;
+    mdc->get_memory_region = virtio_pmem_pci_get_memory_region;
+    mdc->fill_device_info = virtio_pmem_pci_fill_device_info;
+}
+
+static void virtio_pmem_pci_instance_init(Object *obj)
+{
+    VirtIOPMEMPCI *dev = VIRTIO_PMEM_PCI(obj);
+
+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+                                TYPE_VIRTIO_PMEM);
+}
+
+static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = {
+    .base_name             = TYPE_VIRTIO_PMEM_PCI,
+    .generic_name          = "virtio-pmem-pci",
+    .transitional_name     = "virtio-pmem-pci-transitional",
+    .non_transitional_name = "virtio-pmem-pci-non-transitional",
+    .instance_size = sizeof(VirtIOPMEMPCI),
+    .instance_init = virtio_pmem_pci_instance_init,
+    .class_init    = virtio_pmem_pci_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_MEMORY_DEVICE },
+        { }
+    },
+};
+
+void virtio_pmem_pci_pre_plug(VirtIOPMEMPCI *pci_pmem, MachineState *ms,
+                              Error **errp)
+{
+    memory_device_pre_plug(MEMORY_DEVICE(pci_pmem), ms, NULL, errp);
+}
+
+void virtio_pmem_pci_plug(VirtIOPMEMPCI *pci_pmem, MachineState *ms)
+{
+    memory_device_plug(MEMORY_DEVICE(pci_pmem), ms);
+}
+
+void virtio_pmem_pci_unplug(VirtIOPMEMPCI *pci_pmem, MachineState *ms)
+{
+    memory_device_unplug(MEMORY_DEVICE(pci_pmem), ms);
+}
+
+static void virtio_pmem_pci_register_types(void)
+{
+    virtio_pci_types_register(&virtio_pmem_pci_info);
+}
+type_init(virtio_pmem_pci_register_types)
diff --git a/hw/virtio/virtio-pmem-pci.h b/hw/virtio/virtio-pmem-pci.h
new file mode 100644
index 0000000000..4e0db2f673
--- /dev/null
+++ b/hw/virtio/virtio-pmem-pci.h
@@ -0,0 +1,39 @@
+/*
+ * Virtio PMEM PCI device
+ *
+ * Copyright (C) 2018-2019 Red Hat, Inc.
+ *
+ * Authors:
+ *  Pankaj Gupta <pagupta@redhat.com>
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_VIRTIO_PMEM_PCI_H
+#define QEMU_VIRTIO_PMEM_PCI_H
+
+#include "hw/virtio/virtio-pci.h"
+#include "hw/virtio/virtio-pmem.h"
+
+typedef struct VirtIOPMEMPCI VirtIOPMEMPCI;
+
+/*
+ * virtio-pmem-pci: This extends VirtioPCIProxy.
+ */
+#define TYPE_VIRTIO_PMEM_PCI "virtio-pmem-pci-base"
+#define VIRTIO_PMEM_PCI(obj) \
+        OBJECT_CHECK(VirtIOPMEMPCI, (obj), TYPE_VIRTIO_PMEM_PCI)
+
+struct VirtIOPMEMPCI {
+    VirtIOPCIProxy parent_obj;
+    VirtIOPMEM vdev;
+};
+
+void virtio_pmem_pci_pre_plug(VirtIOPMEMPCI *pci_pmem, MachineState *ms,
+                              Error **errp);
+void virtio_pmem_pci_plug(VirtIOPMEMPCI *pci_pmem, MachineState *ms);
+void virtio_pmem_pci_unplug(VirtIOPMEMPCI *pci_pmem, MachineState *ms);
+
+#endif /* QEMU_VIRTIO_PMEM_PCI_H */
-- 
2.17.2

[Qemu-devel] [PATCH RFCv2 7/9] hmp: Handle virtio-pmem when printing memory device infos

[David Hildenbrand]

Print the memory device info just like for PCDIMM/NVDIMM.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hmp.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/hmp.c b/hmp.c
index 8da5fd8760..25c32e0810 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2553,6 +2553,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
     MemoryDeviceInfoList *info_list = qmp_query_memory_devices(&err);
     MemoryDeviceInfoList *info;
+    VirtioPMEMDeviceInfo *vpi;
     MemoryDeviceInfo *value;
     PCDIMMDeviceInfo *di;
 
@@ -2562,19 +2563,9 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
         if (value) {
             switch (value->type) {
             case MEMORY_DEVICE_INFO_KIND_DIMM:
-                di = value->u.dimm.data;
-                break;
-
             case MEMORY_DEVICE_INFO_KIND_NVDIMM:
-                di = value->u.nvdimm.data;
-                break;
-
-            default:
-                di = NULL;
-                break;
-            }
-
-            if (di) {
+                di = value->type == MEMORY_DEVICE_INFO_KIND_DIMM ?
+                     value->u.dimm.data : value->u.nvdimm.data;
                 monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
                                MemoryDeviceInfoKind_str(value->type),
                                di->id ? di->id : "");
@@ -2587,6 +2578,18 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
                                di->hotplugged ? "true" : "false");
                 monitor_printf(mon, "  hotpluggable: %s\n",
                                di->hotpluggable ? "true" : "false");
+                break;
+            case MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM:
+                vpi = value->u.virtio_pmem.data;
+                monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
+                               MemoryDeviceInfoKind_str(value->type),
+                               vpi->id ? vpi->id : "");
+                monitor_printf(mon, "  memaddr: 0x%" PRIx64 "\n", vpi->memaddr);
+                monitor_printf(mon, "  size: %" PRIu64 "\n", vpi->size);
+                monitor_printf(mon, "  memdev: %s\n", vpi->memdev);
+                break;
+            default:
+                g_assert_not_reached();
             }
         }
     }
-- 
2.17.2

[Qemu-devel] [PATCH RFCv2 8/9] numa: Handle virtio-pmem in NUMA stats

[David Hildenbrand]

Account the memory to node 0 for now. Once (if ever) virtio-pmem
supports NUMA, we can account it to the right node.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 numa.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/numa.c b/numa.c
index 50ec016013..f426e00d06 100644
--- a/numa.c
+++ b/numa.c
@@ -548,6 +548,7 @@ static void numa_stat_memory_devices(NumaNodeMem node_mem[])
     MemoryDeviceInfoList *info_list = qmp_memory_device_list();
     MemoryDeviceInfoList *info;
     PCDIMMDeviceInfo     *pcdimm_info;
+    VirtioPMEMDeviceInfo *vpi;
 
     for (info = info_list; info; info = info->next) {
         MemoryDeviceInfo *value = info->value;
@@ -555,22 +556,21 @@ static void numa_stat_memory_devices(NumaNodeMem node_mem[])
         if (value) {
             switch (value->type) {
             case MEMORY_DEVICE_INFO_KIND_DIMM:
-                pcdimm_info = value->u.dimm.data;
-                break;
-
             case MEMORY_DEVICE_INFO_KIND_NVDIMM:
-                pcdimm_info = value->u.nvdimm.data;
-                break;
-
-            default:
-                pcdimm_info = NULL;
-                break;
-            }
-
-            if (pcdimm_info) {
+                pcdimm_info = value->type == MEMORY_DEVICE_INFO_KIND_DIMM ?
+                              value->u.dimm.data : value->u.nvdimm.data;
                 node_mem[pcdimm_info->node].node_mem += pcdimm_info->size;
                 node_mem[pcdimm_info->node].node_plugged_mem +=
                     pcdimm_info->size;
+                break;
+            case MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM:
+                vpi = value->u.virtio_pmem.data;
+                /* TODO: once we support numa, assign to right node */
+                node_mem[0].node_mem += vpi->size;
+                node_mem[0].node_plugged_mem += vpi->size;
+                break;
+            default:
+                g_assert_not_reached();
             }
         }
     }
-- 
2.17.2

[Qemu-devel] [PATCH RFCv2 9/9] pc: Support for virtio-pmem-pci

[David Hildenbrand]

Override the device hotplug handler to properly handle the memory device
part via virtio-pmem-pci callbacks from the machine hotplug handler and
forward to the actual PCI bus hotplug handler.

As PCI hotplug has not been properly factored out into hotplug handlers,
most magic is performed in the (un)realize functions. Also some PCI host
buses don't have a PCI hotplug handler at all yet, just to be sure that
we alway have a hotplug handler on x86, add a simple error check.

Unlocking virtio-pmem will unlock virtio-pmem-pci.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 default-configs/i386-softmmu.mak |  1 +
 hw/i386/pc.c                     | 70 +++++++++++++++++++++++++++++++-
 2 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 64c998c4c8..3f63e95a55 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -52,6 +52,7 @@ CONFIG_APIC=y
 CONFIG_IOAPIC=y
 CONFIG_PVPANIC=y
 CONFIG_MEM_DEVICE=y
+CONFIG_VIRTIO_PMEM=y
 CONFIG_DIMM=y
 CONFIG_NVDIMM=y
 CONFIG_ACPI_NVDIMM=y
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index fd0cb29ba9..6a176caeb9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -75,6 +75,7 @@
 #include "hw/usb.h"
 #include "hw/i386/intel_iommu.h"
 #include "hw/net/ne2000-isa.h"
+#include "hw/virtio/virtio-pmem-pci.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -2224,6 +2225,64 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
     numa_cpu_pre_plug(cpu_slot, dev, errp);
 }
 
+static void pc_virtio_pmem_pci_pre_plug(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
+    Error *local_err = NULL;
+
+    if (!hotplug_dev2) {
+        /*
+         * Without a bus hotplug handler, we cannot control the plug/unplug
+         * order. This should never be the case on x86, however better add
+         * a safety net.
+         */
+        error_setg(errp, "virtio-pmem-pci not supported on this bus.");
+        return;
+    }
+    virtio_pmem_pci_pre_plug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev),
+                             &local_err);
+    if (!local_err) {
+        hotplug_handler_pre_plug(hotplug_dev2, dev, &local_err);
+    }
+    error_propagate(errp, local_err);
+}
+
+static void pc_virtio_pmem_pci_plug(HotplugHandler *hotplug_dev,
+                                    DeviceState *dev, Error **errp)
+{
+    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
+    Error *local_err = NULL;
+
+    virtio_pmem_pci_plug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev));
+    hotplug_handler_plug(hotplug_dev2, dev, &local_err);
+    if (local_err) {
+        virtio_pmem_pci_unplug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev));
+    }
+    error_propagate(errp, local_err);
+}
+
+static void pc_virtio_pmem_pci_unplug_request(HotplugHandler *hotplug_dev,
+                                              DeviceState *dev, Error **errp)
+{
+    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
+
+    hotplug_handler_unplug_request(hotplug_dev2, dev, errp);
+}
+
+static void pc_virtio_pmem_pci_unplug(HotplugHandler *hotplug_dev,
+                                      DeviceState *dev, Error **errp)
+{
+    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
+    Error *local_err = NULL;
+
+    hotplug_handler_unplug(hotplug_dev2, dev, &local_err);
+    if (!local_err) {
+        virtio_pmem_pci_unplug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev));
+    }
+    error_propagate(errp, local_err);
+}
+
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
@@ -2231,6 +2290,8 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
         pc_memory_pre_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_pre_plug(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI)) {
+        pc_virtio_pmem_pci_pre_plug(hotplug_dev, dev, errp);
     }
 }
 
@@ -2241,6 +2302,8 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
         pc_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_plug(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI)) {
+        pc_virtio_pmem_pci_plug(hotplug_dev, dev, errp);
     }
 }
 
@@ -2251,6 +2314,8 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
         pc_memory_unplug_request(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI)) {
+        pc_virtio_pmem_pci_unplug_request(hotplug_dev, dev, errp);
     } else {
         error_setg(errp, "acpi: device unplug request for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -2264,6 +2329,8 @@ static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
         pc_memory_unplug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_unplug_cb(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI)) {
+        pc_virtio_pmem_pci_unplug(hotplug_dev, dev, errp);
     } else {
         error_setg(errp, "acpi: device unplug for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -2274,7 +2341,8 @@ static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
                                              DeviceState *dev)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
-        object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI)) {
         return HOTPLUG_HANDLER(machine);
     }
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH RFCv2 7/9] hmp: Handle virtio-pmem when printing memory device infos

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> Print the memory device info just like for PCDIMM/NVDIMM.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hmp.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 8da5fd8760..25c32e0810 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2553,6 +2553,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
>      Error *err = NULL;
>      MemoryDeviceInfoList *info_list = qmp_query_memory_devices(&err);
>      MemoryDeviceInfoList *info;
> +    VirtioPMEMDeviceInfo *vpi;
>      MemoryDeviceInfo *value;
>      PCDIMMDeviceInfo *di;
>  
> @@ -2562,19 +2563,9 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
>          if (value) {
>              switch (value->type) {
>              case MEMORY_DEVICE_INFO_KIND_DIMM:
> -                di = value->u.dimm.data;
> -                break;
> -
>              case MEMORY_DEVICE_INFO_KIND_NVDIMM:
> -                di = value->u.nvdimm.data;
> -                break;
> -
> -            default:
> -                di = NULL;
> -                break;
> -            }
> -
> -            if (di) {
> +                di = value->type == MEMORY_DEVICE_INFO_KIND_DIMM ?
> +                     value->u.dimm.data : value->u.nvdimm.data;
>                  monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
>                                 MemoryDeviceInfoKind_str(value->type),
>                                 di->id ? di->id : "");
> @@ -2587,6 +2578,18 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
>                                 di->hotplugged ? "true" : "false");
>                  monitor_printf(mon, "  hotpluggable: %s\n",
>                                 di->hotpluggable ? "true" : "false");
> +                break;
> +            case MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM:
> +                vpi = value->u.virtio_pmem.data;
> +                monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
> +                               MemoryDeviceInfoKind_str(value->type),
> +                               vpi->id ? vpi->id : "");
> +                monitor_printf(mon, "  memaddr: 0x%" PRIx64 "\n", vpi->memaddr);
> +                monitor_printf(mon, "  size: %" PRIu64 "\n", vpi->size);
> +                monitor_printf(mon, "  memdev: %s\n", vpi->memdev);
> +                break;
> +            default:
> +                g_assert_not_reached();


Although I'd prefer if that assert was replaced by a print
saying it was an unknown type.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>              }
>          }
>      }
> -- 
> 2.17.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFCv2 3/9] qdev: Provide qdev_get_bus_hotplug_handler()

[Igor Mammedov]

On Wed, 23 Jan 2019 20:55:21 +0100
David Hildenbrand <david@redhat.com> wrote:

> Let's use a wrapper instead of looking it up manually. This function can
> than be reused when we explicitly want to have the bus hotplug handler
> (e.g. when the bus hotplug handler was overwritten by the machine
> hotplug handler).
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
maybe drop the 2nd one?

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/core/qdev.c         | 10 +++++++++-
>  include/hw/qdev-core.h |  1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 7ad45c0bd6..e2207d77a4 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -233,12 +233,20 @@ HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
>      return NULL;
>  }
>  
> +HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev)
> +{
> +    if (dev->parent_bus) {
> +        return dev->parent_bus->hotplug_handler;
> +    }
> +    return NULL;
> +}
> +
>  HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>  {
>      HotplugHandler *hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
>  
>      if (hotplug_ctrl == NULL && dev->parent_bus) {
> -        hotplug_ctrl = dev->parent_bus->hotplug_handler;
> +        hotplug_ctrl = qdev_get_bus_hotplug_handler(dev);
>      }
>      return hotplug_ctrl;
>  }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9081fb0030..cb0ec63c2d 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -280,6 +280,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
>  void qdev_init_nofail(DeviceState *dev);
>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>                                   int required_for_version);
> +HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
>  HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
>  /**
>   * qdev_get_hotplug_handler: Get handler responsible for device wiring

Re: [Qemu-devel] [PATCH RFCv2 3/9] qdev: Provide qdev_get_bus_hotplug_handler()

[David Hildenbrand]

On 28.01.19 15:01, Igor Mammedov wrote:
> On Wed, 23 Jan 2019 20:55:21 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's use a wrapper instead of looking it up manually. This function can
>> than be reused when we explicitly want to have the bus hotplug handler
>> (e.g. when the bus hotplug handler was overwritten by the machine
>> hotplug handler).
>>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> maybe drop the 2nd one?

Whoops, indeed! Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFCv2 1/9] qdev: Let the hotplug_handler_unplug() caller delete the device

[Cornelia Huck]

On Wed, 23 Jan 2019 20:55:19 +0100
David Hildenbrand <david@redhat.com> wrote:

> When unplugging a device, at one point the device will be destroyed
> via object_unparent(). This will, one the one hand, unrealize the
> removed device hierarchy, and on the other hand, destroy/free the
> device hierarchy.
> 
> When chaining hotplug handlers, we want to overwrite a bus hotplug
> handler by the machine hotplug handler, to be able to perform
> some part of the plug/unplug and to forward the calls to the bus hotplug
> handler.
> 
> For now, the bus hotplug handler would trigger an object_unparent(), not
> allowing us to perform some unplug action on a device after we forwarded
> the call to the bus hotplug handler. The device would be gone at that
> point.
> 
> machine_unplug_handler(dev)
>     /* eventually do unplug stuff */
>     bus_unplug_handler(dev)
>     /* dev is gone, we can't do more unplug stuff */
> 
> So move the object_unparent() to the original caller of the unplug. For
> now, keep the unrealize() at the original places of the
> object_unparent(). For implicitly chained hotplug handlers (e.g. pc
> code calling acpi hotplug handlers), the object_unparent() has to be
> done by the outermost caller. So when calling hotplug_handler_unplug()
> from inside an unplug handler, nothing is to be done.
> 
> hotplug_handler_unplug(dev) -> calls machine_unplug_handler()
>     machine_unplug_handler(dev) {
>         /* eventually do unplug stuff */
>         bus_unplug_handler(dev) -> calls unrealize(dev)
>         /* we can do more unplug stuff but device already unrealized */
>     }
> object_unparent(dev)
> 
> In the long run, every unplug action should be factored out of the
> unrealize() function into the unplug handler (especially for PCI). Then
> we can get rid of the additonal unrealize() calls and object_unparent()
> will properly unrealize the device hierarchy after the device has been
> unplugged.
> 
> hotplug_handler_unplug(dev) -> calls machine_unplug_handler()
>     machine_unplug_handler(dev) {
>         /* eventually do unplug stuff */
>         bus_unplug_handler(dev) -> only unplugs, does not unrealize
>         /* we can do more unplug stuff */
>     }
> object_unparent(dev) -> will unrealize
> 
> The original approach was suggested by Igor Mammedov for the PCI
> part, but I extended it to all hotplug handlers. I consider this one
> step into the right direction.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/acpi/cpu.c            |  1 +
>  hw/acpi/memory_hotplug.c |  1 +
>  hw/acpi/pcihp.c          |  3 ++-
>  hw/core/qdev.c           |  3 +--
>  hw/i386/pc.c             |  5 ++---
>  hw/pci/pcie.c            |  3 ++-
>  hw/pci/shpc.c            |  3 ++-
>  hw/ppc/spapr.c           |  4 ++--
>  hw/ppc/spapr_pci.c       |  3 ++-
>  hw/s390x/css-bridge.c    |  2 +-
>  hw/s390x/s390-pci-bus.c  | 13 ++++++++-----
>  qdev-monitor.c           |  9 +++++++--
>  12 files changed, 31 insertions(+), 19 deletions(-)

s390 parts look reasonable.

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH RFCv2 5/9] virtio-pci: Allow to specify additional interfaces for the base type

[Cornelia Huck]

On Wed, 23 Jan 2019 20:55:23 +0100
David Hildenbrand <david@redhat.com> wrote:

> Let's allow to specify additional interfaces for the base type (e.g.
> later TYPE_MEMORY_DEVICE), something that was possible before the
> rework of virtio PCI device instantiation.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 1 +
>  hw/virtio/virtio-pci.h | 1 +
>  2 files changed, 2 insertions(+)

Sounds reasonable.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH RFCv2 0/9] qdev: Hotplug handler chaining + virtio-pmem

[Igor Mammedov]

On Wed, 23 Jan 2019 20:55:18 +0100
David Hildenbrand <david@redhat.com> wrote:

> This series implements supprt for hotplug handler chaining (proposed
> by Igor), something that is necessary to turn selected virtio devices into
> memory devices. Planned devices inlude virtio-mem and virtio-pmem. The
> current prototype of virtio-pmem is included.
> 
> The machine hotplug handler can intercept hotplug handler calls
> to properly prepare/teardown the memory device part of a device. Control
> is then passed on to the actual bus hotplug handler. So the default hotplug
> handler is effectively overwritten to make interception possible.
> 
> It is based on the following patches/series
> - [PATCH v1] pc: Use hotplug_handler_(plug|unplug|unplug_request)
> -- Queued by Paolo
> - [PATCH v3 0/2] s390x/pci: hotplug handler fixes and reworks
> 
> Patch 1-3 are the preparations for hotplug handler chaining.
we probably should merge this ones even without pmem patches.

> The remaining patches are a modified prototype of virtio-pmem.
I'm not sure yet that virtio-pmem should be merged.

Initial goal for fake pmem was to eliminate page-cache in guest
so that only host will have it and cached pages could be shared
between several guest.

However recently (if I read kernel driver thread right),
sharing is to be disabled due to security implication and then
only dropping page cache on one side is left, which could be done
using existing frontends/backends (disabling caching on host side).


> TODO:
> - More testing for patch #1 + eventually write some unplug tests
> - Pankaj has to fixup some things in "virtio-pmem: Prototype"
> -- See patch description for details
> 
> I modified Pankajs work to work with this series. virtio-pmem is included
> as it was requested during review of previous preparations to showcase a
> real user, so we can discuss if this is good enough for us or if we have
> to do further changes.
> 
> More details about virtio-pmem (including the Linux guest driver side)
> can be found at:
>     https://lkml.org/lkml/2018/7/13/102
>     https://lkml.org/lkml/2019/1/9/756
> 
> Example: defining a simple virtio-pmem device (on /dev/zero for simplicity):
> 
>     qemu-system-x86_64 \
>      -machine pc \
>      -monitor stdio \
>      -m 8G,maxmem=20G \
>      -object memory-backend-file,id=mem1,mem-path=/dev/zero,size=4G \
>      -device virtio-pmem-pci,id=vp1,memdev=mem1
> 
> QEMU 3.0.50 monitor - type 'help' for more information
> (qemu) info memory-devices
> Memory device [virtio-pmem]: "vp1"
>   memaddr: 0x240000000
>   size: 4294967296
>   memdev: /objects/mem1
> 
> (qemu) info memory_size_summary
> base memory: 8589934592
> plugged memory: 4294967296
> 
> RFC -> RFCv2
> - "virtio-pmem: Prototype"
> - Minor documentation/style changes
> - "virtio-pci: Proxy for virtio-pmem"
> -- Separate header file virtio-pmem-pci.h
> - "pc: Support for virtio-pmem-pci"
> -- Only handle virtio-pmem-pci specially
> 
> 
> David Hildenbrand (6):
>   qdev: Let the hotplug_handler_unplug() caller delete the device
>   qdev: Provide qdev_get_bus_hotplug_handler()
>   virtio-pci: Allow to specify additional interfaces for the base type
>   hmp: Handle virtio-pmem when printing memory device infos
>   numa: Handle virtio-pmem in NUMA stats
>   pc: Support for virtio-pmem-pci
> 
> Igor Mammedov (1):
>   qdev: Let machine hotplug handler to override bus hotplug handler
> 
> Pankaj Gupta (2):
>   virtio-pmem: Prototype
>   virtio-pci: Proxy for virtio-pmem
> 
>  default-configs/i386-softmmu.mak            |   1 +
>  hmp.c                                       |  27 +--
>  hw/acpi/cpu.c                               |   1 +
>  hw/acpi/memory_hotplug.c                    |   1 +
>  hw/acpi/pcihp.c                             |   3 +-
>  hw/core/qdev.c                              |  19 +-
>  hw/i386/pc.c                                |  75 +++++++-
>  hw/pci/pcie.c                               |   3 +-
>  hw/pci/shpc.c                               |   3 +-
>  hw/ppc/spapr.c                              |   4 +-
>  hw/ppc/spapr_pci.c                          |   3 +-
>  hw/s390x/css-bridge.c                       |   2 +-
>  hw/s390x/s390-pci-bus.c                     |  13 +-
>  hw/virtio/Makefile.objs                     |   3 +
>  hw/virtio/virtio-pci.c                      |   1 +
>  hw/virtio/virtio-pci.h                      |   1 +
>  hw/virtio/virtio-pmem-pci.c                 | 144 +++++++++++++++
>  hw/virtio/virtio-pmem-pci.h                 |  39 ++++
>  hw/virtio/virtio-pmem.c                     | 191 ++++++++++++++++++++
>  include/hw/qdev-core.h                      |  12 ++
>  include/hw/virtio/virtio-pmem.h             |  54 ++++++
>  include/standard-headers/linux/virtio_ids.h |   1 +
>  numa.c                                      |  24 +--
>  qapi/misc.json                              |  28 ++-
>  qdev-monitor.c                              |   9 +-
>  25 files changed, 612 insertions(+), 50 deletions(-)
>  create mode 100644 hw/virtio/virtio-pmem-pci.c
>  create mode 100644 hw/virtio/virtio-pmem-pci.h
>  create mode 100644 hw/virtio/virtio-pmem.c
>  create mode 100644 include/hw/virtio/virtio-pmem.h
> 

Re: [Qemu-devel] [PATCH RFCv2 0/9] qdev: Hotplug handler chaining + virtio-pmem

[David Hildenbrand]

On 28.01.19 15:18, Igor Mammedov wrote:
> On Wed, 23 Jan 2019 20:55:18 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> This series implements supprt for hotplug handler chaining (proposed
>> by Igor), something that is necessary to turn selected virtio devices into
>> memory devices. Planned devices inlude virtio-mem and virtio-pmem. The
>> current prototype of virtio-pmem is included.
>>
>> The machine hotplug handler can intercept hotplug handler calls
>> to properly prepare/teardown the memory device part of a device. Control
>> is then passed on to the actual bus hotplug handler. So the default hotplug
>> handler is effectively overwritten to make interception possible.
>>
>> It is based on the following patches/series
>> - [PATCH v1] pc: Use hotplug_handler_(plug|unplug|unplug_request)
>> -- Queued by Paolo
>> - [PATCH v3 0/2] s390x/pci: hotplug handler fixes and reworks
>>
>> Patch 1-3 are the preparations for hotplug handler chaining.
> we probably should merge this ones even without pmem patches.

Sounds good to me. I'll do more testing.

> 
>> The remaining patches are a modified prototype of virtio-pmem.
> I'm not sure yet that virtio-pmem should be merged.
> 
> Initial goal for fake pmem was to eliminate page-cache in guest
> so that only host will have it and cached pages could be shared
> between several guest.
> 
> However recently (if I read kernel driver thread right),
> sharing is to be disabled due to security implication and then
> only dropping page cache on one side is left, which could be done
> using existing frontends/backends (disabling caching on host side).
> 

Yes, we should at least wait for the kernel part to settle. Anyhow, I
will base my virtio-mem work on this in a very similar fashion!

Thanks Igor!


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFCv2 0/9] qdev: Hotplug handler chaining + virtio-pmem

[David Hildenbrand]

On 28.01.19 15:18, Igor Mammedov wrote:
> On Wed, 23 Jan 2019 20:55:18 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> This series implements supprt for hotplug handler chaining (proposed
>> by Igor), something that is necessary to turn selected virtio devices into
>> memory devices. Planned devices inlude virtio-mem and virtio-pmem. The
>> current prototype of virtio-pmem is included.
>>
>> The machine hotplug handler can intercept hotplug handler calls
>> to properly prepare/teardown the memory device part of a device. Control
>> is then passed on to the actual bus hotplug handler. So the default hotplug
>> handler is effectively overwritten to make interception possible.
>>
>> It is based on the following patches/series
>> - [PATCH v1] pc: Use hotplug_handler_(plug|unplug|unplug_request)
>> -- Queued by Paolo
>> - [PATCH v3 0/2] s390x/pci: hotplug handler fixes and reworks
>>
>> Patch 1-3 are the preparations for hotplug handler chaining.
> we probably should merge this ones even without pmem patches.

Just to verify, does the general approach on how to hotplug virtio based
memory devices now looks okay to you?

I'll resend the qdev parts separately after more testing. Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFCv2 4/9] virtio-pmem: Prototype

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> From: Pankaj Gupta <pagupta@redhat.com>
>
> This is the current protoype of virtio-pmem. Support will require
> machine changes for the architectures that will support it, so it will
> not yet be compiled.
>
> TODO:
> - Use separate struct for tracking requests internally
> - Move request/response structs to linux headers
> - Factor out linux header sync
> - Drop debug printfs
>
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> [ MemoryDevice/MemoryRegion changes, cleanups, addr property "memaddr",
>   split up patches, unplug handler ]
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
[...]
> diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
> index 6d5c3b2d4f..346389565a 100644
> --- a/include/standard-headers/linux/virtio_ids.h
> +++ b/include/standard-headers/linux/virtio_ids.h
> @@ -43,5 +43,6 @@
>  #define VIRTIO_ID_INPUT        18 /* virtio input */
>  #define VIRTIO_ID_VSOCK        19 /* virtio vsock transport */
>  #define VIRTIO_ID_CRYPTO       20 /* virtio crypto */
> +#define VIRTIO_ID_PMEM         25 /* virtio pmem */
>  
>  #endif /* _LINUX_VIRTIO_IDS_H */
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 24d20a880a..b71eca2666 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -2949,16 +2949,42 @@
>            }
>  }
>  
> +##
> +# @VirtioPMEMDeviceInfo:
> +#
> +# VirtioPMEM state information
> +#
> +# @id: device's ID
> +#
> +# @memaddr: physical address in memory, where device is mapped
> +#
> +# @size: size of memory that the device provides
> +#
> +# @memdev: memory backend linked with device
> +#
> +# Since: 4.0
> +##

This is like PCDIMMDeviceInfo less @slot, @node, @hotplugged,
@hotpluggable, and with @addr renamed to @memaddr.

Any particular reason for the rename?

> +{ 'struct': 'VirtioPMEMDeviceInfo',
> +  'data': { '*id': 'str',
> +            'memaddr': 'size',
> +            'size': 'size',
> +            'memdev': 'str'
> +          }
> +}
> +
>  ##
>  # @MemoryDeviceInfo:
>  #
>  # Union containing information about a memory device
>  #
> +# nvdimm is included since 2.12. virtio-pmem is included since 4.0.
> +#

Let's stick to the way we document similar things elsewhere:

   # @nvdimm: since 2.12
   #
   # @virtio-pmem: since 4.0
   #

>  # Since: 2.1
>  ##
>  { 'union': 'MemoryDeviceInfo',
>    'data': { 'dimm': 'PCDIMMDeviceInfo',
> -            'nvdimm': 'PCDIMMDeviceInfo'
> +            'nvdimm': 'PCDIMMDeviceInfo',
> +            'virtio-pmem': 'VirtioPMEMDeviceInfo'
>            }
>  }

Re: [Qemu-devel] [PATCH RFCv2 7/9] hmp: Handle virtio-pmem when printing memory device infos

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * David Hildenbrand (david@redhat.com) wrote:
>> Print the memory device info just like for PCDIMM/NVDIMM.
>> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hmp.c | 27 +++++++++++++++------------
>>  1 file changed, 15 insertions(+), 12 deletions(-)
>> 
>> diff --git a/hmp.c b/hmp.c
>> index 8da5fd8760..25c32e0810 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -2553,6 +2553,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
>>      Error *err = NULL;
>>      MemoryDeviceInfoList *info_list = qmp_query_memory_devices(&err);
>>      MemoryDeviceInfoList *info;
>> +    VirtioPMEMDeviceInfo *vpi;
>>      MemoryDeviceInfo *value;
>>      PCDIMMDeviceInfo *di;
>>  
>> @@ -2562,19 +2563,9 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
>>          if (value) {
>>              switch (value->type) {
>>              case MEMORY_DEVICE_INFO_KIND_DIMM:
>> -                di = value->u.dimm.data;
>> -                break;
>> -
>>              case MEMORY_DEVICE_INFO_KIND_NVDIMM:
>> -                di = value->u.nvdimm.data;
>> -                break;
>> -
>> -            default:
>> -                di = NULL;
>> -                break;
>> -            }
>> -
>> -            if (di) {
>> +                di = value->type == MEMORY_DEVICE_INFO_KIND_DIMM ?
>> +                     value->u.dimm.data : value->u.nvdimm.data;
>>                  monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
>>                                 MemoryDeviceInfoKind_str(value->type),
>>                                 di->id ? di->id : "");
>> @@ -2587,6 +2578,18 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
>>                                 di->hotplugged ? "true" : "false");
>>                  monitor_printf(mon, "  hotpluggable: %s\n",
>>                                 di->hotpluggable ? "true" : "false");
>> +                break;
>> +            case MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM:
>> +                vpi = value->u.virtio_pmem.data;
>> +                monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
>> +                               MemoryDeviceInfoKind_str(value->type),
>> +                               vpi->id ? vpi->id : "");
>> +                monitor_printf(mon, "  memaddr: 0x%" PRIx64 "\n", vpi->memaddr);
>> +                monitor_printf(mon, "  size: %" PRIu64 "\n", vpi->size);
>> +                monitor_printf(mon, "  memdev: %s\n", vpi->memdev);
>> +                break;
>> +            default:
>> +                g_assert_not_reached();
>
>
> Although I'd prefer if that assert was replaced by a print
> saying it was an unknown type.

I would not.  If we reach this, something must have scribbled over
value->type and who knows what else.  Continuing is unsafe.  Looks like
a textbook use of assertions to me.

> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
>>              }
>>          }
>>      }
>> -- 
>> 2.17.2
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFCv2 4/9] virtio-pmem: Prototype

[David Hildenbrand]

On 31.01.19 19:19, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> From: Pankaj Gupta <pagupta@redhat.com>
>>
>> This is the current protoype of virtio-pmem. Support will require
>> machine changes for the architectures that will support it, so it will
>> not yet be compiled.
>>
>> TODO:
>> - Use separate struct for tracking requests internally
>> - Move request/response structs to linux headers
>> - Factor out linux header sync
>> - Drop debug printfs
>>
>> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
>> [ MemoryDevice/MemoryRegion changes, cleanups, addr property "memaddr",
>>   split up patches, unplug handler ]
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
> [...]
>> diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
>> index 6d5c3b2d4f..346389565a 100644
>> --- a/include/standard-headers/linux/virtio_ids.h
>> +++ b/include/standard-headers/linux/virtio_ids.h
>> @@ -43,5 +43,6 @@
>>  #define VIRTIO_ID_INPUT        18 /* virtio input */
>>  #define VIRTIO_ID_VSOCK        19 /* virtio vsock transport */
>>  #define VIRTIO_ID_CRYPTO       20 /* virtio crypto */
>> +#define VIRTIO_ID_PMEM         25 /* virtio pmem */
>>  
>>  #endif /* _LINUX_VIRTIO_IDS_H */
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index 24d20a880a..b71eca2666 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -2949,16 +2949,42 @@
>>            }
>>  }
>>  
>> +##
>> +# @VirtioPMEMDeviceInfo:
>> +#
>> +# VirtioPMEM state information
>> +#
>> +# @id: device's ID
>> +#
>> +# @memaddr: physical address in memory, where device is mapped
>> +#
>> +# @size: size of memory that the device provides
>> +#
>> +# @memdev: memory backend linked with device
>> +#
>> +# Since: 4.0
>> +##
> 
> This is like PCDIMMDeviceInfo less @slot, @node, @hotplugged,
> @hotpluggable, and with @addr renamed to @memaddr.
> 
> Any particular reason for the rename?

I answered the same question already and thought I documented it
somewhere ... but looks like it went missing.

We cannot use the "addr" property as that is already used e.g. for
virtio-pci/pci devices. And we will have e.g. virtio-pmem-pci as a proy.
So we have to chose a different one (unfortunately). We decided to also
use the name of the property in this struct here, as it will otherwise
be terribly confusing for the user.

-device virtio-pmem-pci,id=p0,bus=bux0,addr=0x01,memaddr=0x1000000...

> 
>> +{ 'struct': 'VirtioPMEMDeviceInfo',
>> +  'data': { '*id': 'str',
>> +            'memaddr': 'size',
>> +            'size': 'size',
>> +            'memdev': 'str'
>> +          }
>> +}
>> +
>>  ##
>>  # @MemoryDeviceInfo:
>>  #
>>  # Union containing information about a memory device
>>  #
>> +# nvdimm is included since 2.12. virtio-pmem is included since 4.0.
>> +#
> 
> Let's stick to the way we document similar things elsewhere:
> 
>    # @nvdimm: since 2.12
>    #
>    # @virtio-pmem: since 4.0

Sounds good, doesn't work :)

(tried this already, the checker will complain that these fields don't
exist)

Thanks!

>    #
> 
>>  # Since: 2.1
>>  ##
>>  { 'union': 'MemoryDeviceInfo',
>>    'data': { 'dimm': 'PCDIMMDeviceInfo',
>> -            'nvdimm': 'PCDIMMDeviceInfo'
>> +            'nvdimm': 'PCDIMMDeviceInfo',
>> +            'virtio-pmem': 'VirtioPMEMDeviceInfo'
>>            }
>>  }


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFCv2 4/9] virtio-pmem: Prototype

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 31.01.19 19:19, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
>>> From: Pankaj Gupta <pagupta@redhat.com>
>>>
>>> This is the current protoype of virtio-pmem. Support will require
>>> machine changes for the architectures that will support it, so it will
>>> not yet be compiled.
>>>
>>> TODO:
>>> - Use separate struct for tracking requests internally
>>> - Move request/response structs to linux headers
>>> - Factor out linux header sync
>>> - Drop debug printfs
>>>
>>> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
>>> [ MemoryDevice/MemoryRegion changes, cleanups, addr property "memaddr",
>>>   split up patches, unplug handler ]
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>> [...]
>>> diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
>>> index 6d5c3b2d4f..346389565a 100644
>>> --- a/include/standard-headers/linux/virtio_ids.h
>>> +++ b/include/standard-headers/linux/virtio_ids.h
>>> @@ -43,5 +43,6 @@
>>>  #define VIRTIO_ID_INPUT        18 /* virtio input */
>>>  #define VIRTIO_ID_VSOCK        19 /* virtio vsock transport */
>>>  #define VIRTIO_ID_CRYPTO       20 /* virtio crypto */
>>> +#define VIRTIO_ID_PMEM         25 /* virtio pmem */
>>>  
>>>  #endif /* _LINUX_VIRTIO_IDS_H */
>>> diff --git a/qapi/misc.json b/qapi/misc.json
>>> index 24d20a880a..b71eca2666 100644
>>> --- a/qapi/misc.json
>>> +++ b/qapi/misc.json
>>> @@ -2949,16 +2949,42 @@
>>>            }
>>>  }
>>>  
>>> +##
>>> +# @VirtioPMEMDeviceInfo:
>>> +#
>>> +# VirtioPMEM state information
>>> +#
>>> +# @id: device's ID
>>> +#
>>> +# @memaddr: physical address in memory, where device is mapped
>>> +#
>>> +# @size: size of memory that the device provides
>>> +#
>>> +# @memdev: memory backend linked with device
>>> +#
>>> +# Since: 4.0
>>> +##
>> 
>> This is like PCDIMMDeviceInfo less @slot, @node, @hotplugged,
>> @hotpluggable, and with @addr renamed to @memaddr.
>> 
>> Any particular reason for the rename?
>
> I answered the same question already and thought I documented it
> somewhere ... but looks like it went missing.

Looks like my end-of-year mental garbage collection was overeager again.

> We cannot use the "addr" property as that is already used e.g. for
> virtio-pci/pci devices. And we will have e.g. virtio-pmem-pci as a proy.
> So we have to chose a different one (unfortunately). We decided to also
> use the name of the property in this struct here, as it will otherwise
> be terribly confusing for the user.
>
> -device virtio-pmem-pci,id=p0,bus=bux0,addr=0x01,memaddr=0x1000000...
>
>> 
>>> +{ 'struct': 'VirtioPMEMDeviceInfo',
>>> +  'data': { '*id': 'str',
>>> +            'memaddr': 'size',
>>> +            'size': 'size',
>>> +            'memdev': 'str'
>>> +          }
>>> +}
>>> +
>>>  ##
>>>  # @MemoryDeviceInfo:
>>>  #
>>>  # Union containing information about a memory device
>>>  #
>>> +# nvdimm is included since 2.12. virtio-pmem is included since 4.0.
>>> +#
>> 
>> Let's stick to the way we document similar things elsewhere:
>> 
>>    # @nvdimm: since 2.12
>>    #
>>    # @virtio-pmem: since 4.0
>
> Sounds good, doesn't work :)
>
> (tried this already, the checker will complain that these fields don't
> exist)

Doc generator shortcoming *sigh*.

QAPI part
Acked-by: Markus Armbruster <armbru@redhat.com>

> Thanks!
>
>>    #
>> 
>>>  # Since: 2.1
>>>  ##
>>>  { 'union': 'MemoryDeviceInfo',
>>>    'data': { 'dimm': 'PCDIMMDeviceInfo',
>>> -            'nvdimm': 'PCDIMMDeviceInfo'
>>> +            'nvdimm': 'PCDIMMDeviceInfo',
>>> +            'virtio-pmem': 'VirtioPMEMDeviceInfo'
>>>            }
>>>  }

Re: [Qemu-devel] [PATCH RFCv2 7/9] hmp: Handle virtio-pmem when printing memory device infos

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * David Hildenbrand (david@redhat.com) wrote:
> >> Print the memory device info just like for PCDIMM/NVDIMM.
> >> 
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hmp.c | 27 +++++++++++++++------------
> >>  1 file changed, 15 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/hmp.c b/hmp.c
> >> index 8da5fd8760..25c32e0810 100644
> >> --- a/hmp.c
> >> +++ b/hmp.c
> >> @@ -2553,6 +2553,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
> >>      Error *err = NULL;
> >>      MemoryDeviceInfoList *info_list = qmp_query_memory_devices(&err);
> >>      MemoryDeviceInfoList *info;
> >> +    VirtioPMEMDeviceInfo *vpi;
> >>      MemoryDeviceInfo *value;
> >>      PCDIMMDeviceInfo *di;
> >>  
> >> @@ -2562,19 +2563,9 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
> >>          if (value) {
> >>              switch (value->type) {
> >>              case MEMORY_DEVICE_INFO_KIND_DIMM:
> >> -                di = value->u.dimm.data;
> >> -                break;
> >> -
> >>              case MEMORY_DEVICE_INFO_KIND_NVDIMM:
> >> -                di = value->u.nvdimm.data;
> >> -                break;
> >> -
> >> -            default:
> >> -                di = NULL;
> >> -                break;
> >> -            }
> >> -
> >> -            if (di) {
> >> +                di = value->type == MEMORY_DEVICE_INFO_KIND_DIMM ?
> >> +                     value->u.dimm.data : value->u.nvdimm.data;
> >>                  monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
> >>                                 MemoryDeviceInfoKind_str(value->type),
> >>                                 di->id ? di->id : "");
> >> @@ -2587,6 +2578,18 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
> >>                                 di->hotplugged ? "true" : "false");
> >>                  monitor_printf(mon, "  hotpluggable: %s\n",
> >>                                 di->hotpluggable ? "true" : "false");
> >> +                break;
> >> +            case MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM:
> >> +                vpi = value->u.virtio_pmem.data;
> >> +                monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
> >> +                               MemoryDeviceInfoKind_str(value->type),
> >> +                               vpi->id ? vpi->id : "");
> >> +                monitor_printf(mon, "  memaddr: 0x%" PRIx64 "\n", vpi->memaddr);
> >> +                monitor_printf(mon, "  size: %" PRIu64 "\n", vpi->size);
> >> +                monitor_printf(mon, "  memdev: %s\n", vpi->memdev);
> >> +                break;
> >> +            default:
> >> +                g_assert_not_reached();
> >
> >
> > Although I'd prefer if that assert was replaced by a print
> > saying it was an unknown type.
> 
> I would not.  If we reach this, something must have scribbled over
> value->type and who knows what else.  Continuing is unsafe.  Looks like
> a textbook use of assertions to me.

Or it could be that someone added a new type of memory device and forgot
to update the hmp code.

Dave

> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> >>              }
> >>          }
> >>      }
> >> -- 
> >> 2.17.2
> >> 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFCv2 7/9] hmp: Handle virtio-pmem when printing memory device infos

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>> 
>> > * David Hildenbrand (david@redhat.com) wrote:
>> >> Print the memory device info just like for PCDIMM/NVDIMM.
>> >> 
>> >> Signed-off-by: David Hildenbrand <david@redhat.com>
>> >> ---
>> >>  hmp.c | 27 +++++++++++++++------------
>> >>  1 file changed, 15 insertions(+), 12 deletions(-)
>> >> 
>> >> diff --git a/hmp.c b/hmp.c
>> >> index 8da5fd8760..25c32e0810 100644
>> >> --- a/hmp.c
>> >> +++ b/hmp.c
>> >> @@ -2553,6 +2553,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
>> >>      Error *err = NULL;
>> >>      MemoryDeviceInfoList *info_list = qmp_query_memory_devices(&err);
>> >>      MemoryDeviceInfoList *info;
>> >> +    VirtioPMEMDeviceInfo *vpi;
>> >>      MemoryDeviceInfo *value;
>> >>      PCDIMMDeviceInfo *di;
>> >>  
>> >> @@ -2562,19 +2563,9 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
>> >>          if (value) {
>> >>              switch (value->type) {
>> >>              case MEMORY_DEVICE_INFO_KIND_DIMM:
>> >> -                di = value->u.dimm.data;
>> >> -                break;
>> >> -
>> >>              case MEMORY_DEVICE_INFO_KIND_NVDIMM:
>> >> -                di = value->u.nvdimm.data;
>> >> -                break;
>> >> -
>> >> -            default:
>> >> -                di = NULL;
>> >> -                break;
>> >> -            }
>> >> -
>> >> -            if (di) {
>> >> +                di = value->type == MEMORY_DEVICE_INFO_KIND_DIMM ?
>> >> +                     value->u.dimm.data : value->u.nvdimm.data;
>> >>                  monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
>> >>                                 MemoryDeviceInfoKind_str(value->type),
>> >>                                 di->id ? di->id : "");
>> >> @@ -2587,6 +2578,18 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
>> >>                                 di->hotplugged ? "true" : "false");
>> >>                  monitor_printf(mon, "  hotpluggable: %s\n",
>> >>                                 di->hotpluggable ? "true" : "false");
>> >> +                break;
>> >> +            case MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM:
>> >> +                vpi = value->u.virtio_pmem.data;
>> >> +                monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
>> >> +                               MemoryDeviceInfoKind_str(value->type),
>> >> +                               vpi->id ? vpi->id : "");
>> >> +                monitor_printf(mon, "  memaddr: 0x%" PRIx64 "\n", vpi->memaddr);
>> >> +                monitor_printf(mon, "  size: %" PRIu64 "\n", vpi->size);
>> >> +                monitor_printf(mon, "  memdev: %s\n", vpi->memdev);
>> >> +                break;
>> >> +            default:
>> >> +                g_assert_not_reached();
>> >
>> >
>> > Although I'd prefer if that assert was replaced by a print
>> > saying it was an unknown type.
>> 
>> I would not.  If we reach this, something must have scribbled over
>> value->type and who knows what else.  Continuing is unsafe.  Looks like
>> a textbook use of assertions to me.
>
> Or it could be that someone added a new type of memory device and forgot
> to update the hmp code.

Programming error -> assert.  Nothing catches attention in testing like
an assertion failure.

Re: [Qemu-devel] [PATCH RFCv2 7/9] hmp: Handle virtio-pmem when printing memory device infos

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> >> 
> >> > * David Hildenbrand (david@redhat.com) wrote:
> >> >> Print the memory device info just like for PCDIMM/NVDIMM.
> >> >> 
> >> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> >> ---
> >> >>  hmp.c | 27 +++++++++++++++------------
> >> >>  1 file changed, 15 insertions(+), 12 deletions(-)
> >> >> 
> >> >> diff --git a/hmp.c b/hmp.c
> >> >> index 8da5fd8760..25c32e0810 100644
> >> >> --- a/hmp.c
> >> >> +++ b/hmp.c
> >> >> @@ -2553,6 +2553,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
> >> >>      Error *err = NULL;
> >> >>      MemoryDeviceInfoList *info_list = qmp_query_memory_devices(&err);
> >> >>      MemoryDeviceInfoList *info;
> >> >> +    VirtioPMEMDeviceInfo *vpi;
> >> >>      MemoryDeviceInfo *value;
> >> >>      PCDIMMDeviceInfo *di;
> >> >>  
> >> >> @@ -2562,19 +2563,9 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
> >> >>          if (value) {
> >> >>              switch (value->type) {
> >> >>              case MEMORY_DEVICE_INFO_KIND_DIMM:
> >> >> -                di = value->u.dimm.data;
> >> >> -                break;
> >> >> -
> >> >>              case MEMORY_DEVICE_INFO_KIND_NVDIMM:
> >> >> -                di = value->u.nvdimm.data;
> >> >> -                break;
> >> >> -
> >> >> -            default:
> >> >> -                di = NULL;
> >> >> -                break;
> >> >> -            }
> >> >> -
> >> >> -            if (di) {
> >> >> +                di = value->type == MEMORY_DEVICE_INFO_KIND_DIMM ?
> >> >> +                     value->u.dimm.data : value->u.nvdimm.data;
> >> >>                  monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
> >> >>                                 MemoryDeviceInfoKind_str(value->type),
> >> >>                                 di->id ? di->id : "");
> >> >> @@ -2587,6 +2578,18 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
> >> >>                                 di->hotplugged ? "true" : "false");
> >> >>                  monitor_printf(mon, "  hotpluggable: %s\n",
> >> >>                                 di->hotpluggable ? "true" : "false");
> >> >> +                break;
> >> >> +            case MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM:
> >> >> +                vpi = value->u.virtio_pmem.data;
> >> >> +                monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
> >> >> +                               MemoryDeviceInfoKind_str(value->type),
> >> >> +                               vpi->id ? vpi->id : "");
> >> >> +                monitor_printf(mon, "  memaddr: 0x%" PRIx64 "\n", vpi->memaddr);
> >> >> +                monitor_printf(mon, "  size: %" PRIu64 "\n", vpi->size);
> >> >> +                monitor_printf(mon, "  memdev: %s\n", vpi->memdev);
> >> >> +                break;
> >> >> +            default:
> >> >> +                g_assert_not_reached();
> >> >
> >> >
> >> > Although I'd prefer if that assert was replaced by a print
> >> > saying it was an unknown type.
> >> 
> >> I would not.  If we reach this, something must have scribbled over
> >> value->type and who knows what else.  Continuing is unsafe.  Looks like
> >> a textbook use of assertions to me.
> >
> > Or it could be that someone added a new type of memory device and forgot
> > to update the hmp code.
> 
> Programming error -> assert.  Nothing catches attention in testing like
> an assertion failure.

I don't want to get back through that; we've had this argument before;
but I'll still say in this case I'd prefer a clean error rather than
abort.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFCv2 9/9] pc: Support for virtio-pmem-pci

[Igor Mammedov]

On Wed, 23 Jan 2019 20:55:27 +0100
David Hildenbrand <david@redhat.com> wrote:

> Override the device hotplug handler to properly handle the memory device
> part via virtio-pmem-pci callbacks from the machine hotplug handler and
> forward to the actual PCI bus hotplug handler.
> 
> As PCI hotplug has not been properly factored out into hotplug handlers,
> most magic is performed in the (un)realize functions. Also some PCI host
> buses don't have a PCI hotplug handler at all yet, just to be sure that
> we alway have a hotplug handler on x86, add a simple error check.
> 
> Unlocking virtio-pmem will unlock virtio-pmem-pci.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  default-configs/i386-softmmu.mak |  1 +
>  hw/i386/pc.c                     | 70 +++++++++++++++++++++++++++++++-
>  2 files changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index 64c998c4c8..3f63e95a55 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -52,6 +52,7 @@ CONFIG_APIC=y
>  CONFIG_IOAPIC=y
>  CONFIG_PVPANIC=y
>  CONFIG_MEM_DEVICE=y
> +CONFIG_VIRTIO_PMEM=y
>  CONFIG_DIMM=y
>  CONFIG_NVDIMM=y
>  CONFIG_ACPI_NVDIMM=y
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index fd0cb29ba9..6a176caeb9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -75,6 +75,7 @@
>  #include "hw/usb.h"
>  #include "hw/i386/intel_iommu.h"
>  #include "hw/net/ne2000-isa.h"
> +#include "hw/virtio/virtio-pmem-pci.h"
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -2224,6 +2225,64 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>      numa_cpu_pre_plug(cpu_slot, dev, errp);
>  }
>  
> +static void pc_virtio_pmem_pci_pre_plug(HotplugHandler *hotplug_dev,
> +                                        DeviceState *dev, Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    if (!hotplug_dev2) {
> +        /*
> +         * Without a bus hotplug handler, we cannot control the plug/unplug
> +         * order. This should never be the case on x86, however better add
> +         * a safety net.
> +         */
> +        error_setg(errp, "virtio-pmem-pci not supported on this bus.");
> +        return;
> +    }
> +    virtio_pmem_pci_pre_plug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev),
       ^^^
doesn't do anything beside being dumb proxy to memory_device_pre_plug()
I'd just use memory_device_pre_plug() here and avoid so far needles wrappers

> +                             &local_err);
> +    if (!local_err) {
since logic is not trivial I'd add comment somewhere in this function
explaining why handlers are called in this particular order.

> +        hotplug_handler_pre_plug(hotplug_dev2, dev, &local_err);
> +    }
> +    error_propagate(errp, local_err);
> +}
> +
> +static void pc_virtio_pmem_pci_plug(HotplugHandler *hotplug_dev,
> +                                    DeviceState *dev, Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    virtio_pmem_pci_plug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev));
> +    hotplug_handler_plug(hotplug_dev2, dev, &local_err);
> +    if (local_err) {
> +        virtio_pmem_pci_unplug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev));
> +    }
> +    error_propagate(errp, local_err);
> +}
> +
> +static void pc_virtio_pmem_pci_unplug_request(HotplugHandler *hotplug_dev,
> +                                              DeviceState *dev, Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +
> +    hotplug_handler_unplug_request(hotplug_dev2, dev, errp);
> +}
> +
> +static void pc_virtio_pmem_pci_unplug(HotplugHandler *hotplug_dev,
> +                                      DeviceState *dev, Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    hotplug_handler_unplug(hotplug_dev2, dev, &local_err);
> +    if (!local_err) {
> +        virtio_pmem_pci_unplug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev));
> +    }
> +    error_propagate(errp, local_err);
> +}
> +
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>                                            DeviceState *dev, Error **errp)
>  {
> @@ -2231,6 +2290,8 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>          pc_memory_pre_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_pre_plug(hotplug_dev, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI)) {
> +        pc_virtio_pmem_pci_pre_plug(hotplug_dev, dev, errp);
>      }
>  }
>  
> @@ -2241,6 +2302,8 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>          pc_memory_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_plug(hotplug_dev, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI)) {
> +        pc_virtio_pmem_pci_plug(hotplug_dev, dev, errp);
>      }
>  }
>  
> @@ -2251,6 +2314,8 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>          pc_memory_unplug_request(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI)) {
> +        pc_virtio_pmem_pci_unplug_request(hotplug_dev, dev, errp);
>      } else {
>          error_setg(errp, "acpi: device unplug request for not supported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
> @@ -2264,6 +2329,8 @@ static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>          pc_memory_unplug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_unplug_cb(hotplug_dev, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI)) {
> +        pc_virtio_pmem_pci_unplug(hotplug_dev, dev, errp);
>      } else {
>          error_setg(errp, "acpi: device unplug for not supported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
> @@ -2274,7 +2341,8 @@ static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
>                                               DeviceState *dev)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> -        object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
> +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI)) {
>          return HOTPLUG_HANDLER(machine);
>      }
>  

Re: [Qemu-devel] [PATCH RFCv2 9/9] pc: Support for virtio-pmem-pci

[David Hildenbrand]

On 06.02.19 14:01, Igor Mammedov wrote:
> On Wed, 23 Jan 2019 20:55:27 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Override the device hotplug handler to properly handle the memory device
>> part via virtio-pmem-pci callbacks from the machine hotplug handler and
>> forward to the actual PCI bus hotplug handler.
>>
>> As PCI hotplug has not been properly factored out into hotplug handlers,
>> most magic is performed in the (un)realize functions. Also some PCI host
>> buses don't have a PCI hotplug handler at all yet, just to be sure that
>> we alway have a hotplug handler on x86, add a simple error check.
>>
>> Unlocking virtio-pmem will unlock virtio-pmem-pci.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  default-configs/i386-softmmu.mak |  1 +
>>  hw/i386/pc.c                     | 70 +++++++++++++++++++++++++++++++-
>>  2 files changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
>> index 64c998c4c8..3f63e95a55 100644
>> --- a/default-configs/i386-softmmu.mak
>> +++ b/default-configs/i386-softmmu.mak
>> @@ -52,6 +52,7 @@ CONFIG_APIC=y
>>  CONFIG_IOAPIC=y
>>  CONFIG_PVPANIC=y
>>  CONFIG_MEM_DEVICE=y
>> +CONFIG_VIRTIO_PMEM=y
>>  CONFIG_DIMM=y
>>  CONFIG_NVDIMM=y
>>  CONFIG_ACPI_NVDIMM=y
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index fd0cb29ba9..6a176caeb9 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -75,6 +75,7 @@
>>  #include "hw/usb.h"
>>  #include "hw/i386/intel_iommu.h"
>>  #include "hw/net/ne2000-isa.h"
>> +#include "hw/virtio/virtio-pmem-pci.h"
>>  
>>  /* debug PC/ISA interrupts */
>>  //#define DEBUG_IRQ
>> @@ -2224,6 +2225,64 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>      numa_cpu_pre_plug(cpu_slot, dev, errp);
>>  }
>>  
>> +static void pc_virtio_pmem_pci_pre_plug(HotplugHandler *hotplug_dev,
>> +                                        DeviceState *dev, Error **errp)
>> +{
>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>> +    Error *local_err = NULL;
>> +
>> +    if (!hotplug_dev2) {
>> +        /*
>> +         * Without a bus hotplug handler, we cannot control the plug/unplug
>> +         * order. This should never be the case on x86, however better add
>> +         * a safety net.
>> +         */
>> +        error_setg(errp, "virtio-pmem-pci not supported on this bus.");
>> +        return;
>> +    }
>> +    virtio_pmem_pci_pre_plug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev),
>        ^^^
> doesn't do anything beside being dumb proxy to memory_device_pre_plug()
> I'd just use memory_device_pre_plug() here and avoid so far needles wrappers

Had that initially but considered it cleaner. But I can change that back.

> 
>> +                             &local_err);
>> +    if (!local_err) {
> since logic is not trivial I'd add comment somewhere in this function
> explaining why handlers are called in this particular order.

Good idea, will do that.

Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFCv2 0/9] qdev: Hotplug handler chaining + virtio-pmem

[Igor Mammedov]

On Thu, 31 Jan 2019 15:52:25 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 28.01.19 15:18, Igor Mammedov wrote:
> > On Wed, 23 Jan 2019 20:55:18 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> > 
> >> This series implements supprt for hotplug handler chaining (proposed
> >> by Igor), something that is necessary to turn selected virtio devices into
> >> memory devices. Planned devices inlude virtio-mem and virtio-pmem. The
> >> current prototype of virtio-pmem is included.
> >>
> >> The machine hotplug handler can intercept hotplug handler calls
> >> to properly prepare/teardown the memory device part of a device. Control
> >> is then passed on to the actual bus hotplug handler. So the default hotplug
> >> handler is effectively overwritten to make interception possible.
> >>
> >> It is based on the following patches/series
> >> - [PATCH v1] pc: Use hotplug_handler_(plug|unplug|unplug_request)
> >> -- Queued by Paolo
> >> - [PATCH v3 0/2] s390x/pci: hotplug handler fixes and reworks
> >>
> >> Patch 1-3 are the preparations for hotplug handler chaining.
> > we probably should merge this ones even without pmem patches.
> 
> Just to verify, does the general approach on how to hotplug virtio based
> memory devices now looks okay to you?
it looks fine (minor comments aside)

rant: beside a little bit complex dance with nowadays virtio type names
but that's not related to your series. It just makes understanding virtio
code more difficult not saying it's wasn't already complicated to begin with.

> I'll resend the qdev parts separately after more testing. Thanks!
> 

Re: [Qemu-devel] [PATCH RFCv2 0/9] qdev: Hotplug handler chaining + virtio-pmem

[David Hildenbrand]

On 06.02.19 14:18, Igor Mammedov wrote:
> On Thu, 31 Jan 2019 15:52:25 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 28.01.19 15:18, Igor Mammedov wrote:
>>> On Wed, 23 Jan 2019 20:55:18 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> This series implements supprt for hotplug handler chaining (proposed
>>>> by Igor), something that is necessary to turn selected virtio devices into
>>>> memory devices. Planned devices inlude virtio-mem and virtio-pmem. The
>>>> current prototype of virtio-pmem is included.
>>>>
>>>> The machine hotplug handler can intercept hotplug handler calls
>>>> to properly prepare/teardown the memory device part of a device. Control
>>>> is then passed on to the actual bus hotplug handler. So the default hotplug
>>>> handler is effectively overwritten to make interception possible.
>>>>
>>>> It is based on the following patches/series
>>>> - [PATCH v1] pc: Use hotplug_handler_(plug|unplug|unplug_request)
>>>> -- Queued by Paolo
>>>> - [PATCH v3 0/2] s390x/pci: hotplug handler fixes and reworks
>>>>
>>>> Patch 1-3 are the preparations for hotplug handler chaining.
>>> we probably should merge this ones even without pmem patches.
>>
>> Just to verify, does the general approach on how to hotplug virtio based
>> memory devices now looks okay to you?
> it looks fine (minor comments aside)

Thanks a lot for taking your time to look into this Igor, highly
appreciated!

> 
> rant: beside a little bit complex dance with nowadays virtio type names
> but that's not related to your series. It just makes understanding virtio
> code more difficult not saying it's wasn't already complicated to begin with.

Entropy of an isolated system can never decrease over time :D

-- 

Thanks,

David / dhildenb