[v2,00/38] pci: initialize ids in pci common code

On Wed, May 18, 2011 at 12:17:46PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 18, 2011 at 01:55:17AM +0900, Isaku Yamahata wrote:
> > vender id/device id... in pci configuration space are read-only registers
> > which are commonly defined for all pci devices.
> > So initialize them in common code and it simplifies the initialization a bit.
> > I didn't converted virtio-pci and qxl because it determines ids dynaically.
> > So I'll leave those conversion (or not to convert) to the authors.
> 
> Hmm, virtio doesn't:
> static PCIDeviceInfo virtio_info[] = {
> 	...
> }
> 
> has the array of devices.

Okay. I missed it somehow. I get the following, And I'll leave
qxl convection to Gerd.
The remaining issue is, should I adopt/drop prog_interface conversion?


From c9834234c73eb03d764a3c999cbd34f4814a5553 Mon Sep 17 00:00:00 2001
Message-Id: <c9834234c73eb03d764a3c999cbd34f4814a5553.1305715603.git.yamahata@valinux.co.jp>
In-Reply-To: <cover.1305715602.git.yamahata@valinux.co.jp>
References: <cover.1305715602.git.yamahata@valinux.co.jp>
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Wed, 18 May 2011 19:46:04 +0900
Subject: [PATCH] virtio-pci.c:  convert to PCIDEviceInfo to initialize ids

use PCIDeviceInfo to initialize ids.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/virtio-pci.c |   69 ++++++++++++++++++++++++------------------------------
 1 files changed, 31 insertions(+), 38 deletions(-)

On Wed, May 18, 2011 at 07:55:17PM +0900, Isaku Yamahata wrote:
> On Wed, May 18, 2011 at 12:17:46PM +0300, Michael S. Tsirkin wrote:
> > On Wed, May 18, 2011 at 01:55:17AM +0900, Isaku Yamahata wrote:
> > > vender id/device id... in pci configuration space are read-only registers
> > > which are commonly defined for all pci devices.
> > > So initialize them in common code and it simplifies the initialization a bit.
> > > I didn't converted virtio-pci and qxl because it determines ids dynaically.
> > > So I'll leave those conversion (or not to convert) to the authors.
> > 
> > Hmm, virtio doesn't:
> > static PCIDeviceInfo virtio_info[] = {
> > 	...
> > }
> > 
> > has the array of devices.
> 
> Okay. I missed it somehow. I get the following, And I'll leave
> qxl convection to Gerd.
> The remaining issue is, should I adopt/drop prog_interface conversion?

Drop it I think. I don't see what it buys us.

The point with all this work IMO is to be able
to sort devices by type and
to show device vendor/type/revision in -help as well as
to identify device by vendor/device/revision
instead of the arbitrary qemu names on the monitor.
We can use numeric values, as well as parse pci.ids
if present on system for symbolic ones.

revision is sometimes 0 and initialized later by devices,
so if it's 0 we don't really know what it is, but it's
a bit less important I guess, so while I'm not 100%
we should have it in the table, I'm not sure we shouldn't either.

However none of this seems to apply to prog_interface which is
useful for the OS but not that useful for the user.

