On Tue, Nov 27, 2018 at 11:45:23AM +0100, Cornelia Huck wrote:
> On Tue, 27 Nov 2018 00:49:58 -0200
> Eduardo Habkost <ehabk...@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 <abolo...@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabk...@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 <coh...@redhat.com>
Thanks!
--
Eduardo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
