From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGZ0L-0006cl-J7 for qemu-devel@nongnu.org; Thu, 20 Feb 2014 14:10:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WGZ0D-0001pA-TS for qemu-devel@nongnu.org; Thu, 20 Feb 2014 14:09:53 -0500 Received: from mailout1.w2.samsung.com ([211.189.100.11]:61919 helo=usmailout1.samsung.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGZ0D-0001oo-PD for qemu-devel@nongnu.org; Thu, 20 Feb 2014 14:09:45 -0500 Received: from uscpsbgex2.samsung.com (u123.gpu85.samsung.co.kr [203.254.195.123]) by mailout1.w2.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0N1B0066V6K8FR40@mailout1.w2.samsung.com> for qemu-devel@nongnu.org; Thu, 20 Feb 2014 14:09:44 -0500 (EST) Message-id: <530652EC.5030004@samsung.com> Date: Thu, 20 Feb 2014 11:09:32 -0800 From: Mario Smarduch MIME-version: 1.0 References: <52FD328A.40605@samsung.com> <52FD3593.3040109@samsung.com> <5306458C.3090808@samsung.com> In-reply-to: Content-type: text/plain; charset=UTF-8 Content-transfer-encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: =?UTF-8?B?S09OUkFEIEZyw6lkw6lyaWM=?= , "Michael S. Tsirkin" , QEMU Developers , Anthony Liguori , Patch Tracking 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 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 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 >>>> --- >>>> 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 >