From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v2 3/4] sysctl: Add sysctl interface for querying PCI topology Date: Wed, 07 Jan 2015 10:54:03 -0500 Message-ID: <54AD569B.7070307@oracle.com> References: <1420510737-22813-1-git-send-email-boris.ostrovsky@oracle.com> <1420510737-22813-4-git-send-email-boris.ostrovsky@oracle.com> <54AD08AE02000078000522B7@mail.emea.novell.com> <54AD48D1.5000904@oracle.com> <54AD5C2D02000078000525F0@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: <54AD5C2D02000078000525F0@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: 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, ufimtseva@gmail.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 01/07/2015 10:17 AM, Jan Beulich wrote: >>>> On 07.01.15 at 15:55, wrote: >> On 01/07/2015 04:21 AM, Jan Beulich wrote: >>>>>> On 06.01.15 at 03:18, wrote: >>>> + for ( ; ti->first_dev < ti->num_devs; ti->first_dev++ ) >>>> + { >>>> + xen_sysctl_pcitopo_t pcitopo; >>>> + struct pci_dev *pdev; >>>> + >>>> + if ( copy_from_guest_offset(&pcitopo, ti->pcitopo, >>>> + ti->first_dev, 1) ) >>>> + { >>>> + ret = -EFAULT; >>>> + break; >>>> + } >>>> + >>>> + spin_lock(&pcidevs_lock); >>>> + pdev = pci_get_pdev(pcitopo.pcidev.seg, pcitopo.pcidev.bus, >>>> + pcitopo.pcidev.devfn); >>>> + if ( !pdev || (pdev->node == NUMA_NO_NODE) ) >>>> + pcitopo.node = INVALID_TOPOLOGY_ID; >>>> + else >>>> + pcitopo.node = pdev->node; >>> Are hypervisor-internal node numbers really meaningful to the caller? >> >> This is the same information (pxm -> node mapping ) that we provide in >> XEN_SYSCTL_topologyinfo (renamed in this series to >> XEN_SYSCTL_cputopoinfo). Given that I expect the two topologies to be >> used together I think the answer is yes. > Building your argumentation on potentially mis-designed existing > interfaces is bogus. The question is - what use is a Xen internal > node number to a caller of a particular hypercall (other than it > being purely informational, e.g. for printing human readable > output)? Just as with knowing CPU/memory topology --- this will help with placing a guest if we know what "proximity domain" both the device and the CPUs/memory belong to. Exposing PXM values to the caller would be as good as those internal node IDs. The only (I think) problem is that PXMs are not necessarily zero-based and may not be contiguous and so we need to have some sort of a common mapping for both CPUs and devices. And hypervisor provides such mapping in persistent way. And if we are going to keep this as a sysctl then we need to be consistent with what we do now for CPUs, which is pxm2node[]. Or change CPU topology sysctl as well, which I don't think is a good idea. > > In particular if we were to introduce a new non-sysctl interface, > determining whether the hypervisor internal representation is really > the right one to expose here should be one of the most important > design aspects. Yes. > I personally think that exposing e.g. the firmware > determined (and hence hopefully stable across reboots) PXM would > be more reasonable. Again, the main argument that I see against using PXM values directly is the fact that it's not zero-based/non-contiguous. -boris