Re: [PATCH 4/4] pci: Compare function number and ARI next function number

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

On Sun, Jul 02, 2023 at 04:55:48AM -0400, Michael S. Tsirkin wrote:
> On Sun, Jul 02, 2023 at 05:46:38PM +0900, Akihiko Odaki wrote:
> > On 2023/07/02 13:58, Michael S. Tsirkin wrote:
> > > On Sat, Jul 01, 2023 at 04:01:22PM +0900, Akihiko Odaki wrote:
> > > > The function number must be lower than the next function number
> > > > advertised with ARI.
> > > > 
> > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > 
> > > I don't get this logic at all - where is the limitation coming from?
> > > 
> > > All I see in the spec is:
> > > 	Next Function Number - With non-VFs, this field indicates the Function Number of the next higher
> > > 	numbered Function in the Device, or 00h if there are no higher numbered Functions. Function 0 starts
> > > 	this linked list of Functions.
> > > 	The presence of Shadow Functions does not affect this field.
> > > 	For VFs, this field is undefined since VFs are located using First VF Offset (see § Section 9.3.3.9 ) and VF
> > > 	Stride (see § Section 9.3.3.10 ).
> > > 
> > > and
> > > 
> > > 	 To improve the enumeration performance and create a more deterministic solution, software can
> > > 	enumerate Functions through a linked list of Function Numbers. The next linked list element is
> > > 	communicated through each Function’s ARI Capability Register.
> > > 	i. Function 0 acts as the head of a linked list of Function Numbers. Software detects a
> > > 	non-Zero Next Function Number field within the ARI Capability Register as the next
> > > 	Function within the linked list. Software issues a configuration probe using the Bus Number
> > > 	captured by the Device and the Function Number derived from the ARI Capability Register
> > > 	to locate the next associated Function’s configuration space.
> > > 	ii. Function Numbers may be sparse and non-sequential in their consumption by an ARI
> > > 	Device.
> > 
> > The statement "With non-VFs, this field indicates the Function Number of the
> > next higher numbered Function in the Device, or 00h if there are no higher
> > numbered Functions." implies the Function Number of the device should be
> > lower than the value advertised by the field (for non-VFs; this patch does
> > not check if it's VF or not.)
> 
> 
> Now I get it. Good point! I'd say if we want this check we should add
> it in pcie_ari_init, making that return int.
> But for now it's dead code since your are changing it to 0.
> So maybe a comment in pcie_ari_init is enough:
> 
> /*
>  * Note: nextfn must be the Function Number of the
>  * next higher numbered Function in the Device, or 00h if there are no higher
>  * numbered Functions.
>  * TODO: validate this.
>  */

Or add an assert, and
	TODO: in case this can ever come from command line, we'll have
	to replace the assert below with a runtime check.


> > > 
> > > 
> > > 
> > > 
> > > 
> > > > ---
> > > >   hw/pci/pci.c | 15 +++++++++++++++
> > > >   1 file changed, 15 insertions(+)
> > > > 
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index e2eb4c3b4a..568665ee42 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -2059,6 +2059,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> > > >       Error *local_err = NULL;
> > > >       bool is_default_rom;
> > > >       uint16_t class_id;
> > > > +    uint16_t ari;
> > > > +    uint16_t nextfn;
> > > >       /*
> > > >        * capped by systemd (see: udev-builtin-net_id.c)
> > > > @@ -2121,6 +2123,19 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> > > >           }
> > > >       }
> > > > +    if (pci_is_express(pci_dev)) {
> > > > +        ari = pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI);
> > > > +        if (ari) {
> > > > +            nextfn = (pci_get_long(pci_dev->config + ari + PCI_ARI_CAP) >> 8) & 0xff;
> > > > +            if (nextfn && (pci_dev->devfn & 0xff) >= nextfn) {
> > > > +                error_setg(errp, "PCI: function number %u is not lower than ARI next function number %u",
> > > > +                           pci_dev->devfn & 0xff, nextfn);
> > > > +                pci_qdev_unrealize(DEVICE(pci_dev));
> > > > +                return;
> > > > +            }
> > > > +        }
> > > > +    }
> > > > +
> > > >       if (pci_dev->failover_pair_id) {
> > > >           if (!pci_bus_is_express(pci_get_bus(pci_dev))) {
> > > >               error_setg(errp, "failover primary device must be on "
> > > > -- 
> > > > 2.41.0
> > >