From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48072) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VdGwB-0003x7-6j for qemu-devel@nongnu.org; Mon, 04 Nov 2013 04:59:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VdGw2-0005ec-1l for qemu-devel@nongnu.org; Mon, 04 Nov 2013 04:59:11 -0500 Received: from mail-pb0-f49.google.com ([209.85.160.49]:57227) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VdGw1-0005eR-Rz for qemu-devel@nongnu.org; Mon, 04 Nov 2013 04:59:01 -0500 Received: by mail-pb0-f49.google.com with SMTP id xb4so6842898pbc.8 for ; Mon, 04 Nov 2013 01:59:00 -0800 (PST) Message-ID: <52776FDF.6040103@ozlabs.ru> Date: Mon, 04 Nov 2013 20:58:55 +1100 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1383527453-6489-1-git-send-email-aik@ozlabs.ru> <50BDF498-54CD-45C6-AB84-F7F1252FD8F6@suse.de> In-Reply-To: <50BDF498-54CD-45C6-AB84-F7F1252FD8F6@suse.de> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] ppc: introduce CPUPPCState::cpu_dt_id List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: QEMU Developers , Bharat Bhushan , Paolo Bonzini , Paul Mackerras , Badari Pulavarty , Scott Wood , "list@suse.de:PReP" , David Gibson On 11/04/2013 08:41 PM, Alexander Graf wrote: > > On 04.11.2013, at 02:10, Alexey Kardashevskiy wrote: > >> Normally CPUState::cpu_index is used to pick the right CPU for various >> operations. However default consecutive numbering does not always work >> for POWERPC. >> >> For example, on POWER7 (which supports 4 threads per core), >> "-smp 8,threads=4" should create CPUs with indexes 0,1,2,3,4,5,6,7 and >> "-smp 8,threads=1" should create CPUs with indexes 0,4,8,12,16,20,24,28. >> >> These indexes are reflected in /proc/device-tree/cpus/PowerPC,POWER7@XX >> and used to call KVM VCPU's ioctls. In order to achieve this, >> kvmppc_fixup_cpu() was introduced. Roughly speaking, it multiplies >> cpu_index by the number of threads per core. >> >> This approach has disadvantages such as: >> 1. NUMA configuration stays broken after the fixup; >> 2. CPU-related commands from QEMU Monitor do not work properly as >> the accept fixed CPU indexes and the user does not really know >> what they are after fixup as the number of threads per core changes >> between CPU versions and via QEMU command line. >> >> This introduces a new @cpu_dt_id field in the CPUPPCState struct which >> is set from @cpu_index by default but can be fixed later to the value >> which a hypervisor can accept. This also introduces two POWERPC-arch >> specific functions: >> 1. int ppc_get_vcpu_dt_id(CPUState *cs) - returns a device-tree ID >> for a CPU; >> 2. CPUState *ppc_get_vcpu_by_dt_id(int cpu_dt_id) - finds CPUState by >> a device-tree CPU ID. >> >> This uses the new functions to: >> 1. fix emulated XICS hypercall handlers as they receive fixed CPU indexes; >> 2. fix XICS-KVM to enable in-kernel XICS on right CPU; >> 3. compose correct device-tree. >> >> This removes @cpu_index fixup as @cpu_dt_id is used instead so QEMU monitor >> can accept command-line CPU indexes again. > > So this patch feels awkward. We use the dt fixup at random places in completely dt unrelated code paths. Yep. I called it smp_cpu_index in the first place :) > What we really have are 3 semantically separate entities: > > * QEMU internal cpu id > * KVM internal cpu id > * DT exposed cpu id > > As you have noted, it's a good idea to keep the QEMU internal cpu id > linear, thus completely separate from the others. The DT exposed cpu id > should be 100% local to hw/ppc/spapr*.c. I don't think any code outside > of the DT generation and anything that accesses the "Virtual Processor > Number" in sPAPR needs to care about the DT cpu id. All that code is > 100% KVM agnostic. > > The KVM internal cpu id should probably be a new field in the generic > CPUState that gets used by kvm specific code that needs to know the KVM > internal cpu id a vcpu was created with. The flow should be that > kvm_arch_vcpu_id() tells kvm_init_vcpu() the kvm internal id to use > which then stores it in CPUState. That way you can always get the KVM > intenal cpu id from a CPU struct. All the references to this ID should > _only_ happen from KVM only code. If DT id is local to spapr*, then how do I implement kvm_arch_vcpu_id()? Where will it get the fixed value? Do the same calculation in two different places (for device tree and for kvm)? -- Alexey