[Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Mario Smarduch]

virtio: set virtio-net/virtio-mmio host features

Patch sets 'virtio-net/virtio-mmio' host features to enable network
features based on peer capabilities. Currently host features turn
of all features by default.

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 hw/virtio/virtio-mmio.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 8829eb0..1d940b7 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -23,6 +23,7 @@
 #include "hw/virtio/virtio.h"
 #include "qemu/host-utils.h"
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-net.h"

 /* #define DEBUG_VIRTIO_MMIO */

@@ -92,6 +93,12 @@ typedef struct {
 static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
                                 VirtIOMMIOProxy *dev);

+/* all possible virtio-net features supported */
+static Property virtio_mmio_net_properties[] = {
+    DEFINE_VIRTIO_NET_FEATURES(VirtIOMMIOProxy, host_features),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
 {
     VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
@@ -347,11 +354,33 @@ static void virtio_mmio_reset(DeviceState *d)

 /* virtio-mmio device */

+/* Walk virtio-net possible supported features and set host_features, this
+ * should be done earlier when the object is instantiated but at that point
+ * you don't know what type of device will be plugged in.
+ */
+static void virtio_mmio_set_net_features(Property *prop, uint32_t *features)
+{
+    for (; prop && prop->name; prop++) {
+        if (prop->defval == true) {
+            *features |= (1 << prop->bitnr);
+        }  else  {
+            *features &= ~(1 << prop->bitnr);
+        }
+    }
+}
+
 /* This is called by virtio-bus just after the device is plugged. */
 static void virtio_mmio_device_plugged(DeviceState *opaque)
 {
     VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+    Object *obj = OBJECT(vdev);

+    /* set host features only for virtio-net */
+    if (object_dynamic_cast(obj, TYPE_VIRTIO_NET)) {
+        virtio_mmio_set_net_features(virtio_mmio_net_properties,
+                                                        &proxy->host_features);
+    }
     proxy->host_features |= (0x1 << VIRTIO_F_NOTIFY_ON_EMPTY);
     proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus,
                                                         proxy->host_features);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Peter Maydell]

CCing the virtio maintainers.

thanks
-- PMM

On 13 February 2014 21:13, Mario Smarduch <m.smarduch@samsung.com> wrote:
> virtio: set virtio-net/virtio-mmio host features
>
> Patch sets 'virtio-net/virtio-mmio' host features to enable network
> features based on peer capabilities. Currently host features turn
> of all features by default.
>
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> ---
>  hw/virtio/virtio-mmio.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index 8829eb0..1d940b7 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -23,6 +23,7 @@
>  #include "hw/virtio/virtio.h"
>  #include "qemu/host-utils.h"
>  #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-net.h"
>
>  /* #define DEBUG_VIRTIO_MMIO */
>
> @@ -92,6 +93,12 @@ typedef struct {
>  static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
>                                  VirtIOMMIOProxy *dev);
>
> +/* all possible virtio-net features supported */
> +static Property virtio_mmio_net_properties[] = {
> +    DEFINE_VIRTIO_NET_FEATURES(VirtIOMMIOProxy, host_features),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
> @@ -347,11 +354,33 @@ static void virtio_mmio_reset(DeviceState *d)
>
>  /* virtio-mmio device */
>
> +/* Walk virtio-net possible supported features and set host_features, this
> + * should be done earlier when the object is instantiated but at that point
> + * you don't know what type of device will be plugged in.
> + */
> +static void virtio_mmio_set_net_features(Property *prop, uint32_t *features)
> +{
> +    for (; prop && prop->name; prop++) {
> +        if (prop->defval == true) {
> +            *features |= (1 << prop->bitnr);
> +        }  else  {
> +            *features &= ~(1 << prop->bitnr);
> +        }
> +    }
> +}
> +
>  /* This is called by virtio-bus just after the device is plugged. */
>  static void virtio_mmio_device_plugged(DeviceState *opaque)
>  {
>      VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +    Object *obj = OBJECT(vdev);
>
> +    /* set host features only for virtio-net */
> +    if (object_dynamic_cast(obj, TYPE_VIRTIO_NET)) {
> +        virtio_mmio_set_net_features(virtio_mmio_net_properties,
> +                                                        &proxy->host_features);
> +    }
>      proxy->host_features |= (0x1 << VIRTIO_F_NOTIFY_ON_EMPTY);
>      proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus,
>                                                          proxy->host_features);
> --
> 1.7.9.5
>

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Mario Smarduch]

Hello,

any feedback on this patch, after a brief email exchange Anthony deferred to
Peter. 

Lack of improper host features handling lowers 1g & 10g performance 
substantially on arm-kvm compared to xeon. 

We would like to have this fixed so we don't have to patch every new release
of qemu, especially virtio stuff.

- Mario

On 02/14/2014 03:49 PM, Peter Maydell wrote:
> CCing the virtio maintainers.
> 
> thanks
> -- PMM
> 
> On 13 February 2014 21:13, Mario Smarduch <m.smarduch@samsung.com> wrote:
>> virtio: set virtio-net/virtio-mmio host features
>>
>> Patch sets 'virtio-net/virtio-mmio' host features to enable network
>> features based on peer capabilities. Currently host features turn
>> of all features by default.
>>
>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>> ---
>>  hw/virtio/virtio-mmio.c |   29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
>> index 8829eb0..1d940b7 100644
>> --- a/hw/virtio/virtio-mmio.c
>> +++ b/hw/virtio/virtio-mmio.c
>> @@ -23,6 +23,7 @@
>>  #include "hw/virtio/virtio.h"
>>  #include "qemu/host-utils.h"
>>  #include "hw/virtio/virtio-bus.h"
>> +#include "hw/virtio/virtio-net.h"
>>
>>  /* #define DEBUG_VIRTIO_MMIO */
>>
>> @@ -92,6 +93,12 @@ typedef struct {
>>  static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
>>                                  VirtIOMMIOProxy *dev);
>>
>> +/* all possible virtio-net features supported */
>> +static Property virtio_mmio_net_properties[] = {
>> +    DEFINE_VIRTIO_NET_FEATURES(VirtIOMMIOProxy, host_features),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
>>  {
>>      VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
>> @@ -347,11 +354,33 @@ static void virtio_mmio_reset(DeviceState *d)
>>
>>  /* virtio-mmio device */
>>
>> +/* Walk virtio-net possible supported features and set host_features, this
>> + * should be done earlier when the object is instantiated but at that point
>> + * you don't know what type of device will be plugged in.
>> + */
>> +static void virtio_mmio_set_net_features(Property *prop, uint32_t *features)
>> +{
>> +    for (; prop && prop->name; prop++) {
>> +        if (prop->defval == true) {
>> +            *features |= (1 << prop->bitnr);
>> +        }  else  {
>> +            *features &= ~(1 << prop->bitnr);
>> +        }
>> +    }
>> +}
>> +
>>  /* This is called by virtio-bus just after the device is plugged. */
>>  static void virtio_mmio_device_plugged(DeviceState *opaque)
>>  {
>>      VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
>> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>> +    Object *obj = OBJECT(vdev);
>>
>> +    /* set host features only for virtio-net */
>> +    if (object_dynamic_cast(obj, TYPE_VIRTIO_NET)) {
>> +        virtio_mmio_set_net_features(virtio_mmio_net_properties,
>> +                                                        &proxy->host_features);
>> +    }
>>      proxy->host_features |= (0x1 << VIRTIO_F_NOTIFY_ON_EMPTY);
>>      proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus,
>>                                                          proxy->host_features);
>> --
>> 1.7.9.5
>>

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Peter Maydell]

On 20 February 2014 18:12, Mario Smarduch <m.smarduch@samsung.com> wrote:
>
> Hello,
>
> any feedback on this patch, after a brief email exchange Anthony deferred to
> Peter.
>
> Lack of improper host features handling lowers 1g & 10g performance
> substantially on arm-kvm compared to xeon.
>
> We would like to have this fixed so we don't have to patch every new release
> of qemu, especially virtio stuff.

I don't know enough about virtio to review, really, but
I'll have a go:

>> On 13 February 2014 21:13, Mario Smarduch <m.smarduch@samsung.com> wrote:
>>> virtio: set virtio-net/virtio-mmio host features
>>>
>>> Patch sets 'virtio-net/virtio-mmio' host features to enable network
>>> features based on peer capabilities. Currently host features turn
>>> of all features by default.
>>>
>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>> ---
>>>  hw/virtio/virtio-mmio.c |   29 +++++++++++++++++++++++++++++
>>>  1 file changed, 29 insertions(+)
>>>
>>> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
>>> index 8829eb0..1d940b7 100644
>>> --- a/hw/virtio/virtio-mmio.c
>>> +++ b/hw/virtio/virtio-mmio.c
>>> @@ -23,6 +23,7 @@
>>>  #include "hw/virtio/virtio.h"
>>>  #include "qemu/host-utils.h"
>>>  #include "hw/virtio/virtio-bus.h"
>>> +#include "hw/virtio/virtio-net.h"
>>>
>>>  /* #define DEBUG_VIRTIO_MMIO */
>>>
>>> @@ -92,6 +93,12 @@ typedef struct {
>>>  static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
>>>                                  VirtIOMMIOProxy *dev);
>>>
>>> +/* all possible virtio-net features supported */
>>> +static Property virtio_mmio_net_properties[] = {
>>> +    DEFINE_VIRTIO_NET_FEATURES(VirtIOMMIOProxy, host_features),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>>  static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
>>>  {
>>>      VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
>>> @@ -347,11 +354,33 @@ static void virtio_mmio_reset(DeviceState *d)
>>>
>>>  /* virtio-mmio device */
>>>
>>> +/* Walk virtio-net possible supported features and set host_features, this
>>> + * should be done earlier when the object is instantiated but at that point
>>> + * you don't know what type of device will be plugged in.
>>> + */
>>> +static void virtio_mmio_set_net_features(Property *prop, uint32_t *features)
>>> +{
>>> +    for (; prop && prop->name; prop++) {
>>> +        if (prop->defval == true) {
>>> +            *features |= (1 << prop->bitnr);
>>> +        }  else  {
>>> +            *features &= ~(1 << prop->bitnr);
>>> +        }
>>> +    }
>>> +}
>>> +
>>>  /* This is called by virtio-bus just after the device is plugged. */
>>>  static void virtio_mmio_device_plugged(DeviceState *opaque)
>>>  {
>>>      VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
>>> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>>> +    Object *obj = OBJECT(vdev);
>>>
>>> +    /* set host features only for virtio-net */
>>> +    if (object_dynamic_cast(obj, TYPE_VIRTIO_NET)) {
>>> +        virtio_mmio_set_net_features(virtio_mmio_net_properties,
>>> +                                                        &proxy->host_features);
>>> +    }

This looks weird. We already have a mechanism for saying
"hey the thing we just plugged in gets to tell us about
feature bits":

>>>      proxy->host_features |= (0x1 << VIRTIO_F_NOTIFY_ON_EMPTY);
>>>      proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus,
>>>                                                          proxy->host_features);

...this is making an indirect call to the backend device's
get_features method, which for virtio-net is
virtio_net_get_features(). Why should the transport
need special case code for virtio-net backends?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Mario Smarduch]

Peter thanks.

Questionable in this patch - is cutting through several layers to set
proxy properties. They should be set from "device" instance init
before it's realized. The problem though is that unlike PCI
that sets proxy and virtio-net properties via its virtio_net_properites[],
the virtio-mmio version instantiates the proxy in the machine model,
so it doesn't appear to be good place to set virtio-net
host features since you don't know what virtio device will be plugged
in later. It's virtio_net_properties[] can only set virtio-net
properites when it's instantiated. I think the proper way would
be to refactor virtio-mmio to similar structure PCI version uses then
one virtio_net_properties[] can be used selecting PCI or virtio-mmio at
compile time. But right now it's unclear to me how pci and virtio-mmio
versions intend to co-exist. I'm fairly new to qemu community.

- Mario

On 02/20/2014 10:30 AM, Peter Maydell wrote:
> On 20 February 2014 18:12, Mario Smarduch <m.smarduch@samsung.com> wrote:
>>
>> Hello,
>>
>> any feedback on this patch, after a brief email exchange Anthony deferred to
>> Peter.
>>
>> Lack of improper host features handling lowers 1g & 10g performance
>> substantially on arm-kvm compared to xeon.
>>
>> We would like to have this fixed so we don't have to patch every new release
>> of qemu, especially virtio stuff.
> 
> I don't know enough about virtio to review, really, but
> I'll have a go:
> 
>>> On 13 February 2014 21:13, Mario Smarduch <m.smarduch@samsung.com> wrote:
>>>> virtio: set virtio-net/virtio-mmio host features
>>>>
>>>> Patch sets 'virtio-net/virtio-mmio' host features to enable network
>>>> features based on peer capabilities. Currently host features turn
>>>> of all features by default.
>>>>
>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>> ---
>>>>  hw/virtio/virtio-mmio.c |   29 +++++++++++++++++++++++++++++
>>>>  1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
>>>> index 8829eb0..1d940b7 100644
>>>> --- a/hw/virtio/virtio-mmio.c
>>>> +++ b/hw/virtio/virtio-mmio.c
>>>> @@ -23,6 +23,7 @@
>>>>  #include "hw/virtio/virtio.h"
>>>>  #include "qemu/host-utils.h"
>>>>  #include "hw/virtio/virtio-bus.h"
>>>> +#include "hw/virtio/virtio-net.h"
>>>>
>>>>  /* #define DEBUG_VIRTIO_MMIO */
>>>>
>>>> @@ -92,6 +93,12 @@ typedef struct {
>>>>  static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
>>>>                                  VirtIOMMIOProxy *dev);
>>>>
>>>> +/* all possible virtio-net features supported */
>>>> +static Property virtio_mmio_net_properties[] = {
>>>> +    DEFINE_VIRTIO_NET_FEATURES(VirtIOMMIOProxy, host_features),
>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>> +};
>>>> +
>>>>  static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
>>>>  {
>>>>      VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
>>>> @@ -347,11 +354,33 @@ static void virtio_mmio_reset(DeviceState *d)
>>>>
>>>>  /* virtio-mmio device */
>>>>
>>>> +/* Walk virtio-net possible supported features and set host_features, this
>>>> + * should be done earlier when the object is instantiated but at that point
>>>> + * you don't know what type of device will be plugged in.
>>>> + */
>>>> +static void virtio_mmio_set_net_features(Property *prop, uint32_t *features)
>>>> +{
>>>> +    for (; prop && prop->name; prop++) {
>>>> +        if (prop->defval == true) {
>>>> +            *features |= (1 << prop->bitnr);
>>>> +        }  else  {
>>>> +            *features &= ~(1 << prop->bitnr);
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>>  /* This is called by virtio-bus just after the device is plugged. */
>>>>  static void virtio_mmio_device_plugged(DeviceState *opaque)
>>>>  {
>>>>      VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
>>>> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>>>> +    Object *obj = OBJECT(vdev);
>>>>
>>>> +    /* set host features only for virtio-net */
>>>> +    if (object_dynamic_cast(obj, TYPE_VIRTIO_NET)) {
>>>> +        virtio_mmio_set_net_features(virtio_mmio_net_properties,
>>>> +                                                        &proxy->host_features);
>>>> +    }
> 
> This looks weird. We already have a mechanism for saying
> "hey the thing we just plugged in gets to tell us about
> feature bits":
> 
>>>>      proxy->host_features |= (0x1 << VIRTIO_F_NOTIFY_ON_EMPTY);
>>>>      proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus,
>>>>                                                          proxy->host_features);
> 
> ...this is making an indirect call to the backend device's
> get_features method, which for virtio-net is
> virtio_net_get_features(). Why should the transport
> need special case code for virtio-net backends?
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Peter Maydell]

On 20 February 2014 19:09, Mario Smarduch <m.smarduch@samsung.com> wrote:
> Questionable in this patch - is cutting through several layers to set
> proxy properties. They should be set from "device" instance init
> before it's realized. The problem though is that unlike PCI
> that sets proxy and virtio-net properties via its virtio_net_properites[],
> the virtio-mmio version instantiates the proxy in the machine model,
> so it doesn't appear to be good place to set virtio-net
> host features since you don't know what virtio device will be plugged
> in later.

I think this function is the right place to set these properties,
yes. What I'm saying is that I don't see why you're doing it
this way rather than using the existing per-backend hook.
Maybe there's a reason not to use that hook, but you don't say.

> It's virtio_net_properties[] can only set virtio-net
> properites when it's instantiated. I think the proper way would
> be to refactor virtio-mmio to similar structure PCI version uses then
> one virtio_net_properties[] can be used selecting PCI or virtio-mmio at
> compile time. But right now it's unclear to me how pci and virtio-mmio
> versions intend to co-exist.

This is the result of a refactoring last year to allow virtio-mmio.
Basically the underlying structure now is:

 [some virtio transport device] <- virtio bus -> [virtio backend]

where the transport could be mmio, PCI, CCW, etc, and the
backend is net, blk, balloon, etc. We also provide devices
which are "virtio PCI transport plus a specific backend"
for (a) user convenience and (b) backwards compatibility, since
the pre-refactoring structure put the transport and backend
all in one lump. The all-in-one-lump arrangement is legacy:
we shouldn't break it, but it's not the model for virtio-mmio
to follow either.

The transport shouldn't know anything specific about the
backend it's plugged into (or vice-versa), but it's fine
for there to be hook functions so the transport and backend
can call each other at the appropriate times.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Mario Smarduch]

On 02/20/2014 11:35 AM, Peter Maydell wrote:
> On 20 February 2014 19:09, Mario Smarduch <m.smarduch@samsung.com> wrote:
>> host features since you don't know what virtio device will be plugged
>> in later.
> 
> I think this function is the right place to set these properties,
> yes. What I'm saying is that I don't see why you're doing it
> this way rather than using the existing per-backend hook.
> Maybe there's a reason not to use that hook, but you don't say.
> 

Appears virtio-net beckend hooks are common to several transports, 
and would require virtio-mmio exception to set the host_features. 
If I'm missing something please recommend.

>> It's virtio_net_properties[] can only set virtio-net
>> properites when it's instantiated. I think the proper way would
>> be to refactor virtio-mmio to similar structure PCI version uses then
>> one virtio_net_properties[] can be used selecting PCI or virtio-mmio at
>> compile time. But right now it's unclear to me how pci and virtio-mmio
>> versions intend to co-exist.

> 
> This is the result of a refactoring last year to allow virtio-mmio.
> Basically the underlying structure now is:
> 
>  [some virtio transport device] <- virtio bus -> [virtio backend]
> 
> where the transport could be mmio, PCI, CCW, etc, and the
> backend is net, blk, balloon, etc. We also provide devices
> which are "virtio PCI transport plus a specific backend"
> for (a) user convenience and (b) backwards compatibility, since
> the pre-refactoring structure put the transport and backend
> all in one lump. The all-in-one-lump arrangement is legacy:
> we shouldn't break it, but it's not the model for virtio-mmio
> to follow either.
> 
> The transport shouldn't know anything specific about the
> backend it's plugged into (or vice-versa), but it's fine
> for there to be hook functions so the transport and backend
> can call each other at the appropriate times.
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Peter Maydell]

On 20 February 2014 21:59, Mario Smarduch <m.smarduch@samsung.com> wrote:
> On 02/20/2014 11:35 AM, Peter Maydell wrote:
>> On 20 February 2014 19:09, Mario Smarduch <m.smarduch@samsung.com> wrote:
>>> host features since you don't know what virtio device will be plugged
>>> in later.
>>
>> I think this function is the right place to set these properties,
>> yes. What I'm saying is that I don't see why you're doing it
>> this way rather than using the existing per-backend hook.
>> Maybe there's a reason not to use that hook, but you don't say.
>>
>
> Appears virtio-net beckend hooks are common to several transports,
> and would require virtio-mmio exception to set the host_features.
> If I'm missing something please recommend.

Yes, they're supposed to be common across transports, because
the backend isn't supposed to care about which transport in
particular. If there's a condition where the backend needs to
do something which only happens for one transport, maybe we
need a new hook.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Mario Smarduch]

On 02/20/2014 02:10 PM, Peter Maydell wrote:
> On 20 February 2014 21:59, Mario Smarduch <m.smarduch@samsung.com> wrote:
>> On 02/20/2014 11:35 AM, Peter Maydell wrote:
>>> On 20 February 2014 19:09, Mario Smarduch <m.smarduch@samsung.com> wrote:
>>>> host features since you don't know what virtio device will be plugged
>>>> in later.
>>>
>>> I think this function is the right place to set these properties,
>>> yes. What I'm saying is that I don't see why you're doing it
>>> this way rather than using the existing per-backend hook.
>>> Maybe there's a reason not to use that hook, but you don't say.
>>>
>>
>> Appears virtio-net beckend hooks are common to several transports,
>> and would require virtio-mmio exception to set the host_features.
>> If I'm missing something please recommend.
> 
> Yes, they're supposed to be common across transports, because
> the backend isn't supposed to care about which transport in
> particular. If there's a condition where the backend needs to
> do something which only happens for one transport, maybe we
> need a new hook.

So something like set_transport_features(...) in VirtiIODeviceClass,
and call it from the realize hook where you can access
the virtio-mmio transport class instance.

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Peter Maydell]

On 20 February 2014 22:36, Mario Smarduch <m.smarduch@samsung.com> wrote:
> On 02/20/2014 02:10 PM, Peter Maydell wrote:
>> On 20 February 2014 21:59, Mario Smarduch <m.smarduch@samsung.com> wrote:
>>> On 02/20/2014 11:35 AM, Peter Maydell wrote:
>>>> On 20 February 2014 19:09, Mario Smarduch <m.smarduch@samsung.com> wrote:
>>>>> host features since you don't know what virtio device will be plugged
>>>>> in later.
>>>>
>>>> I think this function is the right place to set these properties,
>>>> yes. What I'm saying is that I don't see why you're doing it
>>>> this way rather than using the existing per-backend hook.
>>>> Maybe there's a reason not to use that hook, but you don't say.
>>>>
>>>
>>> Appears virtio-net beckend hooks are common to several transports,
>>> and would require virtio-mmio exception to set the host_features.
>>> If I'm missing something please recommend.
>>
>> Yes, they're supposed to be common across transports, because
>> the backend isn't supposed to care about which transport in
>> particular. If there's a condition where the backend needs to
>> do something which only happens for one transport, maybe we
>> need a new hook.
>
> So something like set_transport_features(...) in VirtiIODeviceClass,
> and call it from the realize hook where you can access
> the virtio-mmio transport class instance.

Not sure. You should probably also look at whether connecting
a virtio-pci transport to a virtio-net backend gets these feature
bits wrong (that's different from instantiating a single virtio-net-pci
device). I suspect they both get this wrong and this isn't particularly
virtio-mmio specific.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Mario Smarduch]

On 02/20/2014 02:47 PM, Peter Maydell wrote:
> On 20 February 2014 22:36, Mario Smarduch <m.smarduch@samsung.com> wrote:
>> On 02/20/2014 02:10 PM, Peter Maydell wrote:
>>> On 20 February 2014 21:59, Mario Smarduch <m.smarduch@samsung.com> wrote:
>>>> On 02/20/2014 11:35 AM, Peter Maydell wrote:
>>>>> On 20 February 2014 19:09, Mario Smarduch <m.smarduch@samsung.com> wrote:
>>>>>> host features since you don't know what virtio device will be plugged
>>>>>> in later.
>>>>>
>>>>> I think this function is the right place to set these properties,
>>>>> yes. What I'm saying is that I don't see why you're doing it
>>>>> this way rather than using the existing per-backend hook.
>>>>> Maybe there's a reason not to use that hook, but you don't say.
>>>>>
>>>>
>>>> Appears virtio-net beckend hooks are common to several transports,
>>>> and would require virtio-mmio exception to set the host_features.
>>>> If I'm missing something please recommend.
>>>
>>> Yes, they're supposed to be common across transports, because
>>> the backend isn't supposed to care about which transport in
>>> particular. If there's a condition where the backend needs to
>>> do something which only happens for one transport, maybe we
>>> need a new hook.
>>
>> So something like set_transport_features(...) in VirtiIODeviceClass,
>> and call it from the realize hook where you can access
>> the virtio-mmio transport class instance.
> 
> Not sure. You should probably also look at whether connecting
> a virtio-pci transport to a virtio-net backend gets these feature
> bits wrong (that's different from instantiating a single virtio-net-pci
> device). I suspect they both get this wrong and this isn't particularly
> virtio-mmio specific.
> 
> thanks
> -- PMM
> 

It appears separate pci transport/virtio-net backend have the same issue.
I'll look at this closer for a more generic solution.

Thanks,
  Mario