On 20 February 2014 18:12, Mario Smarduch <m.smard...@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.smard...@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.smard...@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
