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

[Qemu-devel] [PATCH 0/4] 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.

Shmulik Ladkani (4):
  vmxnet3: The vmxnet3 device is a PCIE endpoint
  vmxnet3: Change offsets of PCI capabilities
  vmxnet3: Change the offset of the MSIX PBA table
  vmxnet3: Report the Device Serial Number capability

 hw/net/vmxnet3.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/4] 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 5e3a233..ed286cc 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2233,6 +2233,10 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
         VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");
     }
 
+    if (pci_bus_is_express(pci_dev->bus)) {
+        pcie_endpoint_cap_init(pci_dev, 0);
+    }
+
     vmxnet3_net_init(s);
 
     register_savevm(dev, "vmxnet3-msix", -1, 1,
@@ -2568,6 +2572,7 @@ 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;
+    c->is_express = 1;
     dc->desc = "VMWare Paravirtualized Ethernet v3";
     dc->reset = vmxnet3_qdev_reset;
     dc->vmsd = &vmstate_vmxnet3;
-- 
1.9.1

[Qemu-devel] [PATCH 2/4] vmxnet3: Change offsets of PCI capabilities

[Shmulik Ladkani]

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

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

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index ed286cc..48a8242 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -36,6 +36,10 @@
 #define VMXNET3_MSIX_BAR_SIZE 0x2000
 #define MIN_BUF_SIZE 60
 
+#define VMXNET3_EXP_EP_OFFSET  (0x48)
+#define VMXNET3_MSI_OFFSET     (0x84)
+#define VMXNET3_MSIX_OFFSET    (0x9c)
+
 #define VMXNET3_BAR0_IDX      (0)
 #define VMXNET3_BAR1_IDX      (1)
 #define VMXNET3_MSIX_BAR_IDX  (2)
@@ -2103,7 +2107,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);
 
     if (0 > res) {
         VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
@@ -2131,7 +2135,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
     }
 }
 
-#define VMXNET3_MSI_OFFSET        (0x50)
 #define VMXNET3_USE_64BIT         (true)
 #define VMXNET3_PER_VECTOR_MASK   (false)
 
@@ -2234,7 +2237,7 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
     }
 
     if (pci_bus_is_express(pci_dev->bus)) {
-        pcie_endpoint_cap_init(pci_dev, 0);
+        pcie_endpoint_cap_init(pci_dev, VMXNET3_EXP_EP_OFFSET);
     }
 
     vmxnet3_net_init(s);
-- 
1.9.1

[Qemu-devel] [PATCH 3/4] 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 48a8242..1525b7c 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -45,7 +45,7 @@
 #define VMXNET3_MSIX_BAR_IDX  (2)
 
 #define VMXNET3_OFF_MSIX_TABLE (0x000)
-#define VMXNET3_OFF_MSIX_PBA   (0x800)
+#define VMXNET3_OFF_MSIX_PBA   (0x1000)
 
 /* Link speed in Mbps should be shifted by 16 */
 #define VMXNET3_LINK_SPEED      (1000 << 16)
-- 
1.9.1

[Qemu-devel] [PATCH 4/4] 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.

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

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 1525b7c..f31d62e 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -39,6 +39,7 @@
 #define VMXNET3_EXP_EP_OFFSET  (0x48)
 #define VMXNET3_MSI_OFFSET     (0x84)
 #define VMXNET3_MSIX_OFFSET    (0x9c)
+#define VMXNET3_DSN_OFFSET     (0x100)
 
 #define VMXNET3_BAR0_IDX      (0)
 #define VMXNET3_BAR1_IDX      (1)
@@ -2201,6 +2202,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);
@@ -2242,6 +2259,11 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
 
     vmxnet3_net_init(s);
 
+    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,
                     vmxnet3_msix_save, vmxnet3_msix_load, s);
 }
-- 
1.9.1

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

[Jason Wang]

On 11/19/2015 06:52 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 | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 5e3a233..ed286cc 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2233,6 +2233,10 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>          VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");
>      }
>  
> +    if (pci_bus_is_express(pci_dev->bus)) {
> +        pcie_endpoint_cap_init(pci_dev, 0);
> +    }
> +
>      vmxnet3_net_init(s);
>  
>      register_savevm(dev, "vmxnet3-msix", -1, 1,
> @@ -2568,6 +2572,7 @@ 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;
> +    c->is_express = 1;

Should we do this conditionally? And how about the migration
compatibility? Looks like pcie device is using vmstate_pcie_device
instead of vmstate_pci_device, maybe need a new property bit for this.

>
>      dc->reset = vmxnet3_qdev_reset;
>      dc->vmsd = &vmstate_vmxnet3;

Re: [Qemu-devel] [PATCH 2/4] vmxnet3: Change offsets of PCI capabilities

[Jason Wang]

On 11/19/2015 06:52 PM, Shmulik Ladkani wrote:
> Place device reported PCI capabilities at the same offsets as placed by
> the VMware virtual hardware:
>   Express Endpoint at [48], MSI at [84], MSI-X at [9c].
>
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
> ---
>  hw/net/vmxnet3.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index ed286cc..48a8242 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -36,6 +36,10 @@
>  #define VMXNET3_MSIX_BAR_SIZE 0x2000
>  #define MIN_BUF_SIZE 60
>  
> +#define VMXNET3_EXP_EP_OFFSET  (0x48)
> +#define VMXNET3_MSI_OFFSET     (0x84)
> +#define VMXNET3_MSIX_OFFSET    (0x9c)
> +
>  #define VMXNET3_BAR0_IDX      (0)
>  #define VMXNET3_BAR1_IDX      (1)
>  #define VMXNET3_MSIX_BAR_IDX  (2)
> @@ -2103,7 +2107,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);
>  
>      if (0 > res) {
>          VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
> @@ -2131,7 +2135,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>      }
>  }
>  
> -#define VMXNET3_MSI_OFFSET        (0x50)
>  #define VMXNET3_USE_64BIT         (true)
>  #define VMXNET3_PER_VECTOR_MASK   (false)
>  
> @@ -2234,7 +2237,7 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>      }
>  
>      if (pci_bus_is_express(pci_dev->bus)) {
> -        pcie_endpoint_cap_init(pci_dev, 0);
> +        pcie_endpoint_cap_init(pci_dev, VMXNET3_EXP_EP_OFFSET);
>      }
>  
>      vmxnet3_net_init(s);

Looks the same issue as patch 1, this changes is guest visible after
migration.

Re: [Qemu-devel] [PATCH 3/4] vmxnet3: Change the offset of the MSIX PBA table

[Jason Wang]

On 11/19/2015 06:52 PM, Shmulik Ladkani wrote:
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 48a8242..1525b7c 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -45,7 +45,7 @@
>  #define VMXNET3_MSIX_BAR_IDX  (2)
>  
>  #define VMXNET3_OFF_MSIX_TABLE (0x000)
> -#define VMXNET3_OFF_MSIX_PBA   (0x800)
> +#define VMXNET3_OFF_MSIX_PBA   (0x1000)
>  
>  /* Link speed in Mbps should be shifted by 16 */
>  #define VMXNET3_LINK_SPEED      (1000 << 16)

Still guest visible after migration. 

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

[Shmulik Ladkani]

Thanks Jason,

On Wed, 25 Nov 2015 10:38:51 +0800, jasowang@redhat.com wrote:
> > @@ -2568,6 +2572,7 @@ 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;
> > +    c->is_express = 1;
> 
> Should we do this conditionally? And how about the migration
> compatibility? Looks like pcie device is using vmstate_pcie_device
> instead of vmstate_pci_device, maybe need a new property bit for this.

(Responding for the entire series)

Agreed. Will limit these changes for new versions.

What's your suggested plan?
Does it make sense to have a property for each change (as they are not
necessarily related), or is it too tedious and one property will suffice?

Regards,
Shmulik

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

[Jason Wang]

On 11/25/2015 02:18 PM, Shmulik Ladkani wrote:
> Thanks Jason,
>
> On Wed, 25 Nov 2015 10:38:51 +0800, jasowang@redhat.com wrote:
>>> @@ -2568,6 +2572,7 @@ 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;
>>> +    c->is_express = 1;
>> Should we do this conditionally? And how about the migration
>> compatibility? Looks like pcie device is using vmstate_pcie_device
>> instead of vmstate_pci_device, maybe need a new property bit for this.
> (Responding for the entire series)
>
> Agreed. Will limit these changes for new versions.
>
> What's your suggested plan?
> Does it make sense to have a property for each change (as they are not
> necessarily related), or is it too tedious and one property will suffice?

Since they are not necessarily related, we'd better use a property for
each change.

Thanks

>
> Regards,
> Shmulik
>

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

[Shmulik Ladkani]

Hi,

On Wed, 25 Nov 2015 16:24:39 +0800 Jason Wang <jasowang@redhat.com> wrote:
> >>> @@ -2568,6 +2572,7 @@ 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;
> >>> +    c->is_express = 1;
> >> Should we do this conditionally? And how about the migration
> >> compatibility? Looks like pcie device is using vmstate_pcie_device
> >> instead of vmstate_pci_device, maybe need a new property bit for this.
> > (Responding for the entire series)
> >
> > Agreed. Will limit these changes for new versions.
> >
> > What's your suggested plan?
> > Does it make sense to have a property for each change (as they are not
> > necessarily related), or is it too tedious and one property will suffice?
> 
> Since they are not necessarily related, we'd better use a property for
> each change.

Would it make sense if we expose a new vmxnet3 type to differenciate
pcie vs pci instances of vmxnet3?

Otherwise, migration gets more complicated, as we need to use either
vmstate_pci_device or vmstate_pcie_device; also, upon vm load, we need
to preserve the semantics saved (whether the instance was pci or pcie).

I have managed to do so, but is a bit tedious; Exposing a new type seems
cleaner.

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

[Jason Wang]

On 11/30/2015 05:07 AM, Shmulik Ladkani wrote:
> Hi,
>
> On Wed, 25 Nov 2015 16:24:39 +0800 Jason Wang <jasowang@redhat.com> wrote:
>>>>> @@ -2568,6 +2572,7 @@ 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;
>>>>> +    c->is_express = 1;
>>>> Should we do this conditionally? And how about the migration
>>>> compatibility? Looks like pcie device is using vmstate_pcie_device
>>>> instead of vmstate_pci_device, maybe need a new property bit for this.
>>> (Responding for the entire series)
>>>
>>> Agreed. Will limit these changes for new versions.
>>>
>>> What's your suggested plan?
>>> Does it make sense to have a property for each change (as they are not
>>> necessarily related), or is it too tedious and one property will suffice?
>> Since they are not necessarily related, we'd better use a property for
>> each change.
> Would it make sense if we expose a new vmxnet3 type to differenciate
> pcie vs pci instances of vmxnet3?
>
> Otherwise, migration gets more complicated, as we need to use either
> vmstate_pci_device or vmstate_pcie_device; also, upon vm load, we need
> to preserve the semantics saved (whether the instance was pci or pcie).
>
> I have managed to do so, but is a bit tedious; Exposing a new type seems
> cleaner.

Yes, it's a good idea to have a new type.

Thanks