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

[Jason Wang <jasowang@redhat.com>]

On 2018年06月12日 19:54, 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.

So management needs to know the capability of guest to set the backup 
feature. This looks a chicken or egg problem to me.

>
>>> 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.

This sounds much like a kind of bonding in qemu.

>
>>> 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.

Well, even if we can delay the visibility of primary until DRIVER_OK, 
there still be a race I think? And it looks to me it's still a bug of guest:

E.g primary could be probed before failover_register() in guest. Then we 
will miss the enslaving of primary forever.

Thanks

>
>> 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@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