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

["Michael S. Tsirkin" <mst@redhat.com>]

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