From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v2 4/4] libxl: Add interface for querying hypervisor about PCI topology Date: Wed, 07 Jan 2015 09:15:50 -0500 Message-ID: <54AD3F96.2060502@oracle.com> References: <1420510737-22813-1-git-send-email-boris.ostrovsky@oracle.com> <1420510737-22813-5-git-send-email-boris.ostrovsky@oracle.com> <1420621445.17031.18.camel@Abyss.station> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1420621445.17031.18.camel@Abyss.station> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, jbeulich@suse.com, keir@xen.org, ufimtseva@gmail.com List-Id: xen-devel@lists.xenproject.org On 01/07/2015 04:04 AM, Dario Faggioli wrote: > On Mon, 2015-01-05 at 21:18 -0500, Boris Ostrovsky wrote: >> .. and use this new interface to display it along with CPU topology >> and NUMA information when 'xl info -n' command is issued >> > Also, can we see how an `xl info -n' looks, on an IONUMA system? > >> @@ -195,6 +195,24 @@ int xc_cputopoinfo(xc_interface *xch, >> return 0; >> } >> >> +int xc_pcitopoinfo(xc_interface *xch, >> + xc_pcitopoinfo_t *put_info) >> +{ >> + int ret; >> + DECLARE_SYSCTL; >> + >> + sysctl.cmd = XEN_SYSCTL_pcitopoinfo; >> + >> + memcpy(&sysctl.u.pcitopoinfo, put_info, sizeof(*put_info)); >> + >> + if ( (ret = do_sysctl(xch, &sysctl)) != 0 ) >> + return ret; >> + >> + memcpy(put_info, &sysctl.u.pcitopoinfo, sizeof(*put_info)); >> + >> + return 0; >> +} >> @@ -5121,6 +5121,64 @@ libxl_cputopology *libxl_get_cpu_topology(libxl_ctx *ctx, int *nb_cpu_out) >> return ret; >> } >> >> +libxl_pcitopology *libxl_get_pci_topology(libxl_ctx *ctx, int *num_devs) >> +{ >> + GC_INIT(ctx); >> + xc_pcitopoinfo_t tinfo; >> + DECLARE_HYPERCALL_BUFFER(xen_sysctl_pcitopo_t, pcitopo); >> + libxl_pcitopology *ret = NULL; >> + int i, rc; >> + > I see from where this comes from. However, at least from new functions, > I think we should avoid using things like DECLARE_HYPERCALL_BUFFER etc., > in libxl. They belong in libxc, IMO. > > This is basically what Andrew was doing here (although that was on > xc_{topology,numa}info: > > http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/hwloc-support-experimental > http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=blobdiff;f=tools/libxc/xc_misc.c;h=4f672cead5a4d2b1fc23bcddb6fb49e4d6ec72b5;hp=330345459176961cacc7fda506e843831fe7156a;hb=3585994405b6a73c137309dd4be91f48c71e4903;hpb=9a80d5056766535ac624774b96495f8b97b1d28b > > Basically, what I'm suggesting is to have xc_pcitopoinfo() to do most of > the work, with libxl_get_pci_topology being just a wrapper to it. Maybe then I should update libxl_get_cpu_topology() and libxl_get_numainfo() as well for code uniformity. -boris > > In fact, if you grep/cscope for things like DECLARE_HYPERCALL_BUFFER in > tools/libxl, the *only* results are these: > > libxl.c libxl_get_cpu_topology 5076 DECLARE_HYPERCALL_BUFFER(xc_cpu_to_core_t, coremap); > libxl.c libxl_get_cpu_topology 5077 DECLARE_HYPERCALL_BUFFER(xc_cpu_to_socket_t, socketmap); > libxl.c libxl_get_cpu_topology 5078 DECLARE_HYPERCALL_BUFFER(xc_cpu_to_node_t, nodemap); > libxl.c libxl_get_numainfo 5142 DECLARE_HYPERCALL_BUFFER(xc_node_to_memsize_t, memsize); > libxl.c libxl_get_numainfo 5143 DECLARE_HYPERCALL_BUFFER(xc_node_to_memfree_t, memfree); > libxl.c libxl_get_numainfo 5144 DECLARE_HYPERCALL_BUFFER(uint32_t, node_dists); > > And I think we should work toward removing these too, rather than adding > new ones! :-) > > Regards, > Dario >