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

[Jens Freimann]

On Thu, Oct 24, 2019 at 05:03:46AM +0000, Parav Pandit wrote:
> > @@ -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.

First of all, thanks for testing this!
Could you share your commandline please? I can't reproduce it.
> 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().

I'm not sure I understand this point, could you please elaborate?
You're suggesting to move the check for the class into the check for
pci_is_express?

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

I agree that we don't have to limit it to PCI(e) forever. But for this
first shot I think we should and then extend it continually. There are
more things we can support in the future like other hotplug types etc.

regards,
Jens