On Thu, 24 Oct 2019 11:37:54 +0200 Jens Freimann <jfreim...@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