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

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 24 Oct 2019 11:37:54 +0200
Jens Freimann <jfreimann@redhat.com> wrote:

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

Seems like the suggestion is that net_failover_pair_id should be an
option on the base class of PCIDevice (DeviceState?) and only if it's a
PCI device would we check the class code.  But there are dependencies
at the hotplug controller, which I think is why this is currently
specific to PCI.

However, it's an interesting point about pci_is_express().  This test
is really just meant to check whether the hotplug controller supports
this feature, which is only implemented in pciehp via this series.
There's a bit of a mismatch though that pcie_is_express() checks
whether the device is express, not whether the bus it sits on is
express.  I think we really want the latter, so maybe this should be:

pci_bus_is_express(pci_get_bus(dev)

For example this feature should work if I plug an e1000 (not e1000e)
into an express slot, but not if I plug an e1000e into a conventional
slot.
 
> >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.

Yep, long term it seems very generic, but there's a dependency in the
hotplug controller and it is beneficial that PCI has a class code
feature that allows us to error if this is specified on a non-net
device.  Thanks,

Alex