| Message ID | 20230216180356.156832-14-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | pci hotplug tracking | expand |
On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote: > To be used in further patch to identify the device hot-plugged into > pcie-root-port. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru> Wait a second does this work for multifunction devices correctly? > --- > include/hw/pci/pci.h | 1 + > hw/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index d5a40cd058..b6c9c44527 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -341,6 +341,7 @@ void pci_for_each_device_under_bus_reverse(PCIBus *bus, > void pci_for_each_bus_depth_first(PCIBus *bus, pci_bus_ret_fn begin, > pci_bus_fn end, void *parent_state); > PCIDevice *pci_get_function_0(PCIDevice *pci_dev); > +PCIDevice *pci_find_the_only_child(PCIBus *bus, int bus_num, Error **errp); > > /* Use this wrapper when specific scan order is not required. */ > static inline > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 208c16f450..34fd1fb5b8 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1771,6 +1771,39 @@ void pci_for_each_device(PCIBus *bus, int bus_num, > } > } > > +typedef struct TheOnlyChild { > + PCIDevice *dev; > + int count; > +} TheOnlyChild; > + > +static void the_only_child_fn(PCIBus *bus, PCIDevice *dev, void *opaque) > +{ > + TheOnlyChild *s = opaque; > + > + s->dev = dev; > + s->count++; > +} > + > +PCIDevice *pci_find_the_only_child(PCIBus *bus, int bus_num, Error **errp) > +{ > + TheOnlyChild res = {0}; > + > + pci_for_each_device(bus, bus_num, the_only_child_fn, &res); > + > + if (!res.dev) { > + assert(res.count == 0); > + error_setg(errp, "No child devices found"); > + return NULL; > + } > + > + if (res.count > 1) { > + error_setg(errp, "Several child devices found"); > + return NULL; > + } > + > + return res.dev; > +} > + > const pci_class_desc *get_class_desc(int class) > { > const pci_class_desc *desc; > -- > 2.34.1
On 02.03.23 00:09, Michael S. Tsirkin wrote: > On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> To be used in further patch to identify the device hot-plugged into >> pcie-root-port. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru> >> Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru> > Wait a second does this work for multifunction devices correctly? > I thought about that and I'm just lost:) Could several (multifunction?) devices be plugged into one pcie-root-port device? Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot.. On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device.
On Thu, Mar 02, 2023 at 11:28:44AM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 02.03.23 00:09, Michael S. Tsirkin wrote: > > On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > To be used in further patch to identify the device hot-plugged into > > > pcie-root-port. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru> > > > Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru> > > Wait a second does this work for multifunction devices correctly? > > > > I thought about that and I'm just lost:) > > Could several (multifunction?) devices be plugged into one pcie-root-port device? One device per port but one multifunction device is represented as multiple PCIDevice structures. > Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot.. > On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device. > > -- > Best regards, > Vladimir Same thing. ... and let's not get started about sriov and ari ...
On 02.03.23 11:37, Michael S. Tsirkin wrote: > On Thu, Mar 02, 2023 at 11:28:44AM +0300, Vladimir Sementsov-Ogievskiy wrote: >> On 02.03.23 00:09, Michael S. Tsirkin wrote: >>> On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>> To be used in further patch to identify the device hot-plugged into >>>> pcie-root-port. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru> >>>> Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru> >>> Wait a second does this work for multifunction devices correctly? >>> >> >> I thought about that and I'm just lost:) >> >> Could several (multifunction?) devices be plugged into one pcie-root-port device? > > One device per port but one multifunction device is represented as multiple PCIDevice structures. So, it should be OK to send _one_ event for that multifunction device, and the code is shpc_reset() is OK, but reporting "Several child devices found" is bad idea? So, if I change the logic from pci_find_the_only_child() to pci_find_first_child() is it OK? > >> Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot.. >> On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device. >> >> -- >> Best regards, >> Vladimir > > Same thing. > > ... and let's not get started about sriov and ari ... >
On Thu, Mar 02, 2023 at 11:45:00AM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 02.03.23 11:37, Michael S. Tsirkin wrote: > > On Thu, Mar 02, 2023 at 11:28:44AM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > On 02.03.23 00:09, Michael S. Tsirkin wrote: > > > > On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > > > To be used in further patch to identify the device hot-plugged into > > > > > pcie-root-port. > > > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru> > > > > > Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru> > > > > Wait a second does this work for multifunction devices correctly? > > > > > > > > > > I thought about that and I'm just lost:) > > > > > > Could several (multifunction?) devices be plugged into one pcie-root-port device? > > > > One device per port but one multifunction device is represented as multiple PCIDevice structures. > > So, it should be OK to send _one_ event for that multifunction device, and the code is shpc_reset() is OK, but reporting "Several child devices found" is bad idea? I don't know about your new event, we are discussing it separately. yes all functions are removed together normally on real hardware. > So, if I change the logic from pci_find_the_only_child() to pci_find_first_child() is it OK? Yes though I don't like this name either - need to make it clear that multifunction is ok, multiple unrelated devices aren't. > > > > > Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot.. > > > On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device. > > > > > > -- > > > Best regards, > > > Vladimir > > > > Same thing. > > > > ... and let's not get started about sriov and ari ... > > > > -- > Best regards, > Vladimir
On 02.03.23 11:53, Michael S. Tsirkin wrote: > On Thu, Mar 02, 2023 at 11:45:00AM +0300, Vladimir Sementsov-Ogievskiy wrote: >> On 02.03.23 11:37, Michael S. Tsirkin wrote: >>> On Thu, Mar 02, 2023 at 11:28:44AM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>> On 02.03.23 00:09, Michael S. Tsirkin wrote: >>>>> On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>>>> To be used in further patch to identify the device hot-plugged into >>>>>> pcie-root-port. >>>>>> >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru> >>>>>> Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru> >>>>> Wait a second does this work for multifunction devices correctly? >>>>> >>>> >>>> I thought about that and I'm just lost:) >>>> >>>> Could several (multifunction?) devices be plugged into one pcie-root-port device? >>> >>> One device per port but one multifunction device is represented as multiple PCIDevice structures. >> >> So, it should be OK to send _one_ event for that multifunction device, and the code is shpc_reset() is OK, but reporting "Several child devices found" is bad idea? > > I don't know about your new event, we are discussing it separately. > yes all functions are removed together normally on real hardware. > >> So, if I change the logic from pci_find_the_only_child() to pci_find_first_child() is it OK? > > Yes though I don't like this name either - need to make it clear that > multifunction is ok, multiple unrelated devices aren't. Could we check it somehow that all plugged devices represent the one multifunction device? > > >>> >>>> Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot.. >>>> On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device. >>>> >>>> -- >>>> Best regards, >>>> Vladimir >>> >>> Same thing. >>> >>> ... and let's not get started about sriov and ari ... >>> >> >> -- >> Best regards, >> Vladimir >
On Thu, Mar 02, 2023 at 12:35:54PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 02.03.23 11:53, Michael S. Tsirkin wrote: > > On Thu, Mar 02, 2023 at 11:45:00AM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > On 02.03.23 11:37, Michael S. Tsirkin wrote: > > > > On Thu, Mar 02, 2023 at 11:28:44AM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > > > On 02.03.23 00:09, Michael S. Tsirkin wrote: > > > > > > On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > > > > > To be used in further patch to identify the device hot-plugged into > > > > > > > pcie-root-port. > > > > > > > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru> > > > > > > > Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru> > > > > > > Wait a second does this work for multifunction devices correctly? > > > > > > > > > > > > > > > > I thought about that and I'm just lost:) > > > > > > > > > > Could several (multifunction?) devices be plugged into one pcie-root-port device? > > > > > > > > One device per port but one multifunction device is represented as multiple PCIDevice structures. > > > > > > So, it should be OK to send _one_ event for that multifunction device, and the code is shpc_reset() is OK, but reporting "Several child devices found" is bad idea? > > > > I don't know about your new event, we are discussing it separately. > > yes all functions are removed together normally on real hardware. > > > > > So, if I change the logic from pci_find_the_only_child() to pci_find_first_child() is it OK? > > > > Yes though I don't like this name either - need to make it clear that > > multifunction is ok, multiple unrelated devices aren't. > > Could we check it somehow that all plugged devices represent the one multifunction device? You can write a function like this, sure. But really just return a bus+slot pair. > > > > > > > > > > > > > Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot.. > > > > > On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device. > > > > > > > > > > -- > > > > > Best regards, > > > > > Vladimir > > > > > > > > Same thing. > > > > > > > > ... and let's not get started about sriov and ari ... > > > > > > > > > > -- > > > Best regards, > > > Vladimir > > > > -- > Best regards, > Vladimir
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index d5a40cd058..b6c9c44527 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -341,6 +341,7 @@ void pci_for_each_device_under_bus_reverse(PCIBus *bus, void pci_for_each_bus_depth_first(PCIBus *bus, pci_bus_ret_fn begin, pci_bus_fn end, void *parent_state); PCIDevice *pci_get_function_0(PCIDevice *pci_dev); +PCIDevice *pci_find_the_only_child(PCIBus *bus, int bus_num, Error **errp); /* Use this wrapper when specific scan order is not required. */ static inline diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 208c16f450..34fd1fb5b8 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1771,6 +1771,39 @@ void pci_for_each_device(PCIBus *bus, int bus_num, } } +typedef struct TheOnlyChild { + PCIDevice *dev; + int count; +} TheOnlyChild; + +static void the_only_child_fn(PCIBus *bus, PCIDevice *dev, void *opaque) +{ + TheOnlyChild *s = opaque; + + s->dev = dev; + s->count++; +} + +PCIDevice *pci_find_the_only_child(PCIBus *bus, int bus_num, Error **errp) +{ + TheOnlyChild res = {0}; + + pci_for_each_device(bus, bus_num, the_only_child_fn, &res); + + if (!res.dev) { + assert(res.count == 0); + error_setg(errp, "No child devices found"); + return NULL; + } + + if (res.count > 1) { + error_setg(errp, "Several child devices found"); + return NULL; + } + + return res.dev; +} + const pci_class_desc *get_class_desc(int class) { const pci_class_desc *desc;