[Qemu-devel] [PATCH v3 0/7] vmxnet3: Fine-tune device capabilities

[Qemu-devel] [PATCH v3 0/7] vmxnet3: 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 v2: Introduce the compatability properties in separate patches,
          per Jason Wang's comment
Since v1: Added migration compatability, per Jason Wang's comment

Shmulik Ladkani (7):
  vmxnet3: Change offsets of msi/msix pci capabilities
  vmxnet3: Change the offset of the MSIX PBA table
  vmxnet3: Introduce 'x-old-msi-offsets' backword compatability property
  vmxnet3: coding: Introduce VMXNET3Class
  vmxnet3: The vmxnet3 device is a PCIE endpoint
  vmxnet3: Introduce 'x-disable-pcie' backword compatability property
  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 v3 1/7] 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].

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

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 37373e5..d323895 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 */
@@ -2102,7 +2115,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);
@@ -2130,7 +2143,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
     }
 }
 
-#define VMXNET3_MSI_OFFSET        (0x50)
 #define VMXNET3_USE_64BIT         (true)
 #define VMXNET3_PER_VECTOR_MASK   (false)
 
@@ -2140,7 +2152,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);
-- 
1.9.1

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

[Shmulik Ladkani]

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

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 d323895..97f0aea 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)
@@ -2114,7 +2115,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 v3 3/7] vmxnet3: Introduce 'x-old-msi-offsets' backword compatability property

[Shmulik Ladkani]

Following the previous patches, where vmxnet3's pci's msi/msix
capability offsets and msix's PBA table offsets have been changed, this
patch introduces a boolean property 'x-old-msi-offsets' to vmxnet3,
whose default is false.

Setting 'x-old-msi-offsets' to 'on' preserves the old offsets behavior,
which allows migration to older versions.

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

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 97f0aea..d1fe888 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2564,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

Re: [Qemu-devel] [PATCH v3 3/7] vmxnet3: Introduce 'x-old-msi-offsets' backword compatability property

[Jason Wang]

On 12/12/2015 08:00 PM, Shmulik Ladkani wrote:
> Following the previous patches, where vmxnet3's pci's msi/msix
> capability offsets and msix's PBA table offsets have been changed, this
> patch introduces a boolean property 'x-old-msi-offsets' to vmxnet3,
> whose default is false.
>
> Setting 'x-old-msi-offsets' to 'on' preserves the old offsets behavior,
> which allows migration to older versions.
>
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
> ---
>  hw/net/vmxnet3.c    | 2 ++
>  include/hw/compat.h | 4 ++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 97f0aea..d1fe888 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2564,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 \

Too late for 2.5. Need to do this for 2.6, so we probably want either
introduce 2.6 machine type by yourself or wait for somebody else to do
it and do it for HW_COMPAT_2_5 :)

[Qemu-devel] [PATCH v3 4/7] 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 d1fe888..14d4dcb 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;
@@ -2592,6 +2601,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 v3 5/7] vmxnet3: The vmxnet3 device is a PCIE endpoint

[Shmulik Ladkani]

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

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

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 14d4dcb..7ded287 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"
@@ -2256,6 +2261,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);
 }
@@ -2525,6 +2534,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,
@@ -2532,7 +2564,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),
@@ -2567,6 +2601,7 @@ static const VMStateDescription vmstate_vmxnet3 = {
     },
     .subsections = (const VMStateDescription*[]) {
         &vmxstate_vmxnet3_mcast_list,
+        &vmstate_vmxnet3_pcie_device,
         NULL
     }
 };
@@ -2578,10 +2613,24 @@ static Property vmxnet3_properties[] = {
     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;
@@ -2591,6 +2640,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;
-- 
1.9.1

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

[Jason Wang]

On 12/12/2015 08:00 PM, Shmulik Ladkani wrote:
> Report the 'express endpoint' capability if on a PCIE bus.
>
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
> ---
>  hw/net/vmxnet3.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 14d4dcb..7ded287 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"
> @@ -2256,6 +2261,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);
>  }
> @@ -2525,6 +2534,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,
> @@ -2532,7 +2564,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),
> @@ -2567,6 +2601,7 @@ static const VMStateDescription vmstate_vmxnet3 = {
>      },
>      .subsections = (const VMStateDescription*[]) {
>          &vmxstate_vmxnet3_mcast_list,
> +        &vmstate_vmxnet3_pcie_device,
>          NULL
>      }
>  };
> @@ -2578,10 +2613,24 @@ static Property vmxnet3_properties[] = {
>      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;
> +    }

Looking at the other pci express device implementation (e.g nvme). Looks
like we can re-use the "is_express" property of PCIDeviceClass by
setting this to true in vmxnet3_class_init(). If this is ok, there's
probably no need for hacking like this.

> +
> +    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;
> @@ -2591,6 +2640,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;

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

[Shmulik Ladkani]

Thanks Jason,

On Mon, 14 Dec 2015 14:24:36 +0800, jasowang@redhat.com wrote:
> > +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;
> > +    }
> 
> Looking at the other pci express device implementation (e.g nvme). Looks
> like we can re-use the "is_express" property of PCIDeviceClass by
> setting this to true in vmxnet3_class_init(). If this is ok, there's
> probably no need for hacking like this.

Yes, arming PCIDeviceClass.is_express in a 'class_init' method is the
classic way instructing the pci device to get the QEMU_PCI_CAP_EXPRESS
flag.
But this only works when 'is_express' is unconditionally true for all
instances of the class.

However in our case, we need to set it conditionally per instance,
according to the 'x-disable-pcie' device property.

My first attempt was setting QEMU_PCI_CAP_EXPRESS in
'vmxnet3_instance_init', which seems much cleaner (no need to introduce
the 'parent_dc_realize' hack).

This attempt failed, since at time of 'instance_init' invocation, the
device properties are NOT yet processed, so the 'compat_flags' isn't
set with the right value:

qdev_device_add()
   dev = DEVICE(object_new(driver));  # device creation
     object_new_with_type()
       object_initialize_with_type()
         object_init_with_type()
           ti->instance_init(obj);  # instance_init invoked at device creation
   ...
   qemu_opt_foreach(opts, set_property, dev, &err)  # sets properties
   ...

An alternative to introducing 'parent_dc_realize' was direct invocation
of 'pci_qdev_realize' within 'vmxnet3_realize', as I suggested in [1].
However this was rejected by Marcel Apfelbaum, as this might be error
prone since subclass (TYPE_VMXNET3) should have no direct knowledge
about its parent class's (TYPE_PCI_DEVICE) realize method, see [2].

Another attempt I've made is to indroduce a new type vmxnet3e (the
pcie variant of vmxnet3).
I dropped this approach since it was way too cumbersome, introducing
lots of boiler-plate code for the two (otherwise) identical types.

Since virtio-pci device needed the same fix (making it pcie without
breaking compat), and since the above approach was chosen
 (0560b0e virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method)
I've repeated same approach for vmxnet3.

I'm open to other suggestions, if we can come up with something cleaner
that fits all requirements.

Regards,
Shmulik

[1]
https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00043.html

[2]
https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00114.html

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

[Jason Wang]

On 12/14/2015 03:32 PM, Shmulik Ladkani wrote:
> Thanks Jason,
>
> On Mon, 14 Dec 2015 14:24:36 +0800, jasowang@redhat.com wrote:
>>> +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;
>>> +    }
>> Looking at the other pci express device implementation (e.g nvme). Looks
>> like we can re-use the "is_express" property of PCIDeviceClass by
>> setting this to true in vmxnet3_class_init(). If this is ok, there's
>> probably no need for hacking like this.
> Yes, arming PCIDeviceClass.is_express in a 'class_init' method is the
> classic way instructing the pci device to get the QEMU_PCI_CAP_EXPRESS
> flag.
> But this only works when 'is_express' is unconditionally true for all
> instances of the class.

I see.

