From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v5 5/8] sysctl: Add sysctl interface for querying PCI topology Date: Mon, 23 Mar 2015 09:58:07 -0400 Message-ID: <55101BEF.7010605@oracle.com> References: <1426802044-19444-1-git-send-email-boris.ostrovsky@oracle.com> <1426802044-19444-6-git-send-email-boris.ostrovsky@oracle.com> <550C583F020000780006C2AA@mail.emea.novell.com> <550C7CB4.6050708@oracle.com> <550FD9A7020000780006C73D@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <550FD9A7020000780006C73D@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich 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/23/2015 04:15 AM, Jan Beulich wrote: >>>> On 20.03.15 at 21:01, wrote: >> On 03/20/2015 12:26 PM, Jan Beulich wrote: >>>>>> On 19.03.15 at 22:54, 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