* [PATCH] hw/pci: Warn when ARI/SR-IOV device has non-zero Function number
@ 2023-07-12 11:27 Akihiko Odaki
2023-07-12 11:46 ` Michael S. Tsirkin
0 siblings, 1 reply; 5+ messages in thread
From: Akihiko Odaki @ 2023-07-12 11:27 UTC (permalink / raw)
To: Michael S . Tsirkin, Ani Sinha
Cc: qemu-devel, qemu-block, Marcel Apfelbaum, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, Akihiko Odaki
Current SR/IOV implementations assume that hardcoded Function numbers
are always available and will not conflict. It is somewhat non-trivial
to make the Function numbers to use controllable to avoid Function
number conflicts so ensure there is only one PF to make the assumption
hold true.
Also warn when non-SR/IOV multifunction was attempted with ARI enabled;
ARI has the next Function number field register, and currently it's
hardcoded to 0, which prevents non-SR/IOV multifunction. It is
certainly possible to add a logic to determine the correct next Function
number according to the configuration, but it's not worth since all
ARI-capable devices are also SR/IOV devices, which do not support
multiple PFs as stated above.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
hw/pci/pci.c | 59 +++++++++++++++++++++++++++++++++++++---------------
1 file changed, 42 insertions(+), 17 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 784c02a182..50359a0f3a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2124,23 +2124,48 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
}
}
- /*
- * A PCIe Downstream Port that do not have ARI Forwarding enabled must
- * associate only Device 0 with the device attached to the bus
- * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
- * sec 7.3.1).
- * With ARI, PCI_SLOT() can return non-zero value as the traditional
- * 5-bit Device Number and 3-bit Function Number fields in its associated
- * Routing IDs, Requester IDs and Completer IDs are interpreted as a
- * single 8-bit Function Number. Hence, ignore ARI capable devices.
- */
- if (pci_is_express(pci_dev) &&
- !pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
- pcie_has_upstream_port(pci_dev) &&
- PCI_SLOT(pci_dev->devfn)) {
- warn_report("PCI: slot %d is not valid for %s,"
- " parent device only allows plugging into slot 0.",
- PCI_SLOT(pci_dev->devfn), pci_dev->name);
+ if (pci_is_express(pci_dev)) {
+ /*
+ * A PCIe Downstream Port that do not have ARI Forwarding enabled must
+ * associate only Device 0 with the device attached to the bus
+ * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
+ * sec 7.3.1).
+ * With ARI, PCI_SLOT() can return non-zero value as the traditional
+ * 5-bit Device Number and 3-bit Function Number fields in its
+ * associated Routing IDs, Requester IDs and Completer IDs are
+ * interpreted as a single 8-bit Function Number. Hence, ignore ARI
+ * capable devices.
+ */
+ if (!pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
+ pcie_has_upstream_port(pci_dev) &&
+ PCI_SLOT(pci_dev->devfn)) {
+ warn_report("PCI: slot %d is not valid for %s,"
+ " parent device only allows plugging into slot 0.",
+ PCI_SLOT(pci_dev->devfn), pci_dev->name);
+ }
+
+ /*
+ * Current SR/IOV implementations assume that hardcoded Function numbers
+ * are always available. Ensure there is only one PF to make the
+ * assumption hold true.
+ */
+ if (pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV) &&
+ PCI_FUNC(pci_dev->devfn)) {
+ warn_report("PCI: function %d is not valid for %s,"
+ " currently PF can only be assigned to function 0.",
+ PCI_FUNC(pci_dev->devfn), pci_dev->name);
+ }
+
+ /*
+ * ARI has the next Function number field register, and currently it's
+ * hardcoded to 0, which prevents non-SR/IOV multifunction.
+ */
+ if (pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
+ !pci_is_vf(pci_dev) && (pci_dev->devfn & 0xff)) {
+ warn_report("PCI: function %d is not valid for %s,"
+ " non-SR/IOV multifunction is not supported with ARI enabled.",
+ pci_dev->devfn & 0xff, pci_dev->name);
+ }
}
if (pci_dev->failover_pair_id) {
--
2.41.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/pci: Warn when ARI/SR-IOV device has non-zero Function number
2023-07-12 11:27 [PATCH] hw/pci: Warn when ARI/SR-IOV device has non-zero Function number Akihiko Odaki
@ 2023-07-12 11:46 ` Michael S. Tsirkin
2023-07-12 11:50 ` Akihiko Odaki
0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2023-07-12 11:46 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Ani Sinha, qemu-devel, qemu-block, Marcel Apfelbaum,
Sriram Yagnaraman, Jason Wang, Keith Busch, Klaus Jensen
On Wed, Jul 12, 2023 at 08:27:32PM +0900, Akihiko Odaki wrote:
> Current SR/IOV implementations assume that hardcoded Function numbers
> are always available and will not conflict. It is somewhat non-trivial
> to make the Function numbers to use controllable to avoid Function
> number conflicts so ensure there is only one PF to make the assumption
> hold true.
> Also warn when non-SR/IOV multifunction was attempted with ARI enabled;
> ARI has the next Function number field register, and currently it's
> hardcoded to 0, which prevents non-SR/IOV multifunction. It is
> certainly possible to add a logic to determine the correct next Function
> number according to the configuration, but it's not worth since all
> ARI-capable devices are also SR/IOV devices, which do not support
> multiple PFs as stated above.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
I am not really interested in adding this stuff.
The real thing to focus on is fixing ARI emulation, not
warning users about ways in which it's broken.
> ---
> hw/pci/pci.c | 59 +++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 42 insertions(+), 17 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 784c02a182..50359a0f3a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2124,23 +2124,48 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> }
> }
>
> - /*
> - * A PCIe Downstream Port that do not have ARI Forwarding enabled must
> - * associate only Device 0 with the device attached to the bus
> - * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
> - * sec 7.3.1).
> - * With ARI, PCI_SLOT() can return non-zero value as the traditional
> - * 5-bit Device Number and 3-bit Function Number fields in its associated
> - * Routing IDs, Requester IDs and Completer IDs are interpreted as a
> - * single 8-bit Function Number. Hence, ignore ARI capable devices.
> - */
> - if (pci_is_express(pci_dev) &&
> - !pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
> - pcie_has_upstream_port(pci_dev) &&
> - PCI_SLOT(pci_dev->devfn)) {
> - warn_report("PCI: slot %d is not valid for %s,"
> - " parent device only allows plugging into slot 0.",
> - PCI_SLOT(pci_dev->devfn), pci_dev->name);
> + if (pci_is_express(pci_dev)) {
> + /*
> + * A PCIe Downstream Port that do not have ARI Forwarding enabled must
> + * associate only Device 0 with the device attached to the bus
> + * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
> + * sec 7.3.1).
> + * With ARI, PCI_SLOT() can return non-zero value as the traditional
> + * 5-bit Device Number and 3-bit Function Number fields in its
> + * associated Routing IDs, Requester IDs and Completer IDs are
> + * interpreted as a single 8-bit Function Number. Hence, ignore ARI
> + * capable devices.
> + */
> + if (!pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
> + pcie_has_upstream_port(pci_dev) &&
> + PCI_SLOT(pci_dev->devfn)) {
> + warn_report("PCI: slot %d is not valid for %s,"
> + " parent device only allows plugging into slot 0.",
> + PCI_SLOT(pci_dev->devfn), pci_dev->name);
> + }
> +
> + /*
> + * Current SR/IOV implementations assume that hardcoded Function numbers
> + * are always available. Ensure there is only one PF to make the
> + * assumption hold true.
> + */
> + if (pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV) &&
> + PCI_FUNC(pci_dev->devfn)) {
> + warn_report("PCI: function %d is not valid for %s,"
> + " currently PF can only be assigned to function 0.",
> + PCI_FUNC(pci_dev->devfn), pci_dev->name);
> + }
> +
> + /*
> + * ARI has the next Function number field register, and currently it's
> + * hardcoded to 0, which prevents non-SR/IOV multifunction.
> + */
> + if (pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
> + !pci_is_vf(pci_dev) && (pci_dev->devfn & 0xff)) {
> + warn_report("PCI: function %d is not valid for %s,"
> + " non-SR/IOV multifunction is not supported with ARI enabled.",
> + pci_dev->devfn & 0xff, pci_dev->name);
> + }
> }
>
> if (pci_dev->failover_pair_id) {
> --
> 2.41.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/pci: Warn when ARI/SR-IOV device has non-zero Function number
2023-07-12 11:46 ` Michael S. Tsirkin
@ 2023-07-12 11:50 ` Akihiko Odaki
2023-07-12 12:06 ` Michael S. Tsirkin
0 siblings, 1 reply; 5+ messages in thread
From: Akihiko Odaki @ 2023-07-12 11:50 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Ani Sinha, qemu-devel, qemu-block, Marcel Apfelbaum,
Sriram Yagnaraman, Jason Wang, Keith Busch, Klaus Jensen
On 2023/07/12 20:46, Michael S. Tsirkin wrote:
> On Wed, Jul 12, 2023 at 08:27:32PM +0900, Akihiko Odaki wrote:
>> Current SR/IOV implementations assume that hardcoded Function numbers
>> are always available and will not conflict. It is somewhat non-trivial
>> to make the Function numbers to use controllable to avoid Function
>> number conflicts so ensure there is only one PF to make the assumption
>> hold true.
>> Also warn when non-SR/IOV multifunction was attempted with ARI enabled;
>> ARI has the next Function number field register, and currently it's
>> hardcoded to 0, which prevents non-SR/IOV multifunction. It is
>> certainly possible to add a logic to determine the correct next Function
>> number according to the configuration, but it's not worth since all
>> ARI-capable devices are also SR/IOV devices, which do not support
>> multiple PFs as stated above.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> I am not really interested in adding this stuff.
> The real thing to focus on is fixing ARI emulation, not
> warning users about ways in which it's broken.
What do you think about multiple SR/IOV PFs? Do you think it's
worth/easy enough to fix SR/IOV code to support it? Otherwise it's not
worth fixing ARI since currently only SR/IOV devices implement it.
>
>> ---
>> hw/pci/pci.c | 59 +++++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 42 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 784c02a182..50359a0f3a 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2124,23 +2124,48 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>> }
>> }
>>
>> - /*
>> - * A PCIe Downstream Port that do not have ARI Forwarding enabled must
>> - * associate only Device 0 with the device attached to the bus
>> - * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
>> - * sec 7.3.1).
>> - * With ARI, PCI_SLOT() can return non-zero value as the traditional
>> - * 5-bit Device Number and 3-bit Function Number fields in its associated
>> - * Routing IDs, Requester IDs and Completer IDs are interpreted as a
>> - * single 8-bit Function Number. Hence, ignore ARI capable devices.
>> - */
>> - if (pci_is_express(pci_dev) &&
>> - !pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
>> - pcie_has_upstream_port(pci_dev) &&
>> - PCI_SLOT(pci_dev->devfn)) {
>> - warn_report("PCI: slot %d is not valid for %s,"
>> - " parent device only allows plugging into slot 0.",
>> - PCI_SLOT(pci_dev->devfn), pci_dev->name);
>> + if (pci_is_express(pci_dev)) {
>> + /*
>> + * A PCIe Downstream Port that do not have ARI Forwarding enabled must
>> + * associate only Device 0 with the device attached to the bus
>> + * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
>> + * sec 7.3.1).
>> + * With ARI, PCI_SLOT() can return non-zero value as the traditional
>> + * 5-bit Device Number and 3-bit Function Number fields in its
>> + * associated Routing IDs, Requester IDs and Completer IDs are
>> + * interpreted as a single 8-bit Function Number. Hence, ignore ARI
>> + * capable devices.
>> + */
>> + if (!pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
>> + pcie_has_upstream_port(pci_dev) &&
>> + PCI_SLOT(pci_dev->devfn)) {
>> + warn_report("PCI: slot %d is not valid for %s,"
>> + " parent device only allows plugging into slot 0.",
>> + PCI_SLOT(pci_dev->devfn), pci_dev->name);
>> + }
>> +
>> + /*
>> + * Current SR/IOV implementations assume that hardcoded Function numbers
>> + * are always available. Ensure there is only one PF to make the
>> + * assumption hold true.
>> + */
>> + if (pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV) &&
>> + PCI_FUNC(pci_dev->devfn)) {
>> + warn_report("PCI: function %d is not valid for %s,"
>> + " currently PF can only be assigned to function 0.",
>> + PCI_FUNC(pci_dev->devfn), pci_dev->name);
>> + }
>> +
>> + /*
>> + * ARI has the next Function number field register, and currently it's
>> + * hardcoded to 0, which prevents non-SR/IOV multifunction.
>> + */
>> + if (pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
>> + !pci_is_vf(pci_dev) && (pci_dev->devfn & 0xff)) {
>> + warn_report("PCI: function %d is not valid for %s,"
>> + " non-SR/IOV multifunction is not supported with ARI enabled.",
>> + pci_dev->devfn & 0xff, pci_dev->name);
>> + }
>> }
>>
>> if (pci_dev->failover_pair_id) {
>> --
>> 2.41.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/pci: Warn when ARI/SR-IOV device has non-zero Function number
2023-07-12 11:50 ` Akihiko Odaki
@ 2023-07-12 12:06 ` Michael S. Tsirkin
2023-07-13 13:32 ` Akihiko Odaki
0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2023-07-12 12:06 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Ani Sinha, qemu-devel, qemu-block, Marcel Apfelbaum,
Sriram Yagnaraman, Jason Wang, Keith Busch, Klaus Jensen
On Wed, Jul 12, 2023 at 08:50:32PM +0900, Akihiko Odaki wrote:
> On 2023/07/12 20:46, Michael S. Tsirkin wrote:
> > On Wed, Jul 12, 2023 at 08:27:32PM +0900, Akihiko Odaki wrote:
> > > Current SR/IOV implementations assume that hardcoded Function numbers
> > > are always available and will not conflict. It is somewhat non-trivial
> > > to make the Function numbers to use controllable to avoid Function
> > > number conflicts so ensure there is only one PF to make the assumption
> > > hold true.
> > > Also warn when non-SR/IOV multifunction was attempted with ARI enabled;
> > > ARI has the next Function number field register, and currently it's
> > > hardcoded to 0, which prevents non-SR/IOV multifunction. It is
> > > certainly possible to add a logic to determine the correct next Function
> > > number according to the configuration, but it's not worth since all
> > > ARI-capable devices are also SR/IOV devices, which do not support
> > > multiple PFs as stated above.
> > >
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >
> > I am not really interested in adding this stuff.
> > The real thing to focus on is fixing ARI emulation, not
> > warning users about ways in which it's broken.
>
> What do you think about multiple SR/IOV PFs? Do you think it's worth/easy
> enough to fix SR/IOV code to support it? Otherwise it's not worth fixing ARI
> since currently only SR/IOV devices implement it.
There's nothing especially hard about it. You can in particular just
assume the user knows what he's doing and not worry too much about
checking. Creating invalid configs might also come handy e.g. for debug.
The important thing, and that's missing ATM, is giving management
ability to find out TotalVFs, VF offset and VF stride, so it can
avoid creating these conflicts.
For igd maybe we should make VF offset and VF stride just 1 unconditionally -
I have no idea why it was made 2 ATM - could you check what does
real hardware do?
Yes, warning at least is handy for
management debugging. It shouldn't be hard I think, but the
logic does tend to be O(n^2). Maybe add a flag to check,
and management developers can use that for debugging.
> >
> > > ---
> > > hw/pci/pci.c | 59 +++++++++++++++++++++++++++++++++++++---------------
> > > 1 file changed, 42 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 784c02a182..50359a0f3a 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -2124,23 +2124,48 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> > > }
> > > }
> > > - /*
> > > - * A PCIe Downstream Port that do not have ARI Forwarding enabled must
> > > - * associate only Device 0 with the device attached to the bus
> > > - * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
> > > - * sec 7.3.1).
> > > - * With ARI, PCI_SLOT() can return non-zero value as the traditional
> > > - * 5-bit Device Number and 3-bit Function Number fields in its associated
> > > - * Routing IDs, Requester IDs and Completer IDs are interpreted as a
> > > - * single 8-bit Function Number. Hence, ignore ARI capable devices.
> > > - */
> > > - if (pci_is_express(pci_dev) &&
> > > - !pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
> > > - pcie_has_upstream_port(pci_dev) &&
> > > - PCI_SLOT(pci_dev->devfn)) {
> > > - warn_report("PCI: slot %d is not valid for %s,"
> > > - " parent device only allows plugging into slot 0.",
> > > - PCI_SLOT(pci_dev->devfn), pci_dev->name);
> > > + if (pci_is_express(pci_dev)) {
> > > + /*
> > > + * A PCIe Downstream Port that do not have ARI Forwarding enabled must
> > > + * associate only Device 0 with the device attached to the bus
> > > + * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
> > > + * sec 7.3.1).
> > > + * With ARI, PCI_SLOT() can return non-zero value as the traditional
> > > + * 5-bit Device Number and 3-bit Function Number fields in its
> > > + * associated Routing IDs, Requester IDs and Completer IDs are
> > > + * interpreted as a single 8-bit Function Number. Hence, ignore ARI
> > > + * capable devices.
> > > + */
> > > + if (!pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
> > > + pcie_has_upstream_port(pci_dev) &&
> > > + PCI_SLOT(pci_dev->devfn)) {
> > > + warn_report("PCI: slot %d is not valid for %s,"
> > > + " parent device only allows plugging into slot 0.",
> > > + PCI_SLOT(pci_dev->devfn), pci_dev->name);
> > > + }
> > > +
> > > + /*
> > > + * Current SR/IOV implementations assume that hardcoded Function numbers
> > > + * are always available. Ensure there is only one PF to make the
> > > + * assumption hold true.
> > > + */
> > > + if (pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV) &&
> > > + PCI_FUNC(pci_dev->devfn)) {
> > > + warn_report("PCI: function %d is not valid for %s,"
> > > + " currently PF can only be assigned to function 0.",
> > > + PCI_FUNC(pci_dev->devfn), pci_dev->name);
> > > + }
> > > +
> > > + /*
> > > + * ARI has the next Function number field register, and currently it's
> > > + * hardcoded to 0, which prevents non-SR/IOV multifunction.
> > > + */
> > > + if (pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
> > > + !pci_is_vf(pci_dev) && (pci_dev->devfn & 0xff)) {
> > > + warn_report("PCI: function %d is not valid for %s,"
> > > + " non-SR/IOV multifunction is not supported with ARI enabled.",
> > > + pci_dev->devfn & 0xff, pci_dev->name);
> > > + }
> > > }
> > > if (pci_dev->failover_pair_id) {
> > > --
> > > 2.41.0
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/pci: Warn when ARI/SR-IOV device has non-zero Function number
2023-07-12 12:06 ` Michael S. Tsirkin
@ 2023-07-13 13:32 ` Akihiko Odaki
0 siblings, 0 replies; 5+ messages in thread
From: Akihiko Odaki @ 2023-07-13 13:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Ani Sinha, qemu-devel, qemu-block, Marcel Apfelbaum,
Sriram Yagnaraman, Jason Wang, Keith Busch, Klaus Jensen
On 2023/07/12 21:06, Michael S. Tsirkin wrote:
> On Wed, Jul 12, 2023 at 08:50:32PM +0900, Akihiko Odaki wrote:
>> On 2023/07/12 20:46, Michael S. Tsirkin wrote:
>>> On Wed, Jul 12, 2023 at 08:27:32PM +0900, Akihiko Odaki wrote:
>>>> Current SR/IOV implementations assume that hardcoded Function numbers
>>>> are always available and will not conflict. It is somewhat non-trivial
>>>> to make the Function numbers to use controllable to avoid Function
>>>> number conflicts so ensure there is only one PF to make the assumption
>>>> hold true.
>>>> Also warn when non-SR/IOV multifunction was attempted with ARI enabled;
>>>> ARI has the next Function number field register, and currently it's
>>>> hardcoded to 0, which prevents non-SR/IOV multifunction. It is
>>>> certainly possible to add a logic to determine the correct next Function
>>>> number according to the configuration, but it's not worth since all
>>>> ARI-capable devices are also SR/IOV devices, which do not support
>>>> multiple PFs as stated above.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>
>>> I am not really interested in adding this stuff.
>>> The real thing to focus on is fixing ARI emulation, not
>>> warning users about ways in which it's broken.
>>
>> What do you think about multiple SR/IOV PFs? Do you think it's worth/easy
>> enough to fix SR/IOV code to support it? Otherwise it's not worth fixing ARI
>> since currently only SR/IOV devices implement it.
>
> There's nothing especially hard about it. You can in particular just
> assume the user knows what he's doing and not worry too much about
> checking. Creating invalid configs might also come handy e.g. for debug.
> The important thing, and that's missing ATM, is giving management
> ability to find out TotalVFs, VF offset and VF stride, so it can
> avoid creating these conflicts.
>
> For igd maybe we should make VF offset and VF stride just 1 unconditionally -
> I have no idea why it was made 2 ATM - could you check what does
> real hardware do?
The current igb implementation match with real hardware. It is defined
in the datasheet*, section 9.6.4.6. I don't know why it's 2 either.
*
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/82576eg-gbe-datasheet.pdf
>
> Yes, warning at least is handy for
> management debugging. It shouldn't be hard I think, but the
> logic does tend to be O(n^2). Maybe add a flag to check,
> and management developers can use that for debugging.
>
>>>
>>>> ---
>>>> hw/pci/pci.c | 59 +++++++++++++++++++++++++++++++++++++---------------
>>>> 1 file changed, 42 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index 784c02a182..50359a0f3a 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -2124,23 +2124,48 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>> }
>>>> }
>>>> - /*
>>>> - * A PCIe Downstream Port that do not have ARI Forwarding enabled must
>>>> - * associate only Device 0 with the device attached to the bus
>>>> - * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
>>>> - * sec 7.3.1).
>>>> - * With ARI, PCI_SLOT() can return non-zero value as the traditional
>>>> - * 5-bit Device Number and 3-bit Function Number fields in its associated
>>>> - * Routing IDs, Requester IDs and Completer IDs are interpreted as a
>>>> - * single 8-bit Function Number. Hence, ignore ARI capable devices.
>>>> - */
>>>> - if (pci_is_express(pci_dev) &&
>>>> - !pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
>>>> - pcie_has_upstream_port(pci_dev) &&
>>>> - PCI_SLOT(pci_dev->devfn)) {
>>>> - warn_report("PCI: slot %d is not valid for %s,"
>>>> - " parent device only allows plugging into slot 0.",
>>>> - PCI_SLOT(pci_dev->devfn), pci_dev->name);
>>>> + if (pci_is_express(pci_dev)) {
>>>> + /*
>>>> + * A PCIe Downstream Port that do not have ARI Forwarding enabled must
>>>> + * associate only Device 0 with the device attached to the bus
>>>> + * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
>>>> + * sec 7.3.1).
>>>> + * With ARI, PCI_SLOT() can return non-zero value as the traditional
>>>> + * 5-bit Device Number and 3-bit Function Number fields in its
>>>> + * associated Routing IDs, Requester IDs and Completer IDs are
>>>> + * interpreted as a single 8-bit Function Number. Hence, ignore ARI
>>>> + * capable devices.
>>>> + */
>>>> + if (!pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
>>>> + pcie_has_upstream_port(pci_dev) &&
>>>> + PCI_SLOT(pci_dev->devfn)) {
>>>> + warn_report("PCI: slot %d is not valid for %s,"
>>>> + " parent device only allows plugging into slot 0.",
>>>> + PCI_SLOT(pci_dev->devfn), pci_dev->name);
>>>> + }
>>>> +
>>>> + /*
>>>> + * Current SR/IOV implementations assume that hardcoded Function numbers
>>>> + * are always available. Ensure there is only one PF to make the
>>>> + * assumption hold true.
>>>> + */
>>>> + if (pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV) &&
>>>> + PCI_FUNC(pci_dev->devfn)) {
>>>> + warn_report("PCI: function %d is not valid for %s,"
>>>> + " currently PF can only be assigned to function 0.",
>>>> + PCI_FUNC(pci_dev->devfn), pci_dev->name);
>>>> + }
>>>> +
>>>> + /*
>>>> + * ARI has the next Function number field register, and currently it's
>>>> + * hardcoded to 0, which prevents non-SR/IOV multifunction.
>>>> + */
>>>> + if (pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
>>>> + !pci_is_vf(pci_dev) && (pci_dev->devfn & 0xff)) {
>>>> + warn_report("PCI: function %d is not valid for %s,"
>>>> + " non-SR/IOV multifunction is not supported with ARI enabled.",
>>>> + pci_dev->devfn & 0xff, pci_dev->name);
>>>> + }
>>>> }
>>>> if (pci_dev->failover_pair_id) {
>>>> --
>>>> 2.41.0
>>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-07-13 13:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-12 11:27 [PATCH] hw/pci: Warn when ARI/SR-IOV device has non-zero Function number Akihiko Odaki
2023-07-12 11:46 ` Michael S. Tsirkin
2023-07-12 11:50 ` Akihiko Odaki
2023-07-12 12:06 ` Michael S. Tsirkin
2023-07-13 13:32 ` Akihiko Odaki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.