Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net

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

On Wed, Jun 06, 2018 at 11:17:36AM -0700, Samudrala, Sridhar wrote:
> On 6/4/2018 7:06 PM, Jason Wang wrote:
> > 
> > 
> > On 2018年06月05日 09:41, Samudrala, Sridhar wrote:
> > > Ping on this patch now that the kernel patches are accepted into
> > > davem's net-next tree.
> > > https://patchwork.ozlabs.org/cover/920005/
> > > 
> > > 
> > > On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
> > > > This feature bit can be used by hypervisor to indicate
> > > > virtio_net device to
> > > > act as a standby for another device with the same MAC address.
> > > > 
> > > > I tested this with a small change to the patch to mark the
> > > > STANDBY feature 'true'
> > > > by default as i am using libvirt to start the VMs.
> > > > Is there a way to pass the newly added feature bit 'standby' to
> > > > qemu via libvirt
> > > > XML file?
> > > > 
> > 
> > Maybe you can try qemu command line passthrough:
> > 
> > https://libvirt.org/drvqemu.html#qemucommand
> 
> It looks like this can be used to pass command line arguments to qemu.
> Is it possible to specify a virtio specific attribute via this method?
> 
> For ex: to say mrg_rxbuf is off we can add the following line to virtio
> section of the domain xml file.
>   <host mrg_rxbuf='off'/>
> 
> I think libvirt needs to be extended to to support the new 'standby' attribute
> via this mechanism.
> Adding Liane Stump and libvirt to the CC list.
> 
> Michael,
> Can we start with getting this patch into Qemu and an update to libvirt to
> support the 'standby' feature so that this feature can be enabled via
> some scripts/orchestration layer for now.

Unfortunately we don't give the hypothetical orchestration layer
enough info about driver being bound, so it does not know
when is it safe to add a primary.  And a similar issue around reset.

We could add events for that which would be a small patch,
but the issue then is that guest can trigger a storm of
these events.

> We could improve this solution by enhancing Qemu to do automatic management of the
> addition/deletion of the primary device based on feature negotiation as a later patch.

I'm not sure what the point of the back and forth would be though.
Typically after people invest in a specific config and get it working,
it's very hard to move them to another solution.

> 
> > 
> > The patch looks good to me. Let's see if Michael have any comment.
> > 
> > Thanks
> > 
> > > > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > > > ---
> > > >   hw/net/virtio-net.c                         | 2 ++
> > > >   include/standard-headers/linux/virtio_net.h | 3 +++
> > > >   2 files changed, 5 insertions(+)
> > > > 
> > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > index 90502fca7c..38b3140670 100644
> > > > --- a/hw/net/virtio-net.c
> > > > +++ b/hw/net/virtio-net.c
> > > > @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
> > > >                        true),
> > > >       DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed,
> > > > SPEED_UNKNOWN),
> > > >       DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
> > > > +    DEFINE_PROP_BIT64("standby", VirtIONet, host_features,
> > > > VIRTIO_NET_F_STANDBY,
> > > > +                      false),
> > > >       DEFINE_PROP_END_OF_LIST(),
> > > >   };
> > > >   diff --git a/include/standard-headers/linux/virtio_net.h
> > > > b/include/standard-headers/linux/virtio_net.h
> > > > index e9f255ea3f..01ec09684c 100644
> > > > --- a/include/standard-headers/linux/virtio_net.h
> > > > +++ b/include/standard-headers/linux/virtio_net.h
> > > > @@ -57,6 +57,9 @@
> > > >                        * Steering */
> > > >   #define VIRTIO_NET_F_CTRL_MAC_ADDR 23    /* Set MAC address */
> > > >   +#define VIRTIO_NET_F_STANDBY      62    /* Act as standby for
> > > > another device
> > > > +                                         * with the same MAC.
> > > > +                                         */
> > > >   #define VIRTIO_NET_F_SPEED_DUPLEX 63    /* Device set
> > > > linkspeed and duplex */
> > > >     #ifndef VIRTIO_NET_NO_LEGACY
> > > 
> >