[libvirt] [PATCH for-4.0 v3 1/2] virtio: Helper for registering virtio device types

[Eduardo Habkost]

On Tue, Nov 27, 2018 at 11:45:23AM +0100, Cornelia Huck wrote:
> On Tue, 27 Nov 2018 00:49:58 -0200
> Eduardo Habkost <ehabkost at redhat.com> wrote:
> 
> > Introduce a helper for registering different flavours of virtio
> > devices.  Convert code to use the helper, but keep only the
> > existing generic types.  Transitional and non-transitional device
> > types will be added by another patch.
> > 
> > Acked-by: Andrea Bolognani <abologna at redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost at redhat.com>
> > ---
> > Changes v2 -> v3:
> > * Split into two separate patches (type registration helper
> >   and introduction of new types)
> > * Rewrote comments describing each flavour
> > * Rewrote virtio_pci_types_register() completely:
> >   * Replaced magic generation of type names with explicit fields in
> >     VirtioPCIDeviceTypeInfo
> >   * Removed modern_only field (won't be used anymore)
> >   * Don't register a separate base type unless it's required by
> >     the caller
> > ---
> >  hw/virtio/virtio-pci.h        |  54 ++++++++
> >  hw/display/virtio-gpu-pci.c   |   7 +-
> >  hw/display/virtio-vga.c       |   7 +-
> >  hw/virtio/virtio-crypto-pci.c |   7 +-
> >  hw/virtio/virtio-pci.c        | 231 ++++++++++++++++++++++++----------
> >  5 files changed, 228 insertions(+), 78 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > index 813082b0d7..b26731a305 100644
> > --- a/hw/virtio/virtio-pci.h
> > +++ b/hw/virtio/virtio-pci.h
> > @@ -417,4 +417,58 @@ struct VirtIOCryptoPCI {
> >  /* Virtio ABI version, if we increment this, we break the guest driver. */
> >  #define VIRTIO_PCI_ABI_VERSION          0
> >  
> > +/* Input for virtio_pci_types_register() */
> > +typedef struct VirtioPCIDeviceTypeInfo {
> > +    /*
> > +     * Common base class for the subclasses below.
> > +     *
> > +     * Required only if transitional_name or non_transitional_name is set.
> > +     *
> > +     * We need a separate base type instead of making all types
> > +     * inherit from generic_name for two reasons:
> > +     * 1) generic_name implements INTERFACE_PCIE_DEVICE, but
> > +     *    transitional_name don't.
> 
> s/don't/does not/

Fixed, thanks!

> 
> > +     * 2) generic_name has the "disable-legacy" and "disable-modern"
> > +     *    properties, transitional_name and non_transitional name don't.
> > +     */
> > +    const char *base_name;
> > +    /*
> > +     * Generic device type.  Optional.
> > +     *
> > +     * Supports both transitional and non-transitional modes,
> > +     * using the disable-legacy and disable-modern properties.
> > +     * If disable-legacy=auto, (non-)transitional mode is selected
> > +     * depending on the bus where the device is plugged.
> > +     *
> > +     * Implements both INTERFACE_PCIE_DEVICE and INTERFACE_CONVENTIONAL_PCI_DEVICE,
> > +     * but PCI Express is supported only in non-transitional mode.
> > +     *
> > +     * The only type implemented by QEMU 3.1 and older.
> > +     */
> > +    const char *generic_name;
> > +    /*
> > +     * The non-transitional device type.  Optional.
> 
> That's transitional...

Oops.  Fixed, thanks!

> 
> > +     *
> > +     * Implements both INTERFACE_PCIE_DEVICE and INTERFACE_CONVENTIONAL_PCI_DEVICE.
> > +     */
> > +    const char *transitional_name;
> > +    /*
> > +     * The transitional device type.  Optional.
> 
> ...and that's non-transitional :)

Oops.  Fixed, thanks!

> 
> > +     *
> > +     * Implements INTERFACE_CONVENTIONAL_PCI_DEVICE only.
> > +     */
> > +    const char *non_transitional_name;
> > +
> > +    /* Parent type.  If NULL, TYPE_VIRTIO_PCI is used */
> > +    const char *parent;
> > +
> > +    /* Same as TypeInfo fields: */
> > +    size_t instance_size;
> > +    void (*instance_init)(Object *obj);
> > +    void (*class_init)(ObjectClass *klass, void *data);
> > +} VirtioPCIDeviceTypeInfo;
> 
> That's a bit of boilerplate, but the end result seems to be more
> greppable.

Right.  Also, only old devices that have transitional versions
will need to use all fields.  The modern-only devices now
have less boilerplate (because parent is optional).

> 
> > +
> > +/* Register virtio-pci type(s).  @t must be static. */
> > +void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t);
> > +
> >  #endif
> 
> (...)
> 
> Looks sane to me. With the typos fixed,
> 
> Reviewed-by: Cornelia Huck <cohuck at redhat.com>

Thanks!

-- 
Eduardo