[Qemu-devel] Re: [PATCH-RFC 13/13] virtio-net: connect to vhost net backend

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jan 25, 2010 at 03:00:16PM -0600, Anthony Liguori wrote:
> On 01/25/2010 02:27 PM, Michael S. Tsirkin wrote:
>> On Mon, Jan 25, 2010 at 01:58:08PM -0600, Anthony Liguori wrote:
>>    
>>> On 01/11/2010 11:23 AM, Michael S. Tsirkin wrote:
>>>      
>>>> start/stop backend on driver start/stop
>>>>
>>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>>> ---
>>>>    hw/virtio-net.c |   40 ++++++++++++++++++++++++++++++++++++++++
>>>>    1 files changed, 40 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>>>> index c2a389f..99169e1 100644
>>>> --- a/hw/virtio-net.c
>>>> +++ b/hw/virtio-net.c
>>>> @@ -17,6 +17,7 @@
>>>>    #include "net/tap.h"
>>>>    #include "qemu-timer.h"
>>>>    #include "virtio-net.h"
>>>> +#include "vhost_net.h"
>>>>
>>>>    #define VIRTIO_NET_VM_VERSION    11
>>>>
>>>> @@ -47,6 +48,7 @@ typedef struct VirtIONet
>>>>        uint8_t nomulti;
>>>>        uint8_t nouni;
>>>>        uint8_t nobcast;
>>>> +    uint8_t vhost_started;
>>>>        struct {
>>>>            int in_use;
>>>>            int first_multi;
>>>> @@ -114,6 +116,10 @@ static void virtio_net_reset(VirtIODevice *vdev)
>>>>        n->nomulti = 0;
>>>>        n->nouni = 0;
>>>>        n->nobcast = 0;
>>>> +    if (n->vhost_started) {
>>>> +        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
>>>> +        n->vhost_started = 0;
>>>> +    }
>>>>
>>>>        /* Flush any MAC and VLAN filter table state */
>>>>        n->mac_table.in_use = 0;
>>>> @@ -820,6 +826,36 @@ static NetClientInfo net_virtio_info = {
>>>>        .link_status_changed = virtio_net_set_link_status,
>>>>    };
>>>>
>>>> +static void virtio_net_set_status(struct VirtIODevice *vdev)
>>>> +{
>>>> +    VirtIONet *n = to_virtio_net(vdev);
>>>> +    if (!n->nic->nc.peer) {
>>>> +        return;
>>>> +    }
>>>> +    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (!tap_get_vhost_net(n->nic->nc.peer)) {
>>>> +        return;
>>>> +    }
>>>> +    if (!!n->vhost_started == !!(vdev->status&   VIRTIO_CONFIG_S_DRIVER_OK)) {
>>>> +        return;
>>>> +    }
>>>> +    if (vdev->status&   VIRTIO_CONFIG_S_DRIVER_OK) {
>>>> +        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
>>>> +        if (r<   0) {
>>>> +            fprintf(stderr, "unable to start vhost net: "
>>>> +                    "falling back on userspace virtio\n");
>>>> +        } else {
>>>> +            n->vhost_started = 1;
>>>> +        }
>>>> +    } else {
>>>> +        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
>>>> +        n->vhost_started = 0;
>>>> +    }
>>>> +}
>>>> +
>>>>
>>>>        
>>> This function does a number of bad things.  It makes virtio-net have
>>> specific knowledge of backends (like tap) and then has virtio-net pass
>>> device specific state (vdev) to a network backend.
>>>
>>> Ultimately, the following things need to happen:
>>>
>>> 1) when a driver is ready to begin operating:
>>>    a) virtio-net needs to tell vhost the location of the ring in physical
>>> memory
>>>    b) virtio-net needs to tell vhost about any notification it receives
>>> (allowing kvm to shortcut userspace)
>>>    c) virtio-net needs to tell vhost about which irq is being used
>>> (allowing kvm to shortcut userspace)
>>>
>>> What this suggests is that we need an API for the network backends to do
>>> the following:
>>>
>>>   1) probe whether ring passthrough is supported
>>>   2) set the *virtual* address of the ring elements
>>>   3) hand the backend some sort of notification object for sending and
>>> receiving notifications
>>>   4) stop ring passthrough
>>>      
>> Look at how vnet_hdr is setup: frontend probes backend, and has 'if
>> (backend has vnet header) blabla' vhost is really very similar, so I
>> would like to do it in the same way.
>>    
>
> vnet_hdr is IMHO a really bad example to copy from.
>
> vnet_hdr leaks into the migration state via n->has_vnet_hdr.  What this  
> means is that if you want to migrate from -net tap -net nic,model=virtio  
> to -net user -net nic,model=virtio, it will fail.
>
> This is a hard problem to solve in qemu though because it would require  
> that we implement software GSO which so far, no one has stepped up to do.
>
> We don't have to repeat this with vhost-net though because unlike  
> vnet_hdr, we don't have to expose anything to the guest.
>
>> Generally I do not believe in abstractions that only have one
>> implementation behind it: you only know how to abstract interface after you
>> have more than one implementation.  So whoever writes another frontend
>> that can use vhost will be in a better position to add infrastructure to
>> abstract both that new thing and virtio.
>>    
>
> I agree with you, but at the same time, I also believe that layering  
> violations should be avoided.  I'm not suggesting that you need to make  
> anything other than the vhost-net + virtio-net case work.  Just make the  
> interfaces abstract enough that you don't need to hand a vdev to  
> vhost-net and such that you don't have to pass kvm specific data  
> structure (ioeventfd) in what are supposed to be generic interfaces.
>
>>> vhost should not need any direct knowledge of the device.  This
>>> interface should be enough to communicating the required data.  I think
>>> the only bit that is a little fuzzy and perhaps non-obvious for the
>>> current patches is the notification object.  I would expect it look
>>> something like:
>>>
>>> typedef struct IOEvent {
>>>    int type;
>>>    void (*notify)(IOEvent *);
>>>    void (*on_notify)(IOEvent *, void (*cb)(IOEvent *, void *));
>>> } IOEvent;
>>> And then we would have
>>>
>>> typedef struct KVMIOEvent {
>>>    IOEvent event = {.type = KVM_IO_EVENT};
>>>    int fd;
>>> } KVMIOEvent;
>>>
>>> if (kvm_enabled()) in virtio-net, we would allocate a KVMIOEvent for the
>>> PIO notification and hand that to vhost.  vhost would check event.type
>>> and if it's KVM_IOEVENT, downcast and use that to get at the file
>>> descriptor.
>>>      
>> Since we don't have any other IOEvents, I just put the fd
>> in the generic structure directly. If other types surface
>> we'll see how to generalize it.
>>    
>
> I'd feel a whole lot better if we didn't pass the fd around and instead  
> passed around a structure.  With just a tiny bit of layering, we can  
> even avoid making that structure KVM specific :-)
>
>>> The KVMIOEvent should be created, not in the set status callback, but in
>>> the PCI map callback.  And in the PCI map callback, cpu_single_env
>>> should be passed to a kvm specific function to create this KVMIOEvent
>>> object that contains the created eventfd() that's handed to kvm via
>>> ioctl.
>>>      
>> So this specific thing does not work very well because with irqchip, we
>> want to bypass notification and send irq directly from kernel.
>> So I created a structure but it does not have callbacks,
>> only the fd.
>>    
>
> Your structure is an int, right?

