Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET

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

On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote:
> >> +static int virtnet_reset(struct virtnet_info *vi)
> >> +{
> >> +     struct virtio_device *dev = vi->vdev;
> >> +     int ret;
> >> +
> >> +     virtio_config_disable(dev);
> >> +     dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
> >> +     virtnet_freeze_down(dev, true);
> >> +     remove_vq_common(vi);
> >> +
> >> +     virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> >> +     virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> >> +
> >> +     ret = virtio_finalize_features(dev);
> >> +     if (ret)
> >> +             goto err;
> >> +
> >> +     ret = virtnet_restore_up(dev);
> >> +     if (ret)
> >> +             goto err;
> >> +
> >> +     ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
> >> +     if (ret)
> >> +             goto err;
> >> +
> >> +     virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> >> +     virtio_config_enable(dev);
> >> +     return 0;
> >> +
> >> +err:
> >> +     virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
> >> +     return ret;
> >> +}
> >> +
> >>  static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
> >>  {
> >>       struct scatterlist sg;
> >
> > I have a question here though. How do things like MAC address
> > get restored?
> >
> > What about the rx mode?
> >
> > vlans?
> 
> The function as is releases and reinitializes only ring state.
> Device configuration such as mac and vlan persist across
> the reset.

What gave you this impression? Take a look at e.g. this
code in qemu:

static void virtio_net_reset(VirtIODevice *vdev)
{   
    VirtIONet *n = VIRTIO_NET(vdev);
    
    /* Reset back to compatibility mode */
    n->promisc = 1;
    n->allmulti = 0;
    n->alluni = 0;
    n->nomulti = 0;
    n->nouni = 0;
    n->nobcast = 0;
    /* multiqueue is disabled by default */
    n->curr_queues = 1;
    timer_del(n->announce_timer);
    n->announce_counter = 0;
    n->status &= ~VIRTIO_NET_S_ANNOUNCE;
    
    /* Flush any MAC and VLAN filter table state */
    n->mac_table.in_use = 0; 
    n->mac_table.first_multi = 0; 
    n->mac_table.multi_overflow = 0;
    n->mac_table.uni_overflow = 0;
    memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
    memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
    qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
    memset(n->vlans, 0, MAX_VLAN >> 3);
}

So device seems to lose all state, you have to re-program it.


> > Also, it seems that LINK_ANNOUNCE requests will get ignored
> > even if they got set before the reset, leading to downtime.
> 
> Do you mean act on VIRTIO_NET_F_GUEST_ANNOUNCE
> requests? That flag is tested and netdev_notify_peers
> called before resetting virtio ring state.

Yes but I wonder if there's a race where announce
is set after it is read but before NEED_RESET is read.

Re-reading status from the config before reset
might be necessary.

-- 
MST