Linux Virtualization - Jun 2018 - [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net

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.

And on reset or when backup is cleared in some other way, unplug the
primary.

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.

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.

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.

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 <sridhar.samudrala at
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

On 6/5/2018 5:33 AM, Michael S. Tsirkin wrote:
> I don't think this is sufficient.
Sure. This is not sufficient for a complete solution, but is Qemu the right
place
to manage primary/standby interfaces?

I think the other steps including plugging/unplugging the primary interface
needs
to handled by some orchestration layer.

>
> 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.
Yes. Isn't this possible today when a VM is started with a mis-configured
domain
XML file that passes a virtio-net interface and a VF with the same MAC?

>
> I think that we should only make primary visible after guest acked the
> backup feature bit.
The primary can be plugged/unplugged at any time by the management layer. So
i guess this needs to be handled in the libvirt layer.
>
> And on reset or when backup is cleared in some other way, unplug the
> primary.
>
> Something like the below will do the job:
>
> Primary device is added with a special "primary-failover" flag.
Are you suggesting an extension to Qemu to add another flag for
primary device too?
> A virtual machine is then initialized with just a standby virtio
> device. Primary is not yet added.
>
> 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.
>
> 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.
>
> 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 <sridhar.samudrala at
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

On Tue, Jun 05, 2018 at 01:20:33PM -0700, Samudrala, Sridhar
wrote:
> 
> On 6/5/2018 5:33 AM, Michael S. Tsirkin wrote:
> > I don't think this is sufficient.
> 
> Sure. This is not sufficient for a complete solution, but is Qemu the right
place
> to manage primary/standby interfaces?
> 
> I think the other steps including plugging/unplugging the primary interface
needs
> to handled by some orchestration layer.
I don't see any real benefit to it but yes, you can push it up the stack.
Unfortunately the patch still won't be sufficient then, see below :)
> 
> > 
> > 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.
> 
> Yes. Isn't this possible today when a VM is started with a
mis-configured domain
> XML file that passes a virtio-net interface and a VF with the same MAC?
Yes, so if you misconfigure the hypervisor then your guest won't run.
Not sure what you are trying to say by this though.
> 
> > 
> > I think that we should only make primary visible after guest acked the
> > backup feature bit.
> 
> The primary can be plugged/unplugged at any time by the management layer.
> So i guess this needs to be handled in the libvirt layer.
management just wants to give the primary to guest and later take it back,
it really does not care about the details of the process,
so I don't see what does pushing it up the stack buy you.

So I don't think it *needs* to be done in libvirt. It probably can if you
add a bunch of hooks so it knows whenever vm reboots, driver binds and
unbinds from device, and can check that backup flag was set.
If you are pushing for a setup like that please get a buy-in
from libvirt maintainers or better write a patch.
> > 
> > And on reset or when backup is cleared in some other way, unplug the
> > primary.
> > 
> > Something like the below will do the job:
> > 
> > Primary device is added with a special "primary-failover"
flag.
> 
> Are you suggesting an extension to Qemu to add another flag for
> primary device too?
Exactly.
> > A virtual machine is then initialized with just a standby virtio
> > device. Primary is not yet added.
> > 
> > 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.
> > 
> > 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.
> > 
> > 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 <sridhar.samudrala at
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

Good to see this discussion going. I share the same feeling that the
decision of plugging the primary (passthrough) should only be made
until guest driver acknowledges DRIVER_OK and _F_STANDBY.
Architecturally this intelligence should be baken to QEMU itself
rather than moving up to management stack, such as libvirt.

A few questions in line below.

On Tue, Jun 5, 2018 at 5:33 AM, Michael S. Tsirkin <mst at redhat.com>
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.
>
> And on reset or when backup is cleared in some other way, unplug the
> primary.
>
> Something like the below will do the job:
>
> Primary device is added with a special "primary-failover" flag.
I wonder if you envision this flag as a user interface or some
internal attribute/property to QEMU device?

Since QEMU needs to associate this particular primary-failover device
with a virtio standby instance for checking DRIVER_OK as you indicate
below, I wonder if the group ID thing will be helpful to set
"primary-failover" flag internally without having to add another
option in CLI?
> A virtual machine is then initialized with just a standby virtio
> device. Primary is not yet added.
>
> 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.
Sounds OK. While there might be a small window the guest already
starts to use virtio interface before the passthrough gets plugged in,
I think that should be fine.

Another option is to wait until the primary appears and gets
registered in the guest, so it can bring up the upper failover
interface.
>
> 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.
Agreed.

For legacy guest, user might prefer seeing a single passthrough device
rather than a virtio device. I would hope there's an option to allow
it to happen, instead of removing all passthrough devices.

Regards,
-Siwei
>
> 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 <sridhar.samudrala at
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
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe at lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help at lists.oasis-open.org
>

On Tue, Jun 05, 2018 at 02:16:44PM -0700, Siwei Liu
wrote:
> Good to see this discussion going. I share the same feeling that the
> decision of plugging the primary (passthrough) should only be made
> until guest driver acknowledges DRIVER_OK and _F_STANDBY.
> Architecturally this intelligence should be baken to QEMU itself
> rather than moving up to management stack, such as libvirt.
> 
> A few questions in line below.
> 
> On Tue, Jun 5, 2018 at 5:33 AM, Michael S. Tsirkin <mst at
redhat.com> 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.
> >
> > And on reset or when backup is cleared in some other way, unplug the
> > primary.
> >
> > Something like the below will do the job:
> >
> > Primary device is added with a special "primary-failover"
flag.
> 
> I wonder if you envision this flag as a user interface or some
> internal attribute/property to QEMU device?
> 
> Since QEMU needs to associate this particular primary-failover device
> with a virtio standby instance for checking DRIVER_OK as you indicate
> below, I wonder if the group ID thing will be helpful to set
> "primary-failover" flag internally without having to add another
> option in CLI?
I haven't thought about it but it's an option.
> > A virtual machine is then initialized with just a standby virtio
> > device. Primary is not yet added.
> >
> > 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.
> 
> Sounds OK. While there might be a small window the guest already
> starts to use virtio interface before the passthrough gets plugged in,
> I think that should be fine.
> 
> Another option is to wait until the primary appears and gets
> registered in the guest, so it can bring up the upper failover
> interface.
We can't be sure this will ever happen, can we? We might be asked to
migrate at any time.
> >
> > 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.
> 
> Agreed.
> 
> For legacy guest, user might prefer seeing a single passthrough device
> rather than a virtio device. I would hope there's an option to allow
> it to happen, instead of removing all passthrough devices.
> 
> Regards,
> -Siwei
I don't see a way to make it work since then you can't migrate, can you?

The way I see it, if you don't need migration, just use PT without
failover.  If you do, then you either use virtio or failover if guest
supports it.
> >
> > 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 <sridhar.samudrala at
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
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe at lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help at
lists.oasis-open.org
> >

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.
>
> And on reset or when backup is cleared in some other way, unplug the
> primary.
What if guest does not support hotplug?
>
> 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?
>
> 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?
>
> 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?

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 <sridhar.samudrala at
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

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?
> > 
> > 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.
> 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 <sridhar.samudrala at
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