From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54477) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YfTUJ-0003pq-4d for qemu-devel@nongnu.org; Tue, 07 Apr 2015 09:24:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YfTUE-0002wF-1D for qemu-devel@nongnu.org; Tue, 07 Apr 2015 09:24:19 -0400 Received: from cantor2.suse.de ([195.135.220.15]:59906 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YfTUD-0002uL-Mo for qemu-devel@nongnu.org; Tue, 07 Apr 2015 09:24:13 -0400 Message-ID: <5523DA7B.9060008@suse.de> Date: Tue, 07 Apr 2015 15:24:11 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1427932716-11800-1-git-send-email-namit@cs.technion.ac.il> <551D3768.9090404@redhat.com> <5523AE38.6000701@suse.de> <5523B2C6.5080601@redhat.com> <5523B518.5050902@suse.de> <5523B755.2080909@redhat.com> <5523BB00.3040404@suse.de> <5523C62E.6010507@suse.de> <20150407151448.0ec7484d@igors-macbook-pro.local> In-Reply-To: <20150407151448.0ec7484d@igors-macbook-pro.local> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] target-i386: clear bsp bit when designating bsp List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: Eduardo Habkost , mst@redhat.com, Nadav Amit , qemu-devel@nongnu.org, Paolo Bonzini , nadav.amit@gmail.com, rth@twiddle.net Am 07.04.2015 um 15:14 schrieb Igor Mammedov: > On Tue, 07 Apr 2015 13:57:34 +0200 > Andreas F=C3=A4rber wrote: >=20 >> Am 07.04.2015 um 13:09 schrieb Andreas F=C3=A4rber: >>> Am 07.04.2015 um 12:54 schrieb Paolo Bonzini: >>>> On 07/04/2015 12:44, Andreas F=C3=A4rber wrote: >>>>>>> It can change at runtime, though, if you're using the KVM >>>>>>> in-kernel LAPIC. >>>>> Got a pointer? A quick git-grep doesn't show anything in hw/ or >>>>> kvm-all.c or target-i386/ assigning cpu_index, so it'll always >>>>> have the initial value. >>>> >>>> Not cpu_index, s->apicbase's MSR_IA32_APICBASE_BSP bit can change >>>> with KVM in-kernel LAPIC. It cannot change with QEMU's userspace >>>> LAPIC. >>>> >>>> Because it can change, you have to call apic_designate_bsp for all >>>> CPUs and not only on CPU 0. >>> >>> Now I'm even more confused. Surely CPUState is initially >>> zero-initialized. Then we designate one as BSP on reset. That >>> should be the same result as designating all non-BSP CPUs, no? The >>> only way that would change is then cpu_index =3D=3D 0 goes away >>> (hot-unplug, not implemented yet), and in that case it would be >>> about designating a different CPU, not about un-designating one. >>> >>> If this is some issue with sync'ing state back and forth before >>> QEMU and KVM then the real issue has not been explained. > guest can set BSP flag in apicbase of non the first CPU and then o rese= t > on KVM exit it will be propagated into QEMU's state > kvm_arch_post_run() -> cpu_set_apic_base() >=20 > as result with current code after reset we will have the first CPU with > BSP bit and another one since nobody bothered to clear it. >=20 > That's what this patch does.=20 >=20 > [...] >> Unless I'm missing something, since we are in the APIC device's reset >> function, this is effectively a twisted way of writing: >> >> bsp =3D s->apicbase & MSR_IA32_APICBASE_BSP; >> s->apicbase =3D APIC_DEFAULT_ADDRESS | >> (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE; >> >> In which case we already relied on s->cpu and could thus simply change >> this to something like: >> >> bsp =3D CPU(s->cpu)->cpu_index =3D=3D 0; > ^^^ shouldn't be part of APIC code, APIC shouldn't have anything to do > with cpu_index. >=20 > we do above in CPU code currently > /* We hard-wire the BSP to the first CPU. */ > if (s->cpu_index =3D=3D 0) { > apic_designate_bsp(cpu->apic_state); > } I know, that's what this patch is changing, and I am saying that by the same logic the CPU has no business fiddling with the APIC's apicbase field when the APIC's reset is touching that very same field. And I do remember that we arrived at this solution to get to a halfway simple generic reset semantics, moving an external BSP designation into the reset handling. However, this patch is making the twisted logic worse IMO. Andreas >=20 >=20 >> s->apicbase =3D APIC_DEFAULT_ADDRESS | >> (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE; >> >> Then the apicbase manipulation would be nicely encapsulated in the >> APIC rather than the APIC reset retaining it and the CPU reset >> meddling with its state. >> >> Andreas >> >=20 --=20 SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Felix Imend=C3=B6rffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton; HRB 21284 (AG N=C3=BCrnberg)