it *has* an int:
	struct EventNotifier {
		int fd;
	};

> I understand the limits due to lack of  
> in-kernel irqchip but I still think passing around an fd is a mistake.

So API's will all use EventNotifier *.
We'll be able to add downcasting etc if/when we need it.

>>> There
>>> should also be strong separation between the vhost-net code and the
>>> virtio-net device.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>      
>> I don't see the point of this last idea.  vhost is virtio accelerator,
>> not a generic network backend.  Whoever wants to use vhost for
>> non-virtio frontends will have to add infrastructure for this
>> separation, but I do not believe it's practical or desirable.
>>    
>
> vhost is a userspace interface to inject packets into a network device  
> just like a raw socket or a tun/tap device is.  It happens to have some  
> interesting features like it supports remapping of physical addresses  
> and it implements interfaces for notifications that can conveniently be  
> used by KVM to bypass userspace in the fast paths.
>
> We should think of it this way for the same reason that vhost-net  
> doesn't live in kvm.ko.  If it's easy to isolate something and make it  
> more generic, it's almost always the right thing to do.  In this case,  
> isolating vhost-net from virtio-net in qemu just involves passing some  
> information in a function call verses passing a non-public data  
> structure that is then accessed directly.  Regardless of whether you  
> agree with my view of vhost-net, the argument alone to avoid making a  
> non-public structure public should be enough of an argument.
>
> Regards,
>
> Anthony Liguori

I'll add accessors, it's not a big deal.