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

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 27 Nov 2018 00:49:58 -0200
Eduardo Habkost <ehabkost@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@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@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/

> +     * 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...

> +     *
> +     * Implements both INTERFACE_PCIE_DEVICE and INTERFACE_CONVENTIONAL_PCI_DEVICE.
> +     */
> +    const char *transitional_name;
> +    /*
> +     * The transitional device type.  Optional.

...and that's non-transitional :)

> +     *
> +     * 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.

> +
> +/* 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@redhat.com>