Re: [Qemu-devel] [PATCH RFC] virtio-net: announce self by guest

[Jason Wang]

On Fri, 2014-03-28 at 00:33 +0800, Amos Kong wrote:
> On Thu, Mar 13, 2014 at 02:56:41PM +0800, Jason Wang wrote:
> > It's hard to track all mac addresses and their configurations (e.g
> > vlan or ipv6)in qemu. Without those information, it's impossible to
> > build proper garp packet after migration. The only possible solution
> > to this is let guest ( who knew all configurations) to do this.
> > 
> > So, this patch introduces a new rw config status bit of virtio-net,
> > VIRTIO_NET_S_ANNOUNCE which is used to notify guest to announce
> > presence of its link through config update interrupt.When gust have
>                                                              ^^^^^^
>                                                       typo: guest has
> 

Thanks, will correct this.
> > done the announcement, it should ack the notification through
> > VIRTIO_NET_CTRL_ANNOUNCE_ACK cmd. This feature is negotiated by a new
> > feature bit VIRTIO_NET_F_ANNOUNCE (which has already been supported by
> > Linux guest).
> > 
> > During load, a counter of announcing rounds were set so that the after
> > the vm is running it can trigger rounds of config interrupts to notify
> > the guest to build and send the correct garps.
> > 
> > Reference:
> > Last version of discussion is here:
> > https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg01127.html
> > 
> > Changes from last version:
> > - Instead of introducing a global method for each kind of nic, this
> >   version limit the changes into virtio-net itself.
> > 
> > Slightly tested by myself.
> > 
> > Cc: Liuyongan <address@hidden>
> > Signed-off-by: Jason Wang <address@hidden>
> > ---
> > ---
> >  hw/net/virtio-net.c            | 47 
> > ++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/virtio/virtio-net.h | 19 ++++++++++++++++-
> >  2 files changed, 65 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 3626608..a0e66fa 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -25,6 +25,7 @@
> >  #include "monitor/monitor.h"
> >  
> >  #define VIRTIO_NET_VM_VERSION    11
> > +#define VIRTIO_NET_ANNOUNCE_ROUNDS    3
> >  
> >  #define MAC_TABLE_ENTRIES    64
> >  #define MAX_VLAN    (1 << 12)   /* Per 802.1Q definition */
> > @@ -99,6 +100,22 @@ static bool virtio_net_started(VirtIONet *n, uint8_t 
> > status)
> >          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
> >  }
> >  
> > +static void virtio_net_announce_timer(void *opaque)
> > +{
> > +    VirtIONet *n = opaque;
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> > +
> > +    if (n->announce &&
> > +        virtio_net_started(n, vdev->status) &&
> > +        vdev->guest_features & (0x1 << VIRTIO_NET_F_GUEST_ANNOUNCE) &&
> > +        vdev->guest_features & (0x1 << VIRTIO_NET_F_CTRL_VQ)) {
> > +
> > +        n->announce--;
> 
>            if (n->status & VIRTIO_NET_S_ANNOUNCE) {
>                 error_report("announce bit in virtio_net status wasn't 
> cleaned");
>            }
> 
>            If guest driver doesn't ACK announce, VIRTIO_NET_S_ANNOUNCE
>            bit of n->status won't be cleaned. We should output error
>            in this case.
> 

Then it's a guest driver bug, qemu should not warn for guest bugs. A
better idea is to clear this bit during reset.
> > +        n->status |= VIRTIO_NET_S_ANNOUNCE;
> > +        virtio_notify_config(vdev);
> > +    }
> > +}
> > +
> >  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(n);
> > @@ -136,6 +153,8 @@ static void virtio_net_vhost_status(VirtIONet *n, 
> > uint8_t status)
> >          vhost_net_stop(vdev, n->nic->ncs, queues);
> >          n->vhost_started = 0;
> >      }
> > +
> > +    virtio_net_announce_timer(n);
> >
> >  }
> >  
> >  static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t 
> > status)
> > @@ -147,6 +166,11 @@ static void virtio_net_set_status(struct VirtIODevice 
> > *vdev, uint8_t status)
> >  
> >      virtio_net_vhost_status(n, status);
> 
>        virtio_net_announce_timer(n);
> 
> we don't only announce when vhost is used, why do you start first
> announce in the end of "virtio_net_VHOST_status()?
> How about move it after virtio_net_vhost_status(n, status); ?
>   

