* Re: [Qemu-devel] [PATCH] virtio: set virtio-mmio transport features for various backends
2014-02-27 1:22 ` [Qemu-devel] [PATCH] virtio: set virtio-mmio transport features for various backends Mario Smarduch
@ 2014-02-28 0:37 ` Mario Smarduch
2014-03-10 16:42 ` Peter Maydell
1 sibling, 0 replies; 3+ messages in thread
From: Mario Smarduch @ 2014-02-28 0:37 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, Patch Tracking, Anthony Liguori,
Michael S. Tsirkin, KONRAD Frédéric
Anthony, Michael, KONRAD -
I would really appreciate if you (or someone else I should include)
can look over this patch. Currently as things are independent transport and
backend interoperability is broken, since the 'proxy' 'host_features' and
for other transport/backend combinations additional fields are not set
to anything, like they are for legacy combined transport/backend.
With this patch the approach is for every transport to have associated
backend properties defined and use the backend type as index into
its properties and apply them to the transport. Only virtio-mmio
transport is addressed in this patch with few backends to see if
this is acceptable by virtio maintainers.
One issue on applying properties this late is they can't be added to the
object - none of property accessor functions work at this point. This
is something I'm not sure how to handle.
Thanks,
Mario
On 02/26/2014 05:22 PM, Mario Smarduch 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 <m.smarduch@samsung.com>
> ---
> 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);
> + }
> +
> if (klass->device_plugged != NULL) {
> klass->device_plugged(qbus->parent);
> }
> 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 },
> +};
> +
> +
> +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)) {
> + 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()?)
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH] virtio: set virtio-mmio transport features for various backends
2014-02-27 1:22 ` [Qemu-devel] [PATCH] virtio: set virtio-mmio transport features for various backends Mario Smarduch
2014-02-28 0:37 ` Mario Smarduch
@ 2014-03-10 16:42 ` Peter Maydell
1 sibling, 0 replies; 3+ messages in thread
From: Peter Maydell @ 2014-03-10 16:42 UTC (permalink / raw)
To: Mario Smarduch
Cc: KONRAD Frédéric, Michael S. Tsirkin, qemu-devel,
Anthony Liguori, Patch Tracking
On 27 February 2014 01:22, Mario Smarduch <m.smarduch@samsung.com> 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 <m.smarduch@samsung.com>
> ---
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread