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 <liuyon...@huawei.com> > > Signed-off-by: Jason Wang <jasow...@redhat.com> > > --- > > --- > > 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 > > >
