Re: [Qemu-devel] [RFC] net: Peer with existing NIC in netdev_add

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

On Wed, Oct 24, 2012 at 02:49:21PM +0200, Stefan Hajnoczi wrote:
> Allow netdev_del followed by netdev_add to re-peer a NIC and its netdev:
> 
>   (qemu) info network
>   virtio-net-pci.0: type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56
>    \ netdev0: type=user,net=10.0.2.0,restrict=off
> 
>   (qemu) netdev_del netdev0
> 
>   (qemu) netdev_add socket,id=netdev0,listen=:1234
> 
>   (qemu) info network
>   virtio-net-pci.0: type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56
>    \ netdev0: type=socket,
> 
> This makes it possible to switch netdev while the guest is running.  It
> is not necessary to reset the NIC.
> 
> Note that the NIC's link goes down in netdev_del and back up again in
> netdev_add.  Therefore the guest becomes aware that the network has
> changed, although this depends on the emulated NIC model providing link
> status change interrupts.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

I'd be surprised if this patch worked when one or both backends are tap.
tap supports offloads but slirp doesn't, since guest
probes offloads at startup, it assumes it can use offloads.
We also program tap during device operation e.g. on set features.
vhost operation could also be interesting, have not looked into it.

> ---
> This patch addresses the netdev hotplug issue that has been discussed
> previously on the mailing list:
> 
>   https://www.redhat.com/archives/libvir-list/2012-October/msg00629.html

I actually think both we and libvirt should address (2):
	(2) because qemu has no asynchronous event to notify libvirt
	when the guest's PCI detach has actually completed, I have to stick in
	an arbitrary call to sleep() which is generally *way* too long, but may
	be too short in some cases of extremely high load.

libvirt should verify device is gone after calling sleep,
if not yet gone - sleep again.

qemu should report an event when device is ejected.

> Initially I thought we need to add a new monitor command.  After looking at the
> code it seems that simply allowing netdev_del followed by netdev_add (as
> suggested by Laine) is the simplest way to solve the problem.  No new monitor
> commands are necessary.
> 
> The current code is not safe with virtio-net + tap, where the virtio-net device
> reports offload features from the tap device to the guest.  If you try to
> switch to a -netdev user or -netdev socket device, those offload capabilities
> are not present and network communication will fail.
> 
> Laine: Please try this out and see if it works for your use case.
> 
>  net.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/net.c b/net.c
> index ae4bc0d..d7c60b0 100644
> --- a/net.c
> +++ b/net.c
> @@ -619,6 +619,19 @@ static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])(
>          [NET_CLIENT_OPTIONS_KIND_HUBPORT]   = net_init_hubport,
>  };
>  
> +static NICState *net_find_nic_for_peername(const char *name)
> +{
> +    NetClientState *nc;
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> +            continue;
> +        }
> +        if (nc->peer && strcmp(nc->peer->name, name) == 0) {
> +            return (NICState *)nc;
> +        }
> +    }
> +    return NULL;
> +}
>  
>  static int net_client_init1(const void *object, int is_netdev, Error **errp)
>  {
> @@ -678,6 +691,25 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
>                        NetClientOptionsKind_lookup[opts->kind]);
>              return -1;
>          }
> +
> +        /* Peer with existing nic if netdev name matches */
> +        if (is_netdev && opts->kind != NET_CLIENT_OPTIONS_KIND_NIC) {
> +            NICState *nic = net_find_nic_for_peername(name);
> +            if (nic && nic->peer_deleted) {
> +                NetClientState *netdev = qemu_find_netdev(name);
> +
> +                /* TODO vnet_hdr compatibility check */
> +
> +                qemu_free_net_client(nic->nc.peer);
> +                nic->nc.peer = netdev;
> +                netdev->peer = &nic->nc;
> +                nic->peer_deleted = false;
> +                nic->nc.link_down = false;
> +                if (nic->nc.info->link_status_changed) {
> +                    nic->nc.info->link_status_changed(&nic->nc);
> +                }
> +            }
> +        }
>      }
>      return 0;
>  }
> -- 
> 1.7.11.7
>