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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Jul 03, 2018 at 12:21:08PM +0200, Roger Pau Monné wrote:
> 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?

I think that's fine. But it would be better to have some input from Arm
folks since this could impact their real-time constraint.

This isn't a hot path so I think having shorter interval (more calls to
process_pending_softirqs) isn't going to be a problem.

Wei.

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