| Message ID | 530E9360.6070609@samsung.com |
|---|---|
| State | New |
| Headers | show |
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()?) >
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
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()?)
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(+)