[PATCH v2 0/5] pci-ids: virtio cleanup

[PATCH v2 0/5] pci-ids: virtio cleanup

[Gerd Hoffmann]

Drop legacy ids for virtio 1.0 devices, they are not needed
and will be overridden anyway by generic virtio pci code.

Improve modern virtio id documentation.

Gerd Hoffmann (5):
  pci-ids: drop PCI_DEVICE_ID_VIRTIO_IOMMU
  pci-ids: drop PCI_DEVICE_ID_VIRTIO_MEM
  pci-ids: drop PCI_DEVICE_ID_VIRTIO_PMEM
  pci-ids: drop PCI_DEVICE_ID_VIRTIO_VSOCK
  pci-ids: drop list of modern virtio devices

 docs/specs/pci-ids.txt           | 15 ++++++---------
 include/hw/pci/pci.h             |  4 ----
 hw/virtio/vhost-user-vsock-pci.c |  2 --
 hw/virtio/vhost-vsock-pci.c      |  2 --
 hw/virtio/virtio-iommu-pci.c     |  4 +---
 hw/virtio/virtio-mem-pci.c       |  2 --
 hw/virtio/virtio-pmem-pci.c      |  2 --
 7 files changed, 7 insertions(+), 24 deletions(-)

-- 
2.37.3

