[Qemu-devel] [PATCH RFC 00/10] qdev: Hotplug handler chaining + virtio-pmem

[Qemu-devel] [PATCH RFC 00/10] 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
- [PULL v2 01/49] pci/pcie: stop plug/unplug if the slot is locked
-- Soon upstream
- [PATCH v1] pc: Use hotplug_handler_(plug|unplug|unplug_request)
-- Queued by Paolo
- [PATCH v2 0/6] s390x/pci: hotplug handler fixes and reworks
-- Partially queued

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

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


David Hildenbrand (7):
  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 PCI based memory devices
  pc: Enable support for virtio-pmem

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                                |  95 +++++++++-
 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                      |  15 ++
 hw/virtio/virtio-pmem-pci.c                 | 129 +++++++++++++
 hw/virtio/virtio-pmem.c                     | 189 ++++++++++++++++++++
 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                              |  26 ++-
 qdev-monitor.c                              |   9 +-
 24 files changed, 588 insertions(+), 50 deletions(-)
 create mode 100644 hw/virtio/virtio-pmem-pci.c
 create mode 100644 hw/virtio/virtio-pmem.c
 create mode 100644 include/hw/virtio/virtio-pmem.h

-- 
2.17.2

[Qemu-devel] [PATCH RFC 01/10] 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 interrupt 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.

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 8c7c1013f3..9fbf032c15 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 b74f2632ec..234c14e9f2 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 f399eeede6..f6034405d8 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)
@@ -992,7 +995,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);
@@ -1009,7 +1012,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

Re: [Qemu-devel] [PATCH RFC 01/10] qdev: Let the hotplug_handler_unplug() caller delete the device

[Igor Mammedov]

On Wed, 16 Jan 2019 12:35:14 +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 interrupt handlers, we want to overwrite a bus hotplug
s/interrupt/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.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Have you tested all affect use-cases after this patch?

> ---
>  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/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/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));
Is this object_unparent() that you moved from qdev_simple_device_unplug_cb()
in the hunk above?

IS it possible to split patch per subsystem for easier review?
> +        }
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  void qmp_device_del(const char *id, Error **errp)

Re: [Qemu-devel] [PATCH RFC 01/10] qdev: Let the hotplug_handler_unplug() caller delete the device

[David Hildenbrand]

On 18.01.19 10:58, Igor Mammedov wrote:
> On Wed, 16 Jan 2019 12:35:14 +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 interrupt handlers, we want to overwrite a bus hotplug
> s/interrupt/hotplug/

whoops :)

> 
>> 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.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Have you tested all affect use-cases after this patch?

I tested some (e.g. ACPI PCI hotplug/unplug, s390x PCI hotplug/unplug
...) to make sure this fundamentally workd, but certainly not all yet :)

> 
>> ---
>>  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/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/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));
> Is this object_unparent() that you moved from qdev_simple_device_unplug_cb()
> in the hunk above?

Yes, I moved it from all unplug handlers that do an object_unparent().

> 
> IS it possible to split patch per subsystem for easier review?

I am afraid not, unless you have an idea on how to do that (we could
temporarily somehow remember for each hotplug handler if it will delete
device itself or not - but I don't really like that approach).

Thanks!

>> +        }
>>      }
>> +    error_propagate(errp, local_err);
>>  }
>>  
>>  void qmp_device_del(const char *id, Error **errp)


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC 01/10] qdev: Let the hotplug_handler_unplug() caller delete the device

[Igor Mammedov]

On Fri, 18 Jan 2019 13:41:06 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 18.01.19 10:58, Igor Mammedov wrote:
> > On Wed, 16 Jan 2019 12:35:14 +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 interrupt handlers, we want to overwrite a bus hotplug  
> > s/interrupt/hotplug/  
> 
> whoops :)
> 
> >   
> >> 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.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>  
> > 
> > Have you tested all affect use-cases after this patch?  
> 
> I tested some (e.g. ACPI PCI hotplug/unplug, s390x PCI hotplug/unplug
> ...) to make sure this fundamentally workd, but certainly not all yet :)

I've looked at acpi/pci/pcie/shcp and qdev parts and it looks correct,
so for that parts on condition of complete manual testing (a device per affected unplug chain),

Reviewed-by: Igor Mammedov <imammedo@redhat.com>


we have limited coverage in tests, see qtest_qmp_device_del() for devices
that allow surprise removal but that covers only qdev_simple_device_unplug_cb()
part and is not sufficient.

It might be better adding missing ones at least one per category there,
but it implies simulating eject sequence from guest side (we do similar
thing to test device models using qtest accelerator), that should cover
all cases.


> >   
> >> ---
> >>  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/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/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));  
> > Is this object_unparent() that you moved from qdev_simple_device_unplug_cb()
> > in the hunk above?  
> 
> Yes, I moved it from all unplug handlers that do an object_unparent().
> 
> > 
> > IS it possible to split patch per subsystem for easier review?  
> 
> I am afraid not, unless you have an idea on how to do that (we could
> temporarily somehow remember for each hotplug handler if it will delete
> device itself or not - but I don't really like that approach).

ok, It's big but still manageable to review and better than temporary things
that would just add confusion.

> 
> Thanks!
> 
> >> +        }
> >>      }
> >> +    error_propagate(errp, local_err);
> >>  }
> >>  
> >>  void qmp_device_del(const char *id, Error **errp)  
> 
> 

[Qemu-devel] [PATCH RFC 02/10] 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 9614f76ae6..0632057fd6 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -278,6 +278,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 RFC 03/10] 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).

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 0632057fd6..893acc19b9 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -277,6 +277,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

Re: [Qemu-devel] [Qemu-ppc] [PATCH RFC 03/10] qdev: Provide qdev_get_bus_hotplug_handler()

