[Qemu-devel] Re: [PATCH RFC] virtio: add features qdev property

[Michael S. Tsirkin]

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