Re: [PATCH v2 1/1] virtio: failover: define the default device to use in case of error

[Laurent Vivier <lvivier@redhat.com>]

On 11/08/2021 06:18, Jason Wang wrote:
> On Tue, Aug 10, 2021 at 1:14 AM Laurent Vivier <lvivier@redhat.com> wrote:
>>
>> If the guest driver doesn't support the STANDBY feature, by default
>> we keep the virtio-net device and don't hotplug the VFIO device,
>> but in some cases, user can prefer to use the VFIO device rather
>> than the virtio-net one. We can't unplug the virtio-net device
>> (because on migration it is expected on the destination side)
>> but we can force the guest driver to be disabled. Then, we can
>> hotplug the VFIO device that will be unplugged before the migration
>> like in the normal failover migration but without the failover device.
>>
>> This patch adds a new property to virtio-net device: "failover-default".
>>
>> By default, "failover-default" is set to true and thus the default NIC
>> to use if the failover cannot be enabled is the virtio-net device
>> (this is what is done until now with the virtio-net failover).
>>
>> If "failover-default" is set to false, in case of error, the virtio-net
>> device is not the default anymore and the failover primary device
>> is used instead.
>>
>> If the STANDBY feature is supported by guest and host, the virtio-net
>> failover acts as usual.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>  include/hw/virtio/virtio-net.h |  1 +
>>  hw/net/virtio-net.c            | 49 +++++++++++++++++++++++++++++-----
>>  2 files changed, 44 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
>> index 824a69c23f06..ab77930a327e 100644
>> --- a/include/hw/virtio/virtio-net.h
>> +++ b/include/hw/virtio/virtio-net.h
>> @@ -208,6 +208,7 @@ struct VirtIONet {
>>      /* primary failover device is hidden*/
>>      bool failover_primary_hidden;
>>      bool failover;
>> +    bool failover_default;
>>      DeviceListener primary_listener;
>>      Notifier migration_state;
>>      VirtioNetRssData rss_data;
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 16d20cdee52a..972c03232a96 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -935,12 +935,23 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>>          memset(n->vlans, 0xff, MAX_VLAN >> 3);
>>      }
>>
>> -    if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) {
>> -        qapi_event_send_failover_negotiated(n->netclient_name);
>> -        qatomic_set(&n->failover_primary_hidden, false);
>> -        failover_add_primary(n, &err);
>> -        if (err) {
>> -            warn_report_err(err);
>> +    /*
>> +     * if the virtio-net driver has the STANDBY feature, we can plug the primary
>> +     * if not but is not the default failover device,
>> +     * we need to plug the primary alone and the virtio-net driver will
>> +     * be disabled in the validate_features() function but validate_features()
>> +     * is only available with virtio 1.0 spec
>> +     */
>> +    if (n->failover) {
>> +        if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY) ||
>> +           (virtio_has_feature(features, VIRTIO_F_VERSION_1) &&
> 
> I think STANDY implies VERSION_1.
> 
> And if we do this, it means it doesn't work for legacy drivers.
> 
> Not sure if it's an issue.

Yes, you're right. In case of a kernel driver that doesn't support STANDBY and that is not
version 1, the virtio-net device is disabled (and the VFIO device is not plugged).

Thanks,
Laurent