>
> However in our case, we need to set it conditionally per instance,
> according to the 'x-disable-pcie' device property.
>
> My first attempt was setting QEMU_PCI_CAP_EXPRESS in
> 'vmxnet3_instance_init', which seems much cleaner (no need to introduce
> the 'parent_dc_realize' hack).
>
> This attempt failed, since at time of 'instance_init' invocation, the
> device properties are NOT yet processed, so the 'compat_flags' isn't
> set with the right value:
>
> qdev_device_add()
>    dev = DEVICE(object_new(driver));  # device creation
>      object_new_with_type()
>        object_initialize_with_type()
>          object_init_with_type()
>            ti->instance_init(obj);  # instance_init invoked at device creation
>    ...
>    qemu_opt_foreach(opts, set_property, dev, &err)  # sets properties
>    ...
>
> An alternative to introducing 'parent_dc_realize' was direct invocation
> of 'pci_qdev_realize' within 'vmxnet3_realize', as I suggested in [1].
> However this was rejected by Marcel Apfelbaum, as this might be error
> prone since subclass (TYPE_VMXNET3) should have no direct knowledge
> about its parent class's (TYPE_PCI_DEVICE) realize method, see [2].
>
> Another attempt I've made is to indroduce a new type vmxnet3e (the
> pcie variant of vmxnet3).
> I dropped this approach since it was way too cumbersome, introducing
> lots of boiler-plate code for the two (otherwise) identical types.

Yes, that's another solution (as I replied for patch 6). A question
here. If vmware differs pci-e version of vmxnet3 from pci version,
probably we need do the same (and you don't even need to care for
compatibility in the case). At a quick glance, no much duplicated codes.
(if you mean the msi offsets, you can let vmxnet3e use the new offset
unconditionally).

>
> Since virtio-pci device needed the same fix (making it pcie without
> breaking compat), and since the above approach was chosen
>  (0560b0e virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method)
> I've repeated same approach for vmxnet3.
>
> I'm open to other suggestions, if we can come up with something cleaner
> that fits all requirements.
>
> Regards,
> Shmulik
>
> [1]
> https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00043.html
>
> [2]
> https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00114.html

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

[Shmulik Ladkani]

Hi Jason,

On Tue, 15 Dec 2015 10:35:59 +0800 Jason Wang <jasowang@redhat.com> wrote:
> > Another attempt I've made is to indroduce a new type vmxnet3e (the
> > pcie variant of vmxnet3).
> > I dropped this approach since it was way too cumbersome, introducing
> > lots of boiler-plate code for the two (otherwise) identical types.
> 
> Yes, that's another solution (as I replied for patch 6). A question
> here. If vmware differs pci-e version of vmxnet3 from pci version,
> probably we need do the same (and you don't even need to care for
> compatibility in the case). At a quick glance, no much duplicated codes.
> (if you mean the msi offsets, you can let vmxnet3e use the new offset
> unconditionally).

Examples of duplicated boiler plate:

Split to a TYPE_VMXNET3_BASE abstract type having two concrete sub types.

Introduction of 'VMStateDescription vmstate_vmxnet3e' which differs only
due to its '.name' (must be the name of the type, i.e "vmxnet3e") and
the use of VMSTATE_PCIE_DEVICE (instead of VMSTATE_PCI_DEVICE), but
otherwise idential to existing 'VMStateDescription vmstate_vmxnet3'.

Introduction of 'VMStateDescription vmxstate_vmxnet3e_mcast_list' which
differs only by '.name' (must be "vmxnet3e/mcast_list" instead of
"vmxnet3/mcast_list") but otherwise identical to existing
'vmxstate_vmxnet3_mcast_list'.

Also, the vmxnet3 device is indeed a PCIE, and should have been so since
start.
The reason we're keeping the non-pcie variant is not since user would be
interested in an environment having the the non-pcie type, but only for
not breaking migration from old hardware versions.

Thus, suggesting 2 device types, providing the non-pcie variant as a
user visible type, exposes the user with a choice of selecting a type
which ideally shouldn't have existed at all.
This seems less preferrable.

Regards,
Shmulik

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

[Jason Wang]

On 12/15/2015 02:09 PM, Shmulik Ladkani wrote:
> Hi Jason,
>
> On Tue, 15 Dec 2015 10:35:59 +0800 Jason Wang <jasowang@redhat.com> wrote:
>>> Another attempt I've made is to indroduce a new type vmxnet3e (the
>>> pcie variant of vmxnet3).
>>> I dropped this approach since it was way too cumbersome, introducing
>>> lots of boiler-plate code for the two (otherwise) identical types.
>> Yes, that's another solution (as I replied for patch 6). A question
>> here. If vmware differs pci-e version of vmxnet3 from pci version,
>> probably we need do the same (and you don't even need to care for
>> compatibility in the case). At a quick glance, no much duplicated codes.
>> (if you mean the msi offsets, you can let vmxnet3e use the new offset
>> unconditionally).
> Examples of duplicated boiler plate:
>
> Split to a TYPE_VMXNET3_BASE abstract type having two concrete sub types.
>
> Introduction of 'VMStateDescription vmstate_vmxnet3e' which differs only
> due to its '.name' (must be the name of the type, i.e "vmxnet3e") and
> the use of VMSTATE_PCIE_DEVICE (instead of VMSTATE_PCI_DEVICE), but
> otherwise idential to existing 'VMStateDescription vmstate_vmxnet3'.
>
> Introduction of 'VMStateDescription vmxstate_vmxnet3e_mcast_list' which
> differs only by '.name' (must be "vmxnet3e/mcast_list" instead of
> "vmxnet3/mcast_list") but otherwise identical to existing
> 'vmxstate_vmxnet3_mcast_list'.
>
> Also, the vmxnet3 device is indeed a PCIE, and should have been so since
> start.

Yes, so this is a strong reason that we must not introduce a new type.

> The reason we're keeping the non-pcie variant is not since user would be
> interested in an environment having the the non-pcie type, but only for
> not breaking migration from old hardware versions.
>
> Thus, suggesting 2 device types, providing the non-pcie variant as a
> user visible type, exposes the user with a choice of selecting a type
> which ideally shouldn't have existed at all.
> This seems less preferrable.
>
> Regards,
> Shmulik
>

I get the point, thanks for the clarification.

[Qemu-devel] [PATCH v3 6/7] vmxnet3: Introduce 'x-disable-pcie' backword compatability property

[Shmulik Ladkani]

Following the previous patch which changed vmxnet3 to be a pci express
device, this patch introduces a boolean property 'x-disable-pcie' whose
default is false.

Setting 'x-disable-pcie' to 'on' preserves the old 'pci device' (non
express) behavior. This allows migration to older versions.

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

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 7ded287..f9cd02b 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2610,6 +2610,8 @@ 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(),
 };
 
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

Re: [Qemu-devel] [PATCH v3 6/7] vmxnet3: Introduce 'x-disable-pcie' backword compatability property

[Jason Wang]

On 12/12/2015 08:00 PM, Shmulik Ladkani wrote:
> Following the previous patch which changed vmxnet3 to be a pci express
> device, this patch introduces a boolean property 'x-disable-pcie' whose
> default is false.
>
> Setting 'x-disable-pcie' to 'on' preserves the old 'pci device' (non
> express) behavior. This allows migration to older versions.
>
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
> ---
>  hw/net/vmxnet3.c    | 2 ++
>  include/hw/compat.h | 4 ++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 7ded287..f9cd02b 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2610,6 +2610,8 @@ 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(),
>  };
>  
> 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 \

Same as patch 3. Too late for 2.5, need do this for HW_COMPAT_2_5.

Just for pcie, another choice is a new type with "is_express = true".
But both method looks good to me.

[Qemu-devel] [PATCH v3 7/7] 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 f9cd02b..2e4a5a5 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)
@@ -2224,6 +2225,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);
@@ -2261,8 +2278,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