[PATCH v2 1/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_IOMMU

[Gerd Hoffmann]

Not needed for a virtio 1.0 device.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/pci/pci.h         | 1 -
 hw/virtio/virtio-iommu-pci.c | 4 +---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b54b6ef88fc3..89eaca429389 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -85,7 +85,6 @@ extern bool pci_available;
 #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
 #define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
 #define PCI_DEVICE_ID_VIRTIO_PMEM        0x1013
-#define PCI_DEVICE_ID_VIRTIO_IOMMU       0x1014
 #define PCI_DEVICE_ID_VIRTIO_MEM         0x1015
 
 #define PCI_VENDOR_ID_REDHAT             0x1b36
diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
index 844d64770406..79ea8334f04e 100644
--- a/hw/virtio/virtio-iommu-pci.c
+++ b/hw/virtio/virtio-iommu-pci.c
@@ -74,8 +74,6 @@ static void virtio_iommu_pci_class_init(ObjectClass *klass, void *data)
     k->realize = virtio_iommu_pci_realize;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     device_class_set_props(dc, virtio_iommu_pci_properties);
-    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
-    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_IOMMU;
     pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
     pcidev_k->class_id = PCI_CLASS_OTHERS;
     dc->hotpluggable = false;
@@ -90,7 +88,7 @@ static void virtio_iommu_pci_instance_init(Object *obj)
 }
 
 static const VirtioPCIDeviceTypeInfo virtio_iommu_pci_info = {
-    .generic_name          = TYPE_VIRTIO_IOMMU_PCI,
+    .generic_name  = TYPE_VIRTIO_IOMMU_PCI,
     .instance_size = sizeof(VirtIOIOMMUPCI),
     .instance_init = virtio_iommu_pci_instance_init,
     .class_init    = virtio_iommu_pci_class_init,
-- 
2.37.3

[PATCH v2 2/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_MEM

[Gerd Hoffmann]

Not needed for a virtio 1.0 device.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/pci/pci.h       | 1 -
 hw/virtio/virtio-mem-pci.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 89eaca429389..b6aefb33fb17 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -85,7 +85,6 @@ extern bool pci_available;
 #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
 #define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
 #define PCI_DEVICE_ID_VIRTIO_PMEM        0x1013
-#define PCI_DEVICE_ID_VIRTIO_MEM         0x1015
 
 #define PCI_VENDOR_ID_REDHAT             0x1b36
 #define PCI_DEVICE_ID_REDHAT_BRIDGE      0x0001
diff --git a/hw/virtio/virtio-mem-pci.c b/hw/virtio/virtio-mem-pci.c
index be2383b0c522..5c5c1e3ae3da 100644
--- a/hw/virtio/virtio-mem-pci.c
+++ b/hw/virtio/virtio-mem-pci.c
@@ -104,8 +104,6 @@ static void virtio_mem_pci_class_init(ObjectClass *klass, void *data)
 
     k->realize = virtio_mem_pci_realize;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
-    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
-    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_MEM;
     pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
     pcidev_k->class_id = PCI_CLASS_OTHERS;
 
-- 
2.37.3

[PATCH v2 3/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_PMEM

[Gerd Hoffmann]

Not needed for a virtio 1.0 device.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/pci/pci.h        | 1 -
 hw/virtio/virtio-pmem-pci.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b6aefb33fb17..42c83cb5ed00 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -84,7 +84,6 @@ extern bool pci_available;
 #define PCI_DEVICE_ID_VIRTIO_RNG         0x1005
 #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
 #define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
-#define PCI_DEVICE_ID_VIRTIO_PMEM        0x1013
 
 #define PCI_VENDOR_ID_REDHAT             0x1b36
 #define PCI_DEVICE_ID_REDHAT_BRIDGE      0x0001
diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
index 2b2a0b1eae10..7d9f4ec189b9 100644
--- a/hw/virtio/virtio-pmem-pci.c
+++ b/hw/virtio/virtio-pmem-pci.c
@@ -90,8 +90,6 @@ static void virtio_pmem_pci_class_init(ObjectClass *klass, void *data)
 
     k->realize = virtio_pmem_pci_realize;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
-    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
-    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PMEM;
     pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
     pcidev_k->class_id = PCI_CLASS_OTHERS;
 
-- 
2.37.3

[PATCH v2 4/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_VSOCK

[Gerd Hoffmann]

Not needed for a virtio 1.0 device.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/pci/pci.h             | 1 -
 hw/virtio/vhost-user-vsock-pci.c | 2 --
 hw/virtio/vhost-vsock-pci.c      | 2 --
 3 files changed, 5 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 42c83cb5ed00..fe103f35d9d6 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -83,7 +83,6 @@ extern bool pci_available;
 #define PCI_DEVICE_ID_VIRTIO_SCSI        0x1004
 #define PCI_DEVICE_ID_VIRTIO_RNG         0x1005
 #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
-#define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
 
 #define PCI_VENDOR_ID_REDHAT             0x1b36
 #define PCI_DEVICE_ID_REDHAT_BRIDGE      0x0001
diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c
index e5a86e801362..8499b6871f50 100644
--- a/hw/virtio/vhost-user-vsock-pci.c
+++ b/hw/virtio/vhost-user-vsock-pci.c
@@ -55,8 +55,6 @@ static void vhost_user_vsock_pci_class_init(ObjectClass *klass, void *data)
     k->realize = vhost_user_vsock_pci_realize;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     device_class_set_props(dc, vhost_user_vsock_pci_properties);
-    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
-    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_VSOCK;
     pcidev_k->revision = 0x00;
     pcidev_k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
 }
diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c
index 9f34414d3814..170a806b6765 100644
--- a/hw/virtio/vhost-vsock-pci.c
+++ b/hw/virtio/vhost-vsock-pci.c
@@ -65,8 +65,6 @@ static void vhost_vsock_pci_class_init(ObjectClass *klass, void *data)
     k->realize = vhost_vsock_pci_realize;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     device_class_set_props(dc, vhost_vsock_pci_properties);
-    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
-    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_VSOCK;
     pcidev_k->revision = 0x00;
     pcidev_k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
 }
-- 
2.37.3

[PATCH v2 5/5] pci-ids: drop list of modern virtio devices

[Gerd Hoffmann]

Drop the list of modern virtio devices and explain how they
are calculated instead.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/specs/pci-ids.txt | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt
index dd6859d039d0..a289d2de6521 100644
--- a/docs/specs/pci-ids.txt
+++ b/docs/specs/pci-ids.txt
@@ -23,15 +23,12 @@ maintained as part of the virtio specification.
 1af4:1005  entropy generator device (legacy)
 1af4:1009  9p filesystem device (legacy)
 
-1af4:1041  network device (modern)
-1af4:1042  block device (modern)
-1af4:1043  console device (modern)
-1af4:1044  entropy generator device (modern)
-1af4:1045  balloon device (modern)
-1af4:1048  SCSI host bus adapter device (modern)
-1af4:1049  9p filesystem device (modern)
-1af4:1050  virtio gpu device (modern)
-1af4:1052  virtio input device (modern)
+1af4:1040  Start of ID range for modern virtio devices.  The PCI device
+   to      ID is calculated from the virtio device ID by adding the
+1af4:10ef  0x1040 offset.  The virtio IDs are defined in the virtio
+           specification.  The Linux kernel has a header file with
+           defines for all virtio IDs (linux/virtio_ids.h), qemu has a
+           copy in include/standard-headers/.
 
 1af4:10f0  Available for experimental usage without registration.  Must get
    to      official ID when the code leaves the test lab (i.e. when seeking
-- 
2.37.3

Re: [PATCH v2 0/5] pci-ids: virtio cleanup

[David Hildenbrand]

On 30.09.22 15:58, Gerd Hoffmann wrote:
> Drop legacy ids for virtio 1.0 devices, they are not needed
> and will be overridden anyway by generic virtio pci code.
> 
> Improve modern virtio id documentation.
> 
> Gerd Hoffmann (5):
>    pci-ids: drop PCI_DEVICE_ID_VIRTIO_IOMMU
>    pci-ids: drop PCI_DEVICE_ID_VIRTIO_MEM

I remember that for some weird reason, this was required for Linux to 
actually make use of the device. I never figured out why -- maybe that 
changed.

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_IOMMU

[Peter Maydell]

On Fri, 30 Sept 2022 at 14:58, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Not needed for a virtio 1.0 device.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/hw/pci/pci.h         | 1 -
>  hw/virtio/virtio-iommu-pci.c | 4 +---
>  2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index b54b6ef88fc3..89eaca429389 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -85,7 +85,6 @@ extern bool pci_available;
>  #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
>  #define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
>  #define PCI_DEVICE_ID_VIRTIO_PMEM        0x1013
> -#define PCI_DEVICE_ID_VIRTIO_IOMMU       0x1014
>  #define PCI_DEVICE_ID_VIRTIO_MEM         0x1015
>
>  #define PCI_VENDOR_ID_REDHAT             0x1b36
> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
> index 844d64770406..79ea8334f04e 100644
> --- a/hw/virtio/virtio-iommu-pci.c
> +++ b/hw/virtio/virtio-iommu-pci.c
> @@ -74,8 +74,6 @@ static void virtio_iommu_pci_class_init(ObjectClass *klass, void *data)
>      k->realize = virtio_iommu_pci_realize;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>      device_class_set_props(dc, virtio_iommu_pci_properties);
> -    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> -    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_IOMMU;
>      pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
>      pcidev_k->class_id = PCI_CLASS_OTHERS;
>      dc->hotpluggable = false;

So does this mean:
 * these ID values on the device were never used, because some
   other code always overwrites them with the 'modern' ID values?
   If so, what is that other code?
 * this is a change in behaviour? If so, what does the PCI
   device show up as if you don't set these fields at all?
   Do we need to make this only-in-new-machine-versions?

thanks
-- PMM

Re: [PATCH v2 5/5] pci-ids: drop list of modern virtio devices

[Eric Auger]

On 9/30/22 15:58, Gerd Hoffmann wrote:
> Drop the list of modern virtio devices and explain how they
> are calculated instead.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  docs/specs/pci-ids.txt | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt
> index dd6859d039d0..a289d2de6521 100644
> --- a/docs/specs/pci-ids.txt
> +++ b/docs/specs/pci-ids.txt
> @@ -23,15 +23,12 @@ maintained as part of the virtio specification.
>  1af4:1005  entropy generator device (legacy)
>  1af4:1009  9p filesystem device (legacy)
>  
> -1af4:1041  network device (modern)
> -1af4:1042  block device (modern)
> -1af4:1043  console device (modern)
> -1af4:1044  entropy generator device (modern)
> -1af4:1045  balloon device (modern)
> -1af4:1048  SCSI host bus adapter device (modern)
> -1af4:1049  9p filesystem device (modern)
> -1af4:1050  virtio gpu device (modern)
> -1af4:1052  virtio input device (modern)
> +1af4:1040  Start of ID range for modern virtio devices.  The PCI device
> +   to      ID is calculated from the virtio device ID by adding the
> +1af4:10ef  0x1040 offset.  The virtio IDs are defined in the virtio
> +           specification.  The Linux kernel has a header file with
> +           defines for all virtio IDs (linux/virtio_ids.h), qemu has a
> +           copy in include/standard-headers/.
>  
>  1af4:10f0  Available for experimental usage without registration.  Must get
>     to      official ID when the code leaves the test lab (i.e. when seeking

Re: [PATCH v2 1/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_IOMMU

[Eric Auger]

Hi Gerd,

On 9/30/22 15:58, Gerd Hoffmann wrote:
> Not needed for a virtio 1.0 device.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  include/hw/pci/pci.h         | 1 -
>  hw/virtio/virtio-iommu-pci.c | 4 +---
>  2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index b54b6ef88fc3..89eaca429389 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -85,7 +85,6 @@ extern bool pci_available;
>  #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
>  #define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
>  #define PCI_DEVICE_ID_VIRTIO_PMEM        0x1013
> -#define PCI_DEVICE_ID_VIRTIO_IOMMU       0x1014
>  #define PCI_DEVICE_ID_VIRTIO_MEM         0x1015
>  
>  #define PCI_VENDOR_ID_REDHAT             0x1b36
> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
> index 844d64770406..79ea8334f04e 100644
> --- a/hw/virtio/virtio-iommu-pci.c
> +++ b/hw/virtio/virtio-iommu-pci.c
> @@ -74,8 +74,6 @@ static void virtio_iommu_pci_class_init(ObjectClass *klass, void *data)
>      k->realize = virtio_iommu_pci_realize;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>      device_class_set_props(dc, virtio_iommu_pci_properties);
> -    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> -    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_IOMMU;
>      pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
>      pcidev_k->class_id = PCI_CLASS_OTHERS;
>      dc->hotpluggable = false;
> @@ -90,7 +88,7 @@ static void virtio_iommu_pci_instance_init(Object *obj)
>  }
>  
>  static const VirtioPCIDeviceTypeInfo virtio_iommu_pci_info = {
> -    .generic_name          = TYPE_VIRTIO_IOMMU_PCI,
> +    .generic_name  = TYPE_VIRTIO_IOMMU_PCI,
>      .instance_size = sizeof(VirtIOIOMMUPCI),
>      .instance_init = virtio_iommu_pci_instance_init,
>      .class_init    = virtio_iommu_pci_class_init,

Re: [PATCH v2 1/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_IOMMU

[Gerd Hoffmann]

> > diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
> > index 844d64770406..79ea8334f04e 100644
> > --- a/hw/virtio/virtio-iommu-pci.c
> > +++ b/hw/virtio/virtio-iommu-pci.c
> > @@ -74,8 +74,6 @@ static void virtio_iommu_pci_class_init(ObjectClass *klass, void *data)
> >      k->realize = virtio_iommu_pci_realize;
> >      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> >      device_class_set_props(dc, virtio_iommu_pci_properties);
> > -    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> > -    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_IOMMU;
> >      pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
> >      pcidev_k->class_id = PCI_CLASS_OTHERS;
> >      dc->hotpluggable = false;
> 
> So does this mean:
>  * these ID values on the device were never used, because some
>    other code always overwrites them with the 'modern' ID values?

Yes.

>    If so, what is that other code?

searching ...

virtio_pci_device_plugged() does it.
grep for virtio_bus_get_vdev_id to find the exact place.

Guess I should send v3 with that added to the commit messages.

>  * this is a change in behaviour?

No.

take care,
  Gerd

Re: [PATCH v2 4/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_VSOCK

[Stefano Garzarella]

Hi Gerd,

On Fri, Sep 30, 2022 at 03:58:09PM +0200, Gerd Hoffmann wrote:
>Not needed for a virtio 1.0 device.
>
>Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>---
> include/hw/pci/pci.h             | 1 -
> hw/virtio/vhost-user-vsock-pci.c | 2 --
> hw/virtio/vhost-vsock-pci.c      | 2 --
> 3 files changed, 5 deletions(-)
>
>diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>index 42c83cb5ed00..fe103f35d9d6 100644
>--- a/include/hw/pci/pci.h
>+++ b/include/hw/pci/pci.h
>@@ -83,7 +83,6 @@ extern bool pci_available;
> #define PCI_DEVICE_ID_VIRTIO_SCSI        0x1004
> #define PCI_DEVICE_ID_VIRTIO_RNG         0x1005
> #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
>-#define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
>
> #define PCI_VENDOR_ID_REDHAT             0x1b36
> #define PCI_DEVICE_ID_REDHAT_BRIDGE      0x0001
>diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c
>index e5a86e801362..8499b6871f50 100644
>--- a/hw/virtio/vhost-user-vsock-pci.c
>+++ b/hw/virtio/vhost-user-vsock-pci.c
>@@ -55,8 +55,6 @@ static void vhost_user_vsock_pci_class_init(ObjectClass *klass, void *data)
>     k->realize = vhost_user_vsock_pci_realize;
>     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>     device_class_set_props(dc, vhost_user_vsock_pci_properties);
>-    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
>-    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_VSOCK;
>     pcidev_k->revision = 0x00;
>     pcidev_k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
> }
>diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c
>index 9f34414d3814..170a806b6765 100644
>--- a/hw/virtio/vhost-vsock-pci.c
>+++ b/hw/virtio/vhost-vsock-pci.c
>@@ -65,8 +65,6 @@ static void vhost_vsock_pci_class_init(ObjectClass *klass, void *data)
>     k->realize = vhost_vsock_pci_realize;
>     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>     device_class_set_props(dc, vhost_vsock_pci_properties);
>-    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
>-    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_VSOCK;
>     pcidev_k->revision = 0x00;
>     pcidev_k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
> }

Could we have migration issues with this change?

This reminded me that we've had issues already with vsock being 
incorrectly exported as legacy, that we discovered when we added commit 
9b3a35ec82 ("virtio: verify that legacy support is not accidentally 
on").

Then we needed commit d55f518248 ("virtio: skip legacy support check on 
machine types less than 5.1") to avoid migration issues.

And we merged the following commits to force 1.0 in virtio-vsock devices 
for machine types >= 5.1 :
- 6209070503 ("vhost-vsock-pci: force virtio version 1")
- 27eda699f5 ("vhost-user-vsock-pci: force virtio version 1")

Thanks,
Stefano

Re: [PATCH v2 0/5] pci-ids: virtio cleanup

[Gerd Hoffmann]

On Fri, Sep 30, 2022 at 04:22:21PM +0200, David Hildenbrand wrote:
> On 30.09.22 15:58, Gerd Hoffmann wrote:
> > Drop legacy ids for virtio 1.0 devices, they are not needed
> > and will be overridden anyway by generic virtio pci code.
> > 
> > Improve modern virtio id documentation.
> > 
> > Gerd Hoffmann (5):
> >    pci-ids: drop PCI_DEVICE_ID_VIRTIO_IOMMU
> >    pci-ids: drop PCI_DEVICE_ID_VIRTIO_MEM
> 
> I remember that for some weird reason, this was required for Linux to
> actually make use of the device. I never figured out why -- maybe that
> changed.

Looking at 'info pci' the mem + pmem devices look identical with and
without this series ...

  Bus  0, device   3, function 0:
    Class 0255: PCI device 1af4:1058
      PCI subsystem 1af4:1100
      IRQ 11, pin A
      BAR4: 64 bit prefetchable memory at 0xfe000000 [0xfe003fff].
      id ""
  Bus  0, device   4, function 0:
    Class 0255: PCI device 1af4:105b
      PCI subsystem 1af4:1100
      IRQ 10, pin A
      BAR4: 64 bit prefetchable memory at 0xfe004000 [0xfe007fff].
      id ""

... and the PCI ID conforms virtio 1.0 (aka 0x1040 + virtio id).

take care,
  Gerd

Re: [PATCH v2 4/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_VSOCK

[Gerd Hoffmann]

> > diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c
> > index 9f34414d3814..170a806b6765 100644
> > --- a/hw/virtio/vhost-vsock-pci.c
> > +++ b/hw/virtio/vhost-vsock-pci.c
> > @@ -65,8 +65,6 @@ static void vhost_vsock_pci_class_init(ObjectClass *klass, void *data)
> >     k->realize = vhost_vsock_pci_realize;
> >     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> >     device_class_set_props(dc, vhost_vsock_pci_properties);
> > -    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> > -    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_VSOCK;
> >     pcidev_k->revision = 0x00;
> >     pcidev_k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
> > }
> 
> Could we have migration issues with this change?
> 
> This reminded me that we've had issues already with vsock being incorrectly
> exported as legacy, that we discovered when we added commit 9b3a35ec82
> ("virtio: verify that legacy support is not accidentally on").
> 
> Then we needed commit d55f518248 ("virtio: skip legacy support check on
> machine types less than 5.1") to avoid migration issues.
> 
> And we merged the following commits to force 1.0 in virtio-vsock devices for
> machine types >= 5.1 :
> - 6209070503 ("vhost-vsock-pci: force virtio version 1")
> - 27eda699f5 ("vhost-user-vsock-pci: force virtio version 1")

Oh, the virtio_pci_force_virtio_1() call is conditional.  Hmm.

The change will break vsock devices in legacy/transitional mode.  So, if
that is allowed for old machine types for backward compatibility reasons
I guess I should better drop this patch.

take care,
  Gerd

Re: [PATCH v2 0/5] pci-ids: virtio cleanup

[David Hildenbrand]

On 04.10.22 09:44, Gerd Hoffmann wrote:
> On Fri, Sep 30, 2022 at 04:22:21PM +0200, David Hildenbrand wrote:
>> On 30.09.22 15:58, Gerd Hoffmann wrote:
>>> Drop legacy ids for virtio 1.0 devices, they are not needed
>>> and will be overridden anyway by generic virtio pci code.
>>>
>>> Improve modern virtio id documentation.
>>>
>>> Gerd Hoffmann (5):
>>>     pci-ids: drop PCI_DEVICE_ID_VIRTIO_IOMMU
>>>     pci-ids: drop PCI_DEVICE_ID_VIRTIO_MEM
>>
>> I remember that for some weird reason, this was required for Linux to
>> actually make use of the device. I never figured out why -- maybe that
>> changed.
> 
> Looking at 'info pci' the mem + pmem devices look identical with and
> without this series ...
> 
>    Bus  0, device   3, function 0:
>      Class 0255: PCI device 1af4:1058
>        PCI subsystem 1af4:1100
>        IRQ 11, pin A
>        BAR4: 64 bit prefetchable memory at 0xfe000000 [0xfe003fff].
>        id ""
>    Bus  0, device   4, function 0:
>      Class 0255: PCI device 1af4:105b
>        PCI subsystem 1af4:1100
>        IRQ 10, pin A
>        BAR4: 64 bit prefetchable memory at 0xfe004000 [0xfe007fff].
>        id ""
> 
> ... and the PCI ID conforms virtio 1.0 (aka 0x1040 + virtio id).

I just did a quick test and it indeed seems to work as expected. Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_MEM

[David Hildenbrand]

On 30.09.22 15:58, Gerd Hoffmann wrote:
> Not needed for a virtio 1.0 device.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   include/hw/pci/pci.h       | 1 -
>   hw/virtio/virtio-mem-pci.c | 2 --
>   2 files changed, 3 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 89eaca429389..b6aefb33fb17 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -85,7 +85,6 @@ extern bool pci_available;
>   #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
>   #define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
>   #define PCI_DEVICE_ID_VIRTIO_PMEM        0x1013
> -#define PCI_DEVICE_ID_VIRTIO_MEM         0x1015
>   
>   #define PCI_VENDOR_ID_REDHAT             0x1b36
>   #define PCI_DEVICE_ID_REDHAT_BRIDGE      0x0001
> diff --git a/hw/virtio/virtio-mem-pci.c b/hw/virtio/virtio-mem-pci.c
> index be2383b0c522..5c5c1e3ae3da 100644
> --- a/hw/virtio/virtio-mem-pci.c
> +++ b/hw/virtio/virtio-mem-pci.c
> @@ -104,8 +104,6 @@ static void virtio_mem_pci_class_init(ObjectClass *klass, void *data)
>   
>       k->realize = virtio_mem_pci_realize;
>       set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> -    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> -    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_MEM;
>       pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
>       pcidev_k->class_id = PCI_CLASS_OTHERS;
>   

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 3/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_PMEM

[David Hildenbrand]

On 30.09.22 15:58, Gerd Hoffmann wrote:
> Not needed for a virtio 1.0 device.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   include/hw/pci/pci.h        | 1 -
>   hw/virtio/virtio-pmem-pci.c | 2 --
>   2 files changed, 3 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index b6aefb33fb17..42c83cb5ed00 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -84,7 +84,6 @@ extern bool pci_available;
>   #define PCI_DEVICE_ID_VIRTIO_RNG         0x1005
>   #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
>   #define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
> -#define PCI_DEVICE_ID_VIRTIO_PMEM        0x1013
>   
>   #define PCI_VENDOR_ID_REDHAT             0x1b36
>   #define PCI_DEVICE_ID_REDHAT_BRIDGE      0x0001
> diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
> index 2b2a0b1eae10..7d9f4ec189b9 100644
> --- a/hw/virtio/virtio-pmem-pci.c
> +++ b/hw/virtio/virtio-pmem-pci.c
> @@ -90,8 +90,6 @@ static void virtio_pmem_pci_class_init(ObjectClass *klass, void *data)
>   
>       k->realize = virtio_pmem_pci_realize;
>       set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> -    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> -    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PMEM;
>       pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
>       pcidev_k->class_id = PCI_CLASS_OTHERS;
>   

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 4/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_VSOCK

[Cornelia Huck]

On Tue, Oct 04 2022, Gerd Hoffmann <kraxel@redhat.com> wrote:

>> > diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c
>> > index 9f34414d3814..170a806b6765 100644
>> > --- a/hw/virtio/vhost-vsock-pci.c
>> > +++ b/hw/virtio/vhost-vsock-pci.c
>> > @@ -65,8 +65,6 @@ static void vhost_vsock_pci_class_init(ObjectClass *klass, void *data)
>> >     k->realize = vhost_vsock_pci_realize;
>> >     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> >     device_class_set_props(dc, vhost_vsock_pci_properties);
>> > -    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
>> > -    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_VSOCK;
>> >     pcidev_k->revision = 0x00;
>> >     pcidev_k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
>> > }
>> 
>> Could we have migration issues with this change?
>> 
>> This reminded me that we've had issues already with vsock being incorrectly
>> exported as legacy, that we discovered when we added commit 9b3a35ec82
>> ("virtio: verify that legacy support is not accidentally on").
>> 
>> Then we needed commit d55f518248 ("virtio: skip legacy support check on
>> machine types less than 5.1") to avoid migration issues.
>> 
>> And we merged the following commits to force 1.0 in virtio-vsock devices for
>> machine types >= 5.1 :
>> - 6209070503 ("vhost-vsock-pci: force virtio version 1")
>> - 27eda699f5 ("vhost-user-vsock-pci: force virtio version 1")
>
> Oh, the virtio_pci_force_virtio_1() call is conditional.  Hmm.
>
> The change will break vsock devices in legacy/transitional mode.  So, if
> that is allowed for old machine types for backward compatibility reasons
> I guess I should better drop this patch.

Maybe add a comment to prevent others from falling into the same trap?

Re: [PATCH v2 3/5] pci-ids: drop PCI_DEVICE_ID_VIRTIO_PMEM

[Gupta, Pankaj]

On 9/30/2022 3:58 PM, Gerd Hoffmann wrote:
> Not needed for a virtio 1.0 device.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.

Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
Tested-by: Pankaj Gupta <pankaj.gupta@amd.com>

> ---
>   include/hw/pci/pci.h        | 1 -
>   hw/virtio/virtio-pmem-pci.c | 2 --
>   2 files changed, 3 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index b6aefb33fb17..42c83cb5ed00 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -84,7 +84,6 @@ extern bool pci_available;
>   #define PCI_DEVICE_ID_VIRTIO_RNG         0x1005
>   #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
>   #define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
> -#define PCI_DEVICE_ID_VIRTIO_PMEM        0x1013
>   
>   #define PCI_VENDOR_ID_REDHAT             0x1b36
>   #define PCI_DEVICE_ID_REDHAT_BRIDGE      0x0001
> diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
> index 2b2a0b1eae10..7d9f4ec189b9 100644
> --- a/hw/virtio/virtio-pmem-pci.c
> +++ b/hw/virtio/virtio-pmem-pci.c
> @@ -90,8 +90,6 @@ static void virtio_pmem_pci_class_init(ObjectClass *klass, void *data)
>   
>       k->realize = virtio_pmem_pci_realize;
>       set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> -    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> -    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PMEM;
>       pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
>       pcidev_k->class_id = PCI_CLASS_OTHERS;
>