[Murilo Opsfelder Araujo]

Hi, David.

On Wed, Jan 16, 2019 at 12:35:16PM +0100, David Hildenbrand 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).
>
> 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) {

Perhaps we don't need to check dev->parent_bus here since
qdev_get_bus_hotplug_handler() is already checking it.

> -        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 0632057fd6..893acc19b9 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -277,6 +277,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
>
>

--
Murilo

Re: [Qemu-devel] [Qemu-ppc] [PATCH RFC 03/10] qdev: Provide qdev_get_bus_hotplug_handler()

[David Hildenbrand]

On 16.01.19 19:41, Murilo Opsfelder Araujo wrote:
> Hi, David.
> 
> On Wed, Jan 16, 2019 at 12:35:16PM +0100, David Hildenbrand 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).
>>
>> 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) {
> 
> Perhaps we don't need to check dev->parent_bus here since
> qdev_get_bus_hotplug_handler() is already checking it.

This function will also be called from other places (see patch #9), thanks!


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC 03/10] qdev: Provide qdev_get_bus_hotplug_handler()

[Igor Mammedov]

On Wed, 16 Jan 2019 12:35:16 +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).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@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 0632057fd6..893acc19b9 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -277,6 +277,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

[Qemu-devel] [PATCH RFC 04/10] 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                     | 189 ++++++++++++++++++++
 include/hw/virtio/virtio-pmem.h             |  54 ++++++
 include/standard-headers/linux/virtio_ids.h |   1 +
 qapi/misc.json                              |  26 ++-
 5 files changed, 271 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 1b2799cfd8..5463c682f7 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
 endif
diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
new file mode 100644
index 0000000000..6fb78acf87
--- /dev/null
+++ b/hw/virtio/virtio-pmem.c
@@ -0,0 +1,189 @@
+/*
+ * Virtio PMEM device
+ *
+ * Copyright (C) 2018 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;
+    } else 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..85cee3ef39
--- /dev/null
+++ b/include/hw/virtio/virtio-pmem.h
@@ -0,0 +1,54 @@
+/*
+ * Virtio pmem device
+ *
+ * Copyright Red Hat, Inc. 2018
+ *
+ * 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..8ae709636d 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -2949,6 +2949,29 @@
           }
 }
 
+##
+# @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: 3.1
+##
+{ 'struct': 'VirtioPMEMDeviceInfo',
+  'data': { '*id': 'str',
+            'memaddr': 'size',
+            'size': 'size',
+            'memdev': 'str'
+          }
+}
+
 ##
 # @MemoryDeviceInfo:
 #
@@ -2958,7 +2981,8 @@
 ##
 { 'union': 'MemoryDeviceInfo',
   'data': { 'dimm': 'PCDIMMDeviceInfo',
-            'nvdimm': 'PCDIMMDeviceInfo'
+            'nvdimm': 'PCDIMMDeviceInfo',
+            'virtio-pmem': 'VirtioPMEMDeviceInfo'
           }
 }
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH RFC 04/10] virtio-pmem: Prototype

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1827 bytes --]

On 1/16/19 5:35 AM, David Hildenbrand wrote:
> 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>
> ---

> +++ b/qapi/misc.json
> @@ -2949,6 +2949,29 @@
>            }
>  }
>  
> +##
> +# @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: 3.1

Now 4.0

> +##
> +{ 'struct': 'VirtioPMEMDeviceInfo',
> +  'data': { '*id': 'str',
> +            'memaddr': 'size',
> +            'size': 'size',
> +            'memdev': 'str'
> +          }
> +}
> +
>  ##
>  # @MemoryDeviceInfo:
>  #
> @@ -2958,7 +2981,8 @@
>  ##
>  { 'union': 'MemoryDeviceInfo',

Does this union need a documentation update that virtio-pmem was added
in 4.0?

>    'data': { 'dimm': 'PCDIMMDeviceInfo',
> -            'nvdimm': 'PCDIMMDeviceInfo'
> +            'nvdimm': 'PCDIMMDeviceInfo',
> +            'virtio-pmem': 'VirtioPMEMDeviceInfo'
>            }
>  }
>  
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH RFC 04/10] virtio-pmem: Prototype

[David Hildenbrand]

On 16.01.19 15:46, Eric Blake wrote:
> On 1/16/19 5:35 AM, David Hildenbrand wrote:
>> 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>
>> ---
> 
>> +++ b/qapi/misc.json
>> @@ -2949,6 +2949,29 @@
>>            }
>>  }
>>  
>> +##
>> +# @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: 3.1
> 
> Now 4.0

Indeed. (if we'll get it into 4.0 of course ;) )
> 
>> +##
>> +{ 'struct': 'VirtioPMEMDeviceInfo',
>> +  'data': { '*id': 'str',
>> +            'memaddr': 'size',
>> +            'size': 'size',
>> +            'memdev': 'str'
>> +          }
>> +}
>> +
>>  ##
>>  # @MemoryDeviceInfo:
>>  #
>> @@ -2958,7 +2981,8 @@
>>  ##
>>  { 'union': 'MemoryDeviceInfo',
> 
> Does this union need a documentation update that virtio-pmem was added
> in 4.0?

We can add that, makes sense!

Thanks!

> 
>>    'data': { 'dimm': 'PCDIMMDeviceInfo',
>> -            'nvdimm': 'PCDIMMDeviceInfo'
>> +            'nvdimm': 'PCDIMMDeviceInfo',
>> +            'virtio-pmem': 'VirtioPMEMDeviceInfo'
>>            }
>>  }
>>  
>>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC 04/10] virtio-pmem: Prototype

[David Hildenbrand]

On 16.01.19 15:46, Eric Blake wrote:
> On 1/16/19 5:35 AM, David Hildenbrand wrote:
>> 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>
>> ---
> 
>> +++ b/qapi/misc.json
>> @@ -2949,6 +2949,29 @@
>>            }
>>  }
>>  
>> +##
>> +# @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: 3.1
> 
> Now 4.0
> 
>> +##
>> +{ 'struct': 'VirtioPMEMDeviceInfo',
>> +  'data': { '*id': 'str',
>> +            'memaddr': 'size',
>> +            'size': 'size',
>> +            'memdev': 'str'
>> +          }
>> +}
>> +
>>  ##
>>  # @MemoryDeviceInfo:
>>  #
>> @@ -2958,7 +2981,8 @@
>>  ##
>>  { 'union': 'MemoryDeviceInfo',
> 
> Does this union need a documentation update that virtio-pmem was added
> in 4.0?

Seems like:

##
# @MemoryDeviceInfo:
#
# Union containing information about a memory device
#
# @dimm: Information about a pc-dimm device.
#
# @nvdimm: Information about a nvdimm device. (since 2.12)
#
# @virtio-pmem: Information about a virtio-pmem device. (since 4.0)
#
# Since: 2.1
##

Does not work.

In file included from /home/dhildenb/git/qemu/qapi/qapi-schema.json:97:
/home/dhildenb/git/qemu/qapi/misc.json:2975: The following documented
members are not in the declaration: dimm, nvdimm, virtio-pmem

Any idea how to document this correctly?

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC 04/10] virtio-pmem: Prototype

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> On 16.01.19 15:46, Eric Blake wrote:
> > On 1/16/19 5:35 AM, David Hildenbrand wrote:
> >> 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>
> >> ---
> > 
> >> +++ b/qapi/misc.json
> >> @@ -2949,6 +2949,29 @@
> >>            }
> >>  }
> >>  
> >> +##
> >> +# @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: 3.1
> > 
> > Now 4.0
> > 
> >> +##
> >> +{ 'struct': 'VirtioPMEMDeviceInfo',
> >> +  'data': { '*id': 'str',
> >> +            'memaddr': 'size',
> >> +            'size': 'size',
> >> +            'memdev': 'str'
> >> +          }
> >> +}
> >> +
> >>  ##
> >>  # @MemoryDeviceInfo:
> >>  #
> >> @@ -2958,7 +2981,8 @@
> >>  ##
> >>  { 'union': 'MemoryDeviceInfo',
> > 
> > Does this union need a documentation update that virtio-pmem was added
> > in 4.0?
> 
> Seems like:
> 
> ##
> # @MemoryDeviceInfo:
> #
> # Union containing information about a memory device
> #
> # @dimm: Information about a pc-dimm device.
> #
> # @nvdimm: Information about a nvdimm device. (since 2.12)
> #
> # @virtio-pmem: Information about a virtio-pmem device. (since 4.0)
> #
> # Since: 2.1
> ##
> 
> Does not work.
> 
> In file included from /home/dhildenb/git/qemu/qapi/qapi-schema.json:97:
> /home/dhildenb/git/qemu/qapi/misc.json:2975: The following documented
> members are not in the declaration: dimm, nvdimm, virtio-pmem
> 
> Any idea how to document this correctly?

No I don't, but looking at other Union's they only ever seem to document
the base members, not the data members, for example see CpuInfo.

Dave

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

Re: [Qemu-devel] [PATCH RFC 04/10] virtio-pmem: Prototype

[David Hildenbrand]

On 21.01.19 13:02, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> On 16.01.19 15:46, Eric Blake wrote:
>>> On 1/16/19 5:35 AM, David Hildenbrand wrote:
>>>> 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>
>>>> ---
>>>
>>>> +++ b/qapi/misc.json
>>>> @@ -2949,6 +2949,29 @@
>>>>            }
>>>>  }
>>>>  
>>>> +##
>>>> +# @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: 3.1
>>>
>>> Now 4.0
>>>
>>>> +##
>>>> +{ 'struct': 'VirtioPMEMDeviceInfo',
>>>> +  'data': { '*id': 'str',
>>>> +            'memaddr': 'size',
>>>> +            'size': 'size',
>>>> +            'memdev': 'str'
>>>> +          }
>>>> +}
>>>> +
>>>>  ##
>>>>  # @MemoryDeviceInfo:
>>>>  #
>>>> @@ -2958,7 +2981,8 @@
>>>>  ##
>>>>  { 'union': 'MemoryDeviceInfo',
>>>
>>> Does this union need a documentation update that virtio-pmem was added
>>> in 4.0?
>>
>> Seems like:
>>
>> ##
>> # @MemoryDeviceInfo:
>> #
>> # Union containing information about a memory device
>> #
>> # @dimm: Information about a pc-dimm device.
>> #
>> # @nvdimm: Information about a nvdimm device. (since 2.12)
>> #
>> # @virtio-pmem: Information about a virtio-pmem device. (since 4.0)
>> #
>> # Since: 2.1
>> ##
>>
>> Does not work.
>>
>> In file included from /home/dhildenb/git/qemu/qapi/qapi-schema.json:97:
>> /home/dhildenb/git/qemu/qapi/misc.json:2975: The following documented
>> members are not in the declaration: dimm, nvdimm, virtio-pmem
>>
>> Any idea how to document this correctly?
> 
> No I don't, but looking at other Union's they only ever seem to document
> the base members, not the data members, for example see CpuInfo.
> 

Yes, that's also what I noticed. Guess I'll simply change that to

"nvdimm is included since 2.12. virtio-pmem is included since 4.0."

Unless Eric has another idea.

Thanks!

> Dave
> 
>> -- 
>>
>> Thanks,
>>
>> David / dhildenb
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC 04/10] virtio-pmem: Prototype

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

On 1/21/19 7:31 AM, David Hildenbrand wrote:

>>>>>  ##
>>>>>  # @MemoryDeviceInfo:
>>>>>  #
>>>>> @@ -2958,7 +2981,8 @@
>>>>>  ##
>>>>>  { 'union': 'MemoryDeviceInfo',
>>>>
>>>> Does this union need a documentation update that virtio-pmem was added
>>>> in 4.0?
>>>
>>> Seems like:
>>>
>>> ##
>>> # @MemoryDeviceInfo:
>>> #
>>> # Union containing information about a memory device
>>> #
>>> # @dimm: Information about a pc-dimm device.
>>> #
>>> # @nvdimm: Information about a nvdimm device. (since 2.12)
>>> #
>>> # @virtio-pmem: Information about a virtio-pmem device. (since 4.0)
>>> #
>>> # Since: 2.1
>>> ##
>>>
>>> Does not work.
>>>
>>> In file included from /home/dhildenb/git/qemu/qapi/qapi-schema.json:97:
>>> /home/dhildenb/git/qemu/qapi/misc.json:2975: The following documented
>>> members are not in the declaration: dimm, nvdimm, virtio-pmem
>>>
>>> Any idea how to document this correctly?
>>
>> No I don't, but looking at other Union's they only ever seem to document
>> the base members, not the data members, for example see CpuInfo.
>>
> 
> Yes, that's also what I noticed. Guess I'll simply change that to
> 
> "nvdimm is included since 2.12. virtio-pmem is included since 4.0."

Seems okay to me if we can't come up with something better.

> 
> Unless Eric has another idea.

Or maybe Markus, since he's more familiar with the doc generator.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH RFC 04/10] virtio-pmem: Prototype

[Murilo Opsfelder Araujo]

Hi, David.

On Wed, Jan 16, 2019 at 12:35:17PM +0100, David Hildenbrand wrote:
> 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                     | 189 ++++++++++++++++++++
>  include/hw/virtio/virtio-pmem.h             |  54 ++++++
>  include/standard-headers/linux/virtio_ids.h |   1 +
>  qapi/misc.json                              |  26 ++-
>  5 files changed, 271 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 1b2799cfd8..5463c682f7 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
>  endif
> diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
> new file mode 100644
> index 0000000000..6fb78acf87
> --- /dev/null
> +++ b/hw/virtio/virtio-pmem.c
> @@ -0,0 +1,189 @@
> +/*
> + * Virtio PMEM device
> + *
> + * Copyright (C) 2018 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;
> +    } else 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;
> +    }

Perhaps splitting this if-else block could improve readability:

    if (!pmem->memdev) {
        error_setg(...);
	return;
    }

    if (host_memory_backend_is_mapped(pmem->memdev)) {
       /* do stuff */
       return;
    }

    /* do other stuffs */

> +
> +    host_memory_backend_set_mapped(pmem->memdev, true);
> +    virtio_init(vdev, TYPE_VIRTIO_PMEM, VIRTIO_ID_PMEM,
> +                                          sizeof(struct virtio_pmem_config));

I'm not quite sure what's the QEMU style for indenting this.  There
are, for example, calls to warn_report() in other source files that
are indented at the left side after the opening parenthesis.

Perhaps indenting it like this is preferable?

    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..85cee3ef39
> --- /dev/null
> +++ b/include/hw/virtio/virtio-pmem.h
> @@ -0,0 +1,54 @@
> +/*
> + * Virtio pmem device

What if "pmem" was in upper case to match the header in virtio-pmem.c?

> + *
> + * Copyright Red Hat, Inc. 2018

This is slightly different from what is in virtio-pmem.c header.
Perhaps "(C)" is missing here.

> + *
> + * 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)

This indentation is slightly different from the other two macros
below.

> +#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..8ae709636d 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -2949,6 +2949,29 @@
>            }
>  }
>
> +##
> +# @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: 3.1
> +##
> +{ 'struct': 'VirtioPMEMDeviceInfo',
> +  'data': { '*id': 'str',
> +            'memaddr': 'size',
> +            'size': 'size',
> +            'memdev': 'str'
> +          }
> +}
> +
>  ##
>  # @MemoryDeviceInfo:
>  #
> @@ -2958,7 +2981,8 @@
>  ##
>  { 'union': 'MemoryDeviceInfo',
>    'data': { 'dimm': 'PCDIMMDeviceInfo',
> -            'nvdimm': 'PCDIMMDeviceInfo'
> +            'nvdimm': 'PCDIMMDeviceInfo',
> +            'virtio-pmem': 'VirtioPMEMDeviceInfo'
>            }
>  }
>
> --
> 2.17.2
>
>

--
Murilo

Re: [Qemu-devel] [Qemu-ppc] [PATCH RFC 04/10] virtio-pmem: Prototype

[David Hildenbrand]

>> +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;
>> +    } else 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;
>> +    }
> 
> Perhaps splitting this if-else block could improve readability:

Makes sense, this was kept similar to from
hw/mem/pc-dimm.c:pc_dimm_realize()

> 
>     if (!pmem->memdev) {
>         error_setg(...);
> 	return;
>     }
> 
>     if (host_memory_backend_is_mapped(pmem->memdev)) {
>        /* do stuff */
>        return;
>     }
> 
>     /* do other stuffs */
> 
>> +
>> +    host_memory_backend_set_mapped(pmem->memdev, true);
>> +    virtio_init(vdev, TYPE_VIRTIO_PMEM, VIRTIO_ID_PMEM,
>> +                                          sizeof(struct virtio_pmem_config));
> 
> I'm not quite sure what's the QEMU style for indenting this.  There
> are, for example, calls to warn_report() in other source files that
> are indented at the left side after the opening parenthesis.
> 
> Perhaps indenting it like this is preferable?
> 
>     virtio_init(vdev, TYPE_VIRTIO_PMEM, VIRTIO_ID_PMEM,
>                 sizeof(struct virtio_pmem_config));

Indeed, that looks weird.

> 
>> +    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..85cee3ef39
>> --- /dev/null
>> +++ b/include/hw/virtio/virtio-pmem.h
>> @@ -0,0 +1,54 @@
>> +/*
>> + * Virtio pmem device
> 
> What if "pmem" was in upper case to match the header in virtio-pmem.c?
> 
>> + *
>> + * Copyright Red Hat, Inc. 2018
> 
> This is slightly different from what is in virtio-pmem.c header.
> Perhaps "(C)" is missing here.

Thanks for the hint, that is indeed not consistent.

[...]
>> +#define TYPE_VIRTIO_PMEM "virtio-pmem"
>> +
>> +#define VIRTIO_PMEM(obj) \
>> +        OBJECT_CHECK(VirtIOPMEM, (obj), TYPE_VIRTIO_PMEM)
> 
> This indentation is slightly different from the other two macros
> below.

Will be made consistent.

Thanks!

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH RFC 05/10] 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 d05066deb8..62fe1278e0 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1995,6 +1995,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 29b4216107..9d8f41ea15 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -466,6 +466,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 RFC 06/10] 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-pci.h      |  14 ++++
 hw/virtio/virtio-pmem-pci.c | 129 ++++++++++++++++++++++++++++++++++++
 3 files changed, 144 insertions(+)
 create mode 100644 hw/virtio/virtio-pmem-pci.c

diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 5463c682f7..0f724725df 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-pci.h b/hw/virtio/virtio-pci.h
index 9d8f41ea15..3496c7ca09 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -19,6 +19,7 @@
 #include "hw/virtio/virtio-blk.h"
 #include "hw/virtio/virtio-net.h"
 #include "hw/virtio/virtio-rng.h"
+#include "hw/virtio/virtio-pmem.h"
 #include "hw/virtio/virtio-serial.h"
 #include "hw/virtio/virtio-scsi.h"
 #include "hw/virtio/virtio-balloon.h"
@@ -57,6 +58,7 @@ typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
 typedef struct VirtIOGPUPCI VirtIOGPUPCI;
 typedef struct VHostVSockPCI VHostVSockPCI;
 typedef struct VirtIOCryptoPCI VirtIOCryptoPCI;
+typedef struct VirtIOPMEMPCI VirtIOPMEMPCI;
 
 /* virtio-pci-bus */
 
@@ -274,6 +276,18 @@ struct VirtIOBlkPCI {
     VirtIOBlock vdev;
 };
 
+/*
+ * 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;
+};
+
 /*
  * virtio-balloon-pci: This extends VirtioPCIProxy.
  */
diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
new file mode 100644
index 0000000000..5687aebe7d
--- /dev/null
+++ b/hw/virtio/virtio-pmem-pci.c
@@ -0,0 +1,129 @@
+/*
+ * Virtio PMEM PCI device
+ *
+ * Copyright (C) 2018 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 "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-pci.h"
+#include "hw/pci/pci.h"
+#include "qapi/error.h"
+#include "hw/mem/memory-device.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 },
+        { }
+    },
+};
+
+static void virtio_pmem_pci_register_types(void)
+{
+    virtio_pci_types_register(&virtio_pmem_pci_info);
+}
+type_init(virtio_pmem_pci_register_types)
-- 
2.17.2

[Qemu-devel] [PATCH RFC 07/10] 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 RFC 08/10] 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 RFC 09/10] pc: Support for PCI based memory devices

[David Hildenbrand]

Override the PCI device hotplug handler to properly handle the
memory device part 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 buses don't
have a PCI hotplug handler at all yet (not sure if they are actually used
on x86, but just to be sure).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 89 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index fd0cb29ba9..62f83859fa 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/mem/memory-device.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -2224,6 +2225,84 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
     numa_cpu_pre_plug(cpu_slot, dev, errp);
 }
 
+static void pc_pci_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                            Error **errp)
+{
+    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
+    Error *local_err = NULL;
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        if (!hotplug_dev2) {
+            /*
+             * Without a bus hotplug handler, we cannot control the plug/unplug
+             * order. Disallow memory devices on such buses.
+             */
+            error_setg(errp, "Memory devices not supported on this bus.");
+            return;
+        }
+        memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev),
+                               NULL, errp);
+    }
+
+    if (hotplug_dev2) {
+        hotplug_handler_pre_plug(hotplug_dev2, dev, errp);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+}
+
+static void pc_pci_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                        Error **errp)
+{
+    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
+    Error *local_err = NULL;
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
+    }
+
+    if (hotplug_dev2) {
+        hotplug_handler_plug(hotplug_dev2, dev, &local_err);
+    }
+
+    if (local_err) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+            memory_device_unplug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
+        }
+    }
+    error_propagate(errp, local_err);
+}
+
+static void pc_pci_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                  Error **errp)
+{
+    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
+
+    if (hotplug_dev2) {
+        hotplug_handler_unplug_request(hotplug_dev2, dev, errp);
+    }
+}
+
+static void pc_pci_device_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                 Error **errp)
+{
+    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
+    Error *local_err = NULL;
+
+    if (hotplug_dev2) {
+        hotplug_handler_unplug(hotplug_dev2, dev, &local_err);
+    }
+
+    if (!local_err) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+            memory_device_unplug(MEMORY_DEVICE(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 +2310,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_PCI_DEVICE)) {
+        pc_pci_pre_plug(hotplug_dev, dev, errp);
     }
 }
 
@@ -2241,6 +2322,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_PCI_DEVICE)) {
+        pc_pci_plug(hotplug_dev, dev, errp);
     }
 }
 
@@ -2251,6 +2334,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_PCI_DEVICE)) {
+        pc_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 +2349,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_PCI_DEVICE)) {
+        pc_pci_device_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 +2361,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_PCI_DEVICE)) {
         return HOTPLUG_HANDLER(machine);
     }
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH RFC 09/10] pc: Support for PCI based memory devices

[Igor Mammedov]

On Wed, 16 Jan 2019 12:35:22 +0100
David Hildenbrand <david@redhat.com> wrote:

> Override the PCI device hotplug handler to properly handle the
> memory device part 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 buses don't
                                                             ^^^^^^^^ pls, be more specific

> have a PCI hotplug handler at all yet (not sure if they are actually used
> on x86, but just to be sure).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/i386/pc.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index fd0cb29ba9..62f83859fa 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/mem/memory-device.h"
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -2224,6 +2225,84 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>      numa_cpu_pre_plug(cpu_slot, dev, errp);
>  }
>  
> +static void pc_pci_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                            Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> +        if (!hotplug_dev2) {
> +            /*
> +             * Without a bus hotplug handler, we cannot control the plug/unplug
> +             * order. Disallow memory devices on such buses.
> +             */
> +            error_setg(errp, "Memory devices not supported on this bus.");
> +            return;
> +        }
> +        memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev),
> +                               NULL, errp);
> +    }
> +
> +    if (hotplug_dev2) {
> +        hotplug_handler_pre_plug(hotplug_dev2, dev, errp);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +}
> +
> +static void pc_pci_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                        Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> +        memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
> +    }
> +
> +    if (hotplug_dev2) {
> +        hotplug_handler_plug(hotplug_dev2, dev, &local_err);
> +    }
> +
> +    if (local_err) {
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> +            memory_device_unplug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
> +        }
> +    }
> +    error_propagate(errp, local_err);
> +}
> +
> +static void pc_pci_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                                  Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +
> +    if (hotplug_dev2) {
> +        hotplug_handler_unplug_request(hotplug_dev2, dev, errp);
> +    }
> +}
> +
> +static void pc_pci_device_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                                 Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    if (hotplug_dev2) {
> +        hotplug_handler_unplug(hotplug_dev2, dev, &local_err);
> +    }
> +
> +    if (!local_err) {
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> +            memory_device_unplug(MEMORY_DEVICE(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 +2310,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_PCI_DEVICE)) {
> +        pc_pci_pre_plug(hotplug_dev, dev, errp);
>      }
>  }
>  
> @@ -2241,6 +2322,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_PCI_DEVICE)) {
> +        pc_pci_plug(hotplug_dev, dev, errp);
>      }
>  }
>  
> @@ -2251,6 +2334,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_PCI_DEVICE)) {
> +        pc_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 +2349,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_PCI_DEVICE)) {
> +        pc_pci_device_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 +2361,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_PCI_DEVICE)) {
why all PCI devices instead of virtio-pmem-pci one?

>          return HOTPLUG_HANDLER(machine);
>      }
>  

Re: [Qemu-devel] [PATCH RFC 09/10] pc: Support for PCI based memory devices

[David Hildenbrand]

On 18.01.19 11:20, Igor Mammedov wrote:
> On Wed, 16 Jan 2019 12:35:22 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Override the PCI device hotplug handler to properly handle the
>> memory device part 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 buses don't
>                                                              ^^^^^^^^ pls, be more specific

Last time I checked there were some TYPE_PCI_HOST_BRIDGE devices in QEMU
that would not have a hotplug handler. I *think* they are all unrelated
to x86/pc. But as I was not sure if I am missing something, I introduced
a simple error check.

> 
>> have a PCI hotplug handler at all yet (not sure if they are actually used
>> on x86, but just to be sure).
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/i386/pc.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 89 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index fd0cb29ba9..62f83859fa 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/mem/memory-device.h"
>>  
>>  /* debug PC/ISA interrupts */
>>  //#define DEBUG_IRQ
>> @@ -2224,6 +2225,84 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>      numa_cpu_pre_plug(cpu_slot, dev, errp);
>>  }
>>  
>> +static void pc_pci_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> +                            Error **errp)
>> +{
>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>> +    Error *local_err = NULL;
>> +
>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> +        if (!hotplug_dev2) {
>> +            /*
>> +             * Without a bus hotplug handler, we cannot control the plug/unplug
>> +             * order. Disallow memory devices on such buses.
>> +             */
>> +            error_setg(errp, "Memory devices not supported on this bus.");
>> +            return;
>> +        }
>> +        memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev),
>> +                               NULL, errp);
>> +    }
>> +
>> +    if (hotplug_dev2) {
>> +        hotplug_handler_pre_plug(hotplug_dev2, dev, errp);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>> +    }
>> +}
>> +
>> +static void pc_pci_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> +                        Error **errp)
>> +{
>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>> +    Error *local_err = NULL;
>> +
>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> +        memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
>> +    }
>> +
>> +    if (hotplug_dev2) {
>> +        hotplug_handler_plug(hotplug_dev2, dev, &local_err);
>> +    }
>> +
>> +    if (local_err) {
>> +        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> +            memory_device_unplug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
>> +        }
>> +    }
>> +    error_propagate(errp, local_err);
>> +}
>> +
>> +static void pc_pci_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
>> +                                  Error **errp)
>> +{
>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>> +
>> +    if (hotplug_dev2) {
>> +        hotplug_handler_unplug_request(hotplug_dev2, dev, errp);
>> +    }
>> +}
>> +
>> +static void pc_pci_device_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> +                                 Error **errp)
>> +{
>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>> +    Error *local_err = NULL;
>> +
>> +    if (hotplug_dev2) {
>> +        hotplug_handler_unplug(hotplug_dev2, dev, &local_err);
>> +    }
>> +
>> +    if (!local_err) {
>> +        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> +            memory_device_unplug(MEMORY_DEVICE(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 +2310,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_PCI_DEVICE)) {
>> +        pc_pci_pre_plug(hotplug_dev, dev, errp);
>>      }
>>  }
>>  
>> @@ -2241,6 +2322,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_PCI_DEVICE)) {
>> +        pc_pci_plug(hotplug_dev, dev, errp);
>>      }
>>  }
>>  
>> @@ -2251,6 +2334,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_PCI_DEVICE)) {
>> +        pc_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 +2349,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_PCI_DEVICE)) {
>> +        pc_pci_device_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 +2361,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_PCI_DEVICE)) {
> why all PCI devices instead of virtio-pmem-pci one?
> 

Last time I tried to include "hw/virtio/virtio-pci.h" in pc.c it
resulted in an inclusion error madness. But this should be easy to fix I
guess. (the basic idea of this patch is the important bit)

And I just tried to include and it seems to result in no errors right now.

>>          return HOTPLUG_HANDLER(machine);
>>      }
>>  
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC 09/10] pc: Support for PCI based memory devices

