[PATCH 02/11] pci: add option for net failover

Jens Freimann posted 11 patches 6 years, 3 months ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Juan Quintela <quintela@redhat.com>, Alex Williamson <alex.williamson@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Eric Blake <eblake@redhat.com>, Jason Wang <jasowang@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
[PATCH 02/11] pci: add option for net failover
Posted by Jens Freimann 6 years, 3 months ago
This patch adds a net_failover_pair_id property to PCIDev which is
used to link the primary device in a failover pair (the PCI dev) to
a standby (a virtio-net-pci) device.

It only supports ethernet devices. Also currently it only supports
PCIe devices. QEMU will exit with an error message otherwise.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 hw/pci/pci.c         | 17 +++++++++++++++++
 include/hw/pci/pci.h |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index aa05c2b9b2..fa9b5219f8 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -75,6 +75,8 @@ static Property pci_props[] = {
                     QEMU_PCIE_LNKSTA_DLLLA_BITNR, true),
     DEFINE_PROP_BIT("x-pcie-extcap-init", PCIDevice, cap_present,
                     QEMU_PCIE_EXTCAP_INIT_BITNR, true),
+    DEFINE_PROP_STRING("net_failover_pair_id", PCIDevice,
+            net_failover_pair_id),
     DEFINE_PROP_END_OF_LIST()
 };
 
@@ -2077,6 +2079,7 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
     ObjectClass *klass = OBJECT_CLASS(pc);
     Error *local_err = NULL;
     bool is_default_rom;
