Re: [Xen-devel] [PATCH 10/10] vpci/sriov: add support for SR-IOV capability

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jul 03, 2018 at 10:27:59AM +0100, Wei Liu wrote:
> On Wed, Jun 20, 2018 at 04:42:34PM +0200, Roger Pau Monne wrote:
> > +        /* Set the BARs addresses and size. */
> > +        for ( i = 0; i < PCI_SRIOV_NUM_BARS; i += rc )
> > +        {
> > +            unsigned int j, idx = pos + PCI_SRIOV_BAR + i * 4;
> > +            const pci_sbdf_t sbdf = {
> > +                .sbdf = PCI_SBDF3(pdev->seg, pdev->bus, pdev->devfn),
> > +            };
> > +            uint32_t bar = pci_conf_read32(pdev->seg, pdev->bus,
> > +                                           PCI_SLOT(pdev->devfn),
> > +                                           PCI_FUNC(pdev->devfn), idx);
> > +            uint64_t addr, size;
> > +
> > +            rc = pci_size_mem_bar(sbdf, idx, &addr, &size,
> > +                                  PCI_BAR_VF |
> > +                                  ((i == PCI_SRIOV_NUM_BARS - 1) ?
> > +                                   PCI_BAR_LAST : 0));
> 
> This only returns 1 or 2. The return type is unsigned int which means rc
> is never going to be <= 0.

Right... I will replace the if with an ASSERT(rc > 0 && rc <= 2);

> > +             */
> > +            sriov->num_vfs = pci_conf_read16(pdev->seg, pdev->bus,
> > +                                             PCI_SLOT(pdev->devfn),
> > +                                             PCI_FUNC(pdev->devfn),
> > +                                             pos + PCI_SRIOV_NUM_VF);
> > +
> > +            /*
> > +             * NB: VFE needs to be enabled before calling pci_add_device 
> > so Xen
> > +             * can access the config space of VFs.
> > +             */
> > +            pci_conf_write16(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> > +                             PCI_FUNC(pdev->devfn), reg,
> > +                             control | PCI_SRIOV_CTRL_VFE);
> > +
> > +            /*
> > +             * The spec states that the software must wait at least 100ms
> > +             * before attempting to access VF registers when enabling 
> > virtual
> > +             * functions on the PF.
> > +             */
> > +            mdelay(100);
> 
> IMHO delaying 100ms like this in an active system is far too long. It
> would be better to put this into a loop and process softirqs in between
> delays.

Ack, do you think 10ms delays would be OK?

> > +        }
> > +
> > +        for ( i = 0; i < sriov->num_vfs; i++ )
> > +        {
> > +            const pci_sbdf_t bdf = {
> > +                .bdf = PCI_BDF2(pdev->bus, pdev->devfn) + offset + stride 
> > * i,
> > +            };
> > +
> > +            if ( new_enabled )
> > +            {
> > +                const struct pci_dev_info info = {
> > +                    .is_virtfn = true,
> > +                    .physfn.bus = pdev->bus,
> > +                    .physfn.devfn = pdev->devfn,
> > +                };
> > +
> > +                rc = pci_add_device(pdev->seg, bdf.bus, bdf.extfunc, &info,
> > +                                    pdev->node);
> > +            }
> > +            else
> > +                rc = pci_remove_device(pdev->seg, bdf.bus, bdf.extfunc);
> > +            if ( rc )
> > +                gprintk(XENLOG_ERR, "failed to %s VF %04x:%02x:%02x.%u: 
> > %d\n",
> > +                        new_enabled ? "add" : "remove", pdev->seg, bdf.bus,
> > +                        bdf.dev, bdf.func, rc);
> > +
> > +            pcidevs_lock();
> > +            sriov->vf[i] = pci_get_pdev(pdev->seg, bdf.bus, bdf.extfunc);
> > +            pcidevs_unlock();
> 
> So the array is updated regardless of rc?

Yes, in the addition case the code is capable of dealing with a NULL
entry in the array (or that was the intention). In the case of a
failed removal an entry in the array won't be NULL, but I don't think
that's an issue.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel