[Qemu-devel] [PULL 00/21] pc, acpi, pci, virtio: fixes, cleanups, features, tests

[Qemu-devel] [PULL 00/21] pc, acpi, pci, virtio: fixes, cleanups, features, tests

[Michael S. Tsirkin]

The following changes since commit fd479c60f5766f7fb247ad146b9e3c33d03d2055:

  Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20170603' into staging (2017-07-03 09:54:32 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to d2f9ca94165b10c51d6d6cae5fe1cadf1ca42076:

  i386/acpi: update expected acpi files (2017-07-03 22:42:36 +0300)

----------------------------------------------------------------
pc, acpi, pci, virtio: fixes, cleanups, features, tests

Some fixes and cleanups. New tests.
Configurable tx queue size for virtio-net.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Aleksandr Bezzubikov (1):
      hw/acpi: remove dead acpi code

Ben Warren (1):
      tests: Add unit tests for the VM Generation ID feature

Ladi Prosek (1):
      intel_iommu: relax iq tail check on VTD_GCMD_QIE enable

Mao Zhongyi (9):
      pci: Clean up error checking in pci_add_capability()
      pci: Add comment for pci_add_capability2()
      pci: Fix the wrong assertion.
      pci: Make errp the last parameter of pci_add_capability()
      pci: Replace pci_add_capability2() with pci_add_capability()
      pci: Convert to realize
      pci: Convert shpc_init() to Error
      i386/kvm/pci-assign: Fix return type of verify_irqchip_kernel()
      i386/kvm/pci-assign: Use errp directly rather than local_err

Mark Cave-Ayland (2):
      fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize()
      fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1()

Maxime Coquelin (2):
      vhost: ensure vhost_ops are set before calling iotlb callback
      vhost-user: unregister slave req handler at cleanup time

Michael S. Tsirkin (2):
      virtio-net: fix tx queue size for !vhost-user
      i386/acpi: update expected acpi files

Peter Xu (1):
      intel_iommu: fix migration breakage on mr switch

Thomas Huth (1):
      hw/pci-bridge/dec: Classify the DEC PCI bridge as bridge device

Wei Wang (1):
      virtio-net: enable configurable tx queue size

 include/hw/pci/pci.h                 |   2 -
 include/hw/pci/pci_bridge.h          |   3 +-
 include/hw/pci/pcie.h                |   3 +-
 include/hw/pci/shpc.h                |   3 +-
 include/hw/pci/slotid_cap.h          |   3 +-
 include/hw/virtio/virtio-net.h       |   1 +
 hw/i386/acpi-build.c                 |  10 --
 hw/i386/amd_iommu.c                  |  24 +++--
 hw/i386/intel_iommu.c                |  48 ++++++---
 hw/i386/kvm/pci-assign.c             |  54 ++++------
 hw/ide/ich.c                         |   2 +-
 hw/net/e1000e.c                      |  30 +++---
 hw/net/eepro100.c                    |  18 +++-
 hw/net/virtio-net.c                  |  44 +++++++-
 hw/nvram/fw_cfg.c                    |  32 +++---
 hw/pci-bridge/dec.c                  |   2 +
 hw/pci-bridge/i82801b11.c            |  12 +--
 hw/pci-bridge/pci_bridge_dev.c       |  14 ++-
 hw/pci-bridge/pcie_root_port.c       |  18 ++--
 hw/pci-bridge/xio3130_downstream.c   |  20 ++--
 hw/pci-bridge/xio3130_upstream.c     |  20 ++--
 hw/pci/msi.c                         |   2 +-
 hw/pci/msix.c                        |   2 +-
 hw/pci/pci.c                         |  24 +----
 hw/pci/pci_bridge.c                  |   8 +-
 hw/pci/pcie.c                        |  28 +++--
 hw/pci/shpc.c                        |  10 +-
 hw/pci/slotid_cap.c                  |  12 ++-
 hw/usb/hcd-xhci.c                    |   2 +-
 hw/vfio/pci.c                        |  15 +--
 hw/virtio/vhost-backend.c            |  10 +-
 hw/virtio/vhost-user.c               |   1 +
 hw/virtio/virtio-pci.c               |  12 ++-
 tests/vmgenid-test.c                 | 203 +++++++++++++++++++++++++++++++++++
 hw/i386/trace-events                 |   2 +-
 tests/Makefile.include               |   2 +
 tests/acpi-test-data/q35/DSDT        | Bin 7824 -> 7782 bytes
 tests/acpi-test-data/q35/DSDT.bridge | Bin 7841 -> 7799 bytes
 tests/acpi-test-data/q35/DSDT.cphp   | Bin 8287 -> 8245 bytes
 tests/acpi-test-data/q35/DSDT.ipmibt | Bin 7899 -> 7857 bytes
 tests/acpi-test-data/q35/DSDT.memhp  | Bin 9189 -> 9147 bytes
 41 files changed, 492 insertions(+), 204 deletions(-)
 create mode 100644 tests/vmgenid-test.c

[Qemu-devel] [PULL 01/21] virtio-net: enable configurable tx queue size

[Michael S. Tsirkin]

From: Wei Wang <wei.w.wang@intel.com>

This patch enables the virtio-net tx queue size to be configurable
between 256 (the default queue size) and 1024 by the user when the
vhost-user backend is used.

Currently, the maximum tx queue size for other backends is 512 due
to the following limitations:
- QEMU backend: the QEMU backend implementation in some cases may
send 1024+1 iovs to writev.
- Vhost_net backend: there are possibilities that the guest sends
a vring_desc of memory which crosses a MemoryRegion thereby
generating more than 1024 iovs after translation from guest-physical
address in the backend.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/virtio/virtio-net.h |  1 +
 hw/net/virtio-net.c            | 32 ++++++++++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 602b486..b81b6a4 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -36,6 +36,7 @@ typedef struct virtio_net_conf
     int32_t txburst;
     char *tx;
     uint16_t rx_queue_size;
+    uint16_t tx_queue_size;
     uint16_t mtu;
 } virtio_net_conf;
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 91eddaf..a1fc0db 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -34,8 +34,11 @@
 
 /* previously fixed value */
 #define VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE 256
+#define VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE 256
+
 /* for now, only allow larger queues; with virtio-1, guest can downsize */
 #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
+#define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
 
 /*
  * Calculate the number of bytes up to and including the given 'field' of
@@ -1508,15 +1511,18 @@ static void virtio_net_add_queue(VirtIONet *n, int index)
 
     n->vqs[index].rx_vq = virtio_add_queue(vdev, n->net_conf.rx_queue_size,
                                            virtio_net_handle_rx);
+
     if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer);
+            virtio_add_queue(vdev, n->net_conf.tx_queue_size,
+                             virtio_net_handle_tx_timer);
         n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                               virtio_net_tx_timer,
                                               &n->vqs[index]);
     } else {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh);
+            virtio_add_queue(vdev, n->net_conf.tx_queue_size,
+                             virtio_net_handle_tx_bh);
         n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
     }
 
@@ -1927,6 +1933,17 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (n->net_conf.tx_queue_size < VIRTIO_NET_TX_QUEUE_MIN_SIZE ||
+        n->net_conf.tx_queue_size > VIRTQUEUE_MAX_SIZE ||
+        !is_power_of_2(n->net_conf.tx_queue_size)) {
+        error_setg(errp, "Invalid tx_queue_size (= %" PRIu16 "), "
+                   "must be a power of 2 between %d and %d",
+                   n->net_conf.tx_queue_size, VIRTIO_NET_TX_QUEUE_MIN_SIZE,
+                   VIRTQUEUE_MAX_SIZE);
+        virtio_cleanup(vdev);
+        return;
+    }
+
     n->max_queues = MAX(n->nic_conf.peers.queues, 1);
     if (n->max_queues * 2 + 1 > VIRTIO_QUEUE_MAX) {
         error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
@@ -1947,6 +1964,15 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
         error_report("Defaulting to \"bh\"");
     }
 
+    /*
+     * Currently, backends other than vhost-user don't support 1024 queue
+     * size.
+     */
+    if (n->net_conf.tx_queue_size == VIRTQUEUE_MAX_SIZE &&
+        n->nic_conf.peers.ncs[0]->info->type != NET_CLIENT_DRIVER_VHOST_USER) {
+        n->net_conf.tx_queue_size = VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE;
+    }
+
     for (i = 0; i < n->max_queues; i++) {
         virtio_net_add_queue(n, i);
     }
@@ -2106,6 +2132,8 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
     DEFINE_PROP_UINT16("rx_queue_size", VirtIONet, net_conf.rx_queue_size,
                        VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE),
+    DEFINE_PROP_UINT16("tx_queue_size", VirtIONet, net_conf.tx_queue_size,
+                       VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE),
     DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
     DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
                      true),
-- 
MST

[Qemu-devel] [PULL 02/21] hw/pci-bridge/dec: Classify the DEC PCI bridge as bridge device

[Michael S. Tsirkin]

From: Thomas Huth <thuth@redhat.com>

This way the bridge shows up in the correct section of the
"-device help" text.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci-bridge/dec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
index cca9362..eb275e1 100644
--- a/hw/pci-bridge/dec.c
+++ b/hw/pci-bridge/dec.c
@@ -62,6 +62,7 @@ static void dec_21154_pci_bridge_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     k->realize = dec_pci_bridge_realize;
     k->exit = pci_bridge_exitfn;
     k->vendor_id = PCI_VENDOR_ID_DEC;
@@ -118,6 +119,7 @@ static void dec_21154_pci_host_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     k->realize = dec_21154_pci_host_realize;
     k->vendor_id = PCI_VENDOR_ID_DEC;
     k->device_id = PCI_DEVICE_ID_DEC_21154;
-- 
MST

[Qemu-devel] [PULL 03/21] intel_iommu: relax iq tail check on VTD_GCMD_QIE enable

[Michael S. Tsirkin]

From: Ladi Prosek <lprosek@redhat.com>

The VT-d spec (section 6.5.2) prescribes software to zero the
Invalidation Queue Tail Register before enabling the VTD_GCMD_QIE
Global Command Register bit. Windows Server 2012 R2 and possibly
other older Windows versions violate the protocol and set a
non-zero queue tail first, which in effect makes them crash early
on boot with -device intel-iommu,intremap=on.

This commit relaxes the check and instead of failing to enable
VTD_GCMD_QIE with vtd_err_qi_enable, it behaves as if the tail
register was set just after enabling VTD_GCMD_QIE
(see vtd_handle_iqt_write).

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/intel_iommu.c | 33 +++++++++++++++++++--------------
 hw/i386/trace-events  |  2 +-
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index a9b59bd..2ddf3bd 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1450,10 +1450,7 @@ static uint64_t vtd_iotlb_flush(IntelIOMMUState *s, uint64_t val)
     return iaig;
 }
 
-static inline bool vtd_queued_inv_enable_check(IntelIOMMUState *s)
-{
-    return s->iq_tail == 0;
-}
+static void vtd_fetch_inv_desc(IntelIOMMUState *s);
 
 static inline bool vtd_queued_inv_disable_check(IntelIOMMUState *s)
 {
@@ -1468,16 +1465,24 @@ static void vtd_handle_gcmd_qie(IntelIOMMUState *s, bool en)
     trace_vtd_inv_qi_enable(en);
 
     if (en) {
-        if (vtd_queued_inv_enable_check(s)) {
-            s->iq = iqa_val & VTD_IQA_IQA_MASK;
-            /* 2^(x+8) entries */
-            s->iq_size = 1UL << ((iqa_val & VTD_IQA_QS) + 8);
-            s->qi_enabled = true;
-            trace_vtd_inv_qi_setup(s->iq, s->iq_size);
-            /* Ok - report back to driver */
-            vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_QIES);
-        } else {
-            trace_vtd_err_qi_enable(s->iq_tail);
+        s->iq = iqa_val & VTD_IQA_IQA_MASK;
+        /* 2^(x+8) entries */
+        s->iq_size = 1UL << ((iqa_val & VTD_IQA_QS) + 8);
+        s->qi_enabled = true;
+        trace_vtd_inv_qi_setup(s->iq, s->iq_size);
+        /* Ok - report back to driver */
+        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_QIES);
+
+        if (s->iq_tail != 0) {
+            /*
+             * This is a spec violation but Windows guests are known to set up
+             * Queued Invalidation this way so we allow the write and process
+             * Invalidation Descriptors right away.
+             */
+            trace_vtd_warn_invalid_qi_tail(s->iq_tail);
+            if (!(vtd_get_long_raw(s, DMAR_FSTS_REG) & VTD_FSTS_IQE)) {
+                vtd_fetch_inv_desc(s);
+            }
         }
     } else {
         if (vtd_queued_inv_disable_check(s)) {
diff --git a/hw/i386/trace-events b/hw/i386/trace-events
index 5f111d6..42d8a7e 100644
--- a/hw/i386/trace-events
+++ b/hw/i386/trace-events
@@ -74,7 +74,7 @@ vtd_err_dmar_slpte_read_error(uint64_t iova, int level) "iova 0x%"PRIx64" level
 vtd_err_dmar_slpte_perm_error(uint64_t iova, int level, uint64_t slpte, bool is_write) "iova 0x%"PRIx64" level %d slpte 0x%"PRIx64" write %d"
 vtd_err_dmar_slpte_resv_error(uint64_t iova, int level, uint64_t slpte) "iova 0x%"PRIx64" level %d slpte 0x%"PRIx64
 vtd_err_dmar_translate(uint8_t bus, uint8_t slot, uint8_t func, uint64_t iova) "dev %02x:%02x.%02x iova 0x%"PRIx64
-vtd_err_qi_enable(uint16_t tail) "tail 0x%"PRIx16
+vtd_warn_invalid_qi_tail(uint16_t tail) "tail 0x%"PRIx16
 vtd_err_qi_disable(uint16_t head, uint16_t tail, int type) "head 0x%"PRIx16" tail 0x%"PRIx16" last_desc_type %d"
 vtd_err_qi_tail(uint16_t tail, uint16_t size) "tail 0x%"PRIx16" size 0x%"PRIx16
 vtd_err_irte(int index, uint64_t lo, uint64_t hi) "index %d low 0x%"PRIx64" high 0x%"PRIx64
-- 
MST

[Qemu-devel] [PULL 04/21] pci: Clean up error checking in pci_add_capability()

[Michael S. Tsirkin]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

On success, pci_add_capability2() returns a positive value. On
failure, it sets an error and return a negative value.

pci_add_capability() laboriously checks this behavior. No other
caller does. Drop the checks from pci_add_capability().

Cc: mst@redhat.com
Cc: marcel@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci/pci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b7fee4b..3c24888 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2270,12 +2270,8 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
     Error *local_err = NULL;
 
     ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
-    if (local_err) {
-        assert(ret < 0);
+    if (ret < 0) {
         error_report_err(local_err);
-    } else {
-        /* success implies a positive offset in config space */
-        assert(ret > 0);
     }
     return ret;
 }
-- 
MST

[Qemu-devel] [PULL 05/21] pci: Add comment for pci_add_capability2()

[Michael S. Tsirkin]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

Comments for pci_add_capability2() to explain the return
value. This may help to make a correct return value check
for its callers.

Cc: mst@redhat.com
Cc: marcel@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci/pci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 3c24888..3370b76 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2276,6 +2276,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
     return ret;
 }
 
+/*
+ * On success, pci_add_capability2() returns a positive value
+ * that the offset of the pci capability.
+ * On failure, it sets an error and returns a negative error
+ * code.
+ */
 int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
                        uint8_t offset, uint8_t size,
                        Error **errp)
-- 
MST

[Qemu-devel] [PULL 06/21] pci: Fix the wrong assertion.

[Michael S. Tsirkin]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

pci_add_capability returns a strictly positive value on success,
correct asserts.

Cc: dmitry@daynix.com
Cc: jasowang@redhat.com
Cc: kraxel@redhat.com
Cc: alex.williamson@redhat.com
Cc: armbru@redhat.com
Cc: marcel@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/e1000e.c   | 2 +-
 hw/net/eepro100.c | 2 +-
 hw/usb/hcd-xhci.c | 2 +-
 hw/vfio/pci.c     | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 0e0a1dc..08f3626 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -374,7 +374,7 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
 {
     int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
 
-    if (ret >= 0) {
+    if (ret > 0) {
         pci_set_word(pdev->config + offset + PCI_PM_PMC,
                      PCI_PM_CAP_VER_1_1 |
                      pmc);
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 4bf71f2..da36816 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -571,7 +571,7 @@ static void e100_pci_reset(EEPRO100State * s)
         int cfg_offset = 0xdc;
         int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
                                    cfg_offset, PCI_PM_SIZEOF);
-        assert(r >= 0);
+        assert(r > 0);
         pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
 #if 0 /* TODO: replace dummy code for power management emulation. */
         /* TODO: Power Management Control / Status. */
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 760135c..204ea69 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3419,7 +3419,7 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     if (pci_bus_is_express(dev->bus) ||
         xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
         ret = pcie_endpoint_cap_init(dev, 0xa0);
-        assert(ret >= 0);
+        assert(ret > 0);
     }
 
     if (xhci->msix != ON_OFF_AUTO_OFF) {
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 32aca77..5881968 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1744,7 +1744,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
     }
 
     pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
-    if (pos >= 0) {
+    if (pos > 0) {
         vdev->pdev.exp.exp_cap = pos;
     }
 
-- 
MST

[Qemu-devel] [PULL 07/21] pci: Make errp the last parameter of pci_add_capability()

[Michael S. Tsirkin]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

Add Error argument for pci_add_capability() to leverage the errp
to pass info on errors. This way is helpful for its callers to
make a better error handling when moving to 'realize'.

Cc: pbonzini@redhat.com
Cc: rth@twiddle.net
Cc: ehabkost@redhat.com
Cc: mst@redhat.com
Cc: jasowang@redhat.com
Cc: marcel@redhat.com
Cc: alex.williamson@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/pci/pci.h      |  3 ++-
 hw/i386/amd_iommu.c       | 24 +++++++++++++++++-------
 hw/net/e1000e.c           | 30 ++++++++++++++++++------------
 hw/net/eepro100.c         | 18 ++++++++++++++----
 hw/pci-bridge/i82801b11.c |  1 +
 hw/pci/pci.c              | 10 ++++------
 hw/pci/pci_bridge.c       |  7 ++++++-
 hw/pci/pcie.c             | 10 ++++++++--
 hw/pci/shpc.c             |  5 ++++-
 hw/pci/slotid_cap.c       |  7 ++++++-
 hw/vfio/pci.c             |  9 ++++++---
 hw/virtio/virtio-pci.c    | 12 ++++++++----
 12 files changed, 94 insertions(+), 42 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index a37a2d5..fe52aa8 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -356,7 +356,8 @@ void pci_unregister_vga(PCIDevice *pci_dev);
 pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
 
 int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
-                       uint8_t offset, uint8_t size);
+                       uint8_t offset, uint8_t size,
+                       Error **errp);
 int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
                        uint8_t offset, uint8_t size,
                        Error **errp);
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 7b6d4ea..d93ffc2 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1158,13 +1158,23 @@ static void amdvi_realize(DeviceState *dev, Error **err)
     x86_iommu->type = TYPE_AMD;
     qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
     object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
-    s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
-                                         AMDVI_CAPAB_SIZE);
-    assert(s->capab_offset > 0);
-    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
-    assert(ret > 0);
-    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
-    assert(ret > 0);
+    ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
+                                         AMDVI_CAPAB_SIZE, err);
+    if (ret < 0) {
+        return;
+    }
+    s->capab_offset = ret;
+
+    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
+                             AMDVI_CAPAB_REG_SIZE, err);
+    if (ret < 0) {
+        return;
+    }
+    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
+                             AMDVI_CAPAB_REG_SIZE, err);
+    if (ret < 0) {
+        return;
+    }
 
     /* set up MMIO */
     memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 08f3626..6c42b44 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -47,6 +47,7 @@
 #include "e1000e_core.h"
 
 #include "trace.h"
+#include "qapi/error.h"
 
 #define TYPE_E1000E "e1000e"
 #define E1000E(obj) OBJECT_CHECK(E1000EState, (obj), TYPE_E1000E)
@@ -372,21 +373,26 @@ e1000e_gen_dsn(uint8_t *mac)
 static int
 e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
 {
-    int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
+    Error *local_err = NULL;
+    int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
+                                 PCI_PM_SIZEOF, &local_err);
 
-    if (ret > 0) {
-        pci_set_word(pdev->config + offset + PCI_PM_PMC,
-                     PCI_PM_CAP_VER_1_1 |
-                     pmc);
+    if (local_err) {
+        error_report_err(local_err);
+        return ret;
+    }
 
-        pci_set_word(pdev->wmask + offset + PCI_PM_CTRL,
-                     PCI_PM_CTRL_STATE_MASK |
-                     PCI_PM_CTRL_PME_ENABLE |
-                     PCI_PM_CTRL_DATA_SEL_MASK);
+    pci_set_word(pdev->config + offset + PCI_PM_PMC,
+                 PCI_PM_CAP_VER_1_1 |
+                 pmc);
 
-        pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
-                     PCI_PM_CTRL_PME_STATUS);
-    }
+    pci_set_word(pdev->wmask + offset + PCI_PM_CTRL,
+                 PCI_PM_CTRL_STATE_MASK |
+                 PCI_PM_CTRL_PME_ENABLE |
+                 PCI_PM_CTRL_DATA_SEL_MASK);
+
+    pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
+                 PCI_PM_CTRL_PME_STATUS);
 
     return ret;
 }
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index da36816..5a4774a 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -48,6 +48,7 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/dma.h"
 #include "qemu/bitops.h"
+#include "qapi/error.h"
 
 /* QEMU sends frames smaller than 60 bytes to ethernet nics.
  * Such frames are rejected by real nics and their emulations.
@@ -494,7 +495,7 @@ static void eepro100_fcp_interrupt(EEPRO100State * s)
 }
 #endif
 
-static void e100_pci_reset(EEPRO100State * s)
+static void e100_pci_reset(EEPRO100State *s, Error **errp)
 {
     E100PCIDeviceInfo *info = eepro100_get_class(s);
     uint32_t device = s->device;
@@ -570,8 +571,12 @@ static void e100_pci_reset(EEPRO100State * s)
         /* Power Management Capabilities */
         int cfg_offset = 0xdc;
         int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
-                                   cfg_offset, PCI_PM_SIZEOF);
-        assert(r > 0);
+                                   cfg_offset, PCI_PM_SIZEOF,
+                                   errp);
+        if (r < 0) {
+            return;
+        }
+
         pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
 #if 0 /* TODO: replace dummy code for power management emulation. */
         /* TODO: Power Management Control / Status. */
@@ -1858,12 +1863,17 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
 {
     EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
     E100PCIDeviceInfo *info = eepro100_get_class(s);
+    Error *local_err = NULL;
 
     TRACE(OTHER, logout("\n"));
 
     s->device = info->device;
 
-    e100_pci_reset(s);
+    e100_pci_reset(s, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
     /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
      * i82559 and later support 64 or 256 word EEPROM. */
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index 2404e7e..2c065c3 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -44,6 +44,7 @@
 #include "qemu/osdep.h"
 #include "hw/pci/pci.h"
 #include "hw/i386/ich9.h"
+#include "qapi/error.h"
 
 
 /*****************************************************************************/
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 3370b76..3ad6f4e 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
  * in pci config space
  */
 int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
-                       uint8_t offset, uint8_t size)
+                       uint8_t offset, uint8_t size,
+                       Error **errp)
 {
     int ret;
-    Error *local_err = NULL;
 
-    ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
-    if (ret < 0) {
-        error_report_err(local_err);
-    }
+    ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
+
     return ret;
 }
 
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 5118ef4..bb0f3a3 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -33,6 +33,7 @@
 #include "hw/pci/pci_bridge.h"
 #include "hw/pci/pci_bus.h"
 #include "qemu/range.h"
+#include "qapi/error.h"
 
 /* PCI bridge subsystem vendor ID helper functions */
 #define PCI_SSVID_SIZEOF        8
@@ -43,8 +44,12 @@ int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
                           uint16_t svid, uint16_t ssid)
 {
     int pos;
-    pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset, PCI_SSVID_SIZEOF);
+    Error *local_err = NULL;
+
+    pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
+                             PCI_SSVID_SIZEOF, &local_err);
     if (pos < 0) {
+        error_report_err(local_err);
         return pos;
     }
 
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 18e634f..f187512 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -91,11 +91,14 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
     /* PCIe cap v2 init */
     int pos;
     uint8_t *exp_cap;
+    Error *local_err = NULL;
 
     assert(pci_is_express(dev));
 
-    pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER2_SIZEOF);
+    pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
+                             PCI_EXP_VER2_SIZEOF, &local_err);
     if (pos < 0) {
+        error_report_err(local_err);
         return pos;
     }
     dev->exp.exp_cap = pos;
@@ -123,11 +126,14 @@ int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type,
 {
     /* PCIe cap v1 init */
     int pos;
+    Error *local_err = NULL;
 
     assert(pci_is_express(dev));
 
-    pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER1_SIZEOF);
+    pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
+                             PCI_EXP_VER1_SIZEOF, &local_err);
     if (pos < 0) {
+        error_report_err(local_err);
         return pos;
     }
     dev->exp.exp_cap = pos;
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 42fafac..d72d5e4 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -450,9 +450,12 @@ static int shpc_cap_add_config(PCIDevice *d)
 {
     uint8_t *config;
     int config_offset;
+    Error *local_err = NULL;
     config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
-                                       0, SHPC_CAP_LENGTH);
+                                       0, SHPC_CAP_LENGTH,
+                                       &local_err);
     if (config_offset < 0) {
+        error_report_err(local_err);
         return config_offset;
     }
     config = d->config + config_offset;
diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
index aec1e91..bdca205 100644
--- a/hw/pci/slotid_cap.c
+++ b/hw/pci/slotid_cap.c
@@ -2,6 +2,7 @@
 #include "hw/pci/slotid_cap.h"
 #include "hw/pci/pci.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 
 #define SLOTID_CAP_LENGTH 4
 #define SLOTID_NSLOTS_SHIFT ctz32(PCI_SID_ESR_NSLOTS)
@@ -11,6 +12,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
                     unsigned offset)
 {
     int cap;
+    Error *local_err = NULL;
+
     if (!chassis) {
         error_report("Bridge chassis not specified. Each bridge is required "
                      "to be assigned a unique chassis id > 0.");
@@ -21,8 +24,10 @@ int slotid_cap_init(PCIDevice *d, int nslots,
         return -EINVAL;
     }
 
-    cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset, SLOTID_CAP_LENGTH);
+    cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
+                             SLOTID_CAP_LENGTH, &local_err);
     if (cap < 0) {
+        error_report_err(local_err);
         return cap;
     }
     /* We make each chassis unique, this way each bridge is First in Chassis */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 5881968..70bfb59 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1743,11 +1743,14 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
                                PCI_EXP_LNKCAP_MLW | PCI_EXP_LNKCAP_SLS);
     }
 
-    pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
-    if (pos > 0) {
-        vdev->pdev.exp.exp_cap = pos;
+    pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
+                             errp);
+    if (pos < 0) {
+        return pos;
     }
 
+    vdev->pdev.exp.exp_cap = pos;
+
     return pos;
 }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 301920e..93480a7 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1162,8 +1162,8 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
     PCIDevice *dev = &proxy->pci_dev;
     int offset;
 
-    offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len);
-    assert(offset > 0);
+    offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
+                                cap->cap_len, &error_abort);
 
     assert(cap->cap_len >= sizeof *cap);
     memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
@@ -1810,8 +1810,12 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
         pos = pcie_endpoint_cap_init(pci_dev, 0);
         assert(pos > 0);
 
-        pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF);
-        assert(pos > 0);
+        pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0,
+                                 PCI_PM_SIZEOF, errp);
+        if (pos < 0) {
+            return;
+        }
+
         pci_dev->exp.pm_cap = pos;
 
         /*
-- 
MST

[Qemu-devel] [PULL 08/21] pci: Replace pci_add_capability2() with pci_add_capability()

[Michael S. Tsirkin]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

After the patch 'Make errp the last parameter of pci_add_capability()',
pci_add_capability() and pci_add_capability2() now do exactly the same.
So drop the wrapper pci_add_capability() of pci_add_capability2(), then
replace the pci_add_capability2() with pci_add_capability() everywhere.

Cc: pbonzini@redhat.com
Cc: rth@twiddle.net
Cc: ehabkost@redhat.com
Cc: mst@redhat.com
Cc: dmitry@daynix.com
Cc: jasowang@redhat.com
Cc: marcel@redhat.com
Cc: alex.williamson@redhat.com
Cc: armbru@redhat.com
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/pci/pci.h     |  3 ---
 hw/i386/kvm/pci-assign.c | 14 +++++++-------
 hw/ide/ich.c             |  2 +-
 hw/pci/msi.c             |  2 +-
 hw/pci/msix.c            |  2 +-
 hw/pci/pci.c             | 20 ++------------------
 hw/vfio/pci.c            |  6 +++---
 7 files changed, 15 insertions(+), 34 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index fe52aa8..e598b09 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -358,9 +358,6 @@ pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
 int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
                        uint8_t offset, uint8_t size,
                        Error **errp);
-int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
-                       uint8_t offset, uint8_t size,
-                       Error **errp);
 
 void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
 
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 87dcbdd..3d60455 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1254,7 +1254,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
         dev->dev.cap_present |= QEMU_PCI_CAP_MSI;
         dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI;
         /* Only 32-bit/no-mask currently supported */
-        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSI, pos, 10,
+        ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSI, pos, 10,
                                   &local_err);
         if (ret < 0) {
             error_propagate(errp, local_err);
@@ -1288,7 +1288,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
         }
         dev->dev.cap_present |= QEMU_PCI_CAP_MSIX;
         dev->cap.available |= ASSIGNED_DEVICE_CAP_MSIX;
-        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSIX, pos, 12,
+        ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSIX, pos, 12,
                                   &local_err);
         if (ret < 0) {
             error_propagate(errp, local_err);
@@ -1318,7 +1318,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
     if (pos) {
         uint16_t pmc;
 
-        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_PM, pos, PCI_PM_SIZEOF,
+        ret = pci_add_capability(pci_dev, PCI_CAP_ID_PM, pos, PCI_PM_SIZEOF,
                                   &local_err);
         if (ret < 0) {
             error_propagate(errp, local_err);
@@ -1386,7 +1386,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
             return -EINVAL;
         }
 
-        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_EXP, pos, size,
+        ret = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, pos, size,
                                   &local_err);
         if (ret < 0) {
             error_propagate(errp, local_err);
@@ -1462,7 +1462,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
         uint32_t status;
 
         /* Only expose the minimum, 8 byte capability */
-        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_PCIX, pos, 8,
+        ret = pci_add_capability(pci_dev, PCI_CAP_ID_PCIX, pos, 8,
                                   &local_err);
         if (ret < 0) {
             error_propagate(errp, local_err);
@@ -1490,7 +1490,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
     pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_VPD, 0);
     if (pos) {
         /* Direct R/W passthrough */
-        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_VPD, pos, 8,
+        ret = pci_add_capability(pci_dev, PCI_CAP_ID_VPD, pos, 8,
                                   &local_err);
         if (ret < 0) {
             error_propagate(errp, local_err);
@@ -1508,7 +1508,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
         pos += PCI_CAP_LIST_NEXT) {
         uint8_t len = pci_get_byte(pci_dev->config + pos + PCI_CAP_FLAGS);
         /* Direct R/W passthrough */
-        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_VNDR, pos, len,
+        ret = pci_add_capability(pci_dev, PCI_CAP_ID_VNDR, pos, len,
                                   &local_err);
         if (ret < 0) {
             error_propagate(errp, local_err);
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 4599169..989fca5 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -130,7 +130,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
     pci_register_bar(dev, ICH9_MEM_BAR, PCI_BASE_ADDRESS_SPACE_MEMORY,
                      &d->ahci.mem);
 
-    sata_cap_offset = pci_add_capability2(dev, PCI_CAP_ID_SATA,
+    sata_cap_offset = pci_add_capability(dev, PCI_CAP_ID_SATA,
                                           ICH9_SATA_CAP_OFFSET, SATA_CAP_SIZE,
                                           errp);
     if (sata_cap_offset < 0) {
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index a87b227..5e05ce5 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -216,7 +216,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
     }
 
     cap_size = msi_cap_sizeof(flags);
-    config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset,
+    config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset,
                                         cap_size, errp);
     if (config_offset < 0) {
         return config_offset;
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index fc5fe51..5078d3d 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -301,7 +301,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
         return -EINVAL;
     }
 
-    cap = pci_add_capability2(dev, PCI_CAP_ID_MSIX,
+    cap = pci_add_capability(dev, PCI_CAP_ID_MSIX,
                               cap_pos, MSIX_CAP_LENGTH, errp);
     if (cap < 0) {
         return cap;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 3ad6f4e..0c6f74a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2259,28 +2259,12 @@ static void pci_del_option_rom(PCIDevice *pdev)
 }
 
 /*
- * if offset = 0,
- * Find and reserve space and add capability to the linked list
- * in pci config space
- */
-int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
-                       uint8_t offset, uint8_t size,
-                       Error **errp)
-{
-    int ret;
-
-    ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
-
-    return ret;
-}
-
-/*
- * On success, pci_add_capability2() returns a positive value
+ * On success, pci_add_capability() returns a positive value
  * that the offset of the pci capability.
  * On failure, it sets an error and returns a negative error
  * code.
  */
-int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
+int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
                        uint8_t offset, uint8_t size,
                        Error **errp)
 {
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 70bfb59..8de8272 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1837,14 +1837,14 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
     case PCI_CAP_ID_PM:
         vfio_check_pm_reset(vdev, pos);
         vdev->pm_cap = pos;
-        ret = pci_add_capability2(pdev, cap_id, pos, size, errp);
+        ret = pci_add_capability(pdev, cap_id, pos, size, errp);
         break;
     case PCI_CAP_ID_AF:
         vfio_check_af_flr(vdev, pos);
-        ret = pci_add_capability2(pdev, cap_id, pos, size, errp);
+        ret = pci_add_capability(pdev, cap_id, pos, size, errp);
         break;
     default:
-        ret = pci_add_capability2(pdev, cap_id, pos, size, errp);
+        ret = pci_add_capability(pdev, cap_id, pos, size, errp);
         break;
     }
 out:
-- 
MST

[Qemu-devel] [PULL 09/21] pci: Convert to realize

[Michael S. Tsirkin]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

Convert i82801b11, io3130_upstream, io3130_downstream and
pcie_root_port devices to realize.

Cc: mst@redhat.com
Cc: marcel@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/pci/pci_bridge.h        |  3 ++-
 include/hw/pci/pcie.h              |  3 ++-
 hw/pci-bridge/i82801b11.c          | 11 +++++------
 hw/pci-bridge/pcie_root_port.c     | 18 +++++++++---------
 hw/pci-bridge/xio3130_downstream.c | 20 +++++++++-----------
 hw/pci-bridge/xio3130_upstream.c   | 20 +++++++++-----------
 hw/pci/pci_bridge.c                |  7 +++----
 hw/pci/pcie.c                      | 24 +++++++++++++++++-------
 8 files changed, 56 insertions(+), 50 deletions(-)

diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index d5891cd..ff7cbaa 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -33,7 +33,8 @@
 #define PCI_BRIDGE_DEV_PROP_SHPC       "shpc"
 
 int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
-                          uint16_t svid, uint16_t ssid);
+                          uint16_t svid, uint16_t ssid,
+                          Error **errp);
 
 PCIDevice *pci_bridge_get_device(PCIBus *bus);
 PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 3d8f24b..b71e369 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -84,7 +84,8 @@ struct PCIExpressDevice {
 #define COMPAT_PROP_PCP "power_controller_present"
 
 /* PCI express capability helper functions */
-int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port);
+int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type,
+                  uint8_t port, Error **errp);
 int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset,
                      uint8_t type, uint8_t port);
 int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset);
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index 2c065c3..2c1b747 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -59,24 +59,23 @@ typedef struct I82801b11Bridge {
     /*< public >*/
 } I82801b11Bridge;
 
-static int i82801b11_bridge_initfn(PCIDevice *d)
+static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
 {
     int rc;
 
     pci_bridge_initfn(d, TYPE_PCI_BUS);
 
     rc = pci_bridge_ssvid_init(d, I82801ba_SSVID_OFFSET,
-                               I82801ba_SSVID_SVID, I82801ba_SSVID_SSID);
+                               I82801ba_SSVID_SVID, I82801ba_SSVID_SSID,
+                               errp);
     if (rc < 0) {
         goto err_bridge;
     }
     pci_config_set_prog_interface(d->config, PCI_CLASS_BRIDGE_PCI_INF_SUB);
-    return 0;
+    return;
 
 err_bridge:
     pci_bridge_exitfn(d);
-
-    return rc;
 }
 
 static const VMStateDescription i82801b11_bridge_dev_vmstate = {
@@ -96,7 +95,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data)
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82801BA_11;
     k->revision = ICH9_D2P_A2_REVISION;
-    k->init = i82801b11_bridge_initfn;
+    k->realize = i82801b11_bridge_realize;
     k->config_write = pci_bridge_write_config;
     dc->vmsd = &i82801b11_bridge_dev_vmstate;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index cf36318..4d588cb 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -59,29 +59,30 @@ static void rp_realize(PCIDevice *d, Error **errp)
     PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(d);
     PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
     int rc;
-    Error *local_err = NULL;
 
     pci_config_set_interrupt_pin(d->config, 1);
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
-    rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id, rpc->ssid);
+    rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id,
+                               rpc->ssid, errp);
     if (rc < 0) {
-        error_setg(errp, "Can't init SSV ID, error %d", rc);
+        error_append_hint(errp, "Can't init SSV ID, error %d\n", rc);
         goto err_bridge;
     }
 
     if (rpc->interrupts_init) {
-        rc = rpc->interrupts_init(d, &local_err);
+        rc = rpc->interrupts_init(d, errp);
         if (rc < 0) {
-            error_propagate(errp, local_err);
             goto err_bridge;
         }
     }
 
-    rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT, p->port);
+    rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT,
+                       p->port, errp);
     if (rc < 0) {
-        error_setg(errp, "Can't add Root Port capability, error %d", rc);
+        error_append_hint(errp, "Can't add Root Port capability, "
+                          "error %d\n", rc);
         goto err_int;
     }
 
@@ -98,9 +99,8 @@ static void rp_realize(PCIDevice *d, Error **errp)
     }
 
     rc = pcie_aer_init(d, PCI_ERR_VER, rpc->aer_offset,
-                       PCI_ERR_SIZEOF, &local_err);
+                       PCI_ERR_SIZEOF, errp);
     if (rc < 0) {
-        error_propagate(errp, local_err);
         goto err;
     }
     pcie_aer_root_init(d);
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index cfe8a36..e706f36 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
     pci_bridge_reset(qdev);
 }
 
-static int xio3130_downstream_initfn(PCIDevice *d)
+static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
 {
     PCIEPort *p = PCIE_PORT(d);
     PCIESlot *s = PCIE_SLOT(d);
     int rc;
-    Error *err = NULL;
 
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
     rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
                   XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
-                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
+                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
+                  errp);
     if (rc < 0) {
         assert(rc == -ENOTSUP);
-        error_report_err(err);
         goto err_bridge;
     }
 
     rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
-                               XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
+                               XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
+                               errp);
     if (rc < 0) {
         goto err_bridge;
     }
 
     rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM,
-                       p->port);
+                       p->port, errp);
     if (rc < 0) {
         goto err_msi;
     }
@@ -98,13 +98,12 @@ static int xio3130_downstream_initfn(PCIDevice *d)
     }
 
     rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
-                       PCI_ERR_SIZEOF, &err);
+                       PCI_ERR_SIZEOF, errp);
     if (rc < 0) {
-        error_report_err(err);
         goto err;
     }
 
-    return 0;
+    return;
 
 err:
     pcie_chassis_del_slot(s);
@@ -114,7 +113,6 @@ err_msi:
     msi_uninit(d);
 err_bridge:
     pci_bridge_exitfn(d);
-    return rc;
 }
 
 static void xio3130_downstream_exitfn(PCIDevice *d)
@@ -181,7 +179,7 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
     k->is_express = 1;
     k->is_bridge = 1;
     k->config_write = xio3130_downstream_write_config;
-    k->init = xio3130_downstream_initfn;
+    k->realize = xio3130_downstream_realize;
     k->exit = xio3130_downstream_exitfn;
     k->vendor_id = PCI_VENDOR_ID_TI;
     k->device_id = PCI_DEVICE_ID_TI_XIO3130D;
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 401c784..a052224 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -53,32 +53,32 @@ static void xio3130_upstream_reset(DeviceState *qdev)
     pcie_cap_deverr_reset(d);
 }
 
-static int xio3130_upstream_initfn(PCIDevice *d)
+static void xio3130_upstream_realize(PCIDevice *d, Error **errp)
 {
     PCIEPort *p = PCIE_PORT(d);
     int rc;
-    Error *err = NULL;
 
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
     rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
                   XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
-                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
+                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
+                  errp);
     if (rc < 0) {
         assert(rc == -ENOTSUP);
-        error_report_err(err);
         goto err_bridge;
     }
 
     rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
-                               XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
+                               XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
+                               errp);
     if (rc < 0) {
         goto err_bridge;
     }
 
     rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM,
-                       p->port);
+                       p->port, errp);
     if (rc < 0) {
         goto err_msi;
     }
@@ -86,13 +86,12 @@ static int xio3130_upstream_initfn(PCIDevice *d)
     pcie_cap_deverr_init(d);
 
     rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
-                       PCI_ERR_SIZEOF, &err);
+                       PCI_ERR_SIZEOF, errp);
     if (rc < 0) {
-        error_report_err(err);
         goto err;
     }
 
-    return 0;
+    return;
 
 err:
     pcie_cap_exit(d);
@@ -100,7 +99,6 @@ err_msi:
     msi_uninit(d);
 err_bridge:
     pci_bridge_exitfn(d);
-    return rc;
 }
 
 static void xio3130_upstream_exitfn(PCIDevice *d)
@@ -153,7 +151,7 @@ static void xio3130_upstream_class_init(ObjectClass *klass, void *data)
     k->is_express = 1;
     k->is_bridge = 1;
     k->config_write = xio3130_upstream_write_config;
-    k->init = xio3130_upstream_initfn;
+    k->realize = xio3130_upstream_realize;
     k->exit = xio3130_upstream_exitfn;
     k->vendor_id = PCI_VENDOR_ID_TI;
     k->device_id = PCI_DEVICE_ID_TI_XIO3130U;
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index bb0f3a3..720119b 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -41,15 +41,14 @@
 #define PCI_SSVID_SSID          6
 
 int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
-                          uint16_t svid, uint16_t ssid)
+                          uint16_t svid, uint16_t ssid,
+                          Error **errp)
 {
     int pos;
-    Error *local_err = NULL;
 
     pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
-                             PCI_SSVID_SIZEOF, &local_err);
+                             PCI_SSVID_SIZEOF, errp);
     if (pos < 0) {
-        error_report_err(local_err);
         return pos;
     }
 
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index f187512..32191f2 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -86,19 +86,19 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
     pci_set_word(cmask + PCI_EXP_LNKSTA, 0);
 }
 
-int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
+int pcie_cap_init(PCIDevice *dev, uint8_t offset,
+                  uint8_t type, uint8_t port,
+                  Error **errp)
 {
     /* PCIe cap v2 init */
     int pos;
     uint8_t *exp_cap;
-    Error *local_err = NULL;
 
     assert(pci_is_express(dev));
 
     pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
-                             PCI_EXP_VER2_SIZEOF, &local_err);
+                             PCI_EXP_VER2_SIZEOF, errp);
     if (pos < 0) {
-        error_report_err(local_err);
         return pos;
     }
     dev->exp.exp_cap = pos;
@@ -147,6 +147,8 @@ static int
 pcie_endpoint_cap_common_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
 {
     uint8_t type = PCI_EXP_TYPE_ENDPOINT;
+    Error *local_err = NULL;
+    int ret;
 
     /*
      * Windows guests will report Code 10, device cannot start, if
@@ -157,9 +159,17 @@ pcie_endpoint_cap_common_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
         type = PCI_EXP_TYPE_RC_END;
     }
 
-    return (cap_size == PCI_EXP_VER1_SIZEOF)
-        ? pcie_cap_v1_init(dev, offset, type, 0)
-        : pcie_cap_init(dev, offset, type, 0);
+    if (cap_size == PCI_EXP_VER1_SIZEOF) {
+        return pcie_cap_v1_init(dev, offset, type, 0);
+    } else {
+        ret = pcie_cap_init(dev, offset, type, 0, &local_err);
+
+        if (ret < 0) {
+            error_report_err(local_err);
+        }
+
+        return ret;
+    }
 }
 
 int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset)
-- 
MST

[Qemu-devel] [PULL 10/21] pci: Convert shpc_init() to Error

[Michael S. Tsirkin]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

In order to propagate error message better, convert shpc_init() to
Error also convert the pci_bridge_dev_initfn() to realize.

Cc: mst@redhat.com
Cc: marcel@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/pci/shpc.h          |  3 ++-
 include/hw/pci/slotid_cap.h    |  3 ++-
 hw/pci-bridge/pci_bridge_dev.c | 14 ++++++--------
 hw/pci/shpc.c                  | 11 +++++------
 hw/pci/slotid_cap.c            | 11 +++++------
 5 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 71e836b..ee19fec 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -38,7 +38,8 @@ struct SHPCDevice {
 
 void shpc_reset(PCIDevice *d);
 int shpc_bar_size(PCIDevice *dev);
-int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
+int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar,
+              unsigned off, Error **errp);
 void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
 void shpc_free(PCIDevice *dev);
 void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
diff --git a/include/hw/pci/slotid_cap.h b/include/hw/pci/slotid_cap.h
index 70db047..a777ea0 100644
--- a/include/hw/pci/slotid_cap.h
+++ b/include/hw/pci/slotid_cap.h
@@ -5,7 +5,8 @@
 
 int slotid_cap_init(PCIDevice *dev, int nslots,
                     uint8_t chassis,
-                    unsigned offset);
+                    unsigned offset,
+                    Error **errp);
 void slotid_cap_cleanup(PCIDevice *dev);
 
 #endif
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 5dbd933..4373f1d 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -49,7 +49,7 @@ struct PCIBridgeDev {
 };
 typedef struct PCIBridgeDev PCIBridgeDev;
 
-static int pci_bridge_dev_initfn(PCIDevice *dev)
+static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
 {
     PCIBridge *br = PCI_BRIDGE(dev);
     PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
@@ -62,7 +62,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
         dev->config[PCI_INTERRUPT_PIN] = 0x1;
         memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
                            shpc_bar_size(dev));
-        err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
+        err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0, errp);
         if (err) {
             goto shpc_error;
         }
@@ -71,7 +71,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
         bridge_dev->msi = ON_OFF_AUTO_OFF;
     }
 
-    err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
+    err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0, errp);
     if (err) {
         goto slotid_error;
     }
@@ -87,7 +87,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
             /* Can't satisfy user's explicit msi=on request, fail */
             error_append_hint(&local_err, "You have to use msi=auto (default) "
                     "or msi=off with this machine type.\n");
-            error_report_err(local_err);
+            error_propagate(errp, local_err);
             goto msi_error;
         }
         assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO);
@@ -101,7 +101,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
         pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
                          PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
     }
-    return 0;
+    return;
 
 msi_error:
     slotid_cap_cleanup(dev);
@@ -111,8 +111,6 @@ slotid_error:
     }
 shpc_error:
     pci_bridge_exitfn(dev);
-
-    return err;
 }
 
 static void pci_bridge_dev_exitfn(PCIDevice *dev)
@@ -216,7 +214,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
-    k->init = pci_bridge_dev_initfn;
+    k->realize = pci_bridge_dev_realize;
     k->exit = pci_bridge_dev_exitfn;
     k->config_write = pci_bridge_dev_write_config;
     k->vendor_id = PCI_VENDOR_ID_REDHAT;
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index d72d5e4..69fc14b 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -446,16 +446,14 @@ static void shpc_cap_update_dword(PCIDevice *d)
 }
 
 /* Add SHPC capability to the config space for the device. */
-static int shpc_cap_add_config(PCIDevice *d)
+static int shpc_cap_add_config(PCIDevice *d, Error **errp)
 {
     uint8_t *config;
     int config_offset;
-    Error *local_err = NULL;
     config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
                                        0, SHPC_CAP_LENGTH,
-                                       &local_err);
+                                       errp);
     if (config_offset < 0) {
-        error_report_err(local_err);
         return config_offset;
     }
     config = d->config + config_offset;
@@ -584,13 +582,14 @@ void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
 }
 
 /* Initialize the SHPC structure in bridge's BAR. */
-int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar, unsigned offset)
+int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
+              unsigned offset, Error **errp)
 {
     int i, ret;
     int nslots = SHPC_MAX_SLOTS; /* TODO: qdev property? */
     SHPCDevice *shpc = d->shpc = g_malloc0(sizeof(*d->shpc));
     shpc->sec_bus = sec_bus;
-    ret = shpc_cap_add_config(d);
+    ret = shpc_cap_add_config(d, errp);
     if (ret) {
         g_free(d->shpc);
         return ret;
diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
index bdca205..36d021b 100644
--- a/hw/pci/slotid_cap.c
+++ b/hw/pci/slotid_cap.c
@@ -9,14 +9,14 @@
 
 int slotid_cap_init(PCIDevice *d, int nslots,
                     uint8_t chassis,
-                    unsigned offset)
+                    unsigned offset,
+                    Error **errp)
 {
     int cap;
-    Error *local_err = NULL;
 
     if (!chassis) {
-        error_report("Bridge chassis not specified. Each bridge is required "
-                     "to be assigned a unique chassis id > 0.");
+        error_setg(errp, "Bridge chassis not specified. Each bridge is required"
+                   " to be assigned a unique chassis id > 0.");
         return -EINVAL;
     }
     if (nslots < 0 || nslots > (PCI_SID_ESR_NSLOTS >> SLOTID_NSLOTS_SHIFT)) {
@@ -25,9 +25,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
     }
 
     cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
-                             SLOTID_CAP_LENGTH, &local_err);
+                             SLOTID_CAP_LENGTH, errp);
     if (cap < 0) {
-        error_report_err(local_err);
         return cap;
     }
     /* We make each chassis unique, this way each bridge is First in Chassis */
-- 
MST

[Qemu-devel] [PULL 11/21] i386/kvm/pci-assign: Fix return type of verify_irqchip_kernel()

[Michael S. Tsirkin]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

When the function no success value to transmit, it usually make the
function return void. It has turned out not to be a success, because
it means that the extra local_err variable and error_propagate() will
be needed. It leads to cumbersome code, therefore, transmit success/
failure in the return value is worth. So fix the return type to avoid
it.

Cc: pbonzini@redhat.com
Cc: rth@twiddle.net
Cc: ehabkost@redhat.com
Cc: mst@redhat.com
Cc: armbru@redhat.com
Cc: marcel@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 3d60455..b7fdb47 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -824,12 +824,13 @@ static void assign_device(AssignedDevice *dev, Error **errp)
     }
 }
 
-static void verify_irqchip_in_kernel(Error **errp)
+static int verify_irqchip_in_kernel(Error **errp)
 {
     if (kvm_irqchip_in_kernel()) {
-        return;
+        return -1;
     }
     error_setg(errp, "pci-assign requires KVM with in-kernel irqchip enabled");
+    return 0;
 }
 
 static int assign_intx(AssignedDevice *dev, Error **errp)
@@ -838,7 +839,6 @@ static int assign_intx(AssignedDevice *dev, Error **errp)
     PCIINTxRoute intx_route;
     bool intx_host_msi;
     int r;
-    Error *local_err = NULL;
 
     /* Interrupt PIN 0 means don't use INTx */
     if (assigned_dev_pci_read_byte(&dev->dev, PCI_INTERRUPT_PIN) == 0) {
@@ -846,9 +846,7 @@ static int assign_intx(AssignedDevice *dev, Error **errp)
         return 0;
     }
 
-    verify_irqchip_in_kernel(&local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (verify_irqchip_in_kernel(errp) < 0) {
         return -ENOTSUP;
     }
 
@@ -1246,9 +1244,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
      * MSI capability is the 1st capability in capability config */
     pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSI, 0);
     if (pos != 0 && kvm_check_extension(kvm_state, KVM_CAP_ASSIGN_DEV_IRQ)) {
-        verify_irqchip_in_kernel(&local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (verify_irqchip_in_kernel(errp) < 0) {
             return -ENOTSUP;
         }
         dev->dev.cap_present |= QEMU_PCI_CAP_MSI;
@@ -1281,9 +1277,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
         uint32_t msix_table_entry;
         uint16_t msix_max;
 
-        verify_irqchip_in_kernel(&local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (verify_irqchip_in_kernel(errp) < 0) {
             return -ENOTSUP;
         }
         dev->dev.cap_present |= QEMU_PCI_CAP_MSIX;
-- 
MST

[Qemu-devel] [PULL 12/21] i386/kvm/pci-assign: Use errp directly rather than local_err

[Michael S. Tsirkin]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

In assigned_device_pci_cap_init(), first, error messages are filled
to a local_err variable, then through error_propagate() pass to
the parameter of errp. It leads to cumbersome code. In order to
avoid the extra local_err and error_propagate(), drop it and use
errp instead.

Cc: pbonzini@redhat.com
Cc: rth@twiddle.net
Cc: ehabkost@redhat.com
Cc: mst@redhat.com
Cc: armbru@redhat.com
Cc: marcel@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index b7fdb47..9f2615c 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1232,7 +1232,6 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
     AssignedDevice *dev = PCI_ASSIGN(pci_dev);
     PCIRegion *pci_region = dev->real_device.regions;
     int ret, pos;
-    Error *local_err = NULL;
 
     /* Clear initial capabilities pointer and status copied from hw */
     pci_set_byte(pci_dev->config + PCI_CAPABILITY_LIST, 0);
@@ -1251,9 +1250,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
         dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI;
         /* Only 32-bit/no-mask currently supported */
         ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSI, pos, 10,
-                                  &local_err);
+                                  errp);
         if (ret < 0) {
-            error_propagate(errp, local_err);
             return ret;
         }
         pci_dev->msi_cap = pos;
@@ -1283,9 +1281,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
         dev->dev.cap_present |= QEMU_PCI_CAP_MSIX;
         dev->cap.available |= ASSIGNED_DEVICE_CAP_MSIX;
         ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSIX, pos, 12,
-                                  &local_err);
+                                  errp);
         if (ret < 0) {
-            error_propagate(errp, local_err);
             return ret;
         }
         pci_dev->msix_cap = pos;
@@ -1313,9 +1310,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
         uint16_t pmc;
 
         ret = pci_add_capability(pci_dev, PCI_CAP_ID_PM, pos, PCI_PM_SIZEOF,
-                                  &local_err);
+                                  errp);
         if (ret < 0) {
-            error_propagate(errp, local_err);
             return ret;
         }
 
@@ -1381,9 +1377,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
         }
 
         ret = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, pos, size,
-                                  &local_err);
+                                  errp);
         if (ret < 0) {
-            error_propagate(errp, local_err);
             return ret;
         }
 
@@ -1457,9 +1452,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
 
         /* Only expose the minimum, 8 byte capability */
         ret = pci_add_capability(pci_dev, PCI_CAP_ID_PCIX, pos, 8,
-                                  &local_err);
+                                  errp);
         if (ret < 0) {
-            error_propagate(errp, local_err);
             return ret;
         }
 
@@ -1485,9 +1479,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
     if (pos) {
         /* Direct R/W passthrough */
         ret = pci_add_capability(pci_dev, PCI_CAP_ID_VPD, pos, 8,
-                                  &local_err);
+                                  errp);
         if (ret < 0) {
-            error_propagate(errp, local_err);
             return ret;
         }
 
@@ -1503,9 +1496,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
         uint8_t len = pci_get_byte(pci_dev->config + pos + PCI_CAP_FLAGS);
         /* Direct R/W passthrough */
         ret = pci_add_capability(pci_dev, PCI_CAP_ID_VNDR, pos, len,
-                                  &local_err);
+                                  errp);
         if (ret < 0) {
-            error_propagate(errp, local_err);
             return ret;
         }
 
-- 
MST

[Qemu-devel] [PULL 13/21] fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize()

[Michael S. Tsirkin]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

As indicated by Laszlo it is a QOM bug for the realize() method to actually
map the device. Set up the IO regions within fw_cfg_io_realize() and defer
the mapping with sysbus_add_io() to the caller, as already done in
fw_cfg_init_mem_wide().

This makes the iobase and dma_iobase properties now obsolete so they can be
removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/nvram/fw_cfg.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 316fca9..4e4f71a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -96,7 +96,6 @@ struct FWCfgIoState {
     /*< public >*/
 
     MemoryRegion comb_iomem;
-    uint32_t iobase, dma_iobase;
 };
 
 struct FWCfgMemState {
@@ -936,24 +935,30 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
                                 AddressSpace *dma_as)
 {
     DeviceState *dev;
+    SysBusDevice *sbd;
+    FWCfgIoState *ios;
     FWCfgState *s;
     uint32_t version = FW_CFG_VERSION;
     bool dma_requested = dma_iobase && dma_as;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_IO);
-    qdev_prop_set_uint32(dev, "iobase", iobase);
-    qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
     if (!dma_requested) {
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
     fw_cfg_init1(dev);
+
+    sbd = SYS_BUS_DEVICE(dev);
+    ios = FW_CFG_IO(dev);
+    sysbus_add_io(sbd, iobase, &ios->comb_iomem);
+
     s = FW_CFG(dev);
 
     if (s->dma_enabled) {
         /* 64 bits for the address field */
         s->dma_as = dma_as;
         s->dma_addr = 0;
+        sysbus_add_io(sbd, dma_iobase, &s->dma_iomem);
 
         version |= FW_CFG_VERSION_DMA;
     }
@@ -1059,8 +1064,6 @@ static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
 }
 
 static Property fw_cfg_io_properties[] = {
-    DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
-    DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
     DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
                      true),
     DEFINE_PROP_UINT16("x-file-slots", FWCfgIoState, parent_obj.file_slots,
@@ -1071,7 +1074,6 @@ static Property fw_cfg_io_properties[] = {
 static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
 {
     FWCfgIoState *s = FW_CFG_IO(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     Error *local_err = NULL;
 
     fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
@@ -1085,13 +1087,11 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
      * of the i/o region used is FW_CFG_CTL_SIZE */
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
                           FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
-    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
 
     if (FW_CFG(s)->dma_enabled) {
         memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
                               &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
                               sizeof(dma_addr_t));
-        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
     }
 }
 
-- 
MST

[Qemu-devel] [PULL 14/21] fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1()

[Michael S. Tsirkin]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

The setting of the FW_CFG_VERSION_DMA bit is the same across both the
TYPE_FW_CFG_MEM and TYPE_FW_CFG_IO devices, so unify the logic in
fw_cfg_init1().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/nvram/fw_cfg.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 4e4f71a..99bdbc2 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -913,6 +913,7 @@ static void fw_cfg_init1(DeviceState *dev)
 {
     FWCfgState *s = FW_CFG(dev);
     MachineState *machine = MACHINE(qdev_get_machine());
+    uint32_t version = FW_CFG_VERSION;
 
     assert(!object_resolve_path(FW_CFG_PATH, NULL));
 
@@ -927,6 +928,12 @@ static void fw_cfg_init1(DeviceState *dev)
     fw_cfg_bootsplash(s);
     fw_cfg_reboot(s);
 
+    if (s->dma_enabled) {
+        version |= FW_CFG_VERSION_DMA;
+    }
+
+    fw_cfg_add_i32(s, FW_CFG_ID, version);
+
     s->machine_ready.notify = fw_cfg_machine_ready;
     qemu_add_machine_init_done_notifier(&s->machine_ready);
 }
@@ -938,7 +945,6 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
     SysBusDevice *sbd;
     FWCfgIoState *ios;
     FWCfgState *s;
-    uint32_t version = FW_CFG_VERSION;
     bool dma_requested = dma_iobase && dma_as;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_IO);
@@ -959,12 +965,8 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
         s->dma_as = dma_as;
         s->dma_addr = 0;
         sysbus_add_io(sbd, dma_iobase, &s->dma_iomem);
-
-        version |= FW_CFG_VERSION_DMA;
     }
 
-    fw_cfg_add_i32(s, FW_CFG_ID, version);
-
     return s;
 }
 
@@ -980,7 +982,6 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
     DeviceState *dev;
     SysBusDevice *sbd;
     FWCfgState *s;
-    uint32_t version = FW_CFG_VERSION;
     bool dma_requested = dma_addr && dma_as;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
@@ -1001,11 +1002,8 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
         s->dma_as = dma_as;
         s->dma_addr = 0;
         sysbus_mmio_map(sbd, 2, dma_addr);
-        version |= FW_CFG_VERSION_DMA;
     }
 
-    fw_cfg_add_i32(s, FW_CFG_ID, version);
-
     return s;
 }
 
-- 
MST

[Qemu-devel] [PULL 15/21] hw/acpi: remove dead acpi code

[Michael S. Tsirkin]

From: Aleksandr Bezzubikov <zuban32s@gmail.com>

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/acpi-build.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0b8bc62..5464977 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1913,16 +1913,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         build_piix4_pci0_int(dsdt);
     } else {
         sb_scope = aml_scope("_SB");
-        aml_append(sb_scope,
-            aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x0c));
-        aml_append(sb_scope,
-            aml_operation_region("PCSB", AML_SYSTEM_IO, aml_int(0xae0c), 0x01));
-        field = aml_field("PCSB", AML_ANY_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
-        aml_append(field, aml_named_field("PCIB", 8));
-        aml_append(sb_scope, field);
-        aml_append(dsdt, sb_scope);
-
-        sb_scope = aml_scope("_SB");
         dev = aml_device("PCI0");
         aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
         aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
-- 
MST

[Qemu-devel] [PULL 16/21] intel_iommu: fix migration breakage on mr switch

[Michael S. Tsirkin]

From: Peter Xu <peterx@redhat.com>

Migration is broken after the vfio integration work:

qemu-kvm: AHCI: Failed to start FIS receive engine: bad FIS receive buffer address
qemu-kvm: Failed to load ich9_ahci:ahci
qemu-kvm: error while loading state for instance 0x0 of device '0000:00:1f.2/ich9_ahci'
qemu-kvm: load of migration failed: Operation not permitted

The problem is that vfio work introduced dynamic memory region
switching (actually it is also used for future PT mode), and this memory
region layout is not properly delivered to destination when migration
happens. Solution is to rebuild the layout in post_load.

Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1459906
Fixes: 558e0024 ("intel_iommu: allow dynamic switch of IOMMU region")
Reviewed-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/intel_iommu.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 2ddf3bd..88dc042 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2337,11 +2337,26 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu,
     }
 }
 
+static int vtd_post_load(void *opaque, int version_id)
+{
+    IntelIOMMUState *iommu = opaque;
+
+    /*
+     * Memory regions are dynamically turned on/off depending on
+     * context entry configurations from the guest. After migration,
+     * we need to make sure the memory regions are still correct.
+     */
+    vtd_switch_address_space_all(iommu);
+
+    return 0;
+}
+
 static const VMStateDescription vtd_vmstate = {
     .name = "iommu-intel",
     .version_id = 1,
     .minimum_version_id = 1,
     .priority = MIG_PRI_IOMMU,
+    .post_load = vtd_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(root, IntelIOMMUState),
         VMSTATE_UINT64(intr_root, IntelIOMMUState),
-- 
MST

[Qemu-devel] [PULL 17/21] vhost: ensure vhost_ops are set before calling iotlb callback

[Michael S. Tsirkin]

From: Maxime Coquelin <maxime.coquelin@redhat.com>

This patch fixes a crash that happens when vhost-user iommu
support is enabled and vhost-user socket is closed.

When it happens, if an IOTLB invalidation notification is sent
by the IOMMU, vhost_ops's NULL pointer is dereferenced.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/vhost-backend.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 4e31de1..cb055e8 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -309,7 +309,10 @@ int vhost_backend_update_device_iotlb(struct vhost_dev *dev,
         return -EINVAL;
     }
 
-    return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
+    if (dev->vhost_ops && dev->vhost_ops->vhost_send_device_iotlb_msg)
+        return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
+
+    return -ENODEV;
 }
 
 int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev,
@@ -321,7 +324,10 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev,
     imsg.size = len;
     imsg.type = VHOST_IOTLB_INVALIDATE;
 
-    return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
+    if (dev->vhost_ops && dev->vhost_ops->vhost_send_device_iotlb_msg)
+        return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
+
+    return -ENODEV;
 }
 
 int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
-- 
MST

[Qemu-devel] [PULL 18/21] vhost-user: unregister slave req handler at cleanup time

[Michael S. Tsirkin]

From: Maxime Coquelin <maxime.coquelin@redhat.com>

If the backend sends a request just before closing the socket,
the aio dispatcher might schedule its reading after the vhost
device has been cleaned, leading to a NULL pointer dereference
in slave_read();

vhost_user_cleanup() already closes the socket but it is not
enough, the handler has to be unregistered.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/vhost-user.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 958ee09..2203011 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -779,6 +779,7 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
 
     u = dev->opaque;
     if (u->slave_fd >= 0) {
+        qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
         close(u->slave_fd);
         u->slave_fd = -1;
     }
-- 
MST

[Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Michael S. Tsirkin]

From: Ben Warren <ben@skyportsystems.com>

The following tests are implemented:
* test that a GUID passed in by command line is propagated to the guest.
  Read the GUID from guest memory
* test that the "auto" argument to the GUID generates a valid GUID, as
  seen by the guest.
* test that a GUID passed in can be queried from the monitor

  This patch is loosely based on a previous patch from:
  Gal Hammer <ghammer@redhat.com>  and Igor Mammedov <imammedo@redhat.com>

Signed-off-by: Ben Warren <ben@skyportsystems.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/vmgenid-test.c   | 203 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include |   2 +
 2 files changed, 205 insertions(+)
 create mode 100644 tests/vmgenid-test.c

diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
new file mode 100644
index 0000000..e7ba38c
--- /dev/null
+++ b/tests/vmgenid-test.c
@@ -0,0 +1,203 @@
+/*
+ * QTest testcase for VM Generation ID
+ *
+ * Copyright (c) 2016 Red Hat, Inc.
+ * Copyright (c) 2017 Skyport Systems
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <glib.h>
+#include <string.h>
+#include <unistd.h>
+#include "qemu/osdep.h"
+#include "qemu/bitmap.h"
+#include "qemu/uuid.h"
+#include "hw/acpi/acpi-defs.h"
+#include "acpi-utils.h"
+#include "libqtest.h"
+
+#define VGID_GUID "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
+#define VMGENID_GUID_OFFSET 40   /* allow space for
+                                  * OVMF SDT Header Probe Supressor
+                                  */
+#define RSDP_ADDR_INVALID 0x100000 /* RSDP must be below this address */
+#define RSDP_SLEEP_US     100000   /* Sleep for 100ms between tries */
+#define RSDP_TRIES_MAX    100      /* Max total time is 10 seconds */
+
+typedef struct {
+    AcpiTableHeader header;
+    gchar name_op;
+    gchar vgia[4];
+    gchar val_op;
+    uint32_t vgia_val;
+} QEMU_PACKED VgidTable;
+
+static uint32_t acpi_find_vgia(void)
+{
+    uint32_t rsdp_offset;
+    uint32_t guid_offset = 0;
+    AcpiRsdpDescriptor rsdp_table;
+    uint32_t rsdt;
+    AcpiRsdtDescriptorRev1 rsdt_table;
+    int tables_nr;
+    uint32_t *tables;
+    AcpiTableHeader ssdt_table;
+    VgidTable vgid_table;
+    int i;
+
+    /* Tables may take a short time to be set up by the guest */
+    for (i = 0; i < RSDP_TRIES_MAX; i++) {
+        rsdp_offset = acpi_find_rsdp_address();
+        if (rsdp_offset < RSDP_ADDR_INVALID) {
+            break;
+        }
+        g_usleep(RSDP_SLEEP_US);
+    }
+    g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
+
+    acpi_parse_rsdp_table(rsdp_offset, &rsdp_table);
+
+    rsdt = rsdp_table.rsdt_physical_address;
+    /* read the header */
+    ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
+    ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
+
+    /* compute the table entries in rsdt */
+    tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
+                sizeof(uint32_t);
+    g_assert_cmpint(tables_nr, >, 0);
+
+    /* get the addresses of the tables pointed by rsdt */
+    tables = g_new0(uint32_t, tables_nr);
+    ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
+
+    for (i = 0; i < tables_nr; i++) {
+        ACPI_READ_TABLE_HEADER(&ssdt_table, tables[i]);
+        if (!strncmp((char *)ssdt_table.oem_table_id, "VMGENID", 7)) {
+            /* the first entry in the table should be VGIA
+             * That's all we need
+             */
+            ACPI_READ_FIELD(vgid_table.name_op, tables[i]);
+            g_assert(vgid_table.name_op == 0x08);  /* name */
+            ACPI_READ_ARRAY(vgid_table.vgia, tables[i]);
+            g_assert(memcmp(vgid_table.vgia, "VGIA", 4) == 0);
+            ACPI_READ_FIELD(vgid_table.val_op, tables[i]);
+            g_assert(vgid_table.val_op == 0x0C);  /* dword */
+            ACPI_READ_FIELD(vgid_table.vgia_val, tables[i]);
+            /* The GUID is written at a fixed offset into the fw_cfg file
+             * in order to implement the "OVMF SDT Header probe suppressor"
+             * see docs/specs/vmgenid.txt for more details
+             */
+            guid_offset = vgid_table.vgia_val + VMGENID_GUID_OFFSET;
+            break;
+        }
+    }
+    g_free(tables);
+    return guid_offset;
+}
+
+static void read_guid_from_memory(QemuUUID *guid)
+{
+    uint32_t vmgenid_addr;
+    int i;
+
+    vmgenid_addr = acpi_find_vgia();
+    g_assert(vmgenid_addr);
+
+    /* Read the GUID directly from guest memory */
+    for (i = 0; i < 16; i++) {
+        guid->data[i] = readb(vmgenid_addr + i);
+    }
+    /* The GUID is in little-endian format in the guest, while QEMU
+     * uses big-endian.  Swap after reading.
+     */
+    qemu_uuid_bswap(guid);
+}
+
+static void read_guid_from_monitor(QemuUUID *guid)
+{
+    QDict *rsp, *rsp_ret;
+    const char *guid_str;
+
+    rsp = qmp("{ 'execute': 'query-vm-generation-id' }");
+    if (qdict_haskey(rsp, "return")) {
+        rsp_ret = qdict_get_qdict(rsp, "return");
+        g_assert(qdict_haskey(rsp_ret, "guid"));
+        guid_str = qdict_get_str(rsp_ret, "guid");
+        g_assert(qemu_uuid_parse(guid_str, guid) == 0);
+    }
+    QDECREF(rsp);
+}
+
+static void vmgenid_set_guid_test(void)
+{
+    QemuUUID expected, measured;
+    gchar *cmd;
+
+    g_assert(qemu_uuid_parse(VGID_GUID, &expected) == 0);
+
+    cmd = g_strdup_printf("-machine accel=tcg -device vmgenid,id=testvgid,"
+                          "guid=%s", VGID_GUID);
+    qtest_start(cmd);
+
+    /* Read the GUID from accessing guest memory */
+    read_guid_from_memory(&measured);
+    g_assert(memcmp(measured.data, expected.data, sizeof(measured.data)) == 0);
+
+    qtest_quit(global_qtest);
+    g_free(cmd);
+}
+
+static void vmgenid_set_guid_auto_test(void)
+{
+    const char *cmd;
+    QemuUUID measured;
+
+    cmd = "-machine accel=tcg -device vmgenid,id=testvgid," "guid=auto";
+    qtest_start(cmd);
+
+    read_guid_from_memory(&measured);
+
+    /* Just check that the GUID is non-null */
+    g_assert(!qemu_uuid_is_null(&measured));
+
+    qtest_quit(global_qtest);
+}
+
+static void vmgenid_query_monitor_test(void)
+{
+    QemuUUID expected, measured;
+    gchar *cmd;
+
+    g_assert(qemu_uuid_parse(VGID_GUID, &expected) == 0);
+
+    cmd = g_strdup_printf("-machine accel=tcg -device vmgenid,id=testvgid,"
+                          "guid=%s", VGID_GUID);
+    qtest_start(cmd);
+
+    /* Read the GUID via the monitor */
+    read_guid_from_monitor(&measured);
+    g_assert(memcmp(measured.data, expected.data, sizeof(measured.data)) == 0);
+
+    qtest_quit(global_qtest);
+    g_free(cmd);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("/vmgenid/vmgenid/set-guid",
+                   vmgenid_set_guid_test);
+    qtest_add_func("/vmgenid/vmgenid/set-guid-auto",
+                   vmgenid_set_guid_auto_test);
+    qtest_add_func("/vmgenid/vmgenid/query-monitor",
+                   vmgenid_query_monitor_test);
+    ret = g_test_run();
+
+    return ret;
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index ae889ca..18cd06a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -250,6 +250,7 @@ check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
 gcov-files-i386-y += hw/usb/hcd-xhci.c
 check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
 check-qtest-i386-y += tests/q35-test$(EXESUF)
+check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
 gcov-files-i386-y += hw/pci-host/q35.c
 check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
 ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
@@ -760,6 +761,7 @@ tests/test-uuid$(EXESUF): tests/test-uuid.o $(test-util-obj-y)
 tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
 tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 tests/numa-test$(EXESUF): tests/numa-test.o
+tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/acpi-utils.o
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
 	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
-- 
MST

[Qemu-devel] [PULL 20/21] virtio-net: fix tx queue size for !vhost-user

[Michael S. Tsirkin]

Current code segfaults when no nic peer is specified.
Fix it up - fall back to default queue size.

Fixes: 9b02e1618cf26a ("virtio-net: enable configurable tx queue size")
Cc: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/virtio-net.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a1fc0db..5630a9e 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -498,6 +498,24 @@ static void virtio_net_set_mrg_rx_bufs(VirtIONet *n, int mergeable_rx_bufs,
     }
 }
 
+static int virtio_net_max_tx_queue_size(VirtIONet *n)
+{
+    NetClientState *peer = n->nic_conf.peers.ncs[0];
+
+    /*
+     * Backends other than vhost-user don't support max queue size.
+     */
+    if (!peer) {
+        return VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE;
+    }
+
+    if (peer->info->type != NET_CLIENT_DRIVER_VHOST_USER) {
+        return VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE;
+    }
+
+    return VIRTQUEUE_MAX_SIZE;
+}
+
 static int peer_attach(VirtIONet *n, int index)
 {
     NetClientState *nc = qemu_get_subqueue(n->nic, index);
@@ -1964,14 +1982,8 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
         error_report("Defaulting to \"bh\"");
     }
 
-    /*
-     * Currently, backends other than vhost-user don't support 1024 queue
-     * size.
-     */
-    if (n->net_conf.tx_queue_size == VIRTQUEUE_MAX_SIZE &&
-        n->nic_conf.peers.ncs[0]->info->type != NET_CLIENT_DRIVER_VHOST_USER) {
-        n->net_conf.tx_queue_size = VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE;
-    }
+    n->net_conf.tx_queue_size = MIN(virtio_net_max_tx_queue_size(n),
+                                    n->net_conf.tx_queue_size);
 
     for (i = 0; i < n->max_queues; i++) {
         virtio_net_add_queue(n, i);
-- 
MST

[Qemu-devel] [PULL 21/21] i386/acpi: update expected acpi files

[Michael S. Tsirkin]

We dropped some dead code, update extected table binaries.

Fixes: 4d7e7f2702912 ("hw/acpi: remove dead acpi code")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/acpi-test-data/q35/DSDT        | Bin 7824 -> 7782 bytes
 tests/acpi-test-data/q35/DSDT.bridge | Bin 7841 -> 7799 bytes
 tests/acpi-test-data/q35/DSDT.cphp   | Bin 8287 -> 8245 bytes
 tests/acpi-test-data/q35/DSDT.ipmibt | Bin 7899 -> 7857 bytes
 tests/acpi-test-data/q35/DSDT.memhp  | Bin 9189 -> 9147 bytes
 5 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/tests/acpi-test-data/q35/DSDT b/tests/acpi-test-data/q35/DSDT
index 0dccad439b8e8e00b403c8d290a89630c4329d45..a6138c829142265255ac6f7bedd44757e944eb2f 100644
GIT binary patch
delta 22
dcmbPW`^<*RCD<h-O^$(qF?=Fd>c*MhWdTt92V(#L

delta 64
zcmaE6Gr^Y2CD<iof*b<_W9>w)R87tJV5j)#h5+Z_5Jql>bzD4Pwi6>a&pO8FMsA?E
PlS6>BrxVA<ydSavP8<?a

diff --git a/tests/acpi-test-data/q35/DSDT.bridge b/tests/acpi-test-data/q35/DSDT.bridge
index 8cd66c3b3143297a5262480e46be9ee811a6291f..6b90f606f8b76f71ea9c569f4e606aeecd9e239d 100644
GIT binary patch
delta 22
dcmZ2z``w1iCD<jTT#kW(@%u!s)QvMa<N#O32S@+_

delta 64
zcmexvv(T2yCD<iop&SDPqwGYkR87tJV5j)#h5+Z_5Jql>bzD4Pwi6>a&pO8FMsA?E
PlS6>BrxVA<JWe?PMT8MI

diff --git a/tests/acpi-test-data/q35/DSDT.cphp b/tests/acpi-test-data/q35/DSDT.cphp
index 3c28a17a69db15dc92c825c1502a7f86ab975c0d..976755ef2dc8ab68a82b64ab9fff793c4a113db9 100644
GIT binary patch
delta 22
ecmccbu+@RfCD<jzRDpqk@xnx|)QvNb$pZjYfCr-h

delta 64
zcmdn$aNmK;CD<h-UV(vu@!LeMR87tJV5j)#h5+Z_5Jql>bzD4Pwi6>a&pO8FMsA?E
PlS6>BrxVA<yyNl!W7ras

diff --git a/tests/acpi-test-data/q35/DSDT.ipmibt b/tests/acpi-test-data/q35/DSDT.ipmibt
index 3ceb876127725f199e3df903bc71e4023e2fa225..b3fa4359ff2cad5569bb625dff7be8f8b4590ba9 100644
GIT binary patch
delta 22
dcmca@yU~`*CD<ioqZ|VRW6eaa)QvNp<N#Ld2Q~lz

delta 64
zcmdmJd)t=FCD<k8wj2Wk<D!XNshXPc!A|kf4FS%<A&lG%>$rHpY$rx;o^_1Tjod(S
PCx-xMPbZFzdCqbGX)_Wf

diff --git a/tests/acpi-test-data/q35/DSDT.memhp b/tests/acpi-test-data/q35/DSDT.memhp
index bdbefd47a5398ed96498b77bfb6a74f7ea638db1..7341c405bfb7dd21b4ec5df92411963cdf894c4d 100644
GIT binary patch
delta 22
dcmaFrzT2J4CD<iow=x3*WAsF>)QvNJlmS`p2U`FD

delta 64
zcmdn({?whzCD<k8sWJltW8*}wR87tJV5j)#h5+Z_5Jql>bzD4Pwi6>a&pO8FMsA?E
PlS6>BrxVA<JYQu1Z_*M<

-- 
MST

Re: [Qemu-devel] [PULL 01/21] virtio-net: enable configurable tx queue size

[Wei Wang]

On 07/04/2017 03:44 AM, Michael S. Tsirkin wrote:
> From: Wei Wang <wei.w.wang@intel.com>
>
> This patch enables the virtio-net tx queue size to be configurable
> between 256 (the default queue size) and 1024 by the user when the
> vhost-user backend is used.
>
> Currently, the maximum tx queue size for other backends is 512 due
> to the following limitations:
> - QEMU backend: the QEMU backend implementation in some cases may
> send 1024+1 iovs to writev.
> - Vhost_net backend: there are possibilities that the guest sends
> a vring_desc of memory which crosses a MemoryRegion thereby
> generating more than 1024 iovs after translation from guest-physical
> address in the backend.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   include/hw/virtio/virtio-net.h |  1 +
>   hw/net/virtio-net.c            | 32 ++++++++++++++++++++++++++++++--
>   2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index 602b486..b81b6a4 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -36,6 +36,7 @@ typedef struct virtio_net_conf
>       int32_t txburst;
>       char *tx;
>       uint16_t rx_queue_size;
> +    uint16_t tx_queue_size;
>       uint16_t mtu;
>   } virtio_net_conf;
>   
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 91eddaf..a1fc0db 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -34,8 +34,11 @@
>   
>   /* previously fixed value */
>   #define VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE 256
> +#define VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE 256
> +
>   /* for now, only allow larger queues; with virtio-1, guest can downsize */
>   #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
> +#define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
>   
>   /*
>    * Calculate the number of bytes up to and including the given 'field' of
> @@ -1508,15 +1511,18 @@ static void virtio_net_add_queue(VirtIONet *n, int index)
>   
>       n->vqs[index].rx_vq = virtio_add_queue(vdev, n->net_conf.rx_queue_size,
>                                              virtio_net_handle_rx);
> +
>       if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) {
>           n->vqs[index].tx_vq =
> -            virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer);
> +            virtio_add_queue(vdev, n->net_conf.tx_queue_size,
> +                             virtio_net_handle_tx_timer);
>           n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                                 virtio_net_tx_timer,
>                                                 &n->vqs[index]);
>       } else {
>           n->vqs[index].tx_vq =
> -            virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh);
> +            virtio_add_queue(vdev, n->net_conf.tx_queue_size,
> +                             virtio_net_handle_tx_bh);
>           n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
>       }
>   
> @@ -1927,6 +1933,17 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> +    if (n->net_conf.tx_queue_size < VIRTIO_NET_TX_QUEUE_MIN_SIZE ||
> +        n->net_conf.tx_queue_size > VIRTQUEUE_MAX_SIZE ||
> +        !is_power_of_2(n->net_conf.tx_queue_size)) {
> +        error_setg(errp, "Invalid tx_queue_size (= %" PRIu16 "), "
> +                   "must be a power of 2 between %d and %d",
> +                   n->net_conf.tx_queue_size, VIRTIO_NET_TX_QUEUE_MIN_SIZE,
> +                   VIRTQUEUE_MAX_SIZE);
> +        virtio_cleanup(vdev);
> +        return;
> +    }
> +
>       n->max_queues = MAX(n->nic_conf.peers.queues, 1);
>       if (n->max_queues * 2 + 1 > VIRTIO_QUEUE_MAX) {
>           error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
> @@ -1947,6 +1964,15 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>           error_report("Defaulting to \"bh\"");
>       }
>   
> +    /*
> +     * Currently, backends other than vhost-user don't support 1024 queue
> +     * size.
> +     */
> +    if (n->net_conf.tx_queue_size == VIRTQUEUE_MAX_SIZE &&
> +        n->nic_conf.peers.ncs[0]->info->type != NET_CLIENT_DRIVER_VHOST_USER) {
> +        n->net_conf.tx_queue_size = VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE;
> +    }
> +
>       for (i = 0; i < n->max_queues; i++) {
>           virtio_net_add_queue(n, i);
>       }
> @@ -2106,6 +2132,8 @@ static Property virtio_net_properties[] = {
>       DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
>       DEFINE_PROP_UINT16("rx_queue_size", VirtIONet, net_conf.rx_queue_size,
>                          VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE),
> +    DEFINE_PROP_UINT16("tx_queue_size", VirtIONet, net_conf.tx_queue_size,
> +                       VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE),
>       DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
>       DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
>                        true),

Btw, users also expect the support of configuring the tx queue size could
be added to libvirt soon.

Best,
Wei

Re: [Qemu-devel] [PULL 00/21] pc, acpi, pci, virtio: fixes, cleanups, features, tests

[Peter Maydell]

On 3 July 2017 at 20:44, Michael S. Tsirkin <mst@redhat.com> wrote:
> The following changes since commit fd479c60f5766f7fb247ad146b9e3c33d03d2055:
>
>   Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20170603' into staging (2017-07-03 09:54:32 +0100)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to d2f9ca94165b10c51d6d6cae5fe1cadf1ca42076:
>
>   i386/acpi: update expected acpi files (2017-07-03 22:42:36 +0300)
>
> ----------------------------------------------------------------
> pc, acpi, pci, virtio: fixes, cleanups, features, tests
>
> Some fixes and cleanups. New tests.
> Configurable tx queue size for virtio-net.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Peter Maydell]

On 3 July 2017 at 20:45, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Ben Warren <ben@skyportsystems.com>
>
> The following tests are implemented:
> * test that a GUID passed in by command line is propagated to the guest.
>   Read the GUID from guest memory
> * test that the "auto" argument to the GUID generates a valid GUID, as
>   seen by the guest.
> * test that a GUID passed in can be queried from the monitor
>
>   This patch is loosely based on a previous patch from:
>   Gal Hammer <ghammer@redhat.com>  and Igor Mammedov <imammedo@redhat.com>
>
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Hi -- this test seems to intermittently fail:

TEST: tests/vmgenid-test... (pid=15466)
  /i386/vmgenid/vmgenid/set-guid:                                      **
ERROR:/home/peter.maydell/qemu/tests/vmgenid-test.c:65:acpi_find_vgia:
assertion failed (ACPI_ASSERT_CMP
_str == "RSDT"): ("" == "RSDT")
FAIL
GTester: last random seed: R02S27021da892f2124a377287729b848ff8
(pid=15470)
  /i386/vmgenid/vmgenid/set-guid-auto:                                 OK
  /i386/vmgenid/vmgenid/query-monitor:                                 OK
FAIL: tests/vmgenid-test

(that's an aarch32 build).


Earlier in the run I see there was also a warning from acpi-test:

  /i386/acpi/q35/memhp:
"kvm" accelerator not found.

Looking for expected file 'tests/acpi-test-data/q35/DSDT.memhp'

Using expected file 'tests/acpi-test-data/q35/DSDT.memhp'

Looking for expected file 'tests/acpi-test-data/q35/APIC.memhp'

Looking for expected file 'tests/acpi-test-data/q35/APIC'

Using expected file 'tests/acpi-test-data/q35/APIC'

Looking for expected file 'tests/acpi-test-data/q35/HPET.memhp'

Looking for expected file 'tests/acpi-test-data/q35/HPET'

Using expected file 'tests/acpi-test-data/q35/HPET'

Looking for expected file 'tests/acpi-test-data/q35/SRAT.memhp'

Using expected file 'tests/acpi-test-data/q35/SRAT.memhp'

Looking for expected file 'tests/acpi-test-data/q35/SLIT.memhp'

Using expected file 'tests/acpi-test-data/q35/SLIT.memhp'

Looking for expected file 'tests/acpi-test-data/q35/MCFG.memhp'

Looking for expected file 'tests/acpi-test-data/q35/MCFG'

Using expected file 'tests/acpi-test-data/q35/MCFG'
acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-E3X12Y.dsl,
aml:/tmp/aml-GIX12Y], Expected [asl:/tmp/asl-5Z502Y.dsl,
aml:tests/acpi-test-data/q35/DSDT.memhp].
acpi-test: Warning. not showing difference since no diff utility is
specified. Set 'DIFF' environment variable to a preferred diff utility
and run 'make V=1 check' again to see ASL difference.OK

(Shouldn't a DSDT mismatch cause a test failure?)


thanks
-- PMM

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Ben Warren]

Hi Peter,
> On Jul 11, 2017, at 6:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On 3 July 2017 at 20:45, Michael S. Tsirkin <mst@redhat.com> wrote:
>> From: Ben Warren <ben@skyportsystems.com>
>> 
>> The following tests are implemented:
>> * test that a GUID passed in by command line is propagated to the guest.
>>  Read the GUID from guest memory
>> * test that the "auto" argument to the GUID generates a valid GUID, as
>>  seen by the guest.
>> * test that a GUID passed in can be queried from the monitor
>> 
>>  This patch is loosely based on a previous patch from:
>>  Gal Hammer <ghammer@redhat.com>  and Igor Mammedov <imammedo@redhat.com>
>> 
>> Signed-off-by: Ben Warren <ben@skyportsystems.com>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Hi -- this test seems to intermittently fail:
> 
> TEST: tests/vmgenid-test... (pid=15466)
>  /i386/vmgenid/vmgenid/set-guid:                                      **
> ERROR:/home/peter.maydell/qemu/tests/vmgenid-test.c:65:acpi_find_vgia:
> assertion failed (ACPI_ASSERT_CMP
> _str == "RSDT"): ("" == "RSDT")
> FAIL
> GTester: last random seed: R02S27021da892f2124a377287729b848ff8
> (pid=15470)
>  /i386/vmgenid/vmgenid/set-guid-auto:                                 OK
>  /i386/vmgenid/vmgenid/query-monitor:                                 OK
> FAIL: tests/vmgenid-test
> 
> (that's an aarch32 build).
> 
> 
I’ll try to reproduce.  How exactly are the tests called?
> Earlier in the run I see there was also a warning from acpi-test:
> 
>  /i386/acpi/q35/memhp:
> "kvm" accelerator not found.
> 
> Looking for expected file 'tests/acpi-test-data/q35/DSDT.memhp'
> 
> Using expected file 'tests/acpi-test-data/q35/DSDT.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/APIC.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/APIC'
> 
> Using expected file 'tests/acpi-test-data/q35/APIC'
> 
> Looking for expected file 'tests/acpi-test-data/q35/HPET.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/HPET'
> 
> Using expected file 'tests/acpi-test-data/q35/HPET'
> 
> Looking for expected file 'tests/acpi-test-data/q35/SRAT.memhp'
> 
> Using expected file 'tests/acpi-test-data/q35/SRAT.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/SLIT.memhp'
> 
> Using expected file 'tests/acpi-test-data/q35/SLIT.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/MCFG.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/MCFG'
> 
> Using expected file 'tests/acpi-test-data/q35/MCFG'
> acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-E3X12Y.dsl,
> aml:/tmp/aml-GIX12Y], Expected [asl:/tmp/asl-5Z502Y.dsl,
> aml:tests/acpi-test-data/q35/DSDT.memhp].
> acpi-test: Warning. not showing difference since no diff utility is
> specified. Set 'DIFF' environment variable to a preferred diff utility
> and run 'make V=1 check' again to see ASL difference.OK
> 
> (Shouldn't a DSDT mismatch cause a test failure?)
> 
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Peter Maydell]

On 11 July 2017 at 16:07, Ben Warren <ben@skyportsystems.com> wrote:
> Hi Peter,
>> On Jul 11, 2017, at 6:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On 3 July 2017 at 20:45, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> From: Ben Warren <ben@skyportsystems.com>
>>>
>>> The following tests are implemented:
>>> * test that a GUID passed in by command line is propagated to the guest.
>>>  Read the GUID from guest memory
>>> * test that the "auto" argument to the GUID generates a valid GUID, as
>>>  seen by the guest.
>>> * test that a GUID passed in can be queried from the monitor
>>>
>>>  This patch is loosely based on a previous patch from:
>>>  Gal Hammer <ghammer@redhat.com>  and Igor Mammedov <imammedo@redhat.com>
>>>
>>> Signed-off-by: Ben Warren <ben@skyportsystems.com>
>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> Hi -- this test seems to intermittently fail:
>>
>> TEST: tests/vmgenid-test... (pid=15466)
>>  /i386/vmgenid/vmgenid/set-guid:                                      **
>> ERROR:/home/peter.maydell/qemu/tests/vmgenid-test.c:65:acpi_find_vgia:
>> assertion failed (ACPI_ASSERT_CMP
>> _str == "RSDT"): ("" == "RSDT")
>> FAIL
>> GTester: last random seed: R02S27021da892f2124a377287729b848ff8
>> (pid=15470)
>>  /i386/vmgenid/vmgenid/set-guid-auto:                                 OK
>>  /i386/vmgenid/vmgenid/query-monitor:                                 OK
>> FAIL: tests/vmgenid-test
>>
>> (that's an aarch32 build).
>>
>>
> I’ll try to reproduce.  How exactly are the tests called?

'make check V=1', same as usual.

I did a manual run of just this test, with
while QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386 gtester -k
--verbose -m=quick tests/vmgenid-test ;do true ;done

but that didn't seem to fall over, so either the test is very
intermittently flaky, or it depends on load on the machine or
something.

thanks
-- PMM

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Peter Maydell]

On 11 July 2017 at 16:22, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 July 2017 at 16:07, Ben Warren <ben@skyportsystems.com> wrote:
>> Hi Peter,
>>> On Jul 11, 2017, at 6:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Hi -- this test seems to intermittently fail:
>>>
>>> TEST: tests/vmgenid-test... (pid=15466)
>>>  /i386/vmgenid/vmgenid/set-guid:                                      **
>>> ERROR:/home/peter.maydell/qemu/tests/vmgenid-test.c:65:acpi_find_vgia:
>>> assertion failed (ACPI_ASSERT_CMP
>>> _str == "RSDT"): ("" == "RSDT")
>>> FAIL
>>> GTester: last random seed: R02S27021da892f2124a377287729b848ff8
>>> (pid=15470)
>>>  /i386/vmgenid/vmgenid/set-guid-auto:                                 OK
>>>  /i386/vmgenid/vmgenid/query-monitor:                                 OK
>>> FAIL: tests/vmgenid-test
>>>
>>> (that's an aarch32 build).
>>>
>>>
>> I’ll try to reproduce.  How exactly are the tests called?
>
> 'make check V=1', same as usual.

The good news is it's not aarch64-specific. I just hit this on
a build on x86_64 host, gcc, debug build:

  GTESTER check-qtest-x86_64
**
ERROR:/home/petmay01/linaro/qemu-for-merges/tests/vmgenid-test.c:65:acpi_find_vgia:
assertion failed (ACPI_ASSERT_CMP_str == "RSDT"): ("" == "RSDT")
GTester: last random seed: R02S9eefaf38297e67e4f67d5db77989350e
/home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:826:
recipe for target 'check-qtest-x86_64' failed

thanks
-- PMM

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Michael S. Tsirkin]

On Tue, Jul 11, 2017 at 05:49:07PM +0100, Peter Maydell wrote:
> On 11 July 2017 at 16:22, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 11 July 2017 at 16:07, Ben Warren <ben@skyportsystems.com> wrote:
> >> Hi Peter,
> >>> On Jul 11, 2017, at 6:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> Hi -- this test seems to intermittently fail:
> >>>
> >>> TEST: tests/vmgenid-test... (pid=15466)
> >>>  /i386/vmgenid/vmgenid/set-guid:                                      **
> >>> ERROR:/home/peter.maydell/qemu/tests/vmgenid-test.c:65:acpi_find_vgia:
> >>> assertion failed (ACPI_ASSERT_CMP
> >>> _str == "RSDT"): ("" == "RSDT")
> >>> FAIL
> >>> GTester: last random seed: R02S27021da892f2124a377287729b848ff8
> >>> (pid=15470)
> >>>  /i386/vmgenid/vmgenid/set-guid-auto:                                 OK
> >>>  /i386/vmgenid/vmgenid/query-monitor:                                 OK
> >>> FAIL: tests/vmgenid-test
> >>>
> >>> (that's an aarch32 build).
> >>>
> >>>
> >> I’ll try to reproduce.  How exactly are the tests called?
> >
> > 'make check V=1', same as usual.
> 
> The good news is it's not aarch64-specific. I just hit this on
> a build on x86_64 host, gcc, debug build:
> 
>   GTESTER check-qtest-x86_64
> **
> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/vmgenid-test.c:65:acpi_find_vgia:
> assertion failed (ACPI_ASSERT_CMP_str == "RSDT"): ("" == "RSDT")
> GTester: last random seed: R02S9eefaf38297e67e4f67d5db77989350e
> /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:826:
> recipe for target 'check-qtest-x86_64' failed
> 
> thanks
> -- PMM

Couldn't reproduce here. I suspect VM didn't start at all.
Am I right to assume this is without KVM?

-- 
MST

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Peter Maydell]

On 11 July 2017 at 20:10, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Jul 11, 2017 at 05:49:07PM +0100, Peter Maydell wrote:
>> The good news is it's not aarch64-specific. I just hit this on
>> a build on x86_64 host, gcc, debug build:
>>
>>   GTESTER check-qtest-x86_64
>> **
>> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/vmgenid-test.c:65:acpi_find_vgia:
>> assertion failed (ACPI_ASSERT_CMP_str == "RSDT"): ("" == "RSDT")
>> GTester: last random seed: R02S9eefaf38297e67e4f67d5db77989350e
>> /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:826:
>> recipe for target 'check-qtest-x86_64' failed

> Couldn't reproduce here. I suspect VM didn't start at all.
> Am I right to assume this is without KVM?

On aarch64 host, definitely without KVM. On x86-64 host,
I think it is without KVM but am not totally sure.

It may be one of those cases that only triggers if the
host is heavily loaded at the time the test is running.

thanks
-- PMM

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Laszlo Ersek]

On 07/11/17 22:42, Peter Maydell wrote:
> On 11 July 2017 at 20:10, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Tue, Jul 11, 2017 at 05:49:07PM +0100, Peter Maydell wrote:
>>> The good news is it's not aarch64-specific. I just hit this on
>>> a build on x86_64 host, gcc, debug build:
>>>
>>>   GTESTER check-qtest-x86_64
>>> **
>>> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/vmgenid-test.c:65:acpi_find_vgia:
>>> assertion failed (ACPI_ASSERT_CMP_str == "RSDT"): ("" == "RSDT")
>>> GTester: last random seed: R02S9eefaf38297e67e4f67d5db77989350e
>>> /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:826:
>>> recipe for target 'check-qtest-x86_64' failed
> 
>> Couldn't reproduce here. I suspect VM didn't start at all.
>> Am I right to assume this is without KVM?
> 
> On aarch64 host, definitely without KVM. On x86-64 host,
> I think it is without KVM but am not totally sure.
> 
> It may be one of those cases that only triggers if the
> host is heavily loaded at the time the test is running.

(I apologize if the root cause is obvious at this point -- I'm unclear
if the discussion is now about understanding the failure, or about ways
to mitigate it. I'm assuming the former.)

This test can occasionally fail because the test case has to wait until
the guest firmware proceeds far enough with executing the ACPI
linker/loader script. See RSDP_SLEEP_US and RSDP_TRIES_MAX in
acpi_find_vgia(). If the test case pokes at guest RAM "too early", using
monitor commands, then the guest fw will not have yet shaped guest RAM
as required.

(Again, apologies if this has been obvious all along.)

Thanks
Laszlo

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Ben Warren]

Hi Laszlo,

> On Jul 11, 2017, at 3:13 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 07/11/17 22:42, Peter Maydell wrote:
>> On 11 July 2017 at 20:10, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Tue, Jul 11, 2017 at 05:49:07PM +0100, Peter Maydell wrote:
>>>> The good news is it's not aarch64-specific. I just hit this on
>>>> a build on x86_64 host, gcc, debug build:
>>>> 
>>>>  GTESTER check-qtest-x86_64
>>>> **
>>>> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/vmgenid-test.c:65:acpi_find_vgia:
>>>> assertion failed (ACPI_ASSERT_CMP_str == "RSDT"): ("" == "RSDT")
>>>> GTester: last random seed: R02S9eefaf38297e67e4f67d5db77989350e
>>>> /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:826:
>>>> recipe for target 'check-qtest-x86_64' failed
>> 
>>> Couldn't reproduce here. I suspect VM didn't start at all.
>>> Am I right to assume this is without KVM?
>> 
>> On aarch64 host, definitely without KVM. On x86-64 host,
>> I think it is without KVM but am not totally sure.
>> 
>> It may be one of those cases that only triggers if the
>> host is heavily loaded at the time the test is running.
> 
> (I apologize if the root cause is obvious at this point -- I'm unclear
> if the discussion is now about understanding the failure, or about ways
> to mitigate it. I'm assuming the former.)
> 
> This test can occasionally fail because the test case has to wait until
> the guest firmware proceeds far enough with executing the ACPI
> linker/loader script. See RSDP_SLEEP_US and RSDP_TRIES_MAX in
> acpi_find_vgia(). If the test case pokes at guest RAM "too early", using
> monitor commands, then the guest fw will not have yet shaped guest RAM
> as required.
> 
Yes, it’s definitely a setup time problem.  With the values that are checked in, I can’t get it to fail on my setup, but if I wind the numbers down I see the same failure as Peter.  So now we have the ages-old problem of “what new arbitrary value should I use that will not cause false failures but will eventually time out”.  Can you think of an easy way to tell if firmware is running so we can make this more state-driven instead of time-dependent?


> (Again, apologies if this has been obvious all along.)
> 
> Thanks
> Laszlo

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Michael S. Tsirkin]

On Tue, Jul 11, 2017 at 04:43:50PM -0700, Ben Warren wrote:
> Hi Laszlo,
> 
> 
>     On Jul 11, 2017, at 3:13 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
>     On 07/11/17 22:42, Peter Maydell wrote:
> 
>         On 11 July 2017 at 20:10, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>             On Tue, Jul 11, 2017 at 05:49:07PM +0100, Peter Maydell wrote:
> 
>                 The good news is it's not aarch64-specific. I just hit this on
>                 a build on x86_64 host, gcc, debug build:
> 
>                  GTESTER check-qtest-x86_64
>                 **
>                 ERROR:/home/petmay01/linaro/qemu-for-merges/tests/
>                 vmgenid-test.c:65:acpi_find_vgia:
>                 assertion failed (ACPI_ASSERT_CMP_str == "RSDT"): ("" ==
>                 "RSDT")
>                 GTester: last random seed: R02S9eefaf38297e67e4f67d5db77989350e
>                 /home/petmay01/linaro/qemu-for-merges/tests/
>                 Makefile.include:826:
>                 recipe for target 'check-qtest-x86_64' failed
> 
> 
>             Couldn't reproduce here. I suspect VM didn't start at all.
>             Am I right to assume this is without KVM?
> 
> 
>         On aarch64 host, definitely without KVM. On x86-64 host,
>         I think it is without KVM but am not totally sure.
> 
>         It may be one of those cases that only triggers if the
>         host is heavily loaded at the time the test is running.
> 
> 
>     (I apologize if the root cause is obvious at this point -- I'm unclear
>     if the discussion is now about understanding the failure, or about ways
>     to mitigate it. I'm assuming the former.)
> 
>     This test can occasionally fail because the test case has to wait until
>     the guest firmware proceeds far enough with executing the ACPI
>     linker/loader script. See RSDP_SLEEP_US and RSDP_TRIES_MAX in
>     acpi_find_vgia(). If the test case pokes at guest RAM "too early", using
>     monitor commands, then the guest fw will not have yet shaped guest RAM
>     as required.
> 
> 
> Yes, it’s definitely a setup time problem.  With the values that are checked
> in, I can’t get it to fail on my setup, but if I wind the numbers down I see
> the same failure as Peter.  So now we have the ages-old problem of “what new
> arbitrary value should I use that will not cause false failures but will
> eventually time out”.  Can you think of an easy way to tell if firmware is
> running so we can make this more state-driven instead of time-dependent?
> 
> 
> 
>     (Again, apologies if this has been obvious all along.)
> 
>     Thanks
>     Laszlo
> 

I suspect the issue is that io thread runs while CPU thread does not.
How about retrieving the clock id of the VCPU thread
with pthread_getcpuclockid, then getting the time with clock_gettime?
Then keep the current limit but make sure it elapsed in
the VCPU thread.

-- 
MST

Re: [Qemu-devel] [PULL 01/21] virtio-net: enable configurable tx queue size

[Michal Privoznik]

On 07/04/2017 03:44 AM, Wei Wang wrote:
> On 07/04/2017 03:44 AM, Michael S. Tsirkin wrote:
>> From: Wei Wang <wei.w.wang@intel.com>
>>
>> This patch enables the virtio-net tx queue size to be configurable
>> between 256 (the default queue size) and 1024 by the user when the
>> vhost-user backend is used.
>>

<snip/>

> 
> Btw, users also expect the support of configuring the tx queue size could
> be added to libvirt soon.

Indeed. I've just posted patch for that:

https://www.redhat.com/archives/libvir-list/2017-July/msg00433.html

Michal

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Peter Maydell]

On 12 July 2017 at 00:43, Ben Warren <ben@skyportsystems.com> wrote:
> Yes, it’s definitely a setup time problem.  With the values that are checked
> in, I can’t get it to fail on my setup, but if I wind the numbers down I see
> the same failure as Peter.  So now we have the ages-old problem of “what new
> arbitrary value should I use that will not cause false failures but will
> eventually time out”.