> >From c9834234c73eb03d764a3c999cbd34f4814a5553 Mon Sep 17 00:00:00 2001
> Message-Id: <c9834234c73eb03d764a3c999cbd34f4814a5553.1305715603.git.yamahata@valinux.co.jp>
> In-Reply-To: <cover.1305715602.git.yamahata@valinux.co.jp>
> References: <cover.1305715602.git.yamahata@valinux.co.jp>
> From: Isaku Yamahata <yamahata@valinux.co.jp>
> Date: Wed, 18 May 2011 19:46:04 +0900
> Subject: [PATCH] virtio-pci.c:  convert to PCIDEviceInfo to initialize ids
> 
> use PCIDeviceInfo to initialize ids.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/virtio-pci.c |   69 ++++++++++++++++++++++++------------------------------
>  1 files changed, 31 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index c19629d..270e2c7 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -669,9 +669,7 @@ static const VirtIOBindings virtio_pci_bindings = {
>      .vmstate_change = virtio_pci_vmstate_change,
>  };
>  
> -static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
> -                            uint16_t vendor, uint16_t device,
> -                            uint16_t class_code, uint8_t pif)
> +static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
>  {
>      uint8_t *config;
>      uint32_t size;
> @@ -679,19 +677,12 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
>      proxy->vdev = vdev;
>  
>      config = proxy->pci_dev.config;
> -    pci_config_set_vendor_id(config, vendor);
> -    pci_config_set_device_id(config, device);
> -
> -    config[0x08] = VIRTIO_PCI_ABI_VERSION;
> -
> -    config[0x09] = pif;
> -    pci_config_set_class(config, class_code);
> -
> -    config[0x2c] = vendor & 0xFF;
> -    config[0x2d] = (vendor >> 8) & 0xFF;
> -    config[0x2e] = vdev->device_id & 0xFF;
> -    config[0x2f] = (vdev->device_id >> 8) & 0xFF;
>  
> +    if (proxy->class_code) {
> +        pci_config_set_class(config, proxy->class_code);
> +    }
> +    pci_set_word(config + 0x2c, pci_get_word(config + PCI_VENDOR_ID));
> +    pci_set_word(config + 0x2e, vdev->device_id);
>      config[0x3d] = 1;
>  
>      if (vdev->nvectors && !msix_init(&proxy->pci_dev, vdev->nvectors, 1, 0)) {
> @@ -735,10 +726,7 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev)
>          return -1;
>      }
>      vdev->nvectors = proxy->nvectors;
> -    virtio_init_pci(proxy, vdev,
> -                    PCI_VENDOR_ID_REDHAT_QUMRANET,
> -                    PCI_DEVICE_ID_VIRTIO_BLOCK,
> -                    proxy->class_code, 0x00);
> +    virtio_init_pci(proxy, vdev);
>      /* make the actual value visible */
>      proxy->nvectors = vdev->nvectors;
>      return 0;
> @@ -776,10 +764,7 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
>      vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
>                                          ? proxy->serial.max_virtserial_ports + 1
>                                          : proxy->nvectors;
> -    virtio_init_pci(proxy, vdev,
> -                    PCI_VENDOR_ID_REDHAT_QUMRANET,
> -                    PCI_DEVICE_ID_VIRTIO_CONSOLE,
> -                    proxy->class_code, 0x00);
> +    virtio_init_pci(proxy, vdev);
>      proxy->nvectors = vdev->nvectors;
>      return 0;
>  }
> @@ -801,11 +786,7 @@ static int virtio_net_init_pci(PCIDevice *pci_dev)
>      vdev = virtio_net_init(&pci_dev->qdev, &proxy->nic, &proxy->net);
>  
>      vdev->nvectors = proxy->nvectors;
> -    virtio_init_pci(proxy, vdev,
> -                    PCI_VENDOR_ID_REDHAT_QUMRANET,
> -                    PCI_DEVICE_ID_VIRTIO_NET,
> -                    PCI_CLASS_NETWORK_ETHERNET,
> -                    0x00);
> +    virtio_init_pci(proxy, vdev);
>  
>      /* make the actual value visible */
>      proxy->nvectors = vdev->nvectors;
> @@ -827,11 +808,7 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
>      VirtIODevice *vdev;
>  
>      vdev = virtio_balloon_init(&pci_dev->qdev);
> -    virtio_init_pci(proxy, vdev,
> -                    PCI_VENDOR_ID_REDHAT_QUMRANET,
> -                    PCI_DEVICE_ID_VIRTIO_BALLOON,
> -                    PCI_CLASS_MEMORY_RAM,
> -                    0x00);
> +    virtio_init_pci(proxy, vdev);
>      return 0;
>  }
>  
> @@ -843,11 +820,7 @@ static int virtio_9p_init_pci(PCIDevice *pci_dev)
>  
>      vdev = virtio_9p_init(&pci_dev->qdev, &proxy->fsconf);
>      vdev->nvectors = proxy->nvectors;
> -    virtio_init_pci(proxy, vdev,
> -                    PCI_VENDOR_ID_REDHAT_QUMRANET,
> -                    0x1009,
> -                    0x2,
> -                    0x00);
> +    virtio_init_pci(proxy, vdev);
>      /* make the actual value visible */
>      proxy->nvectors = vdev->nvectors;
>      return 0;
> @@ -861,6 +834,10 @@ static PCIDeviceInfo virtio_info[] = {
>          .qdev.size = sizeof(VirtIOPCIProxy),
>          .init      = virtio_blk_init_pci,
>          .exit      = virtio_blk_exit_pci,
> +        .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET,
> +        .device_id = PCI_DEVICE_ID_VIRTIO_BLOCK,
> +        .revision  = VIRTIO_PCI_ABI_VERSION,
> +        .class_id  = PCI_CLASS_STORAGE_SCSI,
>          .qdev.props = (Property[]) {
>              DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>              DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, block),
> @@ -878,6 +855,10 @@ static PCIDeviceInfo virtio_info[] = {
>          .init       = virtio_net_init_pci,
>          .exit       = virtio_net_exit_pci,
>          .romfile    = "pxe-virtio.rom",
> +        .vendor_id  = PCI_VENDOR_ID_REDHAT_QUMRANET,
> +        .device_id  = PCI_DEVICE_ID_VIRTIO_NET,
> +        .revision   = VIRTIO_PCI_ABI_VERSION,
> +        .class_id   = PCI_CLASS_NETWORK_ETHERNET,
>          .qdev.props = (Property[]) {
>              DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
>                              VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
> @@ -898,6 +879,10 @@ static PCIDeviceInfo virtio_info[] = {
>          .qdev.size = sizeof(VirtIOPCIProxy),
>          .init      = virtio_serial_init_pci,
>          .exit      = virtio_serial_exit_pci,
> +        .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET,
> +        .device_id = PCI_DEVICE_ID_VIRTIO_CONSOLE,
> +        .revision  = VIRTIO_PCI_ABI_VERSION,
> +        .class_id  = PCI_CLASS_COMMUNICATION_OTHER,
>          .qdev.props = (Property[]) {
>              DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
>                              VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> @@ -916,6 +901,10 @@ static PCIDeviceInfo virtio_info[] = {
>          .qdev.size = sizeof(VirtIOPCIProxy),
>          .init      = virtio_balloon_init_pci,
>          .exit      = virtio_exit_pci,
> +        .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET,
> +        .device_id = PCI_DEVICE_ID_VIRTIO_BALLOON,
> +        .revision  = VIRTIO_PCI_ABI_VERSION,
> +        .class_id  = PCI_CLASS_MEMORY_RAM,
>          .qdev.props = (Property[]) {
>              DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
>              DEFINE_PROP_END_OF_LIST(),
> @@ -927,6 +916,10 @@ static PCIDeviceInfo virtio_info[] = {
>          .qdev.alias = "virtio-9p",
>          .qdev.size = sizeof(VirtIOPCIProxy),
>          .init      = virtio_9p_init_pci,
> +        .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET,
> +        .device_id = 0x1009,
> +        .revision  = VIRTIO_PCI_ABI_VERSION,
> +        .class_id  = 0x2,
>          .qdev.props = (Property[]) {
>              DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>              DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> -- 
> 1.7.1.1
> 
> 
> -- 
> yamahata

On Wed, May 18, 2011 at 07:55:17PM +0900, Isaku Yamahata wrote:
> On Wed, May 18, 2011 at 12:17:46PM +0300, Michael S. Tsirkin wrote:
> > On Wed, May 18, 2011 at 01:55:17AM +0900, Isaku Yamahata wrote:
> > > vender id/device id... in pci configuration space are read-only registers
> > > which are commonly defined for all pci devices.
> > > So initialize them in common code and it simplifies the initialization a bit.
> > > I didn't converted virtio-pci and qxl because it determines ids dynaically.
> > > So I'll leave those conversion (or not to convert) to the authors.
> > 
> > Hmm, virtio doesn't:
> > static PCIDeviceInfo virtio_info[] = {
> > 	...
> > }
> > 
> > has the array of devices.
> 
> Okay. I missed it somehow. I get the following, And I'll leave
> qxl convection to Gerd.

Well the devel device id can go it seems, but we
need think what to do with the revision.
Maybe it's not so terrible that revision will
be wrong in -help for qxl - any better ideas?

Hi,

> Well the devel device id can go it seems, but we
> need think what to do with the revision.
> Maybe it's not so terrible that revision will
> be wrong in -help for qxl - any better ideas?

Fine with me.  Default is 2 and it can be set to 1 using rev=1 property 
if needed.  Help should show rev 2.

cheers,
   Gerd

On Wed, May 18, 2011 at 03:07:21PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >Well the devel device id can go it seems, but we
> >need think what to do with the revision.
> >Maybe it's not so terrible that revision will
> >be wrong in -help for qxl - any better ideas?
> 
> Fine with me.  Default is 2 and it can be set to 1 using rev=1
> property if needed.  Help should show rev 2.
> 
> cheers,
>   Gerd

I think it's easier to not show the revision ...