[Igor Mammedov]

On Fri, 18 Jan 2019 13:53:14 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 18.01.19 11:20, Igor Mammedov wrote:
> > On Wed, 16 Jan 2019 12:35:22 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Override the PCI device hotplug handler to properly handle the
> >> memory device part 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 buses don't  
> >                                                              ^^^^^^^^ pls, be more specific  
> 
> Last time I checked there were some TYPE_PCI_HOST_BRIDGE devices in QEMU
> that would not have a hotplug handler. I *think* they are all unrelated
> to x86/pc. But as I was not sure if I am missing something, I introduced
> a simple error check.
> 
> >   
> >> have a PCI hotplug handler at all yet (not sure if they are actually used
> >> on x86, but just to be sure).
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/i386/pc.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 89 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index fd0cb29ba9..62f83859fa 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/mem/memory-device.h"
> >>  
> >>  /* debug PC/ISA interrupts */
> >>  //#define DEBUG_IRQ
> >> @@ -2224,6 +2225,84 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >>      numa_cpu_pre_plug(cpu_slot, dev, errp);
> >>  }
> >>  
> >> +static void pc_pci_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> +                            Error **errp)
> >> +{
> >> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> >> +    Error *local_err = NULL;
> >> +
> >> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> >> +        if (!hotplug_dev2) {
> >> +            /*
> >> +             * Without a bus hotplug handler, we cannot control the plug/unplug
> >> +             * order. Disallow memory devices on such buses.
> >> +             */
> >> +            error_setg(errp, "Memory devices not supported on this bus.");
> >> +            return;
> >> +        }
> >> +        memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev),
> >> +                               NULL, errp);
> >> +    }
> >> +
> >> +    if (hotplug_dev2) {
> >> +        hotplug_handler_pre_plug(hotplug_dev2, dev, errp);
> >> +        if (local_err) {
> >> +            error_propagate(errp, local_err);
> >> +            return;
> >> +        }
> >> +    }
> >> +}
> >> +
> >> +static void pc_pci_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> +                        Error **errp)
> >> +{
> >> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> >> +    Error *local_err = NULL;
> >> +
> >> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> >> +        memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
> >> +    }
> >> +
> >> +    if (hotplug_dev2) {
> >> +        hotplug_handler_plug(hotplug_dev2, dev, &local_err);
> >> +    }
> >> +
> >> +    if (local_err) {
> >> +        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> >> +            memory_device_unplug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
> >> +        }
> >> +    }
> >> +    error_propagate(errp, local_err);
> >> +}
> >> +
> >> +static void pc_pci_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> +                                  Error **errp)
> >> +{
> >> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> >> +
> >> +    if (hotplug_dev2) {
> >> +        hotplug_handler_unplug_request(hotplug_dev2, dev, errp);
> >> +    }
> >> +}
> >> +
> >> +static void pc_pci_device_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> +                                 Error **errp)
> >> +{
> >> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> >> +    Error *local_err = NULL;
> >> +
> >> +    if (hotplug_dev2) {
> >> +        hotplug_handler_unplug(hotplug_dev2, dev, &local_err);
> >> +    }
> >> +
> >> +    if (!local_err) {
> >> +        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> >> +            memory_device_unplug(MEMORY_DEVICE(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 +2310,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_PCI_DEVICE)) {
> >> +        pc_pci_pre_plug(hotplug_dev, dev, errp);
> >>      }
> >>  }
> >>  
> >> @@ -2241,6 +2322,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_PCI_DEVICE)) {
> >> +        pc_pci_plug(hotplug_dev, dev, errp);
> >>      }
> >>  }
> >>  
> >> @@ -2251,6 +2334,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_PCI_DEVICE)) {
> >> +        pc_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 +2349,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_PCI_DEVICE)) {
> >> +        pc_pci_device_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 +2361,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_PCI_DEVICE)) {  
> > why all PCI devices instead of virtio-pmem-pci one?
> >   
> 
> Last time I tried to include "hw/virtio/virtio-pci.h" in pc.c it
> resulted in an inclusion error madness. But this should be easy to fix I
> guess. (the basic idea of this patch is the important bit)
> 
> And I just tried to include and it seems to result in no errors right now
I'd prefer override for concrete devices only and the rest PCI devices
handled as they used to. That also should simplify hotplug handlers you
are adding here.

> 
> >>          return HOTPLUG_HANDLER(machine);
> >>      }
> >>    
> >   
> 
> 

