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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 03/23/2015 04:15 AM, Jan Beulich wrote:
>>>> On 20.03.15 at 21:01, <boris.ostrovsky@oracle.com> wrote:
>> On 03/20/2015 12:26 PM, Jan Beulich wrote:
>>>>>> On 19.03.15 at 22:54, <boris.ostrovsky@oracle.com> 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