Re: [Qemu-devel] [PATCH] virtio: set virtio-mmio transport features for various backends

[Peter Maydell]

On 27 February 2014 01:22, Mario Smarduch <address@hidden> wrote:
> virtio:  set virtio-mmio transport features for various backends
>
> Hi Peter, i.e others -
>
> This is a second attempt at setting transport backend features, for
> back-ends that are instantiated separately from transport and dynamically
> plugged in. virtio-mmio transport is addressed by this patch for virtio-net
> and virtio-blk back-ends.
>
> This patch declares features for all back-ends in the transport file,
> and based on the type of back-end applies the back-end features. A hook
> is added in VirtioBusClass that each transport can override
> to apply specific features for a back-end. The hook is called with the
> back-end device when it's plugged in.
>
> Thanks,
>  Mario.
>
> Signed-off-by: Mario Smarduch <address@hidden>
> ---
>  hw/virtio/virtio-bus.c         |    5 +++++
>  hw/virtio/virtio-mmio.c        |   46 
> ++++++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-bus.h |    4 ++++
>  3 files changed, 55 insertions(+)
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index eb77019..5eb9ba1 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -44,8 +44,13 @@ int virtio_bus_device_plugged(VirtIODevice *vdev)
>      BusState *qbus = BUS(qdev_get_parent_bus(qdev));
>      VirtioBusState *bus = VIRTIO_BUS(qbus);
>      VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
> +    DeviceState *proxy = DEVICE(qbus->parent);
>      DPRINTF("%s: plug device.\n", qbus->name);
>
> +    if (klass->set_transport_features != NULL) {
> +        klass->set_transport_features(proxy);
> +    }
> +

This looks wrong. Either:
(a) we don't actually care which transport we're using
when setting up the features on it, in which case we
can just do what we need to do here rather than calling
a method on the transport, or
(b) we do care which transport, in which case we should
just put the code in the device_plugged method, which
we're about to call:

>      if (klass->device_plugged != NULL) {
>          klass->device_plugged(qbus->parent);
>      }

But the device_plugged method already calls

    proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus,
                                                        proxy->host_features);

which calls the get_features method on the backend,
so it looks like we already have most of the plumbing
in place and the interesting question is "why isn't
it working?".

It seems to me like the backend objects should have the
features defined on them (at the moment they're only
set up for the back-compat fused devices which have
both a transport and a backend). Then it's quite possible
that all the plumbing will correctly arrange for the
features to appear as you want.

> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index 8829eb0..f4e67a2 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -23,6 +23,8 @@
>  #include "hw/virtio/virtio.h"
>  #include "qemu/host-utils.h"
>  #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-net.h"
> +#include "hw/virtio/virtio-blk.h"
>
>  /* #define DEBUG_VIRTIO_MMIO */
>
> @@ -89,6 +91,49 @@ typedef struct {
>      VirtioBusState bus;
>  } VirtIOMMIOProxy;
>
> +/* supported features for virtio-net backend */
> +static Property virtio_net_transport_properties[] = {
> +    DEFINE_VIRTIO_NET_FEATURES(VirtIOMMIOProxy, host_features),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +/* supported features for virtio-blk backend */
> +static Property virtio_blk_transport_properties[] = {
> +    DEFINE_VIRTIO_BLK_FEATURES(VirtIOMMIOProxy, host_features),
> +    DEFINE_PROP_END_OF_LIST(),
> +};

> +
> +static struct {
> +    const char *backend_name;
> +    const Property *props;
> +} mmio_transport_backends[] = {
> +    { TYPE_VIRTIO_NET, virtio_net_transport_properties },
> +    { TYPE_VIRTIO_BLK, virtio_blk_transport_properties },
> +    { NULL, NULL },
> +};

This is pretty clearly the wrong structure. virtio-mmio.c
should have no code in it which cares about individual
backends. Where we need to do something backend dependent
we should do it by calling a hook function provided by
the backend.

> +
> +
> +static void virtio_mmio_set_transport_features(DeviceState *d)
> +{
> +    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +    int i;
> +
> +    for (i = 0; mmio_transport_backends[i].backend_name != NULL; i++) {
> +        if (object_dynamic_cast(OBJECT(vdev),
> +                                 mmio_transport_backends[i].backend_name)) {

This in particular is a bad sign -- we're basically doing
a "switch on type of object", whereas we should actually
be calling a method on that object.

> +            const Property *props =  mmio_transport_backends[i].props;
> +            for (; props->qtype == QTYPE_QBOOL && props->qtype != QTYPE_NONE;
> +                   props++) {
> +                if (props->defval == true) {
> +                    proxy->host_features |= (1 << props->bitnr);
> +                } else {
> +                    proxy->host_features &= ~(1 << props->bitnr);
> +                }
> +            }
> +        }
> +    }
> +}
>  static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
>                                  VirtIOMMIOProxy *dev);
>
> @@ -408,6 +453,7 @@ static void virtio_mmio_bus_class_init(ObjectClass 
> *klass, void *data)
>      k->load_config = virtio_mmio_load_config;
>      k->get_features = virtio_mmio_get_features;
>      k->device_plugged = virtio_mmio_device_plugged;
> +    k->set_transport_features = virtio_mmio_set_transport_features;
>      k->has_variable_vring_alignment = true;
>      bus_class->max_dev = 1;
>  }
> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index 0756545..3136fc6 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -62,6 +62,10 @@ typedef struct VirtioBusClass {
>       * This is called by virtio-bus just before the device is unplugged.
>       */
>      void (*device_unplugged)(DeviceState *d);
> +    /* Call before plugging a backend into transport to set features 
> supported
> +     * by the backend.
> +     */
> +    void (*set_transport_features)(DeviceState *d);
>      /*
>       * Does the transport have variable vring alignment?
>       * (ie can it ever call virtio_queue_set_align()?)
> --
> 1.7.9.5

thanks
-- PMM