Re: [Qemu-devel] [V4 PATCH 4/5] virtio-net: notify guest to annouce itself

[Michael S. Tsirkin]

On Tue, Mar 13, 2012 at 04:56:40PM +0800, Jason Wang wrote:
> It's hard to track all mac addresses and their usage (vlan, bondings,
> ipv6) in qemu to send proper gratuitous packet. The better choice is
> to let guest to send them.
> 
> 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
> done the announcement, it should clear that bit. The feature is negotiated by
> a new feature bit VIRTIO_NET_F_ANNOUNCE.
> 
> Signed-off-by: Jason Wang <address@hidden>
> ---
>  hw/virtio-net.c |   19 ++++++++++++++++++-
>  hw/virtio-net.h |    3 +++
>  2 files changed, 21 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index bc5e3a8..a591a48 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -95,6 +95,8 @@ static void virtio_net_set_config(VirtIODevice *vdev, const 
> uint8_t *config)
>          memcpy(n->mac, netcfg.mac, ETH_ALEN);
>          qemu_format_nic_info_str(&n->nic->nc, n->mac);
>      }
> +
> +    memcpy(&n->status, &netcfg.status, sizeof(n->status));

This overwrites all of status, which seems wrong as
it will also overwrite the link state - that should
have been read-only.

>  }
>  
>  static bool virtio_net_started(VirtIONet *n, uint8_t status)
> @@ -227,7 +229,7 @@ static uint32_t virtio_net_get_features(VirtIODevice 
> *vdev, uint32_t features)
>  {
>      VirtIONet *n = to_virtio_net(vdev);
>  
> -    features |= (1 << VIRTIO_NET_F_MAC);
> +    features |= (1 << VIRTIO_NET_F_MAC | 1 << VIRTIO_NET_F_GUEST_ANNOUNCE);
>  

Why do you force this bit on?
We have a property below ...

>      if (peer_has_vnet_hdr(n)) {
>          tap_using_vnet_hdr(n->nic->nc.peer, 1);
> @@ -983,6 +985,20 @@ static void virtio_net_cleanup(VLANClientState *nc)
>      n->nic = NULL;
>  }
>  
> +static int virtio_net_announce(VLANClientState *nc)
> +{
> +    VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
> +
> +    if (n->vdev.guest_features & (0x1 << VIRTIO_NET_F_GUEST_ANNOUNCE)
> +        && n->status & VIRTIO_NET_S_LINK_UP) {
> +        n->status |= VIRTIO_NET_S_ANNOUNCE;
> +        virtio_notify_config(&n->vdev);
> +        return 0;
> +    }
> +
> +    return 1;
> +}
> +
>  static NetClientInfo net_virtio_info = {
>      .type = NET_CLIENT_TYPE_NIC,
>      .size = sizeof(NICState),
> @@ -990,6 +1006,7 @@ static NetClientInfo net_virtio_info = {
>      .receive = virtio_net_receive,
>          .cleanup = virtio_net_cleanup,
>      .link_status_changed = virtio_net_set_link_status,
> +    .announce = virtio_net_announce,
>  };
>  
>  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
> diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> index 4468741..9f8cea7 100644
> --- a/hw/virtio-net.h
> +++ b/hw/virtio-net.h
> @@ -44,8 +44,10 @@
>  #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_GUEST_ANNOUNCE 21  /* Guest can announce itself */
>  
>  #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 */
>  
> @@ -176,6 +178,7 @@ struct virtio_net_ctrl_mac {
>          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), \