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

[Michael S. Tsirkin]

On Thu, Jun 04, 2015 at 11:52:43AM +0100, Daniel P. Berrange wrote:
> 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

Normally people want a transitional device which has both interfaces,
so most people don't set any flags, and we negotiate the best
compatible configuration.

Ability to suppress the modern interface from libvirt seems useful as
a safety latch: makes guest use legacy code.
Ability to suppress the legacy interface from libvirt might
serve to reduce the attack surface slightly.
It also frees up IO resources (which are allocated at boot
whether the interface is used or not) so it useful if
people have a huge # of devices.


To summarise, mgmt never needs to set these automatically,
but it is useful to expose them to users.

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