From mboxrd@z Thu Jan  1 00:00:00 1970
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH v4 5/9] sysctl: Add sysctl interface for
 querying PCI topology
Date: Fri, 13 Mar 2015 12:35:21 -0400
Message-ID: <550311C9.2090306@oracle.com>
References: <1425954475-4913-1-git-send-email-boris.ostrovsky@oracle.com>
	<1425954475-4913-6-git-send-email-boris.ostrovsky@oracle.com>
	<550318610200007800069E58@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <550318610200007800069E58@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: elena.ufimtseva@oracle.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, dario.faggioli@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, keir@xen.org
List-Id: xen-devel@lists.xenproject.org

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