[Qemu-devel] [PATCH v2 0/6] pvscsi: Fine-tune device capabilities

[Qemu-devel] [PATCH v2 0/6] pvscsi: Fine-tune device capabilities

[Shmulik Ladkani]

Various fixes to what the vmw_pvscsi device reports in its PCI
configuration space, to better align with VMware virtual hardware
as exposed by ESXi/Workstation.

Since v1: Added compatability properties for migration

Shmulik Ladkani (6):
  vmw_pvscsi: Set device subsystem and revision
  vmw_pvscsi: Change offset of msi pci capability
  vmw_pvscsi: Introduce 'x-old-pci-configuration' backword compatability
    property
  vmw_pvscsi: coding: Introduce PVSCSIClass
  vmw_pvscsi: The pvscsi device is a PCIE endpoint
  vmw_pvscsi: Introduce 'x-disable-pcie' backword compatability property

 hw/scsi/vmw_pvscsi.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++----
 include/hw/compat.h  |  8 +++++
 2 files changed, 98 insertions(+), 6 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/6] vmw_pvscsi: Set device subsystem and revision

[Shmulik Ladkani]

To be VMware PVSCSI SCSI Controller, rev 02.
As reported by the VMware virtual hardware.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
---
 hw/scsi/vmw_pvscsi.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 9c71f31..ce099f9 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -52,6 +52,14 @@
 #define TYPE_PVSCSI "pvscsi"
 #define PVSCSI(obj) OBJECT_CHECK(PVSCSIState, (obj), TYPE_PVSCSI)
 
+/* Compatability flags for migration */
+#define PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT 0
+#define PVSCSI_COMPAT_OLD_PCI_CONFIGURATION \
+    (1 << PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT)
+
+#define PVSCSI_USE_OLD_PCI_CONFIGURATION(s) \
+    ((s)->compat_flags & PVSCSI_COMPAT_OLD_PCI_CONFIGURATION)
+
 typedef struct PVSCSIRingInfo {
     uint64_t            rs_pa;
     uint32_t            txr_len_mask;
@@ -100,6 +108,8 @@ typedef struct {
 
     PVSCSIRingInfo rings;                /* Data transfer rings manager      */
     uint32_t resetting;                  /* Reset in progress                */
+
+    uint32_t compat_flags;
 } PVSCSIState;
 
 typedef struct PVSCSIRequest {
@@ -1069,9 +1079,16 @@ pvscsi_init(PCIDevice *pci_dev)
 
     trace_pvscsi_state("init");
 
-    /* PCI subsystem ID */
-    pci_dev->config[PCI_SUBSYSTEM_ID] = 0x00;
-    pci_dev->config[PCI_SUBSYSTEM_ID + 1] = 0x10;
+    /* PCI subsystem ID, subsystem vendor ID, revision */
+    if (PVSCSI_USE_OLD_PCI_CONFIGURATION(s)) {
+        pci_set_word(pci_dev->config + PCI_SUBSYSTEM_ID, 0x1000);
+    } else {
+        pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID,
+                     PCI_VENDOR_ID_VMWARE);
+        pci_set_word(pci_dev->config + PCI_SUBSYSTEM_ID,
+                     PCI_DEVICE_ID_VMWARE_PVSCSI);
+        pci_config_set_revision(pci_dev->config, 0x2);
+    }
 
     /* PCI latency timer = 255 */
     pci_dev->config[PCI_LATENCY_TIMER] = 0xff;
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/6] vmw_pvscsi: Change offset of msi pci capability

[Shmulik Ladkani]

Place device reported MSI capability at the same offset as placed by
the VMware virtual hardware - at offset 0x7c.

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

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index ce099f9..be95cff 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -32,7 +32,6 @@
 #include "trace.h"
 
 
-#define PVSCSI_MSI_OFFSET        (0x50)
 #define PVSCSI_USE_64BIT         (true)
 #define PVSCSI_PER_VECTOR_MASK   (false)
 
@@ -59,6 +58,8 @@
 
 #define PVSCSI_USE_OLD_PCI_CONFIGURATION(s) \
     ((s)->compat_flags & PVSCSI_COMPAT_OLD_PCI_CONFIGURATION)
+#define PVSCSI_MSI_OFFSET(s) \
+    (PVSCSI_USE_OLD_PCI_CONFIGURATION(s) ? 0x50 : 0x7c)
 
 typedef struct PVSCSIRingInfo {
     uint64_t            rs_pa;
@@ -1029,7 +1030,7 @@ pvscsi_init_msi(PVSCSIState *s)
     int res;
     PCIDevice *d = PCI_DEVICE(s);
 
-    res = msi_init(d, PVSCSI_MSI_OFFSET, PVSCSI_MSIX_NUM_VECTORS,
+    res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS,
                    PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
     if (res < 0) {
         trace_pvscsi_init_msi_fail(res);
-- 
1.9.1

[Qemu-devel] [PATCH v2 3/6] vmw_pvscsi: Introduce 'x-old-pci-configuration' backword compatability property

[Shmulik Ladkani]

Following the previous patches, which introduced various changes in
pvscsi's pci configuration space (device subsystem id and revision, msi
offset), this patch introduces a boolean property
'x-old-pci-configuration' to pvscsi.

Its default value is false, exposing the above changes in the pci config
space.

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

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

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index be95cff..e785b8b 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1194,6 +1194,8 @@ static const VMStateDescription vmstate_pvscsi = {
 
 static Property pvscsi_properties[] = {
     DEFINE_PROP_UINT8("use_msg", PVSCSIState, use_msg, 1),
+    DEFINE_PROP_BIT("x-old-pci-configuration", PVSCSIState, compat_flags,
+                    PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index d0b1c4f..66e4aff 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -7,6 +7,10 @@
             .property = "scsi",\
             .value    = "true",\
         },{\
+            .driver   = "pvscsi",\
+            .property = "x-old-pci-configuration",\
+            .value    = "on",\
+        },{\
             .driver   = "e1000",\
             .property = "extra_mac_registers",\
             .value    = "off",\
-- 
1.9.1

[Qemu-devel] [PATCH v2 4/6] vmw_pvscsi: coding: Introduce PVSCSIClass

[Shmulik Ladkani]

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

No semantic change.

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

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index e785b8b..00d6900 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -48,9 +48,18 @@
     (stl_le_pci_dma(&container_of(m, PVSCSIState, rings)->parent_obj, \
                  (m)->rs_pa + offsetof(struct PVSCSIRingsState, field), val))
 
+typedef struct PVSCSIClass {
+    PCIDeviceClass parent_class;
+} PVSCSIClass;
+
 #define TYPE_PVSCSI "pvscsi"
 #define PVSCSI(obj) OBJECT_CHECK(PVSCSIState, (obj), TYPE_PVSCSI)
 
+#define PVSCSI_DEVICE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(PVSCSIClass, (klass), TYPE_PVSCSI)
+#define PVSCSI_DEVICE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(PVSCSIClass, (obj), TYPE_PVSCSI)
+
 /* Compatability flags for migration */
 #define PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT 0
 #define PVSCSI_COMPAT_OLD_PCI_CONFIGURATION \
@@ -1222,6 +1231,7 @@ static void pvscsi_class_init(ObjectClass *klass, void *data)
 static const TypeInfo pvscsi_info = {
     .name          = TYPE_PVSCSI,
     .parent        = TYPE_PCI_DEVICE,
+    .class_size    = sizeof(PVSCSIClass),
     .instance_size = sizeof(PVSCSIState),
     .class_init    = pvscsi_class_init,
     .interfaces = (InterfaceInfo[]) {
-- 
1.9.1

[Qemu-devel] [PATCH v2 5/6] vmw_pvscsi: The pvscsi 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/scsi/vmw_pvscsi.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 00d6900..fb53b24 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -50,6 +50,7 @@
 
 typedef struct PVSCSIClass {
     PCIDeviceClass parent_class;
+    DeviceRealize parent_dc_realize;
 } PVSCSIClass;
 
 #define TYPE_PVSCSI "pvscsi"
@@ -64,11 +65,15 @@ typedef struct PVSCSIClass {
 #define PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT 0
 #define PVSCSI_COMPAT_OLD_PCI_CONFIGURATION \
     (1 << PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT)
+#define PVSCSI_COMPAT_DISABLE_PCIE_BIT 1
+#define PVSCSI_COMPAT_DISABLE_PCIE \
+    (1 << PVSCSI_COMPAT_DISABLE_PCIE_BIT)
 
 #define PVSCSI_USE_OLD_PCI_CONFIGURATION(s) \
     ((s)->compat_flags & PVSCSI_COMPAT_OLD_PCI_CONFIGURATION)
 #define PVSCSI_MSI_OFFSET(s) \
     (PVSCSI_USE_OLD_PCI_CONFIGURATION(s) ? 0x50 : 0x7c)
+#define PVSCSI_EXP_EP_OFFSET (0x40)
 
 typedef struct PVSCSIRingInfo {
     uint64_t            rs_pa;
@@ -1112,6 +1117,10 @@ pvscsi_init(PCIDevice *pci_dev)
 
     pvscsi_init_msi(s);
 
+    if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus)) {
+        pcie_endpoint_cap_init(pci_dev, PVSCSI_EXP_EP_OFFSET);
+    }
+
     s->completion_worker = qemu_bh_new(pvscsi_process_completion_queue, s);
     if (!s->completion_worker) {
         pvscsi_cleanup_msi(s);
@@ -1166,6 +1175,27 @@ pvscsi_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool pvscsi_vmstate_need_pcie_device(void *opaque)
+{
+    PVSCSIState *s = PVSCSI(opaque);
+
+    return !(s->compat_flags & PVSCSI_COMPAT_DISABLE_PCIE);
+}
+
+static bool pvscsi_vmstate_test_pci_device(void *opaque, int version_id)
+{
+    return !pvscsi_vmstate_need_pcie_device(opaque);
+}
+
+static const VMStateDescription vmstate_pvscsi_pcie_device = {
+    .name = "pvscsi/pcie",
+    .needed = pvscsi_vmstate_need_pcie_device,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCIE_DEVICE(parent_obj, PVSCSIState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_pvscsi = {
     .name = "pvscsi",
     .version_id = 0,
@@ -1173,7 +1203,9 @@ static const VMStateDescription vmstate_pvscsi = {
     .pre_save = pvscsi_pre_save,
     .post_load = pvscsi_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(parent_obj, PVSCSIState),
+        VMSTATE_STRUCT_TEST(parent_obj, PVSCSIState,
+                            pvscsi_vmstate_test_pci_device, 0,
+                            vmstate_pci_device, PCIDevice),
         VMSTATE_UINT8(msi_used, PVSCSIState),
         VMSTATE_UINT32(resetting, PVSCSIState),
         VMSTATE_UINT64(reg_interrupt_status, PVSCSIState),
@@ -1198,6 +1230,10 @@ static const VMStateDescription vmstate_pvscsi = {
         VMSTATE_UINT64(rings.filled_cmp_ptr, PVSCSIState),
 
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &vmstate_pvscsi_pcie_device,
+        NULL
     }
 };
 
@@ -1208,10 +1244,24 @@ static Property pvscsi_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void pvscsi_realize(DeviceState *qdev, Error **errp)
+{
+    PVSCSIClass *pvs_c = PVSCSI_DEVICE_GET_CLASS(qdev);
+    PCIDevice *pci_dev = PCI_DEVICE(qdev);
+    PVSCSIState *s = PVSCSI(qdev);
+
+    if (!(s->compat_flags & PVSCSI_COMPAT_DISABLE_PCIE)) {
+        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
+    }
+
+    pvs_c->parent_dc_realize(qdev, errp);
+}
+
 static void pvscsi_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    PVSCSIClass *pvs_k = PVSCSI_DEVICE_CLASS(klass);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     k->init = pvscsi_init;
@@ -1220,6 +1270,8 @@ static void pvscsi_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_VMWARE_PVSCSI;
     k->class_id = PCI_CLASS_STORAGE_SCSI;
     k->subsystem_id = 0x1000;
+    pvs_k->parent_dc_realize = dc->realize;
+    dc->realize = pvscsi_realize;
     dc->reset = pvscsi_reset;
     dc->vmsd = &vmstate_pvscsi;
     dc->props = pvscsi_properties;
-- 
1.9.1

[Qemu-devel] [PATCH v2 6/6] vmw_pvscsi: Introduce 'x-disable-pcie' backword compatability property

[Shmulik Ladkani]

Following the previous patch which changed pvscsi to be a pci express
device, this patch introduces a boolean property 'x-disable-pcie'.

Its default value is false, exposing pvscsi as a pcie device.

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/scsi/vmw_pvscsi.c | 2 ++
 include/hw/compat.h  | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index fb53b24..6b4b989 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1241,6 +1241,8 @@ static Property pvscsi_properties[] = {
     DEFINE_PROP_UINT8("use_msg", PVSCSIState, use_msg, 1),
     DEFINE_PROP_BIT("x-old-pci-configuration", PVSCSIState, compat_flags,
                     PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT, false),
+    DEFINE_PROP_BIT("x-disable-pcie", PVSCSIState, compat_flags,
+                    PVSCSI_COMPAT_DISABLE_PCIE_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 66e4aff..bcb36ef 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -11,6 +11,10 @@
             .property = "x-old-pci-configuration",\
             .value    = "on",\
         },{\
+            .driver   = "pvscsi",\
+            .property = "x-disable-pcie",\
+            .value    = "on",\
+        },{\
             .driver   = "e1000",\
             .property = "extra_mac_registers",\
             .value    = "off",\
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 0/6] pvscsi: Fine-tune device capabilities

[Dmitry Fleytman]

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

Acked-by: Dmitry Fleytman <dmitry@daynix.com <mailto:dmitry@daynix.com>>

> On 13 Dec 2015, at 10:08 AM, Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> 
> Various fixes to what the vmw_pvscsi device reports in its PCI
> configuration space, to better align with VMware virtual hardware
> as exposed by ESXi/Workstation.
> 
> Since v1: Added compatability properties for migration
> 
> Shmulik Ladkani (6):
>  vmw_pvscsi: Set device subsystem and revision
>  vmw_pvscsi: Change offset of msi pci capability
>  vmw_pvscsi: Introduce 'x-old-pci-configuration' backword compatability
>    property
>  vmw_pvscsi: coding: Introduce PVSCSIClass
>  vmw_pvscsi: The pvscsi device is a PCIE endpoint
>  vmw_pvscsi: Introduce 'x-disable-pcie' backword compatability property
> 
> hw/scsi/vmw_pvscsi.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++----
> include/hw/compat.h  |  8 +++++
> 2 files changed, 98 insertions(+), 6 deletions(-)
> 
> -- 
> 1.9.1
> 


[-- Attachment #2: Type: text/html, Size: 1763 bytes --]

Re: [Qemu-devel] [PATCH v2 6/6] vmw_pvscsi: Introduce 'x-disable-pcie' backword compatability property

[Paolo Bonzini]

On 13/12/2015 09:08, Shmulik Ladkani wrote:
> Following the previous patch which changed pvscsi to be a pci express
> device, this patch introduces a boolean property 'x-disable-pcie'.
> 
> Its default value is false, exposing pvscsi as a pcie device.
> 
> 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/scsi/vmw_pvscsi.c | 2 ++
>  include/hw/compat.h  | 4 ++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index fb53b24..6b4b989 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1241,6 +1241,8 @@ static Property pvscsi_properties[] = {
>      DEFINE_PROP_UINT8("use_msg", PVSCSIState, use_msg, 1),
>      DEFINE_PROP_BIT("x-old-pci-configuration", PVSCSIState, compat_flags,
>                      PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT, false),
> +    DEFINE_PROP_BIT("x-disable-pcie", PVSCSIState, compat_flags,
> +                    PVSCSI_COMPAT_DISABLE_PCIE_BIT, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 66e4aff..bcb36ef 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -11,6 +11,10 @@
>              .property = "x-old-pci-configuration",\
>              .value    = "on",\
>          },{\
> +            .driver   = "pvscsi",\
> +            .property = "x-disable-pcie",\
> +            .value    = "on",\
> +        },{\
>              .driver   = "e1000",\
>              .property = "extra_mac_registers",\
>              .value    = "off",\
> 

This series is okay, but the use of "x-" in the compat properties struck
me as odd.

I see that the "x-" prefix was first introduced for compat properties in
commit 1811e64 ("hw/virtio: Add PCIe capability to virtio devices",
2015-11-10).  The rationale for "x-" was either 1) to provide fine
tuning for debugging-like options; 2) for places where the API is known
to be somewhat broken and will be fixed later.

Marcel, Michael, what was the justification for using "x-" in commit
1811e64?  That said, I wouldn't mind using the opportunity of "x-" to
remove the double negation and rename the property to just "pcie". :)

Paolo

Re: [Qemu-devel] [PATCH v2 6/6] vmw_pvscsi: Introduce 'x-disable-pcie' backword compatability property

[Michael S. Tsirkin]

On Mon, Dec 14, 2015 at 06:07:31PM +0100, Paolo Bonzini wrote:
> 
> 
> On 13/12/2015 09:08, Shmulik Ladkani wrote:
> > Following the previous patch which changed pvscsi to be a pci express
> > device, this patch introduces a boolean property 'x-disable-pcie'.
> > 
> > Its default value is false, exposing pvscsi as a pcie device.
> > 
> > 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/scsi/vmw_pvscsi.c | 2 ++
> >  include/hw/compat.h  | 4 ++++
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> > index fb53b24..6b4b989 100644
> > --- a/hw/scsi/vmw_pvscsi.c
> > +++ b/hw/scsi/vmw_pvscsi.c
> > @@ -1241,6 +1241,8 @@ static Property pvscsi_properties[] = {
> >      DEFINE_PROP_UINT8("use_msg", PVSCSIState, use_msg, 1),
> >      DEFINE_PROP_BIT("x-old-pci-configuration", PVSCSIState, compat_flags,
> >                      PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT, false),
> > +    DEFINE_PROP_BIT("x-disable-pcie", PVSCSIState, compat_flags,
> > +                    PVSCSI_COMPAT_DISABLE_PCIE_BIT, false),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 66e4aff..bcb36ef 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -11,6 +11,10 @@
> >              .property = "x-old-pci-configuration",\
> >              .value    = "on",\
> >          },{\
> > +            .driver   = "pvscsi",\
> > +            .property = "x-disable-pcie",\
> > +            .value    = "on",\
> > +        },{\
> >              .driver   = "e1000",\
> >              .property = "extra_mac_registers",\
> >              .value    = "off",\
> > 
> 
> This series is okay, but the use of "x-" in the compat properties struck
> me as odd.
> 
> I see that the "x-" prefix was first introduced for compat properties in
> commit 1811e64 ("hw/virtio: Add PCIe capability to virtio devices",
> 2015-11-10).  The rationale for "x-" was either 1) to provide fine
> tuning for debugging-like options; 2) for places where the API is known
> to be somewhat broken and will be fixed later.
> 
> Marcel, Michael, what was the justification for using "x-" in commit
> 1811e64?  That said, I wouldn't mind using the opportunity of "x-" to
> remove the double negation and rename the property to just "pcie". :)
> 
> Paolo

Well qapi/qmp docs say things like:

	Any name (command, event, type, field, or enum value) beginning with
	"x-" is marked experimental, and may be withdrawn or changed
	incompatibly in a future release.

It's thus reasonable to use this for internal properties,
that we don't want users to play with.

BTW, we probably should teach -help to hide these options by default.

-- 
MST

Re: [Qemu-devel] [PATCH v2 5/6] vmw_pvscsi: The pvscsi device is a PCIE endpoint

[Paolo Bonzini]

On 13/12/2015 09:08, Shmulik Ladkani wrote:
> +    pvs_k->parent_dc_realize = dc->realize;

Marcel, Michael,

this creates a really nasty dependency on the contents of pci_qdev_realize.

Can you instead change PCIDeviceClass's pc->is_express to a function
pointer, and provide a sample implementation pci_is_express_true for the
devices that set is_express to true?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v2 6/6] vmw_pvscsi: Introduce 'x-disable-pcie' backword compatability property

[Paolo Bonzini]

> Well qapi/qmp docs say things like:
> 
> 	Any name (command, event, type, field, or enum value) beginning with
> 	"x-" is marked experimental, and may be withdrawn or changed
> 	incompatibly in a future release.
> 
> It's thus reasonable to use this for internal properties,
> that we don't want users to play with.

What distinguishes an internal from an external property?  Everything
except links to backends would be "internal".

We've used a much more restrictive definition so far than that---basically
known-broken or debugging-only---and I think it's more appropriate.

> BTW, we probably should teach -help to hide these options by default.

Please, no obscuring of functionality.  Debugging-only functionality
definitely belongs in -help.

Paolo

Re: [Qemu-devel] [PATCH v2 5/6] vmw_pvscsi: The pvscsi device is a PCIE endpoint

[Shmulik Ladkani]

Hi,

On Mon, 14 Dec 2015 18:14:37 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 13/12/2015 09:08, Shmulik Ladkani wrote:
> > +    pvs_k->parent_dc_realize = dc->realize;
> 
> Marcel, Michael,
> 
> this creates a really nasty dependency on the contents of pci_qdev_realize.
> 
> Can you instead change PCIDeviceClass's pc->is_express to a function
> pointer, and provide a sample implementation pci_is_express_true for the
> devices that set is_express to true?

Thanks Paolo, I like the idea.
Indeed repeating the parent_dc_realize hack for various devices seems
awkward.

If this approach is accepted, I'm okay doing the suggested refactor.

Regards,
Shmulik

Re: [Qemu-devel] [PATCH v2 6/6] vmw_pvscsi: Introduce 'x-disable-pcie' backword compatability property

[Michael S. Tsirkin]

On Mon, Dec 14, 2015 at 12:28:50PM -0500, Paolo Bonzini wrote:
> > Well qapi/qmp docs say things like:
> > 
> > 	Any name (command, event, type, field, or enum value) beginning with
> > 	"x-" is marked experimental, and may be withdrawn or changed
> > 	incompatibly in a future release.
> > 
> > It's thus reasonable to use this for internal properties,
> > that we don't want users to play with.
> 
> What distinguishes an internal from an external property?  Everything
> except links to backends would be "internal".

How do you mean? We have a ton of properties e.g.
to control which offloads are allowed for virtio net.

Internal are properties which are not there for users,
performing some technical function instead.

> We've used a much more restrictive definition so far than that---basically
> known-broken or debugging-only---and I think it's more appropriate.
> 
> > BTW, we probably should teach -help to hide these options by default.
> 
> Please, no obscuring of functionality.  Debugging-only functionality
> definitely belongs in -help.
> 
> Paolo

Sure.  This option is not for debugging though.
It's set internally by machine types to avoid breaking
migration.  I don't see any reason for users to set it.


-- 
MST

Re: [Qemu-devel] [PATCH v2 5/6] vmw_pvscsi: The pvscsi device is a PCIE endpoint

[Michael S. Tsirkin]

On Mon, Dec 14, 2015 at 06:14:37PM +0100, Paolo Bonzini wrote:
> 
> 
> On 13/12/2015 09:08, Shmulik Ladkani wrote:
> > +    pvs_k->parent_dc_realize = dc->realize;
> 
> Marcel, Michael,
> 
> this creates a really nasty dependency on the contents of pci_qdev_realize.
> 
> Can you instead change PCIDeviceClass's pc->is_express to a function
> pointer, and provide a sample implementation pci_is_express_true for the
> devices that set is_express to true?
> 
> Thanks,
> 
> Paolo

I'm not very familiar with vmw code, and I dislike overusing callbacks.
What exactly would this callback do?

-- 
MST

Re: [Qemu-devel] [PATCH v2 6/6] vmw_pvscsi: Introduce 'x-disable-pcie' backword compatability property

[Paolo Bonzini]

On 14/12/2015 18:35, Michael S. Tsirkin wrote:
> > What distinguishes an internal from an external property?  Everything
> > except links to backends would be "internal".
> 
> How do you mean? We have a ton of properties e.g.
> to control which offloads are allowed for virtio net.

Why would users set them?

> It's set internally by machine types to avoid breaking
> migration.  I don't see any reason for users to set it.

But they do set it :) albeit only through machine types.  I don't think
it's different from offloads, just much more specialized.

Or do you mean that it could go away if we decide to remove very old
machine types?  I think we would remove compat properties connected to
those machine types as well, even without "x-".

Paolo

Re: [Qemu-devel] [PATCH v2 5/6] vmw_pvscsi: The pvscsi device is a PCIE endpoint

[Paolo Bonzini]

On 14/12/2015 18:37, Michael S. Tsirkin wrote:
> > Can you instead change PCIDeviceClass's pc->is_express to a function
> > pointer, and provide a sample implementation pci_is_express_true for the
> > devices that set is_express to true?
> 
> I'm not very familiar with vmw code, and I dislike overusing callbacks.
> What exactly would this callback do?

It would be exactly the same for virtio.  The callback would return true
if the device should have a PCIe capability, false if not.  The default
would be always false.

Paolo

Re: [Qemu-devel] [PATCH v2 5/6] vmw_pvscsi: The pvscsi device is a PCIE endpoint

[Marcel Apfelbaum]

On 12/14/2015 07:14 PM, Paolo Bonzini wrote:
>
>
> On 13/12/2015 09:08, Shmulik Ladkani wrote:
>> +    pvs_k->parent_dc_realize = dc->realize;
>
> Marcel, Michael,
>
> this creates a really nasty dependency on the contents of pci_qdev_realize.

Hi Paolo,

In this case is exactly want we want to do. We don't care what pci_qdev_realize does.
We just want to made a change and then call it. We can think of it as "pre-realize."
I think I already saw a "post-realize" or something.

The usage is also documented in include/qom/object.h, how to derive "do_something" (see derived_do_something).

>
> Can you instead change PCIDeviceClass's pc->is_express to a function
> pointer, and provide a sample implementation pci_is_express_true for the
> devices that set is_express to true?

I think I thougth about this too and I have nothing against it.
The only "down side" I can see is that "is_express" means actually "can_be_express",
and some classes keep it true even if sometimes is not actually a PCIe device.

Now we change back the meaning to "is_really_express" :) , but we may encounter some side effects.
(I understand that we keep it true so we don't have to maintain some complicated code
on VMState related to the config space size.)

Thanks,
Marcel

>
> Thanks,
>
> Paolo
>

Re: [Qemu-devel] [PATCH v2 6/6] vmw_pvscsi: Introduce 'x-disable-pcie' backword compatability property

[Michael S. Tsirkin]

On Mon, Dec 14, 2015 at 07:04:33PM +0100, Paolo Bonzini wrote:
> 
> 
> On 14/12/2015 18:35, Michael S. Tsirkin wrote:
> > > What distinguishes an internal from an external property?  Everything
> > > except links to backends would be "internal".
> > 
> > How do you mean? We have a ton of properties e.g.
> > to control which offloads are allowed for virtio net.
> 
> Why would users set them?
> 
> > It's set internally by machine types to avoid breaking
> > migration.  I don't see any reason for users to set it.
> 
> But they do set it :) albeit only through machine types.  I don't think
> it's different from offloads, just much more specialized.
> 
> Or do you mean that it could go away if we decide to remove very old
> machine types?  I think we would remove compat properties connected to
> those machine types as well, even without "x-".
> 
> Paolo

Then we'll break users who set them directly for some reason.
So x- means "not part of stable ABI".
No?

Re: [Qemu-devel] [PATCH v2 5/6] vmw_pvscsi: The pvscsi device is a PCIE endpoint

[Shmulik Ladkani]

Hi Michael,

On Mon, 14 Dec 2015 19:37:29 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Dec 14, 2015 at 06:14:37PM +0100, Paolo Bonzini wrote:
> > 
> > On 13/12/2015 09:08, Shmulik Ladkani wrote:
> > > +    pvs_k->parent_dc_realize = dc->realize;
> > 
> > Marcel, Michael,
> > 
> > this creates a really nasty dependency on the contents of pci_qdev_realize.
> > 
> > Can you instead change PCIDeviceClass's pc->is_express to a function
> > pointer, and provide a sample implementation pci_is_express_true for the
> > devices that set is_express to true?
> > 
> 
> I'm not very familiar with vmw code, and I dislike overusing callbacks.
> What exactly would this callback do?

Not specific to vmw.

Recently we've made virtio-pci a pcie:
  1811e64c hw/virtio: Add PCIe capability to virtio devices

For migration, 'x-pcie-disable' proprety was introduced.
Thus, instances of virtio-pci may be either pci or pcie.

We'd like to do the same for vmxnet3 and vmw_pvscsi.

This exposes a design limitation.

PCIDeviceClass.is_express is a propery of the class.
All pci_dev instances get the QEMU_PCI_CAP_EXPRESS bit assigned into
their 'cap_present' according to their class 'pc->is_express' value,
early in 'pci_qdev_realize', quote:

static void pci_qdev_realize(DeviceState *qdev, Error **errp)
    ...
    /* initialize cap_present for pci_is_express() and pci_config_size() */
    if (pc->is_express) {
        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
    }


However, we'd like to set QEMU_PCI_CAP_EXPRESS conditionally per
instance.

In order to set QEMU_PCI_CAP_EXPRESS conditionally per instance
(for example, according to the given x-pcie-disable property), the
'parent_dc_realize' trick was suggested.

Reasoning is documented in:
  0560b0e9 virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method

Alas, the same trick needs to be repeated for all classes whose
instances may be either pci or pcie.

What Paolo suggest, is having a callback which can return whether the
device *instance* needs the QEMU_PCI_CAP_EXPRESS bit.

So in 'pci_qdev_realize', instead of:

    if (pc->is_express)
        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;

we'll have something like:

    if (pc->is_express(qdev))
        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;

WDYT?

Regards,
Shmulik

Re: [Qemu-devel] [PATCH v2 5/6] vmw_pvscsi: The pvscsi device is a PCIE endpoint

[Michael S. Tsirkin]

On Mon, Dec 14, 2015 at 11:01:05PM +0200, Shmulik Ladkani wrote:
> Hi Michael,
> 
> On Mon, 14 Dec 2015 19:37:29 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Mon, Dec 14, 2015 at 06:14:37PM +0100, Paolo Bonzini wrote:
> > > 
> > > On 13/12/2015 09:08, Shmulik Ladkani wrote:
> > > > +    pvs_k->parent_dc_realize = dc->realize;
> > > 
> > > Marcel, Michael,
> > > 
> > > this creates a really nasty dependency on the contents of pci_qdev_realize.
> > > 
> > > Can you instead change PCIDeviceClass's pc->is_express to a function
> > > pointer, and provide a sample implementation pci_is_express_true for the
> > > devices that set is_express to true?
> > > 
> > 
> > I'm not very familiar with vmw code, and I dislike overusing callbacks.
> > What exactly would this callback do?
> 
> Not specific to vmw.
> 
> Recently we've made virtio-pci a pcie:
>   1811e64c hw/virtio: Add PCIe capability to virtio devices
> 
> For migration, 'x-pcie-disable' proprety was introduced.
> Thus, instances of virtio-pci may be either pci or pcie.
> 
> We'd like to do the same for vmxnet3 and vmw_pvscsi.
> 
> This exposes a design limitation.
> 
> PCIDeviceClass.is_express is a propery of the class.
> All pci_dev instances get the QEMU_PCI_CAP_EXPRESS bit assigned into
> their 'cap_present' according to their class 'pc->is_express' value,
> early in 'pci_qdev_realize', quote:
> 
> static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>     ...
>     /* initialize cap_present for pci_is_express() and pci_config_size() */
>     if (pc->is_express) {
>         pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>     }
> 
> 
> However, we'd like to set QEMU_PCI_CAP_EXPRESS conditionally per
> instance.
> 
> In order to set QEMU_PCI_CAP_EXPRESS conditionally per instance
> (for example, according to the given x-pcie-disable property), the
> 'parent_dc_realize' trick was suggested.
> 
> Reasoning is documented in:
>   0560b0e9 virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method
> 
> Alas, the same trick needs to be repeated for all classes whose
> instances may be either pci or pcie.

Actually, I think you can just move the code to pci core.
There won't be a need for a callback then.

> What Paolo suggest, is having a callback which can return whether the
> device *instance* needs the QEMU_PCI_CAP_EXPRESS bit.
> 
> So in 'pci_qdev_realize', instead of:
> 
>     if (pc->is_express)
>         pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> 
> we'll have something like:
> 
>     if (pc->is_express(qdev))
>         pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> 
> WDYT?
> 
> Regards,
> Shmulik

Re: [Qemu-devel] [PATCH v2 5/6] vmw_pvscsi: The pvscsi device is a PCIE endpoint

[Shmulik Ladkani]

Hi,

On Mon, 14 Dec 2015 23:10:20 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > In order to set QEMU_PCI_CAP_EXPRESS conditionally per instance
> > (for example, according to the given x-pcie-disable property), the
> > 'parent_dc_realize' trick was suggested.
> > 
> > Reasoning is documented in:
> >   0560b0e9 virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method
> > 
> > Alas, the same trick needs to be repeated for all classes whose
> > instances may be either pci or pcie.
> 
> Actually, I think you can just move the code to pci core.
> There won't be a need for a callback then.

Michael, can you please elaborate?

Regards,
Shmulik

Re: [Qemu-devel] [PATCH v2 6/6] vmw_pvscsi: Introduce 'x-disable-pcie' backword compatability property

[Shmulik Ladkani]

Hi,

On Mon, 14 Dec 2015 20:26:35 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > It's set internally by machine types to avoid breaking
> > > migration.  I don't see any reason for users to set it.
> > 
> > But they do set it :) albeit only through machine types.  I don't think
> > it's different from offloads, just much more specialized.
> > 
> > Or do you mean that it could go away if we decide to remove very old
> > machine types?  I think we would remove compat properties connected to
> > those machine types as well, even without "x-".
> > 
> 
> Then we'll break users who set them directly for some reason.
> So x- means "not part of stable ABI".
> No?

BTW, different drivers use different naming approaches.

E.g. d209c744 'hw/audio/intel-hda: Fix MSI capability address'
suggests a "old_msi_addr" property (yes, underscores) for intel-hda.
Michael, you were the reviewer ;-)

Perhaps we can standartize that "c-" prefix denotes compat properties?

Regards,
Shmulik