Re: [PATCH v4 5/9] sysctl: Add sysctl interface for querying PCI topology

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

On 03/13/2015 12:03 PM, Jan Beulich wrote:
>>>> On 10.03.15 at 03:27, <boris.ostrovsky@oracle.com> wrote:
>> +    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;
>> +        }
>> +
>> +        for ( ; ti->first_dev < ti->num_devs; ti->first_dev++ )
>> +        {
>> +            physdev_pci_device_t dev;
>> +            uint8_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;
>> +            else
>> +                node = pdev->node;
>> +            spin_unlock(&pcidevs_lock);
>> +
>> +            if ( copy_to_guest_offset(ti->nodes, ti->first_dev, &node, 1) )
>> +            {
>> +                ret = -EFAULT;
>> +                break;
>> +            }
>>   
>> +            if ( hypercall_preempt_check() )
>> +                break;
>> +        }
>> +
>> +        if ( !ret )
>> +        {
>> +            ti->first_dev++;
> This is correct in the preempt case, but it seems wrong to me in the
> completely-done one. Perhaps it would be better to put the
> increment right before the preempt check?


Callers are really not supposed to use this field as it is useless as 
output.

But for consistency (and to make this look more natural) --- yes, moving it
to the loop above (which will become a 'while') would be better.

-boris