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

[Peter Maydell]

On 20 February 2014 18:12, Mario Smarduch <address@hidden> 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 <address@hidden> 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 <address@hidden>
>>> ---
>>>  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