| Message ID | 20211027095945.86710-1-lvivier@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] failover: specify an alternate MAC address | expand |
On 10/27/21 11:59, Laurent Vivier 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 keep both interfaces if the MAC addresses are different > (to have the same MAC address can cause kernel crash with old > kernel). The VFIO device will be unplugged before the migration > like in the normal failover migration but without a failover device. > > This patch adds a new property to the virtio-net device: > "failover-legacy-mac" > > If an alternate MAC address is provided with "failover-legacy-mac" and > the STANDBY feature is not supported, both interfaces are plugged > but the standby interface (virtio-net) MAC address is set to the > value provided by the "failover-legacy-mac" parameter. > > If the STANDBY feature is supported by guest and QEMU, the virtio-net > failover acts as usual. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > > Notes: > v2: rename alt-mac to failover-legacy-mac > update doc with text provided by MST > > docs/system/virtio-net-failover.rst | 10 ++++++ > hw/net/virtio-net.c | 48 +++++++++++++++++++++++------ > include/hw/virtio/virtio-net.h | 6 ++++ > 3 files changed, 55 insertions(+), 9 deletions(-) > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index f2014d5ea0b3..0d47d287de14 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -45,6 +45,9 @@ > #include "net_rx_pkt.h" > #include "hw/virtio/vhost.h" > > +/* zero MAC address to check with */ > +static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > + Eventual future cleanup, it might be clearer to declare it extern in "net/net.h" and use the one in qemu_macaddr_default_if_unset().
On Wed, Oct 27, 2021 at 6:00 PM 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 keep both interfaces if the MAC addresses are different > (to have the same MAC address can cause kernel crash with old > kernel). The VFIO device will be unplugged before the migration > like in the normal failover migration but without a failover device. > > This patch adds a new property to the virtio-net device: > "failover-legacy-mac" > > If an alternate MAC address is provided with "failover-legacy-mac" and > the STANDBY feature is not supported, both interfaces are plugged > but the standby interface (virtio-net) MAC address is set to the > value provided by the "failover-legacy-mac" parameter. > > If the STANDBY feature is supported by guest and QEMU, the virtio-net > failover acts as usual. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> Acked-by: Jason Wang <jasowang@redhat.com> > --- > > Notes: > v2: rename alt-mac to failover-legacy-mac > update doc with text provided by MST > > docs/system/virtio-net-failover.rst | 10 ++++++ > hw/net/virtio-net.c | 48 +++++++++++++++++++++++------ > include/hw/virtio/virtio-net.h | 6 ++++ > 3 files changed, 55 insertions(+), 9 deletions(-) > > diff --git a/docs/system/virtio-net-failover.rst b/docs/system/virtio-net-failover.rst > index 6002dc5d96e4..99f21cd55ef7 100644 > --- a/docs/system/virtio-net-failover.rst > +++ b/docs/system/virtio-net-failover.rst > @@ -51,6 +51,16 @@ Usage > is only for pairing the devices within QEMU. The guest kernel module > net_failover will match devices with identical MAC addresses. > > + For legacy guests (including bios/EUFI) not supporting VIRTIO_NET_F_STANDBY, > + two options exist: > + > + 1. if failover-legacy-mac has not been configured (default) > + only the standby virtio-net device is visible to the guest > + > + 2. if failover-legacy-mac has been configured, virtio and vfio devices will > + be presented to guest as two NIC devices, with virtio using the > + failover-legacy-mac address. > + > Hotplug > ------- > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index f2014d5ea0b3..0d47d287de14 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -45,6 +45,9 @@ > #include "net_rx_pkt.h" > #include "hw/virtio/vhost.h" > > +/* zero MAC address to check with */ > +static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > + > #define VIRTIO_NET_VM_VERSION 11 > > #define MAC_TABLE_ENTRIES 64 > @@ -126,7 +129,6 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > VirtIONet *n = VIRTIO_NET(vdev); > struct virtio_net_config netcfg; > NetClientState *nc = qemu_get_queue(n->nic); > - static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > > int ret = 0; > memset(&netcfg, 0 , sizeof(struct virtio_net_config)); > @@ -871,10 +873,21 @@ static void failover_add_primary(VirtIONet *n, Error **errp) > error_propagate(errp, err); > } > > +static void failover_plug_primary(VirtIONet *n) > +{ > + Error *err = NULL; > + > + 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); > + } > +} > + > static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) > { > VirtIONet *n = VIRTIO_NET(vdev); > - Error *err = NULL; > int i; > > if (n->mtu_bypass_backend && > @@ -921,12 +934,22 @@ 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 (n->failover) { > + if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) { > + if (memcmp(&n->legacy_mac, &zero, sizeof(zero)) != 0 && > + memcmp(n->mac, &n->legacy_mac, ETH_ALEN) == 0) { > + /* > + * set_features can be called twice, without & with F_STANDBY, > + * so restore original MAC address > + */ > + memcpy(n->mac, &n->nic->conf->macaddr, sizeof(n->mac)); > + qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac); > + } > + failover_plug_primary(n); > + } else if (memcmp(&n->legacy_mac, &zero, sizeof(zero)) != 0) { > + memcpy(n->mac, &n->legacy_mac, ETH_ALEN); > + qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac); > + failover_plug_primary(n); > } > } > } > @@ -3595,9 +3618,15 @@ static bool primary_unplug_pending(void *opaque) > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > VirtIONet *n = VIRTIO_NET(vdev); > > - if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY)) { > + if (!n->failover) { > return false; > } > + > + if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY) && > + memcmp(&n->legacy_mac, &zero, sizeof(zero)) == 0) { > + return false; > + } > + > primary = failover_find_primary_device(n); > return primary ? primary->pending_deleted_event : false; > } > @@ -3672,6 +3701,7 @@ static Property virtio_net_properties[] = { > DEFINE_PROP_UINT32("rsc_interval", VirtIONet, rsc_timeout, > VIRTIO_NET_RSC_DEFAULT_INTERVAL), > DEFINE_NIC_PROPERTIES(VirtIONet, nic_conf), > + DEFINE_PROP_MACADDR("failover-legacy-mac", VirtIONet, legacy_mac), > DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer, > TX_TIMER_INTERVAL), > DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST), > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h > index eb87032627d2..4b9523def5fb 100644 > --- a/include/hw/virtio/virtio-net.h > +++ b/include/hw/virtio/virtio-net.h > @@ -213,6 +213,12 @@ struct VirtIONet { > QDict *primary_opts; > bool primary_opts_from_json; > Notifier migration_state; > + /* > + * failover: to provide an alternate MAC address allows to keep both > + * cards, primary and stand-by, if the STANDBY feature is not supported > + * by the guest > + */ > + MACAddr legacy_mac; > VirtioNetRssData rss_data; > struct NetRxPkt *rx_pkt; > struct EBPFRSSContext ebpf_rss; > -- > 2.31.1 >
On Wed, Oct 27, 2021 at 11:59:45AM +0200, Laurent Vivier 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 keep both interfaces if the MAC addresses are different > (to have the same MAC address can cause kernel crash with old > kernel). The VFIO device will be unplugged before the migration > like in the normal failover migration but without a failover device. > > This patch adds a new property to the virtio-net device: > "failover-legacy-mac" > > If an alternate MAC address is provided with "failover-legacy-mac" and > the STANDBY feature is not supported, both interfaces are plugged > but the standby interface (virtio-net) MAC address is set to the > value provided by the "failover-legacy-mac" parameter. > > If the STANDBY feature is supported by guest and QEMU, the virtio-net > failover acts as usual. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> Wait a second. What if config is read before features are set? Are we going to provide a legacy or a new mac? I guess current guests do not do this, but the spec does allow this, and then the mac will apparently change for the guests. It might be cleaner to just add a PRIMARY_MAC feature - would need guest work though ... > --- > > Notes: > v2: rename alt-mac to failover-legacy-mac > update doc with text provided by MST > > docs/system/virtio-net-failover.rst | 10 ++++++ > hw/net/virtio-net.c | 48 +++++++++++++++++++++++------ > include/hw/virtio/virtio-net.h | 6 ++++ > 3 files changed, 55 insertions(+), 9 deletions(-) > > diff --git a/docs/system/virtio-net-failover.rst b/docs/system/virtio-net-failover.rst > index 6002dc5d96e4..99f21cd55ef7 100644 > --- a/docs/system/virtio-net-failover.rst > +++ b/docs/system/virtio-net-failover.rst > @@ -51,6 +51,16 @@ Usage > is only for pairing the devices within QEMU. The guest kernel module > net_failover will match devices with identical MAC addresses. > > + For legacy guests (including bios/EUFI) not supporting VIRTIO_NET_F_STANDBY, > + two options exist: > + > + 1. if failover-legacy-mac has not been configured (default) > + only the standby virtio-net device is visible to the guest > + > + 2. if failover-legacy-mac has been configured, virtio and vfio devices will > + be presented to guest as two NIC devices, with virtio using the > + failover-legacy-mac address. > + > Hotplug > ------- > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index f2014d5ea0b3..0d47d287de14 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -45,6 +45,9 @@ > #include "net_rx_pkt.h" > #include "hw/virtio/vhost.h" > > +/* zero MAC address to check with */ > +static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > + > #define VIRTIO_NET_VM_VERSION 11 > > #define MAC_TABLE_ENTRIES 64 > @@ -126,7 +129,6 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > VirtIONet *n = VIRTIO_NET(vdev); > struct virtio_net_config netcfg; > NetClientState *nc = qemu_get_queue(n->nic); > - static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > > int ret = 0; > memset(&netcfg, 0 , sizeof(struct virtio_net_config)); > @@ -871,10 +873,21 @@ static void failover_add_primary(VirtIONet *n, Error **errp) > error_propagate(errp, err); > } > > +static void failover_plug_primary(VirtIONet *n) > +{ > + Error *err = NULL; > + > + 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); > + } > +} > + > static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) > { > VirtIONet *n = VIRTIO_NET(vdev); > - Error *err = NULL; > int i; > > if (n->mtu_bypass_backend && > @@ -921,12 +934,22 @@ 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 (n->failover) { > + if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) { > + if (memcmp(&n->legacy_mac, &zero, sizeof(zero)) != 0 && > + memcmp(n->mac, &n->legacy_mac, ETH_ALEN) == 0) { > + /* > + * set_features can be called twice, without & with F_STANDBY, > + * so restore original MAC address Restore can be confusing. Let's just say that we set it to XX. > + */ > + memcpy(n->mac, &n->nic->conf->macaddr, sizeof(n->mac)); > + qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac); > + } > + failover_plug_primary(n); > + } else if (memcmp(&n->legacy_mac, &zero, sizeof(zero)) != 0) { add a comment here too? > + memcpy(n->mac, &n->legacy_mac, ETH_ALEN); > + qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac); > + failover_plug_primary(n); > } > } > } > @@ -3595,9 +3618,15 @@ static bool primary_unplug_pending(void *opaque) > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > VirtIONet *n = VIRTIO_NET(vdev); > > - if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY)) { > + if (!n->failover) { > return false; > } > + > + if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY) && > + memcmp(&n->legacy_mac, &zero, sizeof(zero)) == 0) { > + return false; > + } > + > primary = failover_find_primary_device(n); > return primary ? primary->pending_deleted_event : false; > } > @@ -3672,6 +3701,7 @@ static Property virtio_net_properties[] = { > DEFINE_PROP_UINT32("rsc_interval", VirtIONet, rsc_timeout, > VIRTIO_NET_RSC_DEFAULT_INTERVAL), > DEFINE_NIC_PROPERTIES(VirtIONet, nic_conf), > + DEFINE_PROP_MACADDR("failover-legacy-mac", VirtIONet, legacy_mac), > DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer, > TX_TIMER_INTERVAL), > DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST), > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h > index eb87032627d2..4b9523def5fb 100644 > --- a/include/hw/virtio/virtio-net.h > +++ b/include/hw/virtio/virtio-net.h > @@ -213,6 +213,12 @@ struct VirtIONet { > QDict *primary_opts; > bool primary_opts_from_json; > Notifier migration_state; > + /* > + * failover: to provide an alternate MAC address allows to keep both > + * cards, primary and stand-by, if the STANDBY feature is not supported > + * by the guest > + */ > + MACAddr legacy_mac; > VirtioNetRssData rss_data; > struct NetRxPkt *rx_pkt; > struct EBPFRSSContext ebpf_rss; > -- > 2.31.1
On 01/11/2021 10:39, Michael S. Tsirkin wrote: > On Wed, Oct 27, 2021 at 11:59:45AM +0200, Laurent Vivier 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 keep both interfaces if the MAC addresses are different >> (to have the same MAC address can cause kernel crash with old >> kernel). The VFIO device will be unplugged before the migration >> like in the normal failover migration but without a failover device. >> >> This patch adds a new property to the virtio-net device: >> "failover-legacy-mac" >> >> If an alternate MAC address is provided with "failover-legacy-mac" and >> the STANDBY feature is not supported, both interfaces are plugged >> but the standby interface (virtio-net) MAC address is set to the >> value provided by the "failover-legacy-mac" parameter. >> >> If the STANDBY feature is supported by guest and QEMU, the virtio-net >> failover acts as usual. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > Wait a second. What if config is read before features are set? > Are we going to provide a legacy or a new mac? We provide the new MAC and at this point the primary device is not plugged. When features are set: - if STANDBY is set, the primary device is plugged, and secondary (virtio-net) uses the new MAC - if STANDBY is not set: - if legacy MAC is provided: the primary device is plugged and legacy MAC is used - else the primary device is not plugged and new MAC is used. > I guess current guests do not do this, but the spec does allow this, > and then the mac will apparently change for the guests. What I read in virtio 1.0 specs, "3.1.1 Driver requirements: Device initialization", is the virtio configuration space (step 7) is is accessed after the features are negotiated. I don't think the part in step 4 can involve the MAC address, and moreover the config is not read before, but during the negotiation (I guess we can see that as the config access is part of the negotiation). 3.1.1 Driver Requirements: Device Initialization The driver MUST follow this sequence to initialize a device: 1. Reset the device. 2. Set the ACKNOWLEDGE status bit: the guest OS has notice the device. 3. Set the DRIVER status bit: the guest OS knows how to drive the device. 4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it. 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step. 6. Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not support our subset of features and the device is unusable. 7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup, reading and possibly writing the device’s virtio configuration space, and population of virtqueues. 8. Set the DRIVER_OK status bit. At this point the device is “live”. > > It might be cleaner to just add a PRIMARY_MAC feature - > would need guest work though ... We can't add a new feature: the goal of this patch is to be able to use the primary device (VFIO) with kernel that doesn't support STANDBY feature. If we can add a feature, to add the STANDBY feature would be a better choice. If changing the MAC address is not acceptable we can return to a mix of v1 and v2 of my patch: "virtio: failover: allow to keep the VFIO device rather than the virtio-net one" https://patchew.org/QEMU/20210729191910.317114-1-lvivier@redhat.com/ that disables the virtio-net driver on the module probe. Thanks, Laurent
On Tue, Nov 02, 2021 at 09:14:51AM +0100, Laurent Vivier wrote: > On 01/11/2021 10:39, Michael S. Tsirkin wrote: > > On Wed, Oct 27, 2021 at 11:59:45AM +0200, Laurent Vivier 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 keep both interfaces if the MAC addresses are different > > > (to have the same MAC address can cause kernel crash with old > > > kernel). The VFIO device will be unplugged before the migration > > > like in the normal failover migration but without a failover device. > > > > > > This patch adds a new property to the virtio-net device: > > > "failover-legacy-mac" > > > > > > If an alternate MAC address is provided with "failover-legacy-mac" and > > > the STANDBY feature is not supported, both interfaces are plugged > > > but the standby interface (virtio-net) MAC address is set to the > > > value provided by the "failover-legacy-mac" parameter. > > > > > > If the STANDBY feature is supported by guest and QEMU, the virtio-net > > > failover acts as usual. > > > > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > > > Wait a second. What if config is read before features are set? > > Are we going to provide a legacy or a new mac? > We provide the new MAC and at this point the primary device is not plugged. > > When features are set: > - if STANDBY is set, the primary device is plugged, and secondary > (virtio-net) uses the new MAC > - if STANDBY is not set: > - if legacy MAC is provided: > the primary device is plugged and legacy MAC is used > - else > the primary device is not plugged and new MAC is used. > > > I guess current guests do not do this, but the spec does allow this, > > and then the mac will apparently change for the guests. > > What I read in virtio 1.0 specs, "3.1.1 Driver requirements: Device > initialization", is the virtio configuration space (step 7) is is accessed > after the features are negotiated. I don't think the part in step 4 can > involve the MAC address, and moreover the config is not read before, but > during the negotiation (I guess we can see that as the config access is part > of the negotiation). > > 3.1.1 Driver Requirements: Device Initialization > > The driver MUST follow this sequence to initialize a device: > 1. Reset the device. > 2. Set the ACKNOWLEDGE status bit: the guest OS has notice the device. > 3. Set the DRIVER status bit: the guest OS knows how to drive the device. > 4. Read device feature bits, and write the subset of feature bits understood > by the OS and driver to the device. During this step the driver MAY read > (but MUST NOT write) the device-specific configuration fields to check that > it can support the device before accepting it. > 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature > bits after this step. > 6. Re-read device status to ensure the FEATURES_OK bit is still set: > otherwise, the device does not support our subset of features and the device > is unusable. > 7. Perform device-specific setup, including discovery of virtqueues for the > device, optional per-bus setup, reading and possibly writing the device’s > virtio configuration space, and population of virtqueues. > 8. Set the DRIVER_OK status bit. At this point the device is “live”. OTOH Device Requirements: Device Configuration Space The device MUST allow reading of any device-specific configuration field before FEATURES_OK is set by the driver. This includes fields which are conditional on feature bits, as long as those feature bits are offered by the device. now, we can maybe make an exception for mac. But this is something to discuss on the virtio TC mailing list. > > > > It might be cleaner to just add a PRIMARY_MAC feature - > > would need guest work though ... > > We can't add a new feature: the goal of this patch is to be able to use the > primary device (VFIO) with kernel that doesn't support STANDBY feature. If > we can add a feature, to add the STANDBY feature would be a better choice. > > If changing the MAC address is not acceptable we can return to a mix of v1 and v2 of my patch: > > "virtio: failover: allow to keep the VFIO device rather than the virtio-net one" > > https://patchew.org/QEMU/20210729191910.317114-1-lvivier@redhat.com/ > > that disables the virtio-net driver on the module probe. > > Thanks, > Laurent
On Tue, Nov 02, 2021 at 09:14:51AM +0100, Laurent Vivier wrote: > On 01/11/2021 10:39, Michael S. Tsirkin wrote: > > On Wed, Oct 27, 2021 at 11:59:45AM +0200, Laurent Vivier 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 keep both interfaces if the MAC addresses are different > > > (to have the same MAC address can cause kernel crash with old > > > kernel). The VFIO device will be unplugged before the migration > > > like in the normal failover migration but without a failover device. > > > > > > This patch adds a new property to the virtio-net device: > > > "failover-legacy-mac" > > > > > > If an alternate MAC address is provided with "failover-legacy-mac" and > > > the STANDBY feature is not supported, both interfaces are plugged > > > but the standby interface (virtio-net) MAC address is set to the > > > value provided by the "failover-legacy-mac" parameter. > > > > > > If the STANDBY feature is supported by guest and QEMU, the virtio-net > > > failover acts as usual. > > > > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > > > Wait a second. What if config is read before features are set? > > Are we going to provide a legacy or a new mac? > We provide the new MAC and at this point the primary device is not plugged. > > When features are set: > - if STANDBY is set, the primary device is plugged, and secondary > (virtio-net) uses the new MAC > - if STANDBY is not set: > - if legacy MAC is provided: > the primary device is plugged and legacy MAC is used > - else > the primary device is not plugged and new MAC is used. > > > I guess current guests do not do this, but the spec does allow this, > > and then the mac will apparently change for the guests. > > What I read in virtio 1.0 specs, "3.1.1 Driver requirements: Device > initialization", is the virtio configuration space (step 7) is is accessed > after the features are negotiated. I don't think the part in step 4 can > involve the MAC address, and moreover the config is not read before, but > during the negotiation (I guess we can see that as the config access is part > of the negotiation). > > 3.1.1 Driver Requirements: Device Initialization > > The driver MUST follow this sequence to initialize a device: > 1. Reset the device. > 2. Set the ACKNOWLEDGE status bit: the guest OS has notice the device. > 3. Set the DRIVER status bit: the guest OS knows how to drive the device. > 4. Read device feature bits, and write the subset of feature bits understood > by the OS and driver to the device. During this step the driver MAY read > (but MUST NOT write) the device-specific configuration fields to check that > it can support the device before accepting it. Note this part here. config is read before FEATURES_OK. > 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature > bits after this step. > 6. Re-read device status to ensure the FEATURES_OK bit is still set: > otherwise, the device does not support our subset of features and the device > is unusable. > 7. Perform device-specific setup, including discovery of virtqueues for the > device, optional per-bus setup, reading and possibly writing the device’s > virtio configuration space, and population of virtqueues. > 8. Set the DRIVER_OK status bit. At this point the device is “live”. > > > > > It might be cleaner to just add a PRIMARY_MAC feature - > > would need guest work though ... > > We can't add a new feature: the goal of this patch is to be able to use the > primary device (VFIO) with kernel that doesn't support STANDBY feature. If > we can add a feature, to add the STANDBY feature would be a better choice. The point would be a new feature replacing STANDBY: instead of using the same MAC for both devices it specifies the MAC of the primary. And if we go there, I would add a third MAC as the mac of the failover device itself. Thus: standby-mac: used by legacy virtio drivers primary-mac: used to locate the primary device and by legacy primary drivers failover-mac: programmed by guest driver into whatever device guest is using at the moment What we lose there is all these drivers already supporting STANDBY, but hey, won't matter so much in X years. Some primary devices do not support mac programming. I guess specifying failover-mac=primary-mac would be an option for these. Thoughts? > If changing the MAC address is not acceptable we can return to a mix of v1 and v2 of my patch: > > "virtio: failover: allow to keep the VFIO device rather than the virtio-net one" > > https://patchew.org/QEMU/20210729191910.317114-1-lvivier@redhat.com/ > > that disables the virtio-net driver on the module probe. > > Thanks, > Laurent
diff --git a/docs/system/virtio-net-failover.rst b/docs/system/virtio-net-failover.rst index 6002dc5d96e4..99f21cd55ef7 100644 --- a/docs/system/virtio-net-failover.rst +++ b/docs/system/virtio-net-failover.rst @@ -51,6 +51,16 @@ Usage is only for pairing the devices within QEMU. The guest kernel module net_failover will match devices with identical MAC addresses. + For legacy guests (including bios/EUFI) not supporting VIRTIO_NET_F_STANDBY, + two options exist: + + 1. if failover-legacy-mac has not been configured (default) + only the standby virtio-net device is visible to the guest + + 2. if failover-legacy-mac has been configured, virtio and vfio devices will + be presented to guest as two NIC devices, with virtio using the + failover-legacy-mac address. + Hotplug ------- diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index f2014d5ea0b3..0d47d287de14 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -45,6 +45,9 @@ #include "net_rx_pkt.h" #include "hw/virtio/vhost.h" +/* zero MAC address to check with */ +static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; + #define VIRTIO_NET_VM_VERSION 11 #define MAC_TABLE_ENTRIES 64 @@ -126,7 +129,6 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) VirtIONet *n = VIRTIO_NET(vdev); struct virtio_net_config netcfg; NetClientState *nc = qemu_get_queue(n->nic); - static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; int ret = 0; memset(&netcfg, 0 , sizeof(struct virtio_net_config)); @@ -871,10 +873,21 @@ static void failover_add_primary(VirtIONet *n, Error **errp) error_propagate(errp, err); } +static void failover_plug_primary(VirtIONet *n) +{ + Error *err = NULL; + + 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); + } +} + static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) { VirtIONet *n = VIRTIO_NET(vdev); - Error *err = NULL; int i; if (n->mtu_bypass_backend && @@ -921,12 +934,22 @@ 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 (n->failover) { + if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) { + if (memcmp(&n->legacy_mac, &zero, sizeof(zero)) != 0 && + memcmp(n->mac, &n->legacy_mac, ETH_ALEN) == 0) { + /* + * set_features can be called twice, without & with F_STANDBY, + * so restore original MAC address + */ + memcpy(n->mac, &n->nic->conf->macaddr, sizeof(n->mac)); + qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac); + } + failover_plug_primary(n); + } else if (memcmp(&n->legacy_mac, &zero, sizeof(zero)) != 0) { + memcpy(n->mac, &n->legacy_mac, ETH_ALEN); + qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac); + failover_plug_primary(n); } } } @@ -3595,9 +3618,15 @@ static bool primary_unplug_pending(void *opaque) VirtIODevice *vdev = VIRTIO_DEVICE(dev); VirtIONet *n = VIRTIO_NET(vdev); - if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY)) { + if (!n->failover) { return false; } + + if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY) && + memcmp(&n->legacy_mac, &zero, sizeof(zero)) == 0) { + return false; + } + primary = failover_find_primary_device(n); return primary ? primary->pending_deleted_event : false; } @@ -3672,6 +3701,7 @@ static Property virtio_net_properties[] = { DEFINE_PROP_UINT32("rsc_interval", VirtIONet, rsc_timeout, VIRTIO_NET_RSC_DEFAULT_INTERVAL), DEFINE_NIC_PROPERTIES(VirtIONet, nic_conf), + DEFINE_PROP_MACADDR("failover-legacy-mac", VirtIONet, legacy_mac), DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer, TX_TIMER_INTERVAL), DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST), diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h index eb87032627d2..4b9523def5fb 100644 --- a/include/hw/virtio/virtio-net.h +++ b/include/hw/virtio/virtio-net.h @@ -213,6 +213,12 @@ struct VirtIONet { QDict *primary_opts; bool primary_opts_from_json; Notifier migration_state; + /* + * failover: to provide an alternate MAC address allows to keep both + * cards, primary and stand-by, if the STANDBY feature is not supported + * by the guest + */ + MACAddr legacy_mac; VirtioNetRssData rss_data; struct NetRxPkt *rx_pkt; struct EBPFRSSContext ebpf_rss;
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 keep both interfaces if the MAC addresses are different (to have the same MAC address can cause kernel crash with old kernel). The VFIO device will be unplugged before the migration like in the normal failover migration but without a failover device. This patch adds a new property to the virtio-net device: "failover-legacy-mac" If an alternate MAC address is provided with "failover-legacy-mac" and the STANDBY feature is not supported, both interfaces are plugged but the standby interface (virtio-net) MAC address is set to the value provided by the "failover-legacy-mac" parameter. If the STANDBY feature is supported by guest and QEMU, the virtio-net failover acts as usual. Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- Notes: v2: rename alt-mac to failover-legacy-mac update doc with text provided by MST docs/system/virtio-net-failover.rst | 10 ++++++ hw/net/virtio-net.c | 48 +++++++++++++++++++++++------ include/hw/virtio/virtio-net.h | 6 ++++ 3 files changed, 55 insertions(+), 9 deletions(-)