Re: [libvirt] [PATCH for-4.0 v3 1/2] virtio: Helper for registering virtio device types
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(a)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(a)redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost(a)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(a)redhat.com>
Thanks!
--
Eduardo