diff mbox

[RfC,01/15] virtio-pci: add flags to enable/disable legacy/modern

Message ID 1424687012-18524-2-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann Feb. 23, 2015, 10:23 a.m. UTC 
Add VIRTIO_PCI_FLAG_DISABLE_LEGACY and VIRTIO_PCI_FLAG_DISABLE_MODERN
for VirtIOPCIProxy->flags.  Also add properties for them.  They can be
used to disable modern (virtio 1.0) or legacy (virtio 0.9) modes.  By
default both are advertized and the guest driver can choose.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/virtio/virtio-pci.c | 46 +++++++++++++++++++++++++++++++++-------------
 hw/virtio/virtio-pci.h |  6 ++++++
 2 files changed, 39 insertions(+), 13 deletions(-)

Comments

Max Reitz Feb. 26, 2015, 4:41 p.m. UTC | #1 
On 2015-02-23 at 05:23, Gerd Hoffmann wrote:
> Add VIRTIO_PCI_FLAG_DISABLE_LEGACY and VIRTIO_PCI_FLAG_DISABLE_MODERN
> for VirtIOPCIProxy->flags.  Also add properties for them.  They can be
> used to disable modern (virtio 1.0) or legacy (virtio 0.9) modes.  By
> default both are advertized and the guest driver can choose.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/virtio/virtio-pci.c | 46 +++++++++++++++++++++++++++++++++-------------
>   hw/virtio/virtio-pci.h |  6 ++++++
>   2 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 4c9a0b8..6c0c650 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1233,6 +1233,8 @@ static void virtio_pci_device_plugged(DeviceState *d)
>   {
>       VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>       VirtioBusState *bus = &proxy->bus;
> +    bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
> +    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
>       uint8_t *config;
>       uint32_t size;
>   
> @@ -1240,13 +1242,24 @@ static void virtio_pci_device_plugged(DeviceState *d)
>       if (proxy->class_code) {
>           pci_config_set_class(config, proxy->class_code);
>       }
> -    pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
> -                 pci_get_word(config + PCI_VENDOR_ID));
> -    pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
> +
> +    if (legacy) {
> +        /* legacy and transitional */
> +        pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
> +                     pci_get_word(config + PCI_VENDOR_ID));
> +        pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
> +    } else {
> +        /* pure virtio-1.0 */
> +        pci_set_word(config + PCI_VENDOR_ID,
> +                     PCI_VENDOR_ID_REDHAT_QUMRANET);
> +        pci_set_word(config + PCI_DEVICE_ID,
> +                     0x1040 + virtio_bus_get_vdev_id(bus));
> +        pci_config_set_revision(config, 1);
> +    }

Hm, the virtio 1.0 specification from December 2013 states (4.1.1 PCI 
Device Discovery):

 > Any PCI device with Vendor ID 0x1AF4, and Device ID 0x1000 through 
0x103F inclusive is a virtio device.

 > The Subsystem Device ID indicates which virtio device is supported by 
the device. The Subsystem Vendor
 > ID SHOULD reflect the PCI Vendor ID of the environment (it's 
currently only used for informational purposes
 > by the driver).

So you seem to be following a completely different model here by setting 
the device ID to something above 0x103f and not setting subsytem device 
or vendor ID. Because of that device ID, this won't conflict with this 
part of the specification; but it seems that I'm missing some part of it 
which explains why you're doing this the way you're doing it... Can you 
help me with that?

Max