Ok.
> > +    if (!virtio_net_started(n, status)) {
> > +        n->announce = 0;
> > +        timer_del(n->announce_timer);
> > +    }
> > +
> >      for (i = 0; i < n->max_queues; i++) {
> >          q = &n->vqs[i];
> >  
> > @@ -306,6 +330,7 @@ static void virtio_net_reset(VirtIODevice *vdev)
> >      n->nobcast = 0;
> >      /* multiqueue is disabled by default */
> >      n->curr_queues = 1;
> > +    n->announce = 0;
> >  
> >      /* Flush any MAC and VLAN filter table state */
> >      n->mac_table.in_use = 0;
> > @@ -710,6 +735,22 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, 
> > uint8_t cmd,
> >      return VIRTIO_NET_OK;
> >  }
> >  
> > +static int virtio_net_handle_announce(VirtIONet *n, uint8_t cmd,
> > +                                      struct iovec *iov, unsigned int 
> > iov_cnt)
> > +{
> > +    if (cmd == VIRTIO_NET_CTRL_ANNOUNCE_ACK) {
> > +        n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> > +        if (n->announce) {
> > +            timer_mod(n->announce_timer,
> > +                      qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 50 +
> > +                      (VIRTIO_NET_ANNOUNCE_ROUNDS - n->announce - 1) * 
> > 100);
> 
> 
> What's the reason for the interval  50 .. 150 .. 250 ?
> 

Please have a look at qemu_announce_self_once(), it was used to be
compatible with legacy behavior.
> > +        }
> > +        return VIRTIO_NET_OK;
> > +    } else {
> > +        return VIRTIO_NET_ERR;
> > +    }
> > +}
> > +
> >  static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
> >                                  struct iovec *iov, unsigned int iov_cnt)
> >  {
> > @@ -773,6 +814,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, 
> > VirtQueue *vq)
> >              status = virtio_net_handle_mac(n, ctrl.cmd, iov, iov_cnt);
> >          } else if (ctrl.class == VIRTIO_NET_CTRL_VLAN) {
> >              status = virtio_net_handle_vlan_table(n, ctrl.cmd, iov, 
> > iov_cnt);
> > +        } else if (ctrl.class == VIRTIO_NET_CTRL_ANNOUNCE) {
> > +            status = virtio_net_handle_announce(n, ctrl.cmd, iov, iov_cnt);
> >          } else if (ctrl.class == VIRTIO_NET_CTRL_MQ) {
> >              status = virtio_net_handle_mq(n, ctrl.cmd, iov, iov_cnt);
> >          } else if (ctrl.class == VIRTIO_NET_CTRL_GUEST_OFFLOADS) {
> > @@ -1418,6 +1461,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, 
> > int version_id)
> >          qemu_get_subqueue(n->nic, i)->link_down = link_down;
> >      }
> >  
> > +    n->announce = VIRTIO_NET_ANNOUNCE_ROUNDS;
> >      return 0;
> >  }
> >  
> > @@ -1529,6 +1573,8 @@ static void virtio_net_device_realize(DeviceState 
> > *dev, Error **errp)
> >      qemu_macaddr_default_if_unset(&n->nic_conf.macaddr);
> >      memcpy(&n->mac[0], &n->nic_conf.macaddr, sizeof(n->mac));
> >      n->status = VIRTIO_NET_S_LINK_UP;
> > +    n->announce_timer = timer_new_ns(QEMU_CLOCK_REALTIME,
> > +                                     virtio_net_announce_timer, n);
> >  
> >      if (n->netclient_type) {
> >          /*
> > @@ -1609,6 +1655,7 @@ static void virtio_net_device_unrealize(DeviceState 
> > *dev, Error **errp)
> >          }
> >      }
> >  
> > +    timer_del(n->announce_timer);
> >      g_free(n->vqs);
> >      qemu_del_nic(n->nic);
> >      virtio_cleanup(vdev);
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index df60f16..9f83059 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -48,13 +48,16 @@
> >  #define VIRTIO_NET_F_CTRL_VQ    17      /* Control channel available */
> >  #define VIRTIO_NET_F_CTRL_RX    18      /* Control channel RX mode support 
> > */
> >  #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering 
> > */
> > -#define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support */
> > +#define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control
> > +                                           support */
> 
> ^^ your emacs breaks the comment? 
> 
> > +#define VIRTIO_NET_F_GUEST_ANNOUNCE 21  /* Guest can announce itself */
> >  #define VIRTIO_NET_F_MQ         22      /* Device supports Receive Flow
> >                                           * Steering */
> >  
> >  #define VIRTIO_NET_F_CTRL_MAC_ADDR   23 /* Set MAC address */
> >  
> >  #define VIRTIO_NET_S_LINK_UP    1       /* Link is up */
> > +#define VIRTIO_NET_S_ANNOUNCE   2       /* Announcement is needed */
> >  
> >  #define TX_TIMER_INTERVAL 150000 /* 150 us */
> >  
> > @@ -193,6 +196,8 @@ typedef struct VirtIONet {
> >      char *netclient_name;
> >      char *netclient_type;
> >      uint64_t curr_guest_offloads;
> > +    QEMUTimer *announce_timer;
> > +    int announce;
> >  } VirtIONet;
> >  
> >  #define VIRTIO_NET_CTRL_MAC    1
> > @@ -213,6 +218,17 @@ typedef struct VirtIONet {
> >   #define VIRTIO_NET_CTRL_VLAN_DEL             1
> >  
> >  /*
> > + * Control link announce acknowledgement
> > + *
> > + * The command VIRTIO_NET_CTRL_ANNOUNCE_ACK is used to indicate that
> > + * driver has recevied the notification and device would clear the
> > + * VIRTIO_NET_S_ANNOUNCE bit in the status filed after it received
> > + * this command.
> > + */
> 
> It's a general method for device to notify guest(set status bit, notify)
> and get feedback(by vq command)
> 
> We can extend more vq commands (eg: VIRTIO_NET_CTRL_ACK_SETUP), and
> add a new bit of status 'VIRTIO_NET_S_SETUP'
> 
> So we should rename 'VIRTIO_NET_CTRL_ANNOUNCE' to 'VIRTIO_NET_CTRL_ACK',
> and rename 'VIRTIO_NET_CTRL_ANNOUNCE_ACK' to 'VIRTIO_NET_CTRL_ACK_ANNOUNCE'
> 
> ACK is the action of vq command, ANNOUNCE is the type.

Current method is documented by virito spec. I don't see any other
similar use cases for virtio-net currently. 
> 
> > +#define VIRTIO_NET_CTRL_ANNOUNCE       3
> > + #define VIRTIO_NET_CTRL_ANNOUNCE_ACK         0
> 
> > +
> > +/*
> >   * Control Multiqueue
> >   *
> >   * The command VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET
> > @@ -251,6 +267,7 @@ struct virtio_net_ctrl_mq {
> >          DEFINE_PROP_BIT("guest_tso6", _state, _field, 
> > VIRTIO_NET_F_GUEST_TSO6, true), \
> >          DEFINE_PROP_BIT("guest_ecn", _state, _field, 
> > VIRTIO_NET_F_GUEST_ECN, true), \
> >          DEFINE_PROP_BIT("guest_ufo", _state, _field, 
> > VIRTIO_NET_F_GUEST_UFO, true), \
> > +        DEFINE_PROP_BIT("guest_announce", _state, _field, 
> > VIRTIO_NET_F_GUEST_ANNOUNCE, true), \
> >          DEFINE_PROP_BIT("host_tso4", _state, _field, 
> > VIRTIO_NET_F_HOST_TSO4, true), \
> >          DEFINE_PROP_BIT("host_tso6", _state, _field, 
> > VIRTIO_NET_F_HOST_TSO6, true), \
> >          DEFINE_PROP_BIT("host_ecn", _state, _field, VIRTIO_NET_F_HOST_ECN, 
> > true), \
> > -- 
> > 1.8.3.2
> > 
>