+    uint16_t class_id;
 
     /* initialize cap_present for pci_is_express() and pci_config_size(),
      * Note that hybrid PCIs are not set automatically and need to manage
@@ -2101,6 +2104,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
         }
     }
 
+    if (pci_dev->net_failover_pair_id) {
+        if (!pci_is_express(pci_dev)) {
+            error_setg(errp, "failover device is not a PCIExpress device");
+            error_propagate(errp, local_err);
+            return;
+        }
+        class_id = pci_get_word(pci_dev->config + PCI_CLASS_DEVICE);
+        if (class_id != PCI_CLASS_NETWORK_ETHERNET) {
+            error_setg(errp, "failover device is not an Ethernet device");
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
     /* rom loading */
     is_default_rom = false;
     if (pci_dev->romfile == NULL && pc->romfile != NULL) {
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index f3f0ffd5fb..def5435685 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -352,6 +352,9 @@ struct PCIDevice {
     MSIVectorUseNotifier msix_vector_use_notifier;
     MSIVectorReleaseNotifier msix_vector_release_notifier;
     MSIVectorPollNotifier msix_vector_poll_notifier;
+
+    /* ID of standby device in net_failover pair */
+    char *net_failover_pair_id;
 };
 
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
-- 
2.21.0


Re: [PATCH 02/11] pci: add option for net failover
Posted by Alex Williamson 6 years, 3 months ago
On Fri, 18 Oct 2019 22:20:31 +0200
Jens Freimann <jfreimann@redhat.com> wrote:

> This patch adds a net_failover_pair_id property to PCIDev which is
> used to link the primary device in a failover pair (the PCI dev) to
> a standby (a virtio-net-pci) device.
> 
> It only supports ethernet devices. Also currently it only supports
> PCIe devices. QEMU will exit with an error message otherwise.

Doesn't the PCIe device also need to be hotpluggable?  We can have PCIe
devices attached to the root bus where we don't currently support
hotplug.  Thanks,

Alex
 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  hw/pci/pci.c         | 17 +++++++++++++++++
>  include/hw/pci/pci.h |  3 +++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index aa05c2b9b2..fa9b5219f8 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -75,6 +75,8 @@ static Property pci_props[] = {
>                      QEMU_PCIE_LNKSTA_DLLLA_BITNR, true),
>      DEFINE_PROP_BIT("x-pcie-extcap-init", PCIDevice, cap_present,
>                      QEMU_PCIE_EXTCAP_INIT_BITNR, true),
> +    DEFINE_PROP_STRING("net_failover_pair_id", PCIDevice,
> +            net_failover_pair_id),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> @@ -2077,6 +2079,7 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>      ObjectClass *klass = OBJECT_CLASS(pc);
>      Error *local_err = NULL;
>      bool is_default_rom;
> +    uint16_t class_id;
>  
>      /* initialize cap_present for pci_is_express() and pci_config_size(),
>       * Note that hybrid PCIs are not set automatically and need to manage
> @@ -2101,6 +2104,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>          }
>      }
>  
> +    if (pci_dev->net_failover_pair_id) {
> +        if (!pci_is_express(pci_dev)) {
> +            error_setg(errp, "failover device is not a PCIExpress device");
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +        class_id = pci_get_word(pci_dev->config + PCI_CLASS_DEVICE);
> +        if (class_id != PCI_CLASS_NETWORK_ETHERNET) {
> +            error_setg(errp, "failover device is not an Ethernet device");
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +
>      /* rom loading */
>      is_default_rom = false;
>      if (pci_dev->romfile == NULL && pc->romfile != NULL) {
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index f3f0ffd5fb..def5435685 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -352,6 +352,9 @@ struct PCIDevice {
>      MSIVectorUseNotifier msix_vector_use_notifier;
>      MSIVectorReleaseNotifier msix_vector_release_notifier;
>      MSIVectorPollNotifier msix_vector_poll_notifier;
> +
> +    /* ID of standby device in net_failover pair */
> +    char *net_failover_pair_id;
>  };
>  
>  void pci_register_bar(PCIDevice *pci_dev, int region_num,


Re: [PATCH 02/11] pci: add option for net failover
Posted by Jens Freimann 6 years, 3 months ago
On Mon, Oct 21, 2019 at 08:58:23AM -0600, Alex Williamson wrote:
>On Fri, 18 Oct 2019 22:20:31 +0200
>Jens Freimann <jfreimann@redhat.com> wrote:
>
>> This patch adds a net_failover_pair_id property to PCIDev which is
>> used to link the primary device in a failover pair (the PCI dev) to
>> a standby (a virtio-net-pci) device.
>>
>> It only supports ethernet devices. Also currently it only supports
>> PCIe devices. QEMU will exit with an error message otherwise.
>
>Doesn't the PCIe device also need to be hotpluggable?  We can have PCIe
>devices attached to the root bus where we don't currently support
>hotplug.  Thanks,

How do I recognize such a device? hotpluggable in DeviceClass?

regards,
Jens 


Re: [PATCH 02/11] pci: add option for net failover
Posted by Alex Williamson 6 years, 3 months ago
On Mon, 21 Oct 2019 20:45:46 +0200
Jens Freimann <jfreimann@redhat.com> wrote:

> On Mon, Oct 21, 2019 at 08:58:23AM -0600, Alex Williamson wrote:
> >On Fri, 18 Oct 2019 22:20:31 +0200
> >Jens Freimann <jfreimann@redhat.com> wrote:
> >  
> >> This patch adds a net_failover_pair_id property to PCIDev which is
> >> used to link the primary device in a failover pair (the PCI dev) to
> >> a standby (a virtio-net-pci) device.
> >>
> >> It only supports ethernet devices. Also currently it only supports
> >> PCIe devices. QEMU will exit with an error message otherwise.  
> >
> >Doesn't the PCIe device also need to be hotpluggable?  We can have PCIe
> >devices attached to the root bus where we don't currently support
> >hotplug.  Thanks,  
> 
> How do I recognize such a device? hotpluggable in DeviceClass?

I wouldn't expect it to be a device property, it's more of a function
of whether the bus that it's attached to supports hotplug, so probably
something related to the bus controller.  Thanks,

Alex


Re: [PATCH 02/11] pci: add option for net failover
Posted by Jens Freimann 6 years, 3 months ago
On Mon, Oct 21, 2019 at 01:01:22PM -0600, Alex Williamson wrote:
>On Mon, 21 Oct 2019 20:45:46 +0200
>Jens Freimann <jfreimann@redhat.com> wrote:
>
>> On Mon, Oct 21, 2019 at 08:58:23AM -0600, Alex Williamson wrote:
>> >On Fri, 18 Oct 2019 22:20:31 +0200
>> >Jens Freimann <jfreimann@redhat.com> wrote:
>> >
>> >> This patch adds a net_failover_pair_id property to PCIDev which is
>> >> used to link the primary device in a failover pair (the PCI dev) to
>> >> a standby (a virtio-net-pci) device.
>> >>
>> >> It only supports ethernet devices. Also currently it only supports
>> >> PCIe devices. QEMU will exit with an error message otherwise.
>> >
>> >Doesn't the PCIe device also need to be hotpluggable?  We can have PCIe
>> >devices attached to the root bus where we don't currently support
>> >hotplug.  Thanks,
>>
>> How do I recognize such a device? hotpluggable in DeviceClass?
>
>I wouldn't expect it to be a device property, it's more of a function
>of whether the bus that it's attached to supports hotplug, so probably
>something related to the bus controller.  Thanks,

IIUC this is checked for in qdev_device_add() by this:

  if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
        error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
        return NULL;
  }

So qemu will fail with 'Bus pcie.0 does not allow hotplug' when we try 
to hotplug the device. I tried a primary with bus=pcie.0 and it aborted.
Aborting qemu is not nice so I'll make it print a error message
QERR_BUS_NO_HOTPLUG instead but let it continue. This will be
a change in the virtio-net patch, not in this one. 

regards,
Jens 


Re: [PATCH 02/11] pci: add option for net failover
Posted by Alex Williamson 6 years, 3 months ago
On Mon, 21 Oct 2019 22:28:57 +0200
Jens Freimann <jfreimann@redhat.com> wrote:

> On Mon, Oct 21, 2019 at 01:01:22PM -0600, Alex Williamson wrote:
> >On Mon, 21 Oct 2019 20:45:46 +0200
> >Jens Freimann <jfreimann@redhat.com> wrote:
> >  
> >> On Mon, Oct 21, 2019 at 08:58:23AM -0600, Alex Williamson wrote:  
> >> >On Fri, 18 Oct 2019 22:20:31 +0200
> >> >Jens Freimann <jfreimann@redhat.com> wrote:
> >> >  
> >> >> This patch adds a net_failover_pair_id property to PCIDev which is
> >> >> used to link the primary device in a failover pair (the PCI dev) to
> >> >> a standby (a virtio-net-pci) device.
> >> >>
> >> >> It only supports ethernet devices. Also currently it only supports
> >> >> PCIe devices. QEMU will exit with an error message otherwise.  
> >> >
> >> >Doesn't the PCIe device also need to be hotpluggable?  We can have PCIe
> >> >devices attached to the root bus where we don't currently support
> >> >hotplug.  Thanks,  
> >>
> >> How do I recognize such a device? hotpluggable in DeviceClass?  
> >
> >I wouldn't expect it to be a device property, it's more of a function
> >of whether the bus that it's attached to supports hotplug, so probably
> >something related to the bus controller.  Thanks,  
> 
> IIUC this is checked for in qdev_device_add() by this:
> 
>   if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
>         error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>         return NULL;
>   }
> 
> So qemu will fail with 'Bus pcie.0 does not allow hotplug' when we try 
> to hotplug the device. I tried a primary with bus=pcie.0 and it aborted.
> Aborting qemu is not nice so I'll make it print a error message
> QERR_BUS_NO_HOTPLUG instead but let it continue. This will be
> a change in the virtio-net patch, not in this one. 

qdev_hotplug is only set to true after qdev_machine_creation_done(), so
if we start a VM with cold-plugged primary and failover, wouldn't the
user only learn the configuration is invalid at the time they try to
use it?  I agree that the above should prevent a device from being
hot-added to the bus, but I don't think that's our starting position,
is it?  Thanks,

Alex