From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46148) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJoKg-0007qf-Po for qemu-devel@nongnu.org; Thu, 04 Oct 2012 12:31:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TJoKa-00021x-D9 for qemu-devel@nongnu.org; Thu, 04 Oct 2012 12:31:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:19650) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJoKa-00020i-4t for qemu-devel@nongnu.org; Thu, 04 Oct 2012 12:31:24 -0400 Date: Thu, 4 Oct 2012 18:31:17 +0200 From: Igor Mammedov Message-ID: <20121004183117.4d51b0d2@nial.usersys.redhat.com> In-Reply-To: <506DB634.70001@suse.de> References: <1349361818-15331-1-git-send-email-imammedo@redhat.com> <506DB634.70001@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?ISO-8859-1?B?RuRyYmVy?= Cc: aliguori@us.ibm.com, ehabkost@redhat.com, gleb@redhat.com, Don@CloudSwitch.com, qemu-devel@nongnu.org, jan.kiszka@web.de On Thu, 04 Oct 2012 18:15:48 +0200 Andreas F=E4rber wrote: > Am 04.10.2012 16:43, schrieb Igor Mammedov: > > (L)APIC is a part of cpu [1] so move APIC initialization inside of > > x86_cpu object. Since cpu_model and override flags currently specify > > whether APIC should be created or not, APIC creation&initialization is > > moved into x86_cpu_apic_init() which is called from x86_cpu_realize(). > >=20 > > [1] - all x86 cpus have integrated APIC if we overlook existence of i48= 6, > > and it's more convenient to model after majority of them. > >=20 > > Signed-off-by: Igor Mammedov > > --- > > v2: > > * init APIC mapping at cpu level, due to Peter's objection to putting > > it into APIC's initfn and Jan's suggestion to do it inside cpu. > > v3: > > * create APIC at realize time > > * rebased on top of current qemu tree > > * whitespace fix > > * ifdef only body of x86_cpu_apic_init() > >=20 > > Git tree for testing: > > https://github.com/imammedo/qemu/tree/apic_in_cpu > > --- > > hw/pc.c | 56 > > +++++------------------------------------------------ target-i386/cpu.c= | > > 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files > > changed, 63 insertions(+), 51 deletions(-) > >=20 > > diff --git a/hw/pc.c b/hw/pc.c > > index 7e7e0e2..f4c0579 100644 > > --- a/hw/pc.c > > +++ b/hw/pc.c > > @@ -72,8 +72,6 @@ > > #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3) > > #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4) > > =20 > > -#define MSI_ADDR_BASE 0xfee00000 > > - > > #define E820_NR_ENTRIES 16 > > =20 > > struct e820_entry { > > @@ -847,35 +845,6 @@ DeviceState *cpu_get_current_apic(void) > > } > > } > > =20 > > -static DeviceState *apic_init(void *env, uint8_t apic_id) > > -{ > > - DeviceState *dev; > > - static int apic_mapped; > > - > > - if (kvm_irqchip_in_kernel()) { > > - dev =3D qdev_create(NULL, "kvm-apic"); > > - } else if (xen_enabled()) { > > - dev =3D qdev_create(NULL, "xen-apic"); > > - } else { > > - dev =3D qdev_create(NULL, "apic"); > > - } > > - > > - qdev_prop_set_uint8(dev, "id", apic_id); > > - qdev_prop_set_ptr(dev, "cpu_env", env); > > - qdev_init_nofail(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 not > > - on the global memory bus. */ > > - /* XXX: what if the base changes? */ > > - sysbus_mmio_map(sysbus_from_qdev(dev), 0, MSI_ADDR_BASE); > > - apic_mapped =3D 1; > > - } > > - > > - return dev; > > -} > > - > > void pc_acpi_smi_interrupt(void *opaque, int irq, int level) > > { > > CPUX86State *s =3D opaque; > > @@ -885,24 +854,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, > > int level) } > > } > > =20 > > -static X86CPU *pc_new_cpu(const char *cpu_model) > > -{ > > - X86CPU *cpu; > > - CPUX86State *env; > > - > > - cpu =3D cpu_x86_init(cpu_model); > > - if (cpu =3D=3D NULL) { > > - fprintf(stderr, "Unable to find x86 CPU definition\n"); > > - exit(1); > > - } > > - env =3D &cpu->env; > > - if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { > > - env->apic_state =3D apic_init(env, env->cpuid_apic_id); > > - } > > - cpu_reset(CPU(cpu)); > > - return cpu; > > -} > > - > > void pc_cpus_init(const char *cpu_model) > > { > > int i; > > @@ -916,8 +867,11 @@ void pc_cpus_init(const char *cpu_model) > > #endif > > } > > =20 > > - for(i =3D 0; i < smp_cpus; i++) { > > - pc_new_cpu(cpu_model); > > + for (i =3D 0; i < smp_cpus; i++) { > > + if (!cpu_x86_init(cpu_model)) { > > + fprintf(stderr, "Unable to find x86 CPU definition\n"); > > + exit(1); > > + } > > } > > } > > =20 > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index bb1e44e..612ed5a 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -37,6 +37,12 @@ > > #include > > #endif > > =20 > > +#include "sysemu.h" > > +#include "hw/xen.h" >=20 > Could also go into the #ifndef below. sure >=20 > > +#ifndef CONFIG_USER_ONLY > > +#include "hw/sysbus.h" > > +#endif > > + > > /* feature flags taken from "Intel Processor Identification and the CP= UID > > * Instruction" and AMD's "CPUID Specification". In cases of > > disagreement > > * between feature naming conventions, aliases may be added. > > @@ -1870,6 +1876,56 @@ static void mce_init(X86CPU *cpu) > > } > > } > > =20 > > +#define MSI_ADDR_BASE 0xfee00000 > > + > > +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) > > +{ > > +#ifndef CONFIG_USER_ONLY > > + static int apic_mapped; > > + CPUX86State *env =3D &cpu->env; > > + const char *apic_type =3D "apic"; > > + > > + if (!(env->cpuid_features & CPUID_APIC || smp_cpus > 1)) { > > + return; > > + } >=20 > I would prefer to keep the original logic at the call site > (x86_cpu_initfn) Could you explain it bit more? > rather than silently returning without setting errp. and this one too, pls. >=20 > > + > > + if (kvm_irqchip_in_kernel()) { > > + apic_type =3D "kvm-apic"; > > + } else if (xen_enabled()) { > > + apic_type =3D "xen-apic"; > > + } > > + env->apic_state =3D qdev_create(NULL, apic_type); > > + > > + if (env->apic_state =3D=3D NULL) { > > + error_set(errp, QERR_DEVICE_INIT_FAILED, apic_type); > > + return; > > + } > > + > > + object_property_add_child(OBJECT(cpu), "apic", > > + OBJECT(env->apic_state), NULL); > > + qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id); > > + /* TODO: convert to link<> */ > > + qdev_prop_set_ptr(env->apic_state, "cpu_env", env); >=20 > Do we still need env->apic_state and a property on the APIC at all? From > the X86CPU side we can access the "apic" property to retrieve the > pointer, and from the APIC we should be able to navigate to its parent, > right? I'll look at this. > > > + > > + if (qdev_init(env->apic_state)) { > > + error_set(errp, QERR_DEVICE_INIT_FAILED, > > + object_get_typename(OBJECT(env->apic_state))); > > + return; > > + } > > + > > + /* 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 not > > + on the global memory bus. */ > > + /* XXX: what if the base changes? */ > > + sysbus_mmio_map(sysbus_from_qdev(env->apic_state), 0, > > MSI_ADDR_BASE); > > + apic_mapped =3D 1; > > + } > > + > > + return; > > +#endif > > +} > > + > > void x86_cpu_realize(Object *obj, Error **errp) > > { > > X86CPU *cpu =3D X86_CPU(obj); > > @@ -1878,6 +1934,8 @@ void x86_cpu_realize(Object *obj, Error **errp) > > qemu_register_reset(x86_cpu_machine_reset_cb, cpu); > > #endif > > =20 > > + x86_cpu_apic_init(cpu, errp); >=20 > if (error_is_set(errp)) { > return; > } > ? Something like this patch was in CPU properties series. I'll include it here as well. >=20 > > + > > mce_init(cpu); > > qemu_init_vcpu(&cpu->env); > > cpu_reset(CPU(cpu)); > >=20 >=20 > Andreas >=20 Thanks, Igor