Re: [Qemu-devel] [PATCH 17/33] virtio-pci: add flags to enable/disable legacy/modern

[Daniel P. Berrange]

On Thu, Jun 04, 2015 at 12:34:26PM +0200, 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 only legacy is advertized, modern will be turned on by
> default once all remaining spec compilance issues are addressed.

What is the reason for adding these flags? From the pov of management
app integration I think we'd really want the front/back to be automatically
negotiating the right compatible featureset. Having mgmt need to set these
legacy/modern flags based on guest OS needs seems rather painful. As such
I'd be really loathe to support these in libvirt

> 
> Signed-off-by: Gerd Hoffmann <address@hidden>
> Acked-by: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  hw/virtio/virtio-pci.c | 48 ++++++++++++++++++++++++++++++++++--------------
>  hw/virtio/virtio-pci.h |  6 ++++++
>  2 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ddeaf63..2182b41 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1190,6 +1190,8 @@ static void virtio_pci_device_plugged(DeviceState *d, 
> Error **errp)
>  {
>      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;
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> @@ -1198,13 +1200,24 @@ static void virtio_pci_device_plugged(DeviceState *d, 
> Error **errp)
>      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,
> @@ -1318,18 +1331,21 @@ static void virtio_pci_device_plugged(DeviceState *d, 
> Error **errp)
>  
>      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);
> +
> +        pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
> +                         &proxy->bar);
>      }
>  
> -    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);
> -
>      if (!kvm_has_many_ioeventfds()) {
>          proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
>      }
> @@ -1372,6 +1388,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, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 7a6481f..4e9b2db 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
> 
> 

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|