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

[Samudrala, Sridhar]

On 6/12/2018 4:54 AM, Michael S. Tsirkin wrote:
> On Wed, Jun 06, 2018 at 10:29:03AM +0800, Jason Wang wrote:
> > On 2018年06月05日 20:33, Michael S. Tsirkin wrote:
> > > I don't think this is sufficient.
> > >
> > > If both primary and standby devices are present, a legacy guest without
> > > support for the feature might see two devices with same mac and get
> > > confused.
> > >
> > > I think that we should only make primary visible after guest acked the
> > > backup feature bit.
> > I think we want exactly the reverse? E.g fail the negotiation when guest
> > does not ack backup feature.
> >
> > Otherwise legacy guest won't even have the chance to see primary device in
> > the guest.
> That's by design.
>
> > > And on reset or when backup is cleared in some other way, unplug the
> > > primary.
> > What if guest does not support hotplug?
> It shouldn't ack the backup feature then and it's a good point.
> We should both document this and check kernel config has
> hotplug support. Sridhar could you take a look pls?

Right now we select NET_FAILOVER when virtio-net is enabled,  Should we select
PCI_HOTPLUG when NET_FAILOVER is enabled?

> > > Something like the below will do the job:
> > >
> > > Primary device is added with a special "primary-failover" flag.
> > > A virtual machine is then initialized with just a standby virtio
> > > device. Primary is not yet added.
> > A question is how to do the matching? Qemu knows nothing about e.g mac
> > address of a pass-through device I believe?
> Supply a flag to the VFIO when it's added, this way QEMU will know.
>
> > > Later QEMU detects that guest driver device set DRIVER_OK.
> > > It then exposes the primary device to the guest, and triggers
> > > a device addition event (hot-plug event) for it.
> > Do you mean we won't have primary device in the initial qemu cli?
> No, that's not what I mean.
>
> I mean management will supply a flag to VFIO and then
>
>
> - VFIO defers exposing
> primary to guest until guest acks the feature bit.
> - When we see guest ack, initiate hotplug.
> - On reboot, hide it again.
> - On reset without reboot, request hot-unplug and on eject hide it again.
>
>
> > > If QEMU detects guest driver removal, it initiates a hot-unplug sequence
> > > to remove the primary driver.  In particular, if QEMU detects guest
> > > re-initialization (e.g. by detecting guest reset) it immediately removes
> > > the primary device.
> > I believe guest failover module should handle this gracefully?
> It can't control enumeration order, if primary is enumerated before
> standby then guest will load its driver and it's too late
> when failover driver is loaded.

Does it mean that if guest unloads virtio-net driver, Qemu should automatically
unplug the associated primary device?

What about guest unloading/re-loading primary device driver?


> > Thanks
> >
> > > We can move some of this code to management as well, architecturally it
> > > does not make too much sense but it might be easier implementation-wise.
> > >
> > > HTH
> > >
> > > On Mon, Jun 04, 2018 at 06:41:48PM -0700, 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?
> > > > >
> > > > > Signed-off-by: Sridhar Samudrala <address@hidden>
> > > > > ---
> > > > >     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