Re: [PATCH] hw/pci: Warn when ARI/SR-IOV device has non-zero Function number

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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