[Qemu-devel] [PATCH v2 0/5] Fine-tune device capabilities

[Qemu-devel] [PATCH v2 0/5] Fine-tune device capabilities

[Shmulik Ladkani]

Various fixes to what the vmxnet3 device reports in its PCI
configuration space, in order to be aligned with VMware virtual hardware
exposed by ESXi/Workstation.

Since v1: Added migration compatability, per Jason Wang's comment

Shmulik Ladkani (5):
  vmxnet3: Change offsets of msi/msix pci capabilities
  vmxnet3: Change the offset of the MSIX PBA table
  vmxnet3: coding: Introduce VMXNET3Class
  vmxnet3: The vmxnet3 device is a PCIE endpoint
  vmxnet3: Report the Device Serial Number capability

 hw/net/vmxnet3.c    | 114 +++++++++++++++++++++++++++++++++++++++++++++++++---
 include/hw/compat.h |   8 ++++
 2 files changed, 116 insertions(+), 6 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/5] vmxnet3: Change offsets of msi/msix pci capabilities

[Shmulik Ladkani]

Place device reported PCI capabilities at the same offsets as placed by
the VMware virtual hardware: MSI at [84], MSI-X at [9c].

For compatability, preserve old offsets using 'x-old-msi-offsets' toggle.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
---
 hw/net/vmxnet3.c    | 20 +++++++++++++++++---
 include/hw/compat.h |  4 ++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 5e3a233..1985dcf 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -36,6 +36,16 @@
 #define VMXNET3_MSIX_BAR_SIZE 0x2000
 #define MIN_BUF_SIZE 60
 
+/* Compatability flags for migration */
+#define VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT 0
+#define VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS \
+    (1 << VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT)
+
+#define VMXNET3_MSI_OFFSET(s) \
+    ((s)->compat_flags & VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS ? 0x50 : 0x84)
+#define VMXNET3_MSIX_OFFSET(s) \
+    ((s)->compat_flags & VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS ? 0 : 0x9c)
+
 #define VMXNET3_BAR0_IDX      (0)
 #define VMXNET3_BAR1_IDX      (1)
 #define VMXNET3_MSIX_BAR_IDX  (2)
@@ -313,6 +323,9 @@ typedef struct {
         MACAddr *mcast_list;
         uint32_t mcast_list_len;
         uint32_t mcast_list_buff_size; /* needed for live migration. */
+
+        /* Compatability flags for migration */
+        uint32_t compat_flags;
 } VMXNET3State;
 
 /* Interrupt management */
@@ -2103,7 +2116,7 @@ vmxnet3_init_msix(VMXNET3State *s)
                         VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
                         &s->msix_bar,
                         VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA,
-                        0);
+                        VMXNET3_MSIX_OFFSET(s));
 
     if (0 > res) {
         VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
@@ -2131,7 +2144,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
     }
 }
 
-#define VMXNET3_MSI_OFFSET        (0x50)
 #define VMXNET3_USE_64BIT         (true)
 #define VMXNET3_PER_VECTOR_MASK   (false)
 
@@ -2141,7 +2153,7 @@ vmxnet3_init_msi(VMXNET3State *s)
     PCIDevice *d = PCI_DEVICE(s);
     int res;
 
-    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
+    res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
                    VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
     if (0 > res) {
         VMW_WRPRN("Failed to initialize MSI, error %d", res);
@@ -2552,6 +2564,8 @@ static const VMStateDescription vmstate_vmxnet3 = {
 
 static Property vmxnet3_properties[] = {
     DEFINE_NIC_PROPERTIES(VMXNET3State, conf),
+    DEFINE_PROP_BIT("x-old-msi-offsets", VMXNET3State, compat_flags,
+                    VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index d0b1c4f..01e326d 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -18,6 +18,10 @@
             .driver   = "virtio-pci",\
             .property = "migrate-extra",\
             .value    = "off",\
+        },{\
+            .driver   = "vmxnet3",\
+            .property = "x-old-msi-offsets",\
+            .value    = "on",\
         },
 
 #define HW_COMPAT_2_3 \
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/5] vmxnet3: Change the offset of the MSIX PBA table

[Shmulik Ladkani]

Place the PBA table at 0x1000, as placed by VMware virtual hardware.

The 'x-old-msi-offsets' property is used for backwards compatability.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
---
 hw/net/vmxnet3.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 1985dcf..b176138 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -51,7 +51,8 @@
 #define VMXNET3_MSIX_BAR_IDX  (2)
 
 #define VMXNET3_OFF_MSIX_TABLE (0x000)
-#define VMXNET3_OFF_MSIX_PBA   (0x800)
+#define VMXNET3_OFF_MSIX_PBA(s) \
+    ((s)->compat_flags & VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS ? 0x800 : 0x1000)
 
 /* Link speed in Mbps should be shifted by 16 */
 #define VMXNET3_LINK_SPEED      (1000 << 16)
@@ -2115,7 +2116,7 @@ vmxnet3_init_msix(VMXNET3State *s)
                         &s->msix_bar,
                         VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
                         &s->msix_bar,
-                        VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA,
+                        VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s),
                         VMXNET3_MSIX_OFFSET(s));
 
     if (0 > res) {
-- 
1.9.1

[Qemu-devel] [PATCH v2 3/5] vmxnet3: coding: Introduce VMXNET3Class

[Shmulik Ladkani]

Introduce a class type for vmxnet3, and the usual
DEVICE_CLASS/DEVICE_GET_CLASS macros.

No semantic change.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
---
 hw/net/vmxnet3.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index b176138..d007314 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -119,9 +119,18 @@
 
 #define VMXNET_FLAG_IS_SET(field, flag) (((field) & (flag)) == (flag))
 
+typedef struct VMXNET3Class {
+    PCIDeviceClass parent_class;
+} VMXNET3Class;
+
 #define TYPE_VMXNET3 "vmxnet3"
 #define VMXNET3(obj) OBJECT_CHECK(VMXNET3State, (obj), TYPE_VMXNET3)
 
+#define VMXNET3_DEVICE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(VMXNET3Class, (klass), TYPE_VMXNET3)
+#define VMXNET3_DEVICE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(VMXNET3Class, (obj), TYPE_VMXNET3)
+
 /* Cyclic ring abstraction */
 typedef struct {
     hwaddr pa;
@@ -2593,6 +2602,7 @@ static void vmxnet3_class_init(ObjectClass *class, void *data)
 static const TypeInfo vmxnet3_info = {
     .name          = TYPE_VMXNET3,
     .parent        = TYPE_PCI_DEVICE,
+    .class_size    = sizeof(VMXNET3Class),
     .instance_size = sizeof(VMXNET3State),
     .class_init    = vmxnet3_class_init,
     .instance_init = vmxnet3_instance_init,
-- 
1.9.1

[Qemu-devel] [PATCH v2 4/5] vmxnet3: The vmxnet3 device is a PCIE endpoint

[Shmulik Ladkani]

Report the 'express endpoint' capability if on a PCIE bus.

The 'x-disable-pcie' property is used for backwards compatability.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
---
 hw/net/vmxnet3.c    | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 include/hw/compat.h |  4 ++++
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index d007314..1d0fb66 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -40,7 +40,11 @@
 #define VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT 0
 #define VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS \
     (1 << VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT)
+#define VMXNET3_COMPAT_FLAG_DISABLE_PCIE_BIT 1
+#define VMXNET3_COMPAT_FLAG_DISABLE_PCIE \
+    (1 << VMXNET3_COMPAT_FLAG_DISABLE_PCIE_BIT)
 
+#define VMXNET3_EXP_EP_OFFSET (0x48)
 #define VMXNET3_MSI_OFFSET(s) \
     ((s)->compat_flags & VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS ? 0x50 : 0x84)
 #define VMXNET3_MSIX_OFFSET(s) \
@@ -121,6 +125,7 @@
 
 typedef struct VMXNET3Class {
     PCIDeviceClass parent_class;
+    DeviceRealize parent_dc_realize;
 } VMXNET3Class;
 
 #define TYPE_VMXNET3 "vmxnet3"
@@ -2257,6 +2262,10 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
 
     vmxnet3_net_init(s);
 
+    if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus)) {
+        pcie_endpoint_cap_init(pci_dev, VMXNET3_EXP_EP_OFFSET);
+    }
+
     register_savevm(dev, "vmxnet3-msix", -1, 1,
                     vmxnet3_msix_save, vmxnet3_msix_load, s);
 }
@@ -2526,6 +2535,29 @@ static const VMStateInfo int_state_info = {
     .put = vmxnet3_put_int_state
 };
 
+static bool vmxnet3_vmstate_need_pcie_device(void *opaque)
+{
+    VMXNET3State *s = VMXNET3(opaque);
+
+    return !(s->compat_flags & VMXNET3_COMPAT_FLAG_DISABLE_PCIE);
+}
+
+static bool vmxnet3_vmstate_test_pci_device(void *opaque, int version_id)
+{
+    return !vmxnet3_vmstate_need_pcie_device(opaque);
+}
+
+static const VMStateDescription vmstate_vmxnet3_pcie_device = {
+    .name = "vmxnet3/pcie",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = vmxnet3_vmstate_need_pcie_device,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCIE_DEVICE(parent_obj, VMXNET3State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_vmxnet3 = {
     .name = "vmxnet3",
     .version_id = 1,
@@ -2533,7 +2565,9 @@ static const VMStateDescription vmstate_vmxnet3 = {
     .pre_save = vmxnet3_pre_save,
     .post_load = vmxnet3_post_load,
     .fields = (VMStateField[]) {
-            VMSTATE_PCI_DEVICE(parent_obj, VMXNET3State),
+            VMSTATE_STRUCT_TEST(parent_obj, VMXNET3State,
+                                vmxnet3_vmstate_test_pci_device, 0,
+                                vmstate_pci_device, PCIDevice),
             VMSTATE_BOOL(rx_packets_compound, VMXNET3State),
             VMSTATE_BOOL(rx_vlan_stripping, VMXNET3State),
             VMSTATE_BOOL(lro_supported, VMXNET3State),
@@ -2568,6 +2602,7 @@ static const VMStateDescription vmstate_vmxnet3 = {
     },
     .subsections = (const VMStateDescription*[]) {
         &vmxstate_vmxnet3_mcast_list,
+        &vmstate_vmxnet3_pcie_device,
         NULL
     }
 };
@@ -2576,13 +2611,29 @@ static Property vmxnet3_properties[] = {
     DEFINE_NIC_PROPERTIES(VMXNET3State, conf),
     DEFINE_PROP_BIT("x-old-msi-offsets", VMXNET3State, compat_flags,
                     VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT, false),
+    DEFINE_PROP_BIT("x-disable-pcie", VMXNET3State, compat_flags,
+                    VMXNET3_COMPAT_FLAG_DISABLE_PCIE_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void vmxnet3_realize(DeviceState *qdev, Error **errp)
+{
+    VMXNET3Class *vc = VMXNET3_DEVICE_GET_CLASS(qdev);
+    PCIDevice *pci_dev = PCI_DEVICE(qdev);
+    VMXNET3State *s = VMXNET3(qdev);
+
+    if (!(s->compat_flags & VMXNET3_COMPAT_FLAG_DISABLE_PCIE)) {
+        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
+    }
+
+    vc->parent_dc_realize(qdev, errp);
+}
+
 static void vmxnet3_class_init(ObjectClass *class, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(class);
     PCIDeviceClass *c = PCI_DEVICE_CLASS(class);
+    VMXNET3Class *vc = VMXNET3_DEVICE_CLASS(class);
 
     c->realize = vmxnet3_pci_realize;
     c->exit = vmxnet3_pci_uninit;
@@ -2592,6 +2643,8 @@ static void vmxnet3_class_init(ObjectClass *class, void *data)
     c->class_id = PCI_CLASS_NETWORK_ETHERNET;
     c->subsystem_vendor_id = PCI_VENDOR_ID_VMWARE;
     c->subsystem_id = PCI_DEVICE_ID_VMWARE_VMXNET3;
+    vc->parent_dc_realize = dc->realize;
+    dc->realize = vmxnet3_realize;
     dc->desc = "VMWare Paravirtualized Ethernet v3";
     dc->reset = vmxnet3_qdev_reset;
     dc->vmsd = &vmstate_vmxnet3;
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 01e326d..642b082 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -22,6 +22,10 @@
             .driver   = "vmxnet3",\
             .property = "x-old-msi-offsets",\
             .value    = "on",\
+        },{\
+            .driver   = "vmxnet3",\
+            .property = "x-disable-pcie",\
+            .value    = "on",\
         },
 
 #define HW_COMPAT_2_3 \
-- 
1.9.1

[Qemu-devel] [PATCH v2 5/5] vmxnet3: Report the Device Serial Number capability

[Shmulik Ladkani]

Report the DSN extended PCI capability at 0x100.
DSN value is a transformation of device MAC address, as calculated
by VMware virtual hardware.

DSN is reported only if device is pcie.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
---
 hw/net/vmxnet3.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 1d0fb66..4fb50d8 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -49,6 +49,7 @@
     ((s)->compat_flags & VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS ? 0x50 : 0x84)
 #define VMXNET3_MSIX_OFFSET(s) \
     ((s)->compat_flags & VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS ? 0 : 0x9c)
+#define VMXNET3_DSN_OFFSET     (0x100)
 
 #define VMXNET3_BAR0_IDX      (0)
 #define VMXNET3_BAR1_IDX      (1)
@@ -2225,6 +2226,22 @@ static const MemoryRegionOps b1_ops = {
     },
 };
 
+static uint8_t *vmxnet3_device_serial_num(VMXNET3State *s)
+{
+    static uint64_t dsn_payload;
+    uint8_t *dsnp = (uint8_t *)&dsn_payload;
+
+    dsnp[0] = 0xfe;
+    dsnp[1] = s->conf.macaddr.a[3];
+    dsnp[2] = s->conf.macaddr.a[4];
+    dsnp[3] = s->conf.macaddr.a[5];
+    dsnp[4] = s->conf.macaddr.a[0];
+    dsnp[5] = s->conf.macaddr.a[1];
+    dsnp[6] = s->conf.macaddr.a[2];
+    dsnp[7] = 0xff;
+    return dsnp;
+}
+
 static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
 {
     DeviceState *dev = DEVICE(pci_dev);
@@ -2262,8 +2279,15 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
 
     vmxnet3_net_init(s);
 
-    if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus)) {
-        pcie_endpoint_cap_init(pci_dev, VMXNET3_EXP_EP_OFFSET);
+    if (pci_is_express(pci_dev)) {
+        if (pci_bus_is_express(pci_dev->bus)) {
+            pcie_endpoint_cap_init(pci_dev, VMXNET3_EXP_EP_OFFSET);
+        }
+
+        pcie_add_capability(pci_dev, PCI_EXT_CAP_ID_DSN, 0x1,
+                            VMXNET3_DSN_OFFSET, PCI_EXT_CAP_DSN_SIZEOF);
+        memcpy(pci_dev->config + VMXNET3_DSN_OFFSET + 4,
+               vmxnet3_device_serial_num(s), sizeof(uint64_t));
     }
 
     register_savevm(dev, "vmxnet3-msix", -1, 1,
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 1/5] vmxnet3: Change offsets of msi/msix pci capabilities

[Jason Wang]

On 12/02/2015 11:26 PM, Shmulik Ladkani wrote:
> Place device reported PCI capabilities at the same offsets as placed by
> the VMware virtual hardware: MSI at [84], MSI-X at [9c].
>
> For compatability, preserve old offsets using 'x-old-msi-offsets' toggle.
>
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
> ---
>  hw/net/vmxnet3.c    | 20 +++++++++++++++++---
>  include/hw/compat.h |  4 ++++
>  2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 5e3a233..1985dcf 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -36,6 +36,16 @@
>  #define VMXNET3_MSIX_BAR_SIZE 0x2000
>  #define MIN_BUF_SIZE 60
>  
> +/* Compatability flags for migration */
> +#define VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT 0
> +#define VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS \
> +    (1 << VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT)
> +
> +#define VMXNET3_MSI_OFFSET(s) \
> +    ((s)->compat_flags & VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS ? 0x50 : 0x84)
> +#define VMXNET3_MSIX_OFFSET(s) \
> +    ((s)->compat_flags & VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS ? 0 : 0x9c)
> +
>  #define VMXNET3_BAR0_IDX      (0)
>  #define VMXNET3_BAR1_IDX      (1)
>  #define VMXNET3_MSIX_BAR_IDX  (2)
> @@ -313,6 +323,9 @@ typedef struct {
>          MACAddr *mcast_list;
>          uint32_t mcast_list_len;
>          uint32_t mcast_list_buff_size; /* needed for live migration. */
> +
> +        /* Compatability flags for migration */
> +        uint32_t compat_flags;
>  } VMXNET3State;
>  
>  /* Interrupt management */
> @@ -2103,7 +2116,7 @@ vmxnet3_init_msix(VMXNET3State *s)
>                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
>                          &s->msix_bar,
>                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA,
> -                        0);
> +                        VMXNET3_MSIX_OFFSET(s));
>  
>      if (0 > res) {
>          VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
> @@ -2131,7 +2144,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>      }
>  }
>  
> -#define VMXNET3_MSI_OFFSET        (0x50)
>  #define VMXNET3_USE_64BIT         (true)
>  #define VMXNET3_PER_VECTOR_MASK   (false)
>  
> @@ -2141,7 +2153,7 @@ vmxnet3_init_msi(VMXNET3State *s)
>      PCIDevice *d = PCI_DEVICE(s);
>      int res;
>  
> -    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
> +    res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
>                     VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>      if (0 > res) {
>          VMW_WRPRN("Failed to initialize MSI, error %d", res);
> @@ -2552,6 +2564,8 @@ static const VMStateDescription vmstate_vmxnet3 = {
>  
>  static Property vmxnet3_properties[] = {
>      DEFINE_NIC_PROPERTIES(VMXNET3State, conf),
> +    DEFINE_PROP_BIT("x-old-msi-offsets", VMXNET3State, compat_flags,
> +                    VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index d0b1c4f..01e326d 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -18,6 +18,10 @@
>              .driver   = "virtio-pci",\
>              .property = "migrate-extra",\
>              .value    = "off",\
> +        },{\
> +            .driver   = "vmxnet3",\
> +            .property = "x-old-msi-offsets",\
> +            .value    = "on",\
>          },

To have a better bisection behavior, we'd better enable this and compat
it in patch 2.

>  
>  #define HW_COMPAT_2_3 \

Re: [Qemu-devel] [PATCH v2 4/5] vmxnet3: The vmxnet3 device is a PCIE endpoint

[Jason Wang]

On 12/02/2015 11:26 PM, Shmulik Ladkani wrote:
> Report the 'express endpoint' capability if on a PCIE bus.
>
> The 'x-disable-pcie' property is used for backwards compatability.
>
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
> ---
>  hw/net/vmxnet3.c    | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/hw/compat.h |  4 ++++
>  2 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index d007314..1d0fb66 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -40,7 +40,11 @@
>  #define VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT 0
>  #define VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS \
>      (1 << VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT)
> +#define VMXNET3_COMPAT_FLAG_DISABLE_PCIE_BIT 1
> +#define VMXNET3_COMPAT_FLAG_DISABLE_PCIE \
> +    (1 << VMXNET3_COMPAT_FLAG_DISABLE_PCIE_BIT)
>  
> +#define VMXNET3_EXP_EP_OFFSET (0x48)
>  #define VMXNET3_MSI_OFFSET(s) \
>      ((s)->compat_flags & VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS ? 0x50 : 0x84)
>  #define VMXNET3_MSIX_OFFSET(s) \
> @@ -121,6 +125,7 @@
>  
>  typedef struct VMXNET3Class {
>      PCIDeviceClass parent_class;
> +    DeviceRealize parent_dc_realize;
>  } VMXNET3Class;
>  
>  #define TYPE_VMXNET3 "vmxnet3"
> @@ -2257,6 +2262,10 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>  
>      vmxnet3_net_init(s);
>  
> +    if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus)) {

Looks like pci_bus_is_express() has been checked in
pcie_endpoint_cap_init().

> +        pcie_endpoint_cap_init(pci_dev, VMXNET3_EXP_EP_OFFSET);
> +    }
> +
>      register_savevm(dev, "vmxnet3-msix", -1, 1,
>                      vmxnet3_msix_save, vmxnet3_msix_load, s);
>  }
> @@ -2526,6 +2535,29 @@ static const VMStateInfo int_state_info = {
>      .put = vmxnet3_put_int_state
>  };
>  
> +static bool vmxnet3_vmstate_need_pcie_device(void *opaque)
> +{
> +    VMXNET3State *s = VMXNET3(opaque);
> +
> +    return !(s->compat_flags & VMXNET3_COMPAT_FLAG_DISABLE_PCIE);
> +}
> +
> +static bool vmxnet3_vmstate_test_pci_device(void *opaque, int version_id)
> +{
> +    return !vmxnet3_vmstate_need_pcie_device(opaque);
> +}
> +
> +static const VMStateDescription vmstate_vmxnet3_pcie_device = {
> +    .name = "vmxnet3/pcie",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = vmxnet3_vmstate_need_pcie_device,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCIE_DEVICE(parent_obj, VMXNET3State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_vmxnet3 = {
>      .name = "vmxnet3",
>      .version_id = 1,
> @@ -2533,7 +2565,9 @@ static const VMStateDescription vmstate_vmxnet3 = {
>      .pre_save = vmxnet3_pre_save,
>      .post_load = vmxnet3_post_load,
>      .fields = (VMStateField[]) {
> -            VMSTATE_PCI_DEVICE(parent_obj, VMXNET3State),
> +            VMSTATE_STRUCT_TEST(parent_obj, VMXNET3State,
> +                                vmxnet3_vmstate_test_pci_device, 0,
> +                                vmstate_pci_device, PCIDevice),
>              VMSTATE_BOOL(rx_packets_compound, VMXNET3State),
>              VMSTATE_BOOL(rx_vlan_stripping, VMXNET3State),
>              VMSTATE_BOOL(lro_supported, VMXNET3State),
> @@ -2568,6 +2602,7 @@ static const VMStateDescription vmstate_vmxnet3 = {
>      },
>      .subsections = (const VMStateDescription*[]) {
>          &vmxstate_vmxnet3_mcast_list,
> +        &vmstate_vmxnet3_pcie_device,
>          NULL
>      }
>  };
> @@ -2576,13 +2611,29 @@ static Property vmxnet3_properties[] = {
>      DEFINE_NIC_PROPERTIES(VMXNET3State, conf),	
>      DEFINE_PROP_BIT("x-old-msi-offsets", VMXNET3State, compat_flags,
>                      VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT, false),
> +    DEFINE_PROP_BIT("x-disable-pcie", VMXNET3State, compat_flags,
> +                    VMXNET3_COMPAT_FLAG_DISABLE_PCIE_BIT, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static void vmxnet3_realize(DeviceState *qdev, Error **errp)
> +{
> +    VMXNET3Class *vc = VMXNET3_DEVICE_GET_CLASS(qdev);
> +    PCIDevice *pci_dev = PCI_DEVICE(qdev);
> +    VMXNET3State *s = VMXNET3(qdev);
> +
> +    if (!(s->compat_flags & VMXNET3_COMPAT_FLAG_DISABLE_PCIE)) {
> +        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> +    }
> +
> +    vc->parent_dc_realize(qdev, errp);
> +}

It's not clear that how the class helps here. Why not simply do
everthing in vmxnet3_pci_realize()?

> +
>  static void vmxnet3_class_init(ObjectClass *class, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(class);
>      PCIDeviceClass *c = PCI_DEVICE_CLASS(class);
> +    VMXNET3Class *vc = VMXNET3_DEVICE_CLASS(class);
>  
>      c->realize = vmxnet3_pci_realize;
>      c->exit = vmxnet3_pci_uninit;
> @@ -2592,6 +2643,8 @@ static void vmxnet3_class_init(ObjectClass *class, void *data)
>      c->class_id = PCI_CLASS_NETWORK_ETHERNET;
>      c->subsystem_vendor_id = PCI_VENDOR_ID_VMWARE;
>      c->subsystem_id = PCI_DEVICE_ID_VMWARE_VMXNET3;
> +    vc->parent_dc_realize = dc->realize;
> +    dc->realize = vmxnet3_realize;
>      dc->desc = "VMWare Paravirtualized Ethernet v3";
>      dc->reset = vmxnet3_qdev_reset;
>      dc->vmsd = &vmstate_vmxnet3;
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 01e326d..642b082 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -22,6 +22,10 @@
>              .driver   = "vmxnet3",\
>              .property = "x-old-msi-offsets",\
>              .value    = "on",\
> +        },{\
> +            .driver   = "vmxnet3",\
> +            .property = "x-disable-pcie",\
> +            .value    = "on",\
>          },

To have a better bisection behavior, we'd better enable and compat this
in next patch.

>  
>  #define HW_COMPAT_2_3 \

Re: [Qemu-devel] [PATCH v2 1/5] vmxnet3: Change offsets of msi/msix pci capabilities

[Shmulik Ladkani]

Thanks Jason,

On Fri, 4 Dec 2015 16:49:23 +0800 Jason Wang <jasowang@redhat.com> wrote:
> > @@ -18,6 +18,10 @@
> >              .driver   = "virtio-pci",\
> >              .property = "migrate-extra",\
> >              .value    = "off",\
> > +        },{\
> > +            .driver   = "vmxnet3",\
> > +            .property = "x-old-msi-offsets",\
> > +            .value    = "on",\
> >          },
> 
> To have a better bisection behavior, we'd better enable this and compat
> it in patch 2.

Did you mean the following:

#1 Introduce the new functionality with its 'x-old-msi-offsets'
   property, with the property set to true by default

#2 Change 'x-old-msi-offsets' default value to false, and introduce the
   above compat

Regards,
Shmulik

Re: [Qemu-devel] [PATCH v2 4/5] vmxnet3: The vmxnet3 device is a PCIE endpoint

[Shmulik Ladkani]

Thanks Jason,

On Fri, 4 Dec 2015 16:49:52 +0800 Jason Wang <jasowang@redhat.com> wrote:
> > @@ -2257,6 +2262,10 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
> >  
> >      vmxnet3_net_init(s);
> >  
> > +    if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus)) {
> 
> Looks like pci_bus_is_express() has been checked in
> pcie_endpoint_cap_init().

Yes, but only for toggling between 'type = PCI_EXP_TYPE_ENDPOINT' vs.
'type = PCI_EXP_TYPE_RC_END'.

We would not like to expose the capability stating device is an express
endpoint (of any kind) unless it is located on a pcie bus.

> > +static void vmxnet3_realize(DeviceState *qdev, Error **errp)
> > +{
> > +    VMXNET3Class *vc = VMXNET3_DEVICE_GET_CLASS(qdev);
> > +    PCIDevice *pci_dev = PCI_DEVICE(qdev);
> > +    VMXNET3State *s = VMXNET3(qdev);
> > +
> > +    if (!(s->compat_flags & VMXNET3_COMPAT_FLAG_DISABLE_PCIE)) {
> > +        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > +    }
> > +
> > +    vc->parent_dc_realize(qdev, errp);
> > +}
> 
> It's not clear that how the class helps here. Why not simply do
> everthing in vmxnet3_pci_realize()?

Since 'vmxnet3_pci_realize' is invoked too late. By the time it's
invoked, the config space is already allocated during 'pci_qdev_realize',
without correctly knowing whether the device is pci or pcie.

We must call 'pci_qdev_realize' (parent_dc_realize) only after
'cap_present' is properly set.
See discussion here:
  https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00043.html

Regards,
Shmulik

Re: [Qemu-devel] [PATCH v2 1/5] vmxnet3: Change offsets of msi/msix pci capabilities

[Jason Wang]

On 12/05/2015 03:38 AM, Shmulik Ladkani wrote:
> Thanks Jason,
>
> On Fri, 4 Dec 2015 16:49:23 +0800 Jason Wang <jasowang@redhat.com> wrote:
>>> @@ -18,6 +18,10 @@
>>>              .driver   = "virtio-pci",\
>>>              .property = "migrate-extra",\
>>>              .value    = "off",\
>>> +        },{\
>>> +            .driver   = "vmxnet3",\
>>> +            .property = "x-old-msi-offsets",\
>>> +            .value    = "on",\
>>>          },
>> To have a better bisection behavior, we'd better enable this and compat
>> it in patch 2.
> Did you mean the following:
>
> #1 Introduce the new functionality with its 'x-old-msi-offsets'
>    property, with the property set to true by default
>
> #2 Change 'x-old-msi-offsets' default value to false, and introduce the
>    above compat
>
> Regards,
> Shmulik

Sorry for being unclear, I mean we'd better, keep the compat flag like
what has been done in the patch but introduce the property bit and
compat it in next patch.

Re: [Qemu-devel] [PATCH v2 4/5] vmxnet3: The vmxnet3 device is a PCIE endpoint

[Jason Wang]

On 12/05/2015 03:57 AM, Shmulik Ladkani wrote:
> Thanks Jason,
>
> On Fri, 4 Dec 2015 16:49:52 +0800 Jason Wang <jasowang@redhat.com> wrote:
>>> @@ -2257,6 +2262,10 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>>>  
>>>      vmxnet3_net_init(s);
>>>  
>>> +    if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus)) {
>> Looks like pci_bus_is_express() has been checked in
>> pcie_endpoint_cap_init().
> Yes, but only for toggling between 'type = PCI_EXP_TYPE_ENDPOINT' vs.
> 'type = PCI_EXP_TYPE_RC_END'.
>
> We would not like to expose the capability stating device is an express
> endpoint (of any kind) unless it is located on a pcie bus.

Ok

>>> +static void vmxnet3_realize(DeviceState *qdev, Error **errp)
>>> +{
>>> +    VMXNET3Class *vc = VMXNET3_DEVICE_GET_CLASS(qdev);
>>> +    PCIDevice *pci_dev = PCI_DEVICE(qdev);
>>> +    VMXNET3State *s = VMXNET3(qdev);
>>> +
>>> +    if (!(s->compat_flags & VMXNET3_COMPAT_FLAG_DISABLE_PCIE)) {
>>> +        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>>> +    }
>>> +
>>> +    vc->parent_dc_realize(qdev, errp);
>>> +}
>> It's not clear that how the class helps here. Why not simply do
>> everthing in vmxnet3_pci_realize()?
> Since 'vmxnet3_pci_realize' is invoked too late. By the time it's
> invoked, the config space is already allocated during 'pci_qdev_realize',
> without correctly knowing whether the device is pci or pcie.
>
> We must call 'pci_qdev_realize' (parent_dc_realize) only after
> 'cap_present' is properly set.
> See discussion here:
>   https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00043.html

Thanks for the pointer, consider 2.5 release is near. I will consider
this series for 2.6 (after find some time to re-evaluate this patch).


>
> Regards,
> Shmulik
>