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

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 14 Nov 2018 21:38:31 -0200
Eduardo Habkost <ehabkost@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>

> +    /* 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/ ?

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

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

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