From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=44794 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pl57b-0005xL-5U for qemu-devel@nongnu.org; Thu, 03 Feb 2011 14:45:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pl55V-0002Nc-R4 for qemu-devel@nongnu.org; Thu, 03 Feb 2011 14:43:31 -0500 Received: from mail-px0-f173.google.com ([209.85.212.173]:35511) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pl55V-0002NY-IC for qemu-devel@nongnu.org; Thu, 03 Feb 2011 14:43:29 -0500 Received: by pxi16 with SMTP id 16so341547pxi.4 for ; Thu, 03 Feb 2011 11:43:28 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <4D4B02E8.7060104@siemens.com> References: <4D4B02E8.7060104@siemens.com> From: Blue Swirl Date: Thu, 3 Feb 2011 19:43:08 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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: Jan Kiszka Cc: qemu-devel On Thu, Feb 3, 2011 at 7:32 PM, Jan Kiszka wrote: > 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. > > =C2=A0hw/apic.c | =C2=A0 38 +++++++++++++++++++++++++++++++++++++- > =C2=A0hw/pc.c =C2=A0 | =C2=A0 10 ---------- > =C2=A02 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_mod= e, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0trigger_mode); > =C2=A0} > > +static void apic_update_mapping(APICState *s) > +{ > + =C2=A0 =C2=A0target_phys_addr_t new_addr; > + =C2=A0 =C2=A0bool overlap =3D false; > + =C2=A0 =C2=A0APICState *apic_iter; > + =C2=A0 =C2=A0int i; > + > + =C2=A0 =C2=A0for (i =3D 0; i < MAX_APICS; i++) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0apic_iter =3D local_apics[i]; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!apic_iter || apic_iter =3D=3D s) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0continue; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if ((apic_iter->apicbase & MSR_IA32_APICBASE= _BASE) =3D=3D > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0s->busdev.mmio[0].addr) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0overlap =3D true; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0} > + =C2=A0 =C2=A0if (overlap) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0/* > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 * As APICs are pre-CPU devices, they may ha= ve identical base > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 * addresses. We must avoid unregistering an= old io-region that is > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 * still in use by another APIC. > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 */ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->busdev.mmio[0].addr =3D (target_phys_addr= _t)-1; > + =C2=A0 =C2=A0} > + =C2=A0 =C2=A0if (s->apicbase & MSR_IA32_APICBASE_ENABLE) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0new_addr =3D s->apicbase & MSR_IA32_APICBASE= _BASE; > + =C2=A0 =C2=A0} else { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0new_addr =3D (target_phys_addr_t)-1; > + =C2=A0 =C2=A0} > + =C2=A0 =C2=A0sysbus_mmio_map(&s->busdev, 0, new_addr); > +} > + > =C2=A0void cpu_set_apic_base(DeviceState *d, uint64_t val) > =C2=A0{ > =C2=A0 =C2=A0 APICState *s =3D DO_UPCAST(APICState, busdev.qdev, d); > @@ -302,7 +336,7 @@ void cpu_set_apic_base(DeviceState *d, uint64_t val) > > =C2=A0 =C2=A0 if (!s) > =C2=A0 =C2=A0 =C2=A0 =C2=A0 return; > - =C2=A0 =C2=A0s->apicbase =3D (val & 0xfffff000) | > + =C2=A0 =C2=A0s->apicbase =3D (val & MSR_IA32_APICBASE_BASE) | > =C2=A0 =C2=A0 =C2=A0 =C2=A0 (s->apicbase & (MSR_IA32_APICBASE_BSP | MSR_I= A32_APICBASE_ENABLE)); > =C2=A0 =C2=A0 /* if disabled, cannot be enabled again */ > =C2=A0 =C2=A0 if (!(val & MSR_IA32_APICBASE_ENABLE)) { > @@ -310,6 +344,7 @@ void cpu_set_apic_base(DeviceState *d, uint64_t val) > =C2=A0 =C2=A0 =C2=A0 =C2=A0 cpu_clear_apic_feature(s->cpu_env); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 s->spurious_vec &=3D ~APIC_SV_ENABLE; > =C2=A0 =C2=A0 } > + =C2=A0 =C2=A0apic_update_mapping(s); > =C2=A0} > > =C2=A0uint64_t cpu_get_apic_base(DeviceState *d) > @@ -948,6 +983,7 @@ static void apic_reset(DeviceState *d) > =C2=A0 =C2=A0 bsp =3D cpu_is_bsp(s->cpu_env); > =C2=A0 =C2=A0 s->apicbase =3D 0xfee00000 | > =C2=A0 =C2=A0 =C2=A0 =C2=A0 (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_= APICBASE_ENABLE; > + =C2=A0 =C2=A0apic_update_mapping(s); Here the device maps itself at reset, which is not OK. > > =C2=A0 =C2=A0 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 apic= _id) > =C2=A0{ > =C2=A0 =C2=A0 DeviceState *dev; > =C2=A0 =C2=A0 SysBusDevice *d; > - =C2=A0 =C2=A0static int apic_mapped; > > =C2=A0 =C2=A0 dev =3D qdev_create(NULL, "apic"); > =C2=A0 =C2=A0 qdev_prop_set_uint8(dev, "id", apic_id); > @@ -867,15 +866,6 @@ static DeviceState *apic_init(void *env, uint8_t api= c_id) > =C2=A0 =C2=A0 qdev_init_nofail(dev); > =C2=A0 =C2=A0 d =3D sysbus_from_qdev(dev); > > - =C2=A0 =C2=A0/* XXX: mapping more APICs at the same memory location */ > - =C2=A0 =C2=A0if (apic_mapped =3D=3D 0) { > - =C2=A0 =C2=A0 =C2=A0 =C2=A0/* NOTE: the APIC is directly connected to t= he CPU - it is not > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 on the global memory bus. */ > - =C2=A0 =C2=A0 =C2=A0 =C2=A0/* XXX: what if the base changes? */ > - =C2=A0 =C2=A0 =C2=A0 =C2=A0sysbus_mmio_map(d, 0, MSI_ADDR_BASE); > - =C2=A0 =C2=A0 =C2=A0 =C2=A0apic_mapped =3D 1; TBH, this is not so cool either. Maybe APIC should not be a Sysbus device at all.