Re: [Xen-devel] [PATCH v5 5/8] sysctl: Add sysctl interface for querying PCI topology

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 03/23/2015 04:15 AM, Jan Beulich wrote:
> > > > On 20.03.15 at 21:01, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > On 03/20/2015 12:26 PM, Jan Beulich wrote:
> > > > > > On 19.03.15 at 22:54, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > > > --- a/xen/common/sysctl.c
> > > > +++ b/xen/common/sysctl.c
> > > > @@ -399,6 +399,67 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> > > > u_sysctl)
> > > >            break;
> > > >    #endif
> > > >    
> > > > +#ifdef HAS_PCI
> > > > +    case XEN_SYSCTL_pcitopoinfo:
> > > > +    {
> > > > +        xen_sysctl_pcitopoinfo_t *ti = &op->u.pcitopoinfo;
> > > > +
> > > > +        if ( guest_handle_is_null(ti->devs) ||
> > > > +             guest_handle_is_null(ti->nodes) ||
> > > > +             (ti->first_dev > ti->num_devs) )
> > > > +        {
> > > > +            ret = -EINVAL;
> > > > +            break;
> > > > +        }
> > > > +
> > > > +        while ( ti->first_dev < ti->num_devs )
> > > > +        {
> > > > +            physdev_pci_device_t dev;
> > > > +            uint32_t node;
> > > > +            struct pci_dev *pdev;
> > > > +
> > > > +            if ( copy_from_guest_offset(&dev, ti->devs, ti->first_dev, 1) )
> > > > +            {
> > > > +                ret = -EFAULT;
> > > > +                break;
> > > > +            }
> > > > +
> > > > +            spin_lock(&pcidevs_lock);
> > > > +            pdev = pci_get_pdev(dev.seg, dev.bus, dev.devfn);
> > > > +            if ( !pdev || (pdev->node == NUMA_NO_NODE) )
> > > > +                node = XEN_INVALID_NODE_ID;
> > > I really think the two cases folded here should be distinguishable
> > > by the caller.
> > How about making  ti->devs array an IN/OUT argument and updating the
> > entry with -1s (which I think is an invalid PCI device)? This will make
> > the original deviceID disappear though so the callers would be expected
> > to stash the array before making the call if they want to know which
> > devices were not reported.
> Sadly all ones in physdev_pci_device_t still could be a valid device.
>
> > Alternatively, since node is 32-bit value while nodeid_t is 8-bit, I can
> > add another token that signifies an invalid device. The main problem
> > with this approach is that logically we use 'nodes' array for passing
> > nodeIDs, not information about devices.
> I realize that. I wonder whether passing in a bad device shouldn't
> simply result in -ENODEV, perhaps with first_dev pointing at the
> bad slot?
Yes, this will work. The caller can then make a note of the bad device, 
increment first_dev and retry the call.
-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel