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

["Michael S. Tsirkin" <mst@redhat.com>]

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
> 
>