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

[Parav Pandit <parav@mellanox.com>]

> -----Original Message-----
> From: Jens Freimann <jfreimann@redhat.com>
> Sent: Wednesday, October 23, 2019 3:27 AM
> To: qemu-devel@nongnu.org
> Cc: ehabkost@redhat.com; mst@redhat.com; berrange@redhat.com;
> pkrempa@redhat.com; laine@redhat.com; aadam@redhat.com;
> ailan@redhat.com; Parav Pandit <parav@mellanox.com>;
> dgilbert@redhat.com; alex.williamson@redhat.com
> Subject: [PATCH v5 02/11] pci: add option for net failover
> 
> 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)) {

I am testing and integrating this piece with mlx5 devices.
I see that pci_is_express() return true only for first PCI function.
Didn't yet dig the API.
Commenting out this check and below class check progresses further.

While reviewing, I realized that we shouldn't have this check for below reasons.

1. It is user's responsibility to pass networking device.
But its ok to check the class, if PCI Device is passed.
So class comparison should be inside the pci_check().

2. It is limiting to only consider PCI devices. 
Automated and regression tests should be able validate this feature without PCI Device.
This will enhance the stability of feature in long run.

3. net failover driver doesn't limit it to have it over only PCI device.
So similarly hypervisor should be limiting.

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