From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NK8ry-0007oi-1j for qemu-devel@nongnu.org; Mon, 14 Dec 2009 06:13:38 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NK8rt-0007me-Il for qemu-devel@nongnu.org; Mon, 14 Dec 2009 06:13:37 -0500 Received: from [199.232.76.173] (port=52018 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NK8rt-0007mb-BS for qemu-devel@nongnu.org; Mon, 14 Dec 2009 06:13:33 -0500 Received: from mx1.redhat.com ([209.132.183.28]:17628) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NK8rs-0001vI-US for qemu-devel@nongnu.org; Mon, 14 Dec 2009 06:13:33 -0500 Date: Mon, 14 Dec 2009 13:10:47 +0200 From: "Michael S. Tsirkin" Message-ID: <20091214111046.GB32355@redhat.com> References: <20091213204341.GA25823@redhat.com> <4B260846.9020503@redhat.com> <20091214094255.GA32140@redhat.com> <4B261269.7080801@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B261269.7080801@redhat.com> Subject: [Qemu-devel] Re: [PATCH RFC] virtio: add features qdev property List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org On Mon, Dec 14, 2009 at 11:24:41AM +0100, Gerd Hoffmann wrote: > On 12/14/09 10:42, Michael S. Tsirkin wrote: >> On Mon, Dec 14, 2009 at 10:41:26AM +0100, Gerd Hoffmann wrote: >>> On 12/13/09 21:43, Michael S. Tsirkin wrote: >>>> Add features property to virtio. This makes it >>>> possible to e.g. define machine without indirect >>>> buffer support, which is required for 0.10 >>>> compatibility. or without hardware checksum >>>> support, which is required for 0.11 compatibility. >>> >>> I'd suggest to add flags for the individual features to the drivers >>> which actually use it instead, so you'll have >>> >>> -device virtio-net-pci,hw-checksum=0 >>> >>> and >>> >>> -device virtio-blk-pci,indirect-buffers=0 >>> >>> cheers, >>> Gerd >> >> Hmm. I hoped to avoid it, there are lots of features so it's a lot of >> work and in practice, this will most likely be set by machine >> description ... > > MSI-X aka vectors property is already done this way, so I'd tend to > continue this way. It is also more user friendly. Sure, these are most > likely not used on a daily base by users, but being able to turn off -- > say -- indirect buffers for testing and/or bug hunting reasons without > having to construct magic hex numbers from virtio header files would be > nice. > > Can you give a list of features? The patch description sounded like it > is just the two listed above ... > > cheers, > Gerd Heh, it's a long list. transport features (common to all): VIRTIO_F_NOTIFY_ON_EMPTY; VIRTIO_RING_F_INDIRECT_DESC; VIRTIO_F_BAD_FEATURE; <- not sure we need a flag for this for net: uint32_t features = (1 << VIRTIO_NET_F_MAC) | (1 << VIRTIO_NET_F_MRG_RXBUF) | (1 << VIRTIO_NET_F_STATUS) | (1 << VIRTIO_NET_F_CTRL_VQ) | (1 << VIRTIO_NET_F_CTRL_RX) | (1 << VIRTIO_NET_F_CTRL_VLAN) | (1 << VIRTIO_NET_F_CTRL_RX_EXTRA); if (peer_has_vnet_hdr(n)) { tap_using_vnet_hdr(n->nic->nc.peer, 1); features |= (1 << VIRTIO_NET_F_CSUM); features |= (1 << VIRTIO_NET_F_HOST_TSO4); features |= (1 << VIRTIO_NET_F_HOST_TSO6); features |= (1 << VIRTIO_NET_F_HOST_ECN); features |= (1 << VIRTIO_NET_F_GUEST_CSUM); features |= (1 << VIRTIO_NET_F_GUEST_TSO4); features |= (1 << VIRTIO_NET_F_GUEST_TSO6); features |= (1 << VIRTIO_NET_F_GUEST_ECN); if (peer_has_ufo(n)) { features |= (1 << VIRTIO_NET_F_GUEST_UFO); features |= (1 << VIRTIO_NET_F_HOST_UFO); } for block: features |= (1 << VIRTIO_BLK_F_SEG_MAX); features |= (1 << VIRTIO_BLK_F_GEOMETRY); if (bdrv_enable_write_cache(s->bs)) features |= (1 << VIRTIO_BLK_F_WCACHE); #ifdef __linux__ features |= (1 << VIRTIO_BLK_F_SCSI); #endif if (strcmp(s->serial_str, "0")) features |= 1 << VIRTIO_BLK_F_IDENTIFY; if (bdrv_is_read_only(s->bs)) features |= 1 << VIRTIO_BLK_F_RO; I could try and group features, but this way we loose in flexibility ... How about I name properties exactly like virtio macros? e.g. VIRTIO_BLK_F_IDENTIFY etc? This way maybe I can use preprocessor magic to reduce duplication ... Also, I'd like these things to be saved in bits and not add a ton of fields in device. Ideas how to do this? -- MST