[Qemu-devel] [PATCH 0/5] qdev: compat properties.

[Qemu-devel] [PATCH 0/5] qdev: compat properties.

[Gerd Hoffmann]

  Hi,

This is the proof-of-concept patch posted earlier today splitted into a
nice patch series.  The first adds support for compat attribute lists.
The second adds a new pc-0.10 machine type.  The last three add code and
properties to switch certain virtio devices into compat mode and fill
the compat property list for pc-0.10.

cheers,
  Gerd

[Qemu-devel] [PATCH 1/5] qdev/compat: compat property infrastructure.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/boards.h          |    1 +
 hw/qdev-properties.c |   19 +++++++++++++++++++
 hw/qdev.c            |    1 +
 hw/qdev.h            |   11 +++++++++++
 vl.c                 |    2 ++
 5 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/hw/boards.h b/hw/boards.h
index f6733b7..5a07d07 100644
--- a/hw/boards.h
+++ b/hw/boards.h
@@ -17,6 +17,7 @@ typedef struct QEMUMachine {
     int use_scsi;
     int max_cpus;
     int is_default;
+    struct CompatProperty *compat_props;
     struct QEMUMachine *next;
 } QEMUMachine;
 
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index b4f4f21..5617cab 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -205,3 +205,22 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props)
     }
 }
 
+static CompatProperty *compat_props;
+
+void qdev_register_compat_props(CompatProperty *props)
+{
+    compat_props = props;
+}
+
+void qdev_prop_set_compat(DeviceState *dev)
+{
+    CompatProperty *prop;
+
+    if (!compat_props)
+        return;
+    for (prop = compat_props; prop->driver != NULL; prop++) {
+        if (strcmp(dev->info->name, prop->driver) != 0)
+            continue;
+        qdev_prop_parse(dev, prop->property, prop->value);
+    }
+}
diff --git a/hw/qdev.c b/hw/qdev.c
index baa7d16..fb9de3d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -130,6 +130,7 @@ DeviceState *qdev_create(BusState *bus, const char *name)
     dev->parent_bus = bus;
     qdev_prop_set_defaults(dev, dev->info->props);
     qdev_prop_set_defaults(dev, dev->parent_bus->info->props);
+    qdev_prop_set_compat(dev);
     LIST_INSERT_HEAD(&bus->children, dev, sibling);
     return dev;
 }
diff --git a/hw/qdev.h b/hw/qdev.h
index 3203a3e..ab99ecd 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -8,6 +8,8 @@ typedef struct Property Property;
 
 typedef struct PropertyInfo PropertyInfo;
 
+typedef struct CompatProperty CompatProperty;
+
 typedef struct DeviceInfo DeviceInfo;
 
 typedef struct BusState BusState;
@@ -61,6 +63,12 @@ struct PropertyInfo {
     int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
 };
 
+struct CompatProperty {
+    const char *driver;
+    const char *property;
+    const char *value;
+};
+
 /*** Board API.  This should go away once we have a machine config file.  ***/
 
 DeviceState *qdev_create(BusState *bus, const char *name);
@@ -153,4 +161,7 @@ int qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value);
 int qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 void qdev_prop_set_defaults(DeviceState *dev, Property *props);
 
+void qdev_register_compat_props(CompatProperty *props);
+void qdev_prop_set_compat(DeviceState *dev);
+
 #endif
diff --git a/vl.c b/vl.c
index 3017f7b..30b6ba1 100644
--- a/vl.c
+++ b/vl.c
@@ -6112,6 +6112,8 @@ int main(int argc, char **argv, char **envp)
 
     module_call_init(MODULE_INIT_DEVICE);
 
+    if (machine->compat_props)
+        qdev_register_compat_props(machine->compat_props);
     machine->init(ram_size, boot_devices,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/5] qdev/compat: add pc-0.10 machine type.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pc.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index ccd2fed..6ba6b25 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1457,6 +1457,16 @@ static QEMUMachine pc_machine = {
     .is_default = 1,
 };
 
+static QEMUMachine pc_machine_v0_10 = {
+    .name = "pc-0.10",
+    .desc = "Standard PC, qemu 0.10",
+    .init = pc_init_pci,
+    .max_cpus = 255,
+    .compat_props = (CompatProperty[]) {
+        { /* end of list */ }
+    },
+};
+
 static QEMUMachine isapc_machine = {
     .name = "isapc",
     .desc = "ISA-only PC",
@@ -1467,6 +1477,7 @@ static QEMUMachine isapc_machine = {
 static void pc_machine_init(void)
 {
     qemu_register_machine(&pc_machine);
+    qemu_register_machine(&pc_machine_v0_10);
     qemu_register_machine(&isapc_machine);
 }
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/5] qdev/compat: virtio-blk-pci 0.10 compatibility.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pc.c         |    5 +++++
 hw/virtio-pci.c |   22 ++++++++++++++--------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 6ba6b25..0574283 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1463,6 +1463,11 @@ static QEMUMachine pc_machine_v0_10 = {
     .init = pc_init_pci,
     .max_cpus = 255,
     .compat_props = (CompatProperty[]) {
+        {
+            .driver   = "virtio-blk-pci",
+            .property = "class",
+            .value    = "0x0180", /* PCI_CLASS_STORAGE_OTHER */
+        },
         { /* end of list */ }
     },
 };
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 0b3f41f..3727b0d 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -86,12 +86,7 @@ typedef struct {
     PCIDevice pci_dev;
     VirtIODevice *vdev;
     uint32_t addr;
-
-    uint16_t vendor;
-    uint16_t device;
-    uint16_t subvendor;
-    uint16_t class_code;
-    uint8_t pif;
+    uint32_t class_code;
 } VirtIOPCIProxy;
 
 /* virtio device */
@@ -419,12 +414,15 @@ static void virtio_blk_init_pci(PCIDevice *pci_dev)
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
     VirtIODevice *vdev;
 
+    if (proxy->class_code != PCI_CLASS_STORAGE_SCSI &&
+        proxy->class_code != PCI_CLASS_STORAGE_OTHER)
+        proxy->class_code = PCI_CLASS_STORAGE_SCSI;
+
     vdev = virtio_blk_init(&pci_dev->qdev);
     virtio_init_pci(proxy, vdev,
                     PCI_VENDOR_ID_REDHAT_QUMRANET,
                     PCI_DEVICE_ID_VIRTIO_BLOCK,
-                    PCI_CLASS_STORAGE_OTHER,
-                    0x00);
+                    proxy->class_code, 0x00);
 }
 
 static void virtio_console_init_pci(PCIDevice *pci_dev)
@@ -471,6 +469,14 @@ static PCIDeviceInfo virtio_info[] = {
         .qdev.name = "virtio-blk-pci",
         .qdev.size = sizeof(VirtIOPCIProxy),
         .init      = virtio_blk_init_pci,
+        .qdev.props = (Property[]) {
+            {
+                .name   = "class",
+                .info   = &qdev_prop_hex32,
+                .offset = offsetof(VirtIOPCIProxy, class_code),
+            },
+            {/* end of list */}
+        },
     },{
         .qdev.name  = "virtio-net-pci",
         .qdev.alias = "virtio",
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/5] qdev/compat: virtio-balloon-pci 0.10 compatibility.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pc.c         |    4 ++++
 hw/virtio-pci.c |   16 ++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 0574283..0b7c24b 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1467,6 +1467,10 @@ static QEMUMachine pc_machine_v0_10 = {
             .driver   = "virtio-blk-pci",
             .property = "class",
             .value    = "0x0180", /* PCI_CLASS_STORAGE_OTHER */
+        },{
+            .driver   = "virtio-console-pci",
+            .property = "class",
+            .value    = "0x0380", /* PCI_CLASS_DISPLAY_OTHER */
         },
         { /* end of list */ }
     },
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 3727b0d..9e5343d 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -430,12 +430,16 @@ static void virtio_console_init_pci(PCIDevice *pci_dev)
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
     VirtIODevice *vdev;
 
+    if (proxy->class_code != PCI_CLASS_COMMUNICATION_OTHER &&
+        proxy->class_code != PCI_CLASS_DISPLAY_OTHER && /* qemu 0.10 */
+        proxy->class_code != PCI_CLASS_OTHERS)          /* qemu-kvm  */
+        proxy->class_code = PCI_CLASS_COMMUNICATION_OTHER;
+
     vdev = virtio_console_init(&pci_dev->qdev);
     virtio_init_pci(proxy, vdev,
                     PCI_VENDOR_ID_REDHAT_QUMRANET,
                     PCI_DEVICE_ID_VIRTIO_CONSOLE,
-                    PCI_CLASS_DISPLAY_OTHER,
-                    0x00);
+                    proxy->class_code, 0x00);
 }
 
 static void virtio_net_init_pci(PCIDevice *pci_dev)
@@ -487,6 +491,14 @@ static PCIDeviceInfo virtio_info[] = {
         .qdev.name = "virtio-console-pci",
         .qdev.size = sizeof(VirtIOPCIProxy),
         .init      = virtio_console_init_pci,
+        .qdev.props = (Property[]) {
+            {
+                .name   = "class",
+                .info   = &qdev_prop_hex32,
+                .offset = offsetof(VirtIOPCIProxy, class_code),
+            },
+            {/* end of list */}
+        },
     },{
         .qdev.name = "virtio-balloon-pci",
         .qdev.size = sizeof(VirtIOPCIProxy),
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/5] qdev/compat: virtio-net-pci 0.10 compatibility.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pc.c         |    4 ++++
 hw/virtio-pci.c |   13 ++++++++++++-
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 0b7c24b..ecb618d 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1471,6 +1471,10 @@ static QEMUMachine pc_machine_v0_10 = {
             .driver   = "virtio-console-pci",
             .property = "class",
             .value    = "0x0380", /* PCI_CLASS_DISPLAY_OTHER */
+        },{
+            .driver   = "virtio-net-pci",
+            .property = "msi",
+            .value    = "0",
         },
         { /* end of list */ }
     },
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 9e5343d..d284621 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -87,6 +87,7 @@ typedef struct {
     VirtIODevice *vdev;
     uint32_t addr;
     uint32_t class_code;
+    uint32_t msi;
 } VirtIOPCIProxy;
 
 /* virtio device */
@@ -387,7 +388,8 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
 
     config[0x3d] = 1;
 
-    if (vdev->nvectors && !msix_init(&proxy->pci_dev, vdev->nvectors, 1, 0)) {
+    if (vdev->nvectors && proxy->msi &&
+        !msix_init(&proxy->pci_dev, vdev->nvectors, 1, 0)) {
         pci_register_bar(&proxy->pci_dev, 1,
                          msix_bar_size(&proxy->pci_dev),
                          PCI_ADDRESS_SPACE_MEM,
@@ -487,6 +489,15 @@ static PCIDeviceInfo virtio_info[] = {
         .qdev.size  = sizeof(VirtIOPCIProxy),
         .qdev.class = DEV_CLASS_NETWORK,
         .init       = virtio_net_init_pci,
+        .qdev.props = (Property[]) {
+            {
+                .name   = "msi",
+                .info   = &qdev_prop_uint32,
+                .offset = offsetof(VirtIOPCIProxy, msi),
+                .defval = (uint32_t[]) { 1 }, /* enabled */
+            },
+            {/* end of list */}
+        },
     },{
         .qdev.name = "virtio-console-pci",
         .qdev.size = sizeof(VirtIOPCIProxy),
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 5/5] qdev/compat: virtio-net-pci 0.10 compatibility.

[Michael S. Tsirkin]

On Thu, Jul 09, 2009 at 03:02:33PM +0200, Gerd Hoffmann wrote:
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pc.c         |    4 ++++
>  hw/virtio-pci.c |   13 ++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 0b7c24b..ecb618d 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1471,6 +1471,10 @@ static QEMUMachine pc_machine_v0_10 = {
>              .driver   = "virtio-console-pci",
>              .property = "class",
>              .value    = "0x0380", /* PCI_CLASS_DISPLAY_OTHER */
> +        },{
> +            .driver   = "virtio-net-pci",
> +            .property = "msi",
> +            .value    = "0",
>          },
>          { /* end of list */ }
>      },

The number of vectors is part of hardware config so it's not a good idea
to use a binary property here. Since <= vectors is not a legal msi
configuration, it's enough to implement a "vectors" property instead:
virtio already interprets zero vectors as no msi.  This also matches
command line/monitor syntax, which is a good thing.

Most virtio-net changes become then unnecessary.

-- 
MST

Re: [Qemu-devel] [PATCH 3/5] qdev/compat: virtio-blk-pci 0.10 compatibility.

[Michael S. Tsirkin]

Hi Gerd,
a couple of questions:

On Thu, Jul 09, 2009 at 03:02:31PM +0200, Gerd Hoffmann wrote:
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pc.c         |    5 +++++
>  hw/virtio-pci.c |   22 ++++++++++++++--------
>  2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 6ba6b25..0574283 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1463,6 +1463,11 @@ static QEMUMachine pc_machine_v0_10 = {
>      .init = pc_init_pci,
>      .max_cpus = 255,
>      .compat_props = (CompatProperty[]) {
> +        {
> +            .driver   = "virtio-blk-pci",
> +            .property = "class",
> +            .value    = "0x0180", /* PCI_CLASS_STORAGE_OTHER */

it seems annoying that we can't use the symbolic name. Ideas how to fix this?

> +        },
>          { /* end of list */ }
>      },
>  };
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 0b3f41f..3727b0d 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -86,12 +86,7 @@ typedef struct {
>      PCIDevice pci_dev;
>      VirtIODevice *vdev;
>      uint32_t addr;
> -
> -    uint16_t vendor;
> -    uint16_t device;
> -    uint16_t subvendor;
> -    uint16_t class_code;
> -    uint8_t pif;

Are the other fields unused? If yes can be a separate patch ...

> +    uint32_t class_code;
>  } VirtIOPCIProxy;
>  
>  /* virtio device */
> @@ -419,12 +414,15 @@ static void virtio_blk_init_pci(PCIDevice *pci_dev)
>      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>      VirtIODevice *vdev;
>  
> +    if (proxy->class_code != PCI_CLASS_STORAGE_SCSI &&
> +        proxy->class_code != PCI_CLASS_STORAGE_OTHER)
> +        proxy->class_code = PCI_CLASS_STORAGE_SCSI;
> +

what does this do?

>      vdev = virtio_blk_init(&pci_dev->qdev);
>      virtio_init_pci(proxy, vdev,
>                      PCI_VENDOR_ID_REDHAT_QUMRANET,
>                      PCI_DEVICE_ID_VIRTIO_BLOCK,
> -                    PCI_CLASS_STORAGE_OTHER,
> -                    0x00);
> +                    proxy->class_code, 0x00);
>  }
>  
>  static void virtio_console_init_pci(PCIDevice *pci_dev)

does this mean that virtio block was broken by some previous
patch? It's not a good way to split changes: bisecting won't work.

> @@ -471,6 +469,14 @@ static PCIDeviceInfo virtio_info[] = {
>          .qdev.name = "virtio-blk-pci",
>          .qdev.size = sizeof(VirtIOPCIProxy),
>          .init      = virtio_blk_init_pci,
> +        .qdev.props = (Property[]) {
> +            {
> +                .name   = "class",
> +                .info   = &qdev_prop_hex32,
> +                .offset = offsetof(VirtIOPCIProxy, class_code),
> +            },
> +            {/* end of list */}
> +        },
>      },{
>          .qdev.name  = "virtio-net-pci",
>          .qdev.alias = "virtio",
> -- 
> 1.6.2.5
> 
> 

Re: [Qemu-devel] [PATCH 3/5] qdev/compat: virtio-blk-pci 0.10 compatibility.

[Gerd Hoffmann]

On 07/09/09 16:52, Michael S. Tsirkin wrote:
>>       .compat_props = (CompatProperty[]) {
>> +        {
>> +            .driver   = "virtio-blk-pci",
>> +            .property = "class",
>> +            .value    = "0x0180", /* PCI_CLASS_STORAGE_OTHER */
>
> it seems annoying that we can't use the symbolic name. Ideas how to fix this?

We could add a special property type instead of using hex32.  Then we 
can have a string <-> int mapping and use something like

              .value = "storage-other",

Not sure it is worth the trouble though.

>> -
>> -    uint16_t vendor;
>> -    uint16_t device;
>> -    uint16_t subvendor;
>> -    uint16_t class_code;
>> -    uint8_t pif;
>
> Are the other fields unused? If yes can be a separate patch ...

Yes, they are all unused to date.  This patch puts class_code into use.

>> +    if (proxy->class_code != PCI_CLASS_STORAGE_SCSI&&
>> +        proxy->class_code != PCI_CLASS_STORAGE_OTHER)
>> +        proxy->class_code = PCI_CLASS_STORAGE_SCSI;
>> +
>
> what does this do?

Make sure proxy->class_code has one of the two allowed values.

>>       vdev = virtio_blk_init(&pci_dev->qdev);
>>       virtio_init_pci(proxy, vdev,
>>                       PCI_VENDOR_ID_REDHAT_QUMRANET,
>>                       PCI_DEVICE_ID_VIRTIO_BLOCK,
>> -                    PCI_CLASS_STORAGE_OTHER,
>> -                    0x00);
>> +                    proxy->class_code, 0x00);
>>   }
>>
>>   static void virtio_console_init_pci(PCIDevice *pci_dev)
>
> does this mean that virtio block was broken by some previous
> patch? It's not a good way to split changes: bisecting won't work.

Huh?  virtio block wasn't broken.  What makes you think it was?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 5/5] qdev/compat: virtio-net-pci 0.10 compatibility.

[Gerd Hoffmann]

On 07/09/09 16:40, Michael S. Tsirkin wrote:
> The number of vectors is part of hardware config so it's not a good idea
> to use a binary property here. Since<= vectors is not a legal msi
> configuration, it's enough to implement a "vectors" property instead:
> virtio already interprets zero vectors as no msi.  This also matches
> command line/monitor syntax, which is a good thing.

Ok, it makes sense to have a vectors property then.
Will rewrite the patch.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/5] qdev/compat: virtio-blk-pci 0.10 compatibility.

[Anthony Liguori]

Michael S. Tsirkin wrote:
> Hi Gerd,
> a couple of questions:
>
> On Thu, Jul 09, 2009 at 03:02:31PM +0200, Gerd Hoffmann wrote:
>   
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  hw/pc.c         |    5 +++++
>>  hw/virtio-pci.c |   22 ++++++++++++++--------
>>  2 files changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 6ba6b25..0574283 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -1463,6 +1463,11 @@ static QEMUMachine pc_machine_v0_10 = {
>>      .init = pc_init_pci,
>>      .max_cpus = 255,
>>      .compat_props = (CompatProperty[]) {
>> +        {
>> +            .driver   = "virtio-blk-pci",
>> +            .property = "class",
>> +            .value    = "0x0180", /* PCI_CLASS_STORAGE_OTHER */
>>     
>
> it seems annoying that we can't use the symbolic name. Ideas how to fix this?
>   

#define strify_i(a) # a
#define strify(a) strify_i(a)

.value = strify(PCI_CLASS_STORAGE_OTHER),

Will expand to:

.value = "0x0180",

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/5] qdev/compat: virtio-blk-pci 0.10 compatibility.

[Michael S. Tsirkin]

On Thu, Jul 09, 2009 at 05:09:41PM +0200, Gerd Hoffmann wrote:
> On 07/09/09 16:52, Michael S. Tsirkin wrote:
>>>       .compat_props = (CompatProperty[]) {
>>> +        {
>>> +            .driver   = "virtio-blk-pci",
>>> +            .property = "class",
>>> +            .value    = "0x0180", /* PCI_CLASS_STORAGE_OTHER */
>>
>> it seems annoying that we can't use the symbolic name. Ideas how to fix this?
>
> We could add a special property type instead of using hex32.  Then we  
> can have a string <-> int mapping and use something like
>
>              .value = "storage-other",
>
> Not sure it is worth the trouble though.
>
>>> -
>>> -    uint16_t vendor;
>>> -    uint16_t device;
>>> -    uint16_t subvendor;
>>> -    uint16_t class_code;
>>> -    uint8_t pif;
>>
>> Are the other fields unused? If yes can be a separate patch ...
>
> Yes, they are all unused to date.  This patch puts class_code into use.
>
>>> +    if (proxy->class_code != PCI_CLASS_STORAGE_SCSI&&
>>> +        proxy->class_code != PCI_CLASS_STORAGE_OTHER)
>>> +        proxy->class_code = PCI_CLASS_STORAGE_SCSI;
>>> +
>>
>> what does this do?
>
> Make sure proxy->class_code has one of the two allowed values.
>
>>>       vdev = virtio_blk_init(&pci_dev->qdev);
>>>       virtio_init_pci(proxy, vdev,
>>>                       PCI_VENDOR_ID_REDHAT_QUMRANET,
>>>                       PCI_DEVICE_ID_VIRTIO_BLOCK,
>>> -                    PCI_CLASS_STORAGE_OTHER,
>>> -                    0x00);
>>> +                    proxy->class_code, 0x00);
>>>   }
>>>
>>>   static void virtio_console_init_pci(PCIDevice *pci_dev)
>>
>> does this mean that virtio block was broken by some previous
>> patch? It's not a good way to split changes: bisecting won't work.
>
> Huh?  virtio block wasn't broken.  What makes you think it was?
>
> cheers,
>   Gerd

This is adding a parameter to a function. Before this patch it was missing
this parameter so was broken?

Re: [Qemu-devel] [PATCH 3/5] qdev/compat: virtio-blk-pci 0.10 compatibility.

[Filip Navara]

On Thu, Jul 9, 2009 at 8:20 PM, Michael S. Tsirkin<mst@redhat.com> wrote:
> On Thu, Jul 09, 2009 at 05:09:41PM +0200, Gerd Hoffmann wrote:
>> On 07/09/09 16:52, Michael S. Tsirkin wrote:
>>>>       .compat_props = (CompatProperty[]) {
>>>> +        {
>>>> +            .driver   = "virtio-blk-pci",
>>>> +            .property = "class",
>>>> +            .value    = "0x0180", /* PCI_CLASS_STORAGE_OTHER */
>>>
>>> it seems annoying that we can't use the symbolic name. Ideas how to fix this?
>>
>> We could add a special property type instead of using hex32.  Then we
>> can have a string <-> int mapping and use something like
>>
>>              .value = "storage-other",
>>
>> Not sure it is worth the trouble though.
>>
>>>> -
>>>> -    uint16_t vendor;
>>>> -    uint16_t device;
>>>> -    uint16_t subvendor;
>>>> -    uint16_t class_code;
>>>> -    uint8_t pif;
>>>
>>> Are the other fields unused? If yes can be a separate patch ...
>>
>> Yes, they are all unused to date.  This patch puts class_code into use.
>>
>>>> +    if (proxy->class_code != PCI_CLASS_STORAGE_SCSI&&
>>>> +        proxy->class_code != PCI_CLASS_STORAGE_OTHER)
>>>> +        proxy->class_code = PCI_CLASS_STORAGE_SCSI;
>>>> +
>>>
>>> what does this do?
>>
>> Make sure proxy->class_code has one of the two allowed values.
>>
>>>>       vdev = virtio_blk_init(&pci_dev->qdev);
>>>>       virtio_init_pci(proxy, vdev,
>>>>                       PCI_VENDOR_ID_REDHAT_QUMRANET,
>>>>                       PCI_DEVICE_ID_VIRTIO_BLOCK,
>>>> -                    PCI_CLASS_STORAGE_OTHER,
>>>> -                    0x00);
>>>> +                    proxy->class_code, 0x00);
>>>>   }
>>>>
>>>>   static void virtio_console_init_pci(PCIDevice *pci_dev)
>>>
>>> does this mean that virtio block was broken by some previous
>>> patch? It's not a good way to split changes: bisecting won't work.
>>
>> Huh?  virtio block wasn't broken.  What makes you think it was?
>>
>> cheers,
>>   Gerd
>
> This is adding a parameter to a function. Before this patch it was missing
> this parameter so was broken?
>
It is not! Two parameters removed, two added.

F.

Re: [Qemu-devel] [PATCH 3/5] qdev/compat: virtio-blk-pci 0.10 compatibility.

[Michael S. Tsirkin]

On Thu, Jul 09, 2009 at 09:42:47PM +0200, Filip Navara wrote:
> On Thu, Jul 9, 2009 at 8:20 PM, Michael S. Tsirkin<mst@redhat.com> wrote:
> > On Thu, Jul 09, 2009 at 05:09:41PM +0200, Gerd Hoffmann wrote:
> >> On 07/09/09 16:52, Michael S. Tsirkin wrote:
> >>>>       .compat_props = (CompatProperty[]) {
> >>>> +        {
> >>>> +            .driver   = "virtio-blk-pci",
> >>>> +            .property = "class",
> >>>> +            .value    = "0x0180", /* PCI_CLASS_STORAGE_OTHER */
> >>>
> >>> it seems annoying that we can't use the symbolic name. Ideas how to fix this?
> >>
> >> We could add a special property type instead of using hex32.  Then we
> >> can have a string <-> int mapping and use something like
> >>
> >>              .value = "storage-other",
> >>
> >> Not sure it is worth the trouble though.
> >>
> >>>> -
> >>>> -    uint16_t vendor;
> >>>> -    uint16_t device;
> >>>> -    uint16_t subvendor;
> >>>> -    uint16_t class_code;
> >>>> -    uint8_t pif;
> >>>
> >>> Are the other fields unused? If yes can be a separate patch ...
> >>
> >> Yes, they are all unused to date.  This patch puts class_code into use.
> >>
> >>>> +    if (proxy->class_code != PCI_CLASS_STORAGE_SCSI&&
> >>>> +        proxy->class_code != PCI_CLASS_STORAGE_OTHER)
> >>>> +        proxy->class_code = PCI_CLASS_STORAGE_SCSI;
> >>>> +
> >>>
> >>> what does this do?
> >>
> >> Make sure proxy->class_code has one of the two allowed values.
> >>
> >>>>       vdev = virtio_blk_init(&pci_dev->qdev);
> >>>>       virtio_init_pci(proxy, vdev,
> >>>>                       PCI_VENDOR_ID_REDHAT_QUMRANET,
> >>>>                       PCI_DEVICE_ID_VIRTIO_BLOCK,
> >>>> -                    PCI_CLASS_STORAGE_OTHER,
> >>>> -                    0x00);
> >>>> +                    proxy->class_code, 0x00);
> >>>>   }
> >>>>
> >>>>   static void virtio_console_init_pci(PCIDevice *pci_dev)
> >>>
> >>> does this mean that virtio block was broken by some previous
> >>> patch? It's not a good way to split changes: bisecting won't work.
> >>
> >> Huh?  virtio block wasn't broken.  What makes you think it was?
> >>
> >> cheers,
> >>   Gerd
> >
> > This is adding a parameter to a function. Before this patch it was missing
> > this parameter so was broken?
> >
> It is not! Two parameters removed, two added.
> 

Looks like my eyes need to be checked. Sorry about the noise.

-- 
MST