From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=47821 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pl6fH-0000mx-SW for qemu-devel@nongnu.org; Thu, 03 Feb 2011 16:24:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pl6fG-0000r3-G8 for qemu-devel@nongnu.org; Thu, 03 Feb 2011 16:24:31 -0500 Received: from fmmailgate01.web.de ([217.72.192.221]:33399) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pl6fF-0000qt-OK for qemu-devel@nongnu.org; Thu, 03 Feb 2011 16:24:30 -0500 Message-ID: <4D4B1D0A.5090901@web.de> Date: Thu, 03 Feb 2011 22:24:26 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <4D4B02E8.7060104@siemens.com> In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigC204538EB73236FB6258248C" Sender: jan.kiszka@web.de Subject: [Qemu-devel] Re: [RFC][PATCH] apic: Fix relocation List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigC204538EB73236FB6258248C Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 2011-02-03 20:43, Blue Swirl wrote: > On Thu, Feb 3, 2011 at 7:32 PM, Jan Kiszka wro= te: >> When the guest remaps an APIC by modifying MSR_IA32_APICBASE, we need = to >> update its mmio mapping. This is a bit tricky as multiple APICs might = be >> mapped to the same address. So walk through the full list to avoid >> unmapping a region that is still in use. >> >> Signed-off-by: Jan Kiszka >> --- >> >> RFC as I did not yet have a chance to test actual relocation. Standard= >> OSes don't do this, otherwise we would have noticed this earlier. >> >> hw/apic.c | 38 +++++++++++++++++++++++++++++++++++++- >> hw/pc.c | 10 ---------- >> 2 files changed, 37 insertions(+), 11 deletions(-) >> >> diff --git a/hw/apic.c b/hw/apic.c >> index 05a115f..b64af59 100644 >> --- a/hw/apic.c >> +++ b/hw/apic.c >> @@ -294,6 +294,40 @@ void apic_deliver_irq(uint8_t dest, uint8_t dest_= mode, >> trigger_mode); >> } >> >> +static void apic_update_mapping(APICState *s) >> +{ >> + target_phys_addr_t new_addr; >> + bool overlap =3D false; >> + APICState *apic_iter; >> + int i; >> + >> + for (i =3D 0; i < MAX_APICS; i++) { >> + apic_iter =3D local_apics[i]; >> + if (!apic_iter || apic_iter =3D=3D s) { >> + continue; >> + } >> + if ((apic_iter->apicbase & MSR_IA32_APICBASE_BASE) =3D=3D >> + s->busdev.mmio[0].addr) { >> + overlap =3D true; >> + break; >> + } >> + } >> + if (overlap) { >> + /* >> + * As APICs are pre-CPU devices, they may have identical base= >> + * addresses. We must avoid unregistering an old io-region th= at is >> + * still in use by another APIC. >> + */ >> + s->busdev.mmio[0].addr =3D (target_phys_addr_t)-1; >> + } >> + if (s->apicbase & MSR_IA32_APICBASE_ENABLE) { >> + new_addr =3D s->apicbase & MSR_IA32_APICBASE_BASE; >> + } else { >> + new_addr =3D (target_phys_addr_t)-1; >> + } >> + sysbus_mmio_map(&s->busdev, 0, new_addr); >> +} >> + >> void cpu_set_apic_base(DeviceState *d, uint64_t val) >> { >> APICState *s =3D DO_UPCAST(APICState, busdev.qdev, d); >> @@ -302,7 +336,7 @@ void cpu_set_apic_base(DeviceState *d, uint64_t va= l) >> >> if (!s) >> return; >> - s->apicbase =3D (val & 0xfffff000) | >> + s->apicbase =3D (val & MSR_IA32_APICBASE_BASE) | >> (s->apicbase & (MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENAB= LE)); >> /* if disabled, cannot be enabled again */ >> if (!(val & MSR_IA32_APICBASE_ENABLE)) { >> @@ -310,6 +344,7 @@ void cpu_set_apic_base(DeviceState *d, uint64_t va= l) >> cpu_clear_apic_feature(s->cpu_env); >> s->spurious_vec &=3D ~APIC_SV_ENABLE; >> } >> + apic_update_mapping(s); >> } >> >> uint64_t cpu_get_apic_base(DeviceState *d) >> @@ -948,6 +983,7 @@ static void apic_reset(DeviceState *d) >> bsp =3D cpu_is_bsp(s->cpu_env); >> s->apicbase =3D 0xfee00000 | >> (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE; >> + apic_update_mapping(s); >=20 > Here the device maps itself at reset, which is not OK. >=20 >> >> apic_init_reset(d); >> >> diff --git a/hw/pc.c b/hw/pc.c >> index 4dfdc0b..294aa66 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -859,7 +859,6 @@ static DeviceState *apic_init(void *env, uint8_t a= pic_id) >> { >> DeviceState *dev; >> SysBusDevice *d; >> - static int apic_mapped; >> >> dev =3D qdev_create(NULL, "apic"); >> qdev_prop_set_uint8(dev, "id", apic_id); >> @@ -867,15 +866,6 @@ static DeviceState *apic_init(void *env, uint8_t = apic_id) >> qdev_init_nofail(dev); >> d =3D sysbus_from_qdev(dev); >> >> - /* XXX: mapping more APICs at the same memory location */ >> - if (apic_mapped =3D=3D 0) { >> - /* NOTE: the APIC is directly connected to the CPU - it is no= t >> - on the global memory bus. */ >> - /* XXX: what if the base changes? */ >> - sysbus_mmio_map(d, 0, MSI_ADDR_BASE); >> - apic_mapped =3D 1; >=20 > TBH, this is not so cool either. Maybe APIC should not be a Sysbus > device at all. You actually need the APIC to A) be part of or a child of its CPU and B) be part of a second, APIC bus (where we would find IOAPICs as well, also multi-homed). We don't model this ATM, so we have global local_apics[] and soon ioapics[]. And we need to coordinate remapping of the APIC where we have those full views. But even that's not the actual problem. The problem here is that we need per-CPU MMIO mapping and handling here, and that is not supported by QEMU. We would furthermore have to refactor everything around apicbase updates, and I surely don't want to do this in this fix. It targets the obvious breakage based on the current QEMU architecture. We can always improve on top of course. Jan --------------enigC204538EB73236FB6258248C Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.15 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/ iEYEARECAAYFAk1LHQoACgkQitSsb3rl5xQDHACggSZy+jSb5+fawaO2ZD9BIU4N QtQAoIF/Z26dypSqH/DuJS2a3k1dyY/e =sWP9 -----END PGP SIGNATURE----- --------------enigC204538EB73236FB6258248C--