Re: [Qemu-devel] [PATCH RFC 09/10] pc: Support for PCI based memory devices

[David Hildenbrand]

On 18.01.19 15:37, Igor Mammedov wrote:
> On Fri, 18 Jan 2019 13:53:14 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 18.01.19 11:20, Igor Mammedov wrote:
>>> On Wed, 16 Jan 2019 12:35:22 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> Override the PCI device hotplug handler to properly handle the
>>>> memory device part 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 buses don't  
>>>                                                              ^^^^^^^^ pls, be more specific  
>>
>> Last time I checked there were some TYPE_PCI_HOST_BRIDGE devices in QEMU
>> that would not have a hotplug handler. I *think* they are all unrelated
>> to x86/pc. But as I was not sure if I am missing something, I introduced
>> a simple error check.
>>
>>>   
>>>> have a PCI hotplug handler at all yet (not sure if they are actually used
>>>> on x86, but just to be sure).
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  hw/i386/pc.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 89 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index fd0cb29ba9..62f83859fa 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/mem/memory-device.h"
>>>>  
>>>>  /* debug PC/ISA interrupts */
>>>>  //#define DEBUG_IRQ
>>>> @@ -2224,6 +2225,84 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>>>      numa_cpu_pre_plug(cpu_slot, dev, errp);
>>>>  }
>>>>  
>>>> +static void pc_pci_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>> +                            Error **errp)
>>>> +{
>>>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>>>> +        if (!hotplug_dev2) {
>>>> +            /*
>>>> +             * Without a bus hotplug handler, we cannot control the plug/unplug
>>>> +             * order. Disallow memory devices on such buses.
>>>> +             */
>>>> +            error_setg(errp, "Memory devices not supported on this bus.");
>>>> +            return;
>>>> +        }
>>>> +        memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev),
>>>> +                               NULL, errp);
>>>> +    }
>>>> +
>>>> +    if (hotplug_dev2) {
>>>> +        hotplug_handler_pre_plug(hotplug_dev2, dev, errp);
>>>> +        if (local_err) {
>>>> +            error_propagate(errp, local_err);
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +static void pc_pci_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>> +                        Error **errp)
>>>> +{
>>>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>>>> +        memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
>>>> +    }
>>>> +
>>>> +    if (hotplug_dev2) {
>>>> +        hotplug_handler_plug(hotplug_dev2, dev, &local_err);
>>>> +    }
>>>> +
>>>> +    if (local_err) {
>>>> +        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>>>> +            memory_device_unplug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
>>>> +        }
>>>> +    }
>>>> +    error_propagate(errp, local_err);
>>>> +}
>>>> +
>>>> +static void pc_pci_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>> +                                  Error **errp)
>>>> +{
>>>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>>>> +
>>>> +    if (hotplug_dev2) {
>>>> +        hotplug_handler_unplug_request(hotplug_dev2, dev, errp);
>>>> +    }
>>>> +}
>>>> +
>>>> +static void pc_pci_device_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>> +                                 Error **errp)
>>>> +{
>>>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    if (hotplug_dev2) {
>>>> +        hotplug_handler_unplug(hotplug_dev2, dev, &local_err);
>>>> +    }
>>>> +
>>>> +    if (!local_err) {
>>>> +        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>>>> +            memory_device_unplug(MEMORY_DEVICE(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 +2310,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_PCI_DEVICE)) {
>>>> +        pc_pci_pre_plug(hotplug_dev, dev, errp);
>>>>      }
>>>>  }
>>>>  
>>>> @@ -2241,6 +2322,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_PCI_DEVICE)) {
>>>> +        pc_pci_plug(hotplug_dev, dev, errp);
>>>>      }
>>>>  }
>>>>  
>>>> @@ -2251,6 +2334,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_PCI_DEVICE)) {
>>>> +        pc_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 +2349,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_PCI_DEVICE)) {
>>>> +        pc_pci_device_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 +2361,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_PCI_DEVICE)) {  
>>> why all PCI devices instead of virtio-pmem-pci one?
>>>   
>>
>> Last time I tried to include "hw/virtio/virtio-pci.h" in pc.c it
>> resulted in an inclusion error madness. But this should be easy to fix I
>> guess. (the basic idea of this patch is the important bit)
>>
>> And I just tried to include and it seems to result in no errors right now
> I'd prefer override for concrete devices only and the rest PCI devices
> handled as they used to. That also should simplify hotplug handlers you
> are adding here.

Yes, will do for the next round as long as gates to inclusion hell don't
open up again :)

Thanks!


-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH RFC 10/10] pc: Enable support for virtio-pmem

[David Hildenbrand]

As we can now properly hotplug PCI based memory devices on x86, we can
now unlock support for virtio-pmem (which will unlock virtio-pmem-pci).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 default-configs/i386-softmmu.mak | 1 +
 1 file changed, 1 insertion(+)

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

Re: [Qemu-devel] [PATCH RFC 00/10] qdev: Hotplug handler chaining + virtio-pmem

[David Hildenbrand]

On 16.01.19 12:35, David Hildenbrand 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
> - [PULL v2 01/49] pci/pcie: stop plug/unplug if the slot is locked
> -- Soon upstream
> - [PATCH v1] pc: Use hotplug_handler_(plug|unplug|unplug_request)
> -- Queued by Paolo
> - [PATCH v2 0/6] s390x/pci: hotplug handler fixes and reworks
> -- Partially queued
> 
> Patch 1-3 are the preparations for hotplug handler chaining. The remaining
> patches are a modified prototype of virtio-pmem.
> 
> 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
> 

The current state can be found on:
https://github.com/davidhildenbrand/qemu.git : memory_device

-- 

Thanks,

David / dhildenb