>       config[PCI_INTERRUPT_PIN] = 1;
>   
>   
> -    if (1) { /* TODO: Make this optional, dependent on virtio 1.0 */
> +    if (modern) {
>           struct virtio_pci_cap common = {
>               .cfg_type = VIRTIO_PCI_CAP_COMMON_CFG,
>               .cap_len = sizeof common,
> @@ -1359,17 +1372,20 @@ static void virtio_pci_device_plugged(DeviceState *d)
>   
>       proxy->pci_dev.config_write = virtio_write_config;
>   
> -    size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
> -         + virtio_bus_get_vdev_config_len(bus);
> -    if (size & (size - 1)) {
> -        size = 1 << qemu_fls(size);
> -    }
> +    if (legacy) {
> +        size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
> +            + virtio_bus_get_vdev_config_len(bus);
> +        if (size & (size - 1)) {
> +            size = 1 << qemu_fls(size);
> +        }
>   
> -    memory_region_init_io(&proxy->bar, OBJECT(proxy), &virtio_pci_config_ops,
> -                          proxy, "virtio-pci", size);
> +        memory_region_init_io(&proxy->bar, OBJECT(proxy),
> +                              &virtio_pci_config_ops,
> +                              proxy, "virtio-pci", size);
>   
> -    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
> -                     &proxy->bar);
> +        pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
> +                         &proxy->bar);
> +    }
>   
>       if (!kvm_has_many_ioeventfds()) {
>           proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
> @@ -1416,6 +1432,10 @@ static void virtio_pci_reset(DeviceState *qdev)
>   static Property virtio_pci_properties[] = {
>       DEFINE_PROP_BIT("virtio-pci-bus-master-bug-migration", VirtIOPCIProxy, flags,
>                       VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false),
> +    DEFINE_PROP_BIT("disable-legacy", VirtIOPCIProxy, flags,
> +                    VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false),
> +    DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags,
> +                    VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, false),
>       DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
>       DEFINE_PROP_END_OF_LIST(),
>   };
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 2cddd6a..3068a63 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -63,6 +63,12 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
>   #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
>   #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1 << VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
>   
> +/* virtio version flags */
> +#define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2
> +#define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3
> +#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)
> +#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)
> +
>   typedef struct {
>       MSIMessage msg;
>       int virq;
Gerd Hoffmann Feb. 27, 2015, 11:15 a.m. UTC | #2 
> Hm, the virtio 1.0 specification from December 2013 states (4.1.1 PCI 
> Device Discovery):

Outdated draft, look here for the final specs:

http://docs.oasis-open.org/virtio/virtio/v1.0/

cheers,
  Gerd
Max Reitz Feb. 27, 2015, 2:20 p.m. UTC | #3 
On 2015-02-27 at 06:15, Gerd Hoffmann wrote:
>> Hm, the virtio 1.0 specification from December 2013 states (4.1.1 PCI
>> Device Discovery):
> Outdated draft, look here for the final specs:
>
> http://docs.oasis-open.org/virtio/virtio/v1.0/

Thanks!

Max
Michael S. Tsirkin March 2, 2015, 12:34 p.m. UTC | #4 
On Mon, Feb 23, 2015 at 11:23:17AM +0100, Gerd Hoffmann wrote:
> Add VIRTIO_PCI_FLAG_DISABLE_LEGACY and VIRTIO_PCI_FLAG_DISABLE_MODERN
> for VirtIOPCIProxy->flags.  Also add properties for them.  They can be
> used to disable modern (virtio 1.0) or legacy (virtio 0.9) modes.  By
> default both are advertized and the guest driver can choose.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

I merged patches 1 and 2. I'm guessing you don't want gpu merged yet?

> ---
>  hw/virtio/virtio-pci.c | 46 +++++++++++++++++++++++++++++++++-------------
>  hw/virtio/virtio-pci.h |  6 ++++++
>  2 files changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 4c9a0b8..6c0c650 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1233,6 +1233,8 @@ static void virtio_pci_device_plugged(DeviceState *d)
>  {
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>      VirtioBusState *bus = &proxy->bus;
> +    bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
> +    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
>      uint8_t *config;
>      uint32_t size;
>  
> @@ -1240,13 +1242,24 @@ static void virtio_pci_device_plugged(DeviceState *d)
>      if (proxy->class_code) {
>          pci_config_set_class(config, proxy->class_code);
>      }
> -    pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
> -                 pci_get_word(config + PCI_VENDOR_ID));
> -    pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
> +
> +    if (legacy) {
> +        /* legacy and transitional */
> +        pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
> +                     pci_get_word(config + PCI_VENDOR_ID));
> +        pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
> +    } else {
> +        /* pure virtio-1.0 */
> +        pci_set_word(config + PCI_VENDOR_ID,
> +                     PCI_VENDOR_ID_REDHAT_QUMRANET);
> +        pci_set_word(config + PCI_DEVICE_ID,
> +                     0x1040 + virtio_bus_get_vdev_id(bus));
> +        pci_config_set_revision(config, 1);
> +    }
>      config[PCI_INTERRUPT_PIN] = 1;
>  
>  
> -    if (1) { /* TODO: Make this optional, dependent on virtio 1.0 */
> +    if (modern) {
>          struct virtio_pci_cap common = {
>              .cfg_type = VIRTIO_PCI_CAP_COMMON_CFG,
>              .cap_len = sizeof common,
> @@ -1359,17 +1372,20 @@ static void virtio_pci_device_plugged(DeviceState *d)
>  
>      proxy->pci_dev.config_write = virtio_write_config;
>  
> -    size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
> -         + virtio_bus_get_vdev_config_len(bus);
> -    if (size & (size - 1)) {
> -        size = 1 << qemu_fls(size);
> -    }
> +    if (legacy) {
> +        size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
> +            + virtio_bus_get_vdev_config_len(bus);
> +        if (size & (size - 1)) {
> +            size = 1 << qemu_fls(size);
> +        }
>  
> -    memory_region_init_io(&proxy->bar, OBJECT(proxy), &virtio_pci_config_ops,
> -                          proxy, "virtio-pci", size);
> +        memory_region_init_io(&proxy->bar, OBJECT(proxy),
> +                              &virtio_pci_config_ops,
> +                              proxy, "virtio-pci", size);
>  
> -    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
> -                     &proxy->bar);
> +        pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
> +                         &proxy->bar);
> +    }
>  
>      if (!kvm_has_many_ioeventfds()) {
>          proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
> @@ -1416,6 +1432,10 @@ static void virtio_pci_reset(DeviceState *qdev)
>  static Property virtio_pci_properties[] = {
>      DEFINE_PROP_BIT("virtio-pci-bus-master-bug-migration", VirtIOPCIProxy, flags,
>                      VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false),
> +    DEFINE_PROP_BIT("disable-legacy", VirtIOPCIProxy, flags,
> +                    VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false),
> +    DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags,
> +                    VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, false),
>      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 2cddd6a..3068a63 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -63,6 +63,12 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
>  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
>  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1 << VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
>  
> +/* virtio version flags */
> +#define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2
> +#define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3
> +#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)
> +#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)
> +
>  typedef struct {
>      MSIMessage msg;
>      int virq;
> -- 
> 1.8.3.1
Gerd Hoffmann March 2, 2015, 1:25 p.m. UTC | #5 
Hi,

> I merged patches 1 and 2. I'm guessing you don't want gpu merged yet?

Thanks.  Yes, gpu not yet, and I plan to merge that through my vga queue
anyway (once all virtio dependencies are upstream).

cheers,
  Gerd
diff mbox

Patch

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 4c9a0b8..6c0c650 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1233,6 +1233,8 @@  static void virtio_pci_device_plugged(DeviceState *d)
 {
     VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
     VirtioBusState *bus = &proxy->bus;
+    bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
+    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
     uint8_t *config;
     uint32_t size;
 
@@ -1240,13 +1242,24 @@  static void virtio_pci_device_plugged(DeviceState *d)
     if (proxy->class_code) {
         pci_config_set_class(config, proxy->class_code);
     }
-    pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
-                 pci_get_word(config + PCI_VENDOR_ID));
-    pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
+
+    if (legacy) {
+        /* legacy and transitional */
+        pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
+                     pci_get_word(config + PCI_VENDOR_ID));
+        pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
+    } else {
+        /* pure virtio-1.0 */
+        pci_set_word(config + PCI_VENDOR_ID,
+                     PCI_VENDOR_ID_REDHAT_QUMRANET);
+        pci_set_word(config + PCI_DEVICE_ID,
+                     0x1040 + virtio_bus_get_vdev_id(bus));
+        pci_config_set_revision(config, 1);
+    }
     config[PCI_INTERRUPT_PIN] = 1;
 
 
-    if (1) { /* TODO: Make this optional, dependent on virtio 1.0 */
+    if (modern) {
         struct virtio_pci_cap common = {
             .cfg_type = VIRTIO_PCI_CAP_COMMON_CFG,
             .cap_len = sizeof common,
@@ -1359,17 +1372,20 @@  static void virtio_pci_device_plugged(DeviceState *d)
 
     proxy->pci_dev.config_write = virtio_write_config;
 
-    size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
-         + virtio_bus_get_vdev_config_len(bus);
-    if (size & (size - 1)) {
-        size = 1 << qemu_fls(size);
-    }
+    if (legacy) {
+        size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
+            + virtio_bus_get_vdev_config_len(bus);
+        if (size & (size - 1)) {
+            size = 1 << qemu_fls(size);
+        }
 
-    memory_region_init_io(&proxy->bar, OBJECT(proxy), &virtio_pci_config_ops,
-                          proxy, "virtio-pci", size);
+        memory_region_init_io(&proxy->bar, OBJECT(proxy),
+                              &virtio_pci_config_ops,
+                              proxy, "virtio-pci", size);
 
-    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
-                     &proxy->bar);
+        pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
+                         &proxy->bar);
+    }
 
     if (!kvm_has_many_ioeventfds()) {
         proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
@@ -1416,6 +1432,10 @@  static void virtio_pci_reset(DeviceState *qdev)
 static Property virtio_pci_properties[] = {
     DEFINE_PROP_BIT("virtio-pci-bus-master-bug-migration", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false),
+    DEFINE_PROP_BIT("disable-legacy", VirtIOPCIProxy, flags,
+                    VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false),
+    DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags,
+                    VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, false),
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 2cddd6a..3068a63 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -63,6 +63,12 @@  typedef struct VirtioBusClass VirtioPCIBusClass;
 #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
 #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1 << VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
 
+/* virtio version flags */
+#define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2
+#define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3
+#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)
+#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)
+
 typedef struct {
     MSIMessage msg;
     int virq;