Empirically, we already have an answer to this, in the form
of the existing code in tests/boot-sector.c, which is used
by both that test and the bios-tables-test.c code to wait
for the BIOS initialization to complete, and which doesn't
cause false test timeouts in practice.

Can we make this test just use that existing function to
wait for the BIOS to be done, rather than having its own
timeout loop?

thanks
-- PMM

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Laszlo Ersek]

On 07/13/17 12:47, Peter Maydell wrote:
> On 12 July 2017 at 00:43, Ben Warren <ben@skyportsystems.com> wrote:
>> Yes, it’s definitely a setup time problem.  With the values that are checked
>> in, I can’t get it to fail on my setup, but if I wind the numbers down I see
>> the same failure as Peter.  So now we have the ages-old problem of “what new
>> arbitrary value should I use that will not cause false failures but will
>> eventually time out”.
> 
> Empirically, we already have an answer to this, in the form
> of the existing code in tests/boot-sector.c, which is used
> by both that test and the bios-tables-test.c code to wait
> for the BIOS initialization to complete, and which doesn't
> cause false test timeouts in practice.
> 
> Can we make this test just use that existing function to
> wait for the BIOS to be done, rather than having its own
> timeout loop?

This is incredibly cool. Now that I've looked at "tests/boot-sector.c"
(again), I recall having seen it earlier, but I couldn't have remembered
it off-hand.

Perhaps this boot sector code should be factored out and moved to
"tests/acpi-utils".

Marc-André, do you think it would be feasible for the vmcoreinfo unit
test as well? (Which is derived from the vmgenid unit test.)

Thanks!
Laszlo

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Marc-André Lureau]

Hi

On Thu, Jul 13, 2017 at 1:32 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/13/17 12:47, Peter Maydell wrote:
> > On 12 July 2017 at 00:43, Ben Warren <ben@skyportsystems.com> wrote:
> >> Yes, it’s definitely a setup time problem.  With the values that are
> checked
> >> in, I can’t get it to fail on my setup, but if I wind the numbers down
> I see
> >> the same failure as Peter.  So now we have the ages-old problem of
> “what new
> >> arbitrary value should I use that will not cause false failures but will
> >> eventually time out”.
> >
> > Empirically, we already have an answer to this, in the form
> > of the existing code in tests/boot-sector.c, which is used
> > by both that test and the bios-tables-test.c code to wait
> > for the BIOS initialization to complete, and which doesn't
> > cause false test timeouts in practice.
> >
> > Can we make this test just use that existing function to
> > wait for the BIOS to be done, rather than having its own
> > timeout loop?
>
> This is incredibly cool. Now that I've looked at "tests/boot-sector.c"
> (again), I recall having seen it earlier, but I couldn't have remembered
> it off-hand.
>
> Perhaps this boot sector code should be factored out and moved to
> "tests/acpi-utils".
>
> Marc-André, do you think it would be feasible for the vmcoreinfo unit
> test as well? (Which is derived from the vmgenid unit test.)
>

It seems likely.

Ben, are you going to work on the fix?

Thanks
-- 
Marc-André Lureau

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Ben Warren]

Hi,
> On Jul 13, 2017, at 4:51 AM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> 
> Hi
> 
> On Thu, Jul 13, 2017 at 1:32 PM Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> wrote:
> On 07/13/17 12:47, Peter Maydell wrote:
> > On 12 July 2017 at 00:43, Ben Warren <ben@skyportsystems.com <mailto:ben@skyportsystems.com>> wrote:
> >> Yes, it’s definitely a setup time problem.  With the values that are checked
> >> in, I can’t get it to fail on my setup, but if I wind the numbers down I see
> >> the same failure as Peter.  So now we have the ages-old problem of “what new
> >> arbitrary value should I use that will not cause false failures but will
> >> eventually time out”.
> >
> > Empirically, we already have an answer to this, in the form
> > of the existing code in tests/boot-sector.c, which is used
> > by both that test and the bios-tables-test.c code to wait
> > for the BIOS initialization to complete, and which doesn't
> > cause false test timeouts in practice.
> >
> > Can we make this test just use that existing function to
> > wait for the BIOS to be done, rather than having its own
> > timeout loop?
> 
> This is incredibly cool. Now that I've looked at "tests/boot-sector.c"
> (again), I recall having seen it earlier, but I couldn't have remembered
> it off-hand.
> 
> Perhaps this boot sector code should be factored out and moved to
> "tests/acpi-utils".
> 
> Marc-André, do you think it would be feasible for the vmcoreinfo unit
> test as well? (Which is derived from the vmgenid unit test.)
> 
> It seems likely.
> 
> Ben, are you going to work on the fix?
> 
Yes, I will take care of this.  I remember seeing the boot-sector synchronization code before, but didn’t really grok it.  This time I’ll dig deeper.
> Thanks
> -- 
> Marc-André Lureau

—Ben

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Michael S. Tsirkin]

On Thu, Jul 13, 2017 at 06:34:33AM -0700, Ben Warren wrote:
> Hi,
> 
>     On Jul 13, 2017, at 4:51 AM, Marc-André Lureau <marcandre.lureau@gmail.com>
>     wrote:
> 
>     Hi
> 
>     On Thu, Jul 13, 2017 at 1:32 PM Laszlo Ersek <lersek@redhat.com> wrote:
> 
>         On 07/13/17 12:47, Peter Maydell wrote:
>         > On 12 July 2017 at 00:43, Ben Warren <ben@skyportsystems.com> wrote:
>         >> Yes, it’s definitely a setup time problem.  With the values that are
>         checked
>         >> in, I can’t get it to fail on my setup, but if I wind the numbers
>         down I see
>         >> the same failure as Peter.  So now we have the ages-old problem of
>         “what new
>         >> arbitrary value should I use that will not cause false failures but
>         will
>         >> eventually time out”.
>         >
>         > Empirically, we already have an answer to this, in the form
>         > of the existing code in tests/boot-sector.c, which is used
>         > by both that test and the bios-tables-test.c code to wait
>         > for the BIOS initialization to complete, and which doesn't
>         > cause false test timeouts in practice.
>         >
>         > Can we make this test just use that existing function to
>         > wait for the BIOS to be done, rather than having its own
>         > timeout loop?
> 
>         This is incredibly cool. Now that I've looked at "tests/boot-sector.c"
>         (again), I recall having seen it earlier, but I couldn't have
>         remembered
>         it off-hand.
> 
>         Perhaps this boot sector code should be factored out and moved to
>         "tests/acpi-utils".
> 
>         Marc-André, do you think it would be feasible for the vmcoreinfo unit
>         test as well? (Which is derived from the vmgenid unit test.)
> 
> 
>     It seems likely.
> 
>     Ben, are you going to work on the fix?
> 
> 
> Yes, I will take care of this.  I remember seeing the boot-sector
> synchronization code before, but didn’t really grok it.  This time I’ll dig
> deeper.

It's already a library, I don't think you need changes or refactoring.
You can just link boot_sector.c and call boot_sector_test like the pxe
test does. If this passes you know all acpi tables are in memory.

I'll post a patch.

There's no clever logic around timeout there though - right now your
timeout is 10 seconds while boot_sector_test has a timeout of 90
seconds.

So it is still worth improving to check the per-thread clock imho,
but I agree it's best to rework vm gen id test.
While at it, I think it's best to drop the assumption that vmgenid
is the 1st table in the ssdt.

>     Thanks
>     -- 
>     Marc-André Lureau
> 
> 
> —Ben
> 

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Peter Maydell]

On 13 July 2017 at 17:38, Michael S. Tsirkin <mst@redhat.com> wrote:
> It's already a library, I don't think you need changes or refactoring.
> You can just link boot_sector.c and call boot_sector_test like the pxe
> test does. If this passes you know all acpi tables are in memory.
>
> I'll post a patch.

Hi -- you asked me on IRC if I could test that "use boot_sector_test"
patch, but I can't find it in my email archive or in patchwork.
Did you forget to send it, or has it got lost in transit? If you can
provide a pointer to a copy I'll give it a test...

thanks
-- PMM

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Michael S. Tsirkin]

On Fri, Jul 14, 2017 at 02:11:27PM +0100, Peter Maydell wrote:
> On 13 July 2017 at 17:38, Michael S. Tsirkin <mst@redhat.com> wrote:
> > It's already a library, I don't think you need changes or refactoring.
> > You can just link boot_sector.c and call boot_sector_test like the pxe
> > test does. If this passes you know all acpi tables are in memory.
> >
> > I'll post a patch.
> 
> Hi -- you asked me on IRC if I could test that "use boot_sector_test"
> patch, but I can't find it in my email archive or in patchwork.
> Did you forget to send it, or has it got lost in transit? If you can
> provide a pointer to a copy I'll give it a test...
> 
> thanks
> -- PMM

Strange - neither can I. I was sure I sent it.
I just resent - pls take a look.

-- 
MST

Re: [Qemu-devel] [PULL 09/21] pci: Convert to realize

[Eduardo Habkost]

On Mon, Jul 03, 2017 at 10:45:16PM +0300, Michael S. Tsirkin wrote:
> From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> 
> Convert i82801b11, io3130_upstream, io3130_downstream and
> pcie_root_port devices to realize.
> 
> Cc: mst@redhat.com
> Cc: marcel@redhat.com
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
[...]
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index cfe8a36..e706f36 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
>      pci_bridge_reset(qdev);
>  }
>  
> -static int xio3130_downstream_initfn(PCIDevice *d)
> +static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
>  {
[...]
>      pcie_chassis_create(s->chassis);
>      rc = pcie_chassis_add_slot(s);
>      if (rc < 0) {
>          goto err_pcie_cap;

Missing error_setg() call here.  If pcie_chassis_add_slot() fails, realize
won't report an error properly.  Causes crash with:

$ ./x86_64-softmmu/qemu-system-x86_64 -device ioh3420 -device xio3130-downstream
qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/memory.c:2166: memory_region_del_subregion: Assertion `subregion->container == mr' failed.
Aborted (core dumped)


>      }
>  
>      rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
> -                       PCI_ERR_SIZEOF, &err);
> +                       PCI_ERR_SIZEOF, errp);
>      if (rc < 0) {
> -        error_report_err(err);
>          goto err;
>      }
>  
> -    return 0;
> +    return;
>  
>  err:
>      pcie_chassis_del_slot(s);
> @@ -114,7 +113,6 @@ err_msi:
>      msi_uninit(d);
>  err_bridge:
>      pci_bridge_exitfn(d);
> -    return rc;
>  }
>  
[...]

-- 
Eduardo

Re: [Qemu-devel] [PULL 09/21] pci: Convert to realize

[Michael S. Tsirkin]

On Fri, Aug 25, 2017 at 12:17:42PM -0300, Eduardo Habkost wrote:
> On Mon, Jul 03, 2017 at 10:45:16PM +0300, Michael S. Tsirkin wrote:
> > From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> > 
> > Convert i82801b11, io3130_upstream, io3130_downstream and
> > pcie_root_port devices to realize.
> > 
> > Cc: mst@redhat.com
> > Cc: marcel@redhat.com
> > Cc: armbru@redhat.com
> > Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> [...]
> > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> > index cfe8a36..e706f36 100644
> > --- a/hw/pci-bridge/xio3130_downstream.c
> > +++ b/hw/pci-bridge/xio3130_downstream.c
> > @@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
> >      pci_bridge_reset(qdev);
> >  }
> >  
> > -static int xio3130_downstream_initfn(PCIDevice *d)
> > +static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
> >  {
> [...]
> >      pcie_chassis_create(s->chassis);
> >      rc = pcie_chassis_add_slot(s);
> >      if (rc < 0) {
> >          goto err_pcie_cap;
> 
> Missing error_setg() call here.  If pcie_chassis_add_slot() fails, realize
> won't report an error properly.  Causes crash with:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -device ioh3420 -device xio3130-downstream
> qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/memory.c:2166: memory_region_del_subregion: Assertion `subregion->container == mr' failed.
> Aborted (core dumped)
> 
> 
> >      }
> >  
> >      rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
> > -                       PCI_ERR_SIZEOF, &err);
> > +                       PCI_ERR_SIZEOF, errp);
> >      if (rc < 0) {
> > -        error_report_err(err);
> >          goto err;
> >      }
> >  
> > -    return 0;
> > +    return;
> >  
> >  err:
> >      pcie_chassis_del_slot(s);
> > @@ -114,7 +113,6 @@ err_msi:
> >      msi_uninit(d);
> >  err_bridge:
> >      pci_bridge_exitfn(d);
> > -    return rc;
> >  }
> >  
> [...]

Good catch!  Patch?

> -- 
> Eduardo

Re: [Qemu-devel] [PULL 09/21] pci: Convert to realize

[Eduardo Habkost]

On Fri, Aug 25, 2017 at 07:57:57PM +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 25, 2017 at 12:17:42PM -0300, Eduardo Habkost wrote:
> > On Mon, Jul 03, 2017 at 10:45:16PM +0300, Michael S. Tsirkin wrote:
> > > From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> > > 
> > > Convert i82801b11, io3130_upstream, io3130_downstream and
> > > pcie_root_port devices to realize.
> > > 
> > > Cc: mst@redhat.com
> > > Cc: marcel@redhat.com
> > > Cc: armbru@redhat.com
> > > Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> > > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > [...]
> > > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> > > index cfe8a36..e706f36 100644
> > > --- a/hw/pci-bridge/xio3130_downstream.c
> > > +++ b/hw/pci-bridge/xio3130_downstream.c
> > > @@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
> > >      pci_bridge_reset(qdev);
> > >  }
> > >  
> > > -static int xio3130_downstream_initfn(PCIDevice *d)
> > > +static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
> > >  {
> > [...]
> > >      pcie_chassis_create(s->chassis);
> > >      rc = pcie_chassis_add_slot(s);
> > >      if (rc < 0) {
> > >          goto err_pcie_cap;
> > 
> > Missing error_setg() call here.  If pcie_chassis_add_slot() fails, realize
> > won't report an error properly.  Causes crash with:
> > 
> > $ ./x86_64-softmmu/qemu-system-x86_64 -device ioh3420 -device xio3130-downstream
> > qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/memory.c:2166: memory_region_del_subregion: Assertion `subregion->container == mr' failed.
> > Aborted (core dumped)
> > 
> > 
> > >      }
> > >  
> > >      rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
> > > -                       PCI_ERR_SIZEOF, &err);
> > > +                       PCI_ERR_SIZEOF, errp);
> > >      if (rc < 0) {
> > > -        error_report_err(err);
> > >          goto err;
> > >      }
> > >  
> > > -    return 0;
> > > +    return;
> > >  
> > >  err:
> > >      pcie_chassis_del_slot(s);
> > > @@ -114,7 +113,6 @@ err_msi:
> > >      msi_uninit(d);
> > >  err_bridge:
> > >      pci_bridge_exitfn(d);
> > > -    return rc;
> > >  }
> > >  
> > [...]
> 
> Good catch!  Patch?

I will send one later today.

-- 
Eduardo