[libvirt] [PATCH for-4.0 v2] virtio: Provide version-specific variants of virtio PCI devices

[Eduardo Habkost]

On Thu, Nov 15, 2018 at 12:21:55PM +0100, Cornelia Huck wrote:
> On Wed, 14 Nov 2018 21:38:31 -0200
> Eduardo Habkost <ehabkost at redhat.com> wrote:
> 
> > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > index 813082b0d7..1d2a11504f 100644
> > --- a/hw/virtio/virtio-pci.h
> > +++ b/hw/virtio/virtio-pci.h
> 
> (...)
> 
> > +/**
> > + * VirtioPCIDeviceTypeInfo:
> > + *
> > + * Template for virtio PCI device types.  See virtio_pci_types_register()
> > + * for details.
> > + */
> > +typedef struct VirtioPCIDeviceTypeInfo {
> > +    /* Prefix for the class names */
> > +    const char *name_prefix;
> 
> <bikeshed>Maybe call this the vpci_name instead? It's not really a
> prefix in the way I would usually use the term, but rather a type name
> with a possible suffix tacked onto it.</bikeshed>

I'm considering getting rid of all magic type name generation,
and make the struct explicit list all the type names to be
registered.  This way people won't be confused when grepping the
code for the type names.


> 
> > +    /* Parent type.  If NULL, TYPE_VIRTIO_PCI is used */
> > +    const char *parent;
> > +    /* If modern_only is true, only <name_prefix> type will be registered */
> > +    bool modern_only;
> > +
> > +    /* Same as TypeInfo fields: */
> > +    size_t instance_size;
> > +    void (*instance_init)(Object *obj);
> > +    void (*class_init)(ObjectClass *klass, void *data);
> > +} VirtioPCIDeviceTypeInfo;
> > +
> > +/*
> > + * Register virtio-pci types.  Each virtio-pci device type will be available
> > + * in 3 flavors:
> > + * - <prefix>-transitional: modern device supporting legacy drivers
> > + *   - Supports Conventional PCI buses only
> > + * - <prefix>-non-transitional: modern-only
> > + *   - Supports Conventional PCI and PCI Express buses
> > + * - <prefix>: virtio version configurable, legacy driver support
> > + *                  automatically selected when device is plugged
> > + *   _ This was the only flavor supported until QEMU 3.1
> 
> s/_/-/
> s/until/up to/ ?

Done.  Thanks!

> 
> > + *   - Supports Conventional PCI and PCI Express buses
> > + *
> > + * All the types above will inherit from "<prefix>-base", so generic
> > + * code can use "<prefix>-base" on type casts.
> > + *
> > + * We need a separate "<prefix>-base" type instead of making all types inherit
> > + * from <prefix> for two reasons:
> > + * 1) <prefix> will implement INTERFACE_PCIE_DEVICE, but
> > + *    <prefix>-transitional won't.
> > + * 2) <prefix> will have the "disable-legacy" and "disable-modern" properties,
> > + *    <prefix>-transitional and <prefix>-non-transitional won't.
> 
> <bikeshed>I'd formulate this rather as "x implements/has something, y
> and z do not", as the code actually does that (and does not just intend
> to do so :)</bikeshed>

Done.  Thanks!

> 
> > + */
> > +void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t);
> > +
> >  #endif
> 
> (...)
> 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index a954799267..0fa248d68c 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> 
> (...)
> 
> > +static void virtio_pci_1_0_instance_init(Object *obj)
> 
> Ditch the _0? I don't expect this to change the name when virtio 1.1
> arrives.

I was going to rename this to
virtio_pci_non_transitional_instance_init() for consistency, but
I forgot.  Thanks for noting.

> 
> > +{
> > +    VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
> > +
> > +    proxy->disable_legacy = ON_OFF_AUTO_ON;
> > +    proxy->disable_modern = false;
> > +}
> 
> (...)
> 
> After a quick look, this seems fine; have not actually tried to run it
> yet.

-- 
Eduardo