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

[Akihiko Odaki]

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