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