From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v5 0/8] Display IO topology when PXM data is available (plus some cleanup) Date: Mon, 23 Mar 2015 11:33:55 -0400 Message-ID: <55103263.2030205@oracle.com> References: <1426802044-19444-1-git-send-email-boris.ostrovsky@oracle.com> <55100A26.7080801@linaro.org> <55101964.7020303@oracle.com> <55103F02020000780006CA42@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: <55103F02020000780006CA42@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, Julien Grall , ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 03/23/2015 11:27 AM, Jan Beulich wrote: >>>> On 23.03.15 at 14:47, wrote: >> How about this (only x86 compile-tested). And perhaps, while at it, fix >> types for cpu_core_id and phys_proc_id. >> >> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c >> index c73dfc9..b319be7 100644 >> --- a/xen/common/sysctl.c >> +++ b/xen/common/sysctl.c >> @@ -354,10 +354,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) >> if ( cpu_present(i) ) >> { >> cputopo.core = cpu_to_core(i); >> - if ( cputopo.core == BAD_APICID ) >> + if ( cputopo.core == INVALID_COREID ) >> cputopo.core = XEN_INVALID_CORE_ID; >> cputopo.socket = cpu_to_socket(i); >> - if ( cputopo.socket == BAD_APICID ) >> + if ( cputopo.socket == INVALID_SOCKETID ) >> cputopo.socket = XEN_INVALID_SOCKET_ID; > > Why not use XEN_INVALID_CORE_ID / XEN_INVALID_SOCKET_ID > for those return values right away? > >> --- a/xen/include/asm-x86/processor.h >> +++ b/xen/include/asm-x86/processor.h >> @@ -214,8 +214,19 @@ extern void detect_extended_topology(struct cpuinfo_x86 *c); >> >> extern void detect_ht(struct cpuinfo_x86 *c); >> >> -#define cpu_to_core(_cpu) (cpu_data[_cpu].cpu_core_id) >> -#define cpu_to_socket(_cpu) (cpu_data[_cpu].phys_proc_id) >> +inline int cpu_to_core(unsigned cpu) >> +{ >> + if ( cpu_data[cpu].cpu_core_id == BAD_APICID ) >> + return INVALID_COREID; >> + return cpu_data[cpu].cpu_core_id; >> +} >> + >> +inline int cpu_to_socket(unsigned cpu) >> +{ >> + if ( cpu_data[cpu].phys_proc_id == BAD_APICID ) >> + return INVALID_SOCKETID; >> + return cpu_data[cpu].phys_proc_id; >> +} > > Apart from them needing to be static, I don't think we want the > extra conditionals in x86 code. Hence I think you rather should > introduce wrappers for the specific us in sysctl.c. If we use XEN_INVALID_CORE_ID/XEN_INVALID_SOCKET_ID in cpu_data.cpu_core_id/phys_proc_id then no conditionals would be needed. (I didn't do it in the above patch because these two fields are currently signed. I'll make them unsigned). -boris