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 <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
>