Re: [Qemu-devel] [PATCH qom-next 11/12] target-i386: initialize APIC at CPU level

[Jan Kiszka]

On 2012-05-30 00:10, Igor Mammedov wrote:
> (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 is moved into
> cpu_model property setter. And APIC initialization is moved into
> x86_cpu_apic_init() which is called from x86_cpu_realize().
> 
> [1] - all x86 cpus have integrated APIC if we overlook existence of i486,
> and it's more convenient to model after majority of them.
> 
> 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.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  hw/pc.c           |   44 -------------------------------------
>  target-i386/cpu.c |   62 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index d178801..986f119 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -42,7 +42,6 @@
>  #include "sysbus.h"
>  #include "sysemu.h"
>  #include "kvm.h"
> -#include "xen.h"
>  #include "blockdev.h"
>  #include "ui/qemu-spice.h"
>  #include "memory.h"
> @@ -70,8 +69,6 @@
>  #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
>  #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
>  
> -#define MSI_ADDR_BASE 0xfee00000
> -
>  #define E820_NR_ENTRIES              16
>  
>  struct e820_entry {
> @@ -879,42 +876,6 @@ DeviceState *cpu_get_current_apic(void)
>      }
>  }
>  
> -static DeviceState *apic_init(void *env, uint8_t apic_id)
> -{
> -    DeviceState *dev;
> -    Error *error = NULL;
> -    static int apic_mapped;
> -
> -    if (kvm_irqchip_in_kernel()) {
> -        dev = qdev_create(NULL, "kvm-apic");
> -    } else if (xen_enabled()) {
> -        dev = qdev_create(NULL, "xen-apic");
> -    } else {
> -        dev = qdev_create(NULL, "apic");
> -    }
> -
> -    qdev_prop_set_uint8(dev, "id", apic_id);
> -    object_property_set_link(OBJECT(dev), OBJECT(ENV_GET_CPU(env)), "cpu",
> -                             &error);
> -    if (error_is_set(&error)) {
> -        qerror_report_err(error);
> -        error_free(error);
> -        exit(1);
> -    }
> -    qdev_init_nofail(dev);
> -
> -    /* XXX: mapping more APICs at the same memory location */
> -    if (apic_mapped == 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 = 1;
> -    }
> -
> -    return dev;
> -}
> -
>  void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>  {
>      CPUX86State *s = opaque;
> @@ -933,17 +894,12 @@ static void pc_cpu_reset(void *opaque)
>  static X86CPU *pc_new_cpu(const char *cpu_model)
>  {
>      X86CPU *cpu;
> -    CPUX86State *env;
>  
>      cpu = cpu_x86_init(cpu_model);
>      if (cpu == NULL) {
>          exit(1);
>      }
> -    env = &cpu->env;
>  
> -    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> -        env->apic_state = apic_init(env, env->cpuid_apic_id);
> -    }
>      qemu_register_reset(pc_cpu_reset, cpu);
>      pc_cpu_reset(cpu);
>      return cpu;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 2610d96..b649904 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -23,6 +23,7 @@
>  
>  #include "cpu.h"
>  #include "kvm.h"
> +#include "hw/xen.h"
>  
>  #include "qemu-option.h"
>  #include "qemu-config.h"
> @@ -31,6 +32,11 @@
>  
>  #include "hyperv.h"
>  
> +#include "sysemu.h"
> +#ifndef CONFIG_USER_ONLY
> +#include "hw/sysbus.h"
> +#endif
> +
>  /* feature flags taken from "Intel Processor Identification and the CPUID
>   * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
>   * between feature naming conventions, aliases may be added.
> @@ -1749,13 +1755,69 @@ static void x86_set_cpu_model(Object *obj, const char 
> *value, Error **errp)
>      if (cpu_x86_register(cpu, env->cpu_model_str) < 0) {
>          fprintf(stderr, "Unable to find x86 CPU definition\n");
>          error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> +        return;
> +    }
> +
> +#ifndef CONFIG_USER_ONLY
> +    if (((env->cpuid_features & CPUID_APIC) || smp_cpus > 1)) {
> +        if (kvm_irqchip_in_kernel()) {
> +            env->apic_state = qdev_create(NULL, "kvm-apic");
> +        } else if (xen_enabled()) {
> +            env->apic_state = qdev_create(NULL, "xen-apic");
> +        } else {
> +            env->apic_state = qdev_create(NULL, "apic");
> +        }
> +        object_property_add_child(OBJECT(cpu), "apic",
> +            OBJECT(env->apic_state), NULL);
> +
> +        qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
> +        object_property_set_link(OBJECT(env->apic_state), OBJECT(cpu), "cpu",
> +                                 errp);
> +        if (error_is_set(errp)) {
> +            return;
> +        }
> +    }
> +#endif
> +}
> +
> +#ifndef CONFIG_USER_ONLY
> +#define MSI_ADDR_BASE 0xfee00000
> +
> +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> +{
> +    static int apic_mapped;
> +    CPUX86State *env = &cpu->env;
> +
> +    if (env->apic_state == NULL) {
> +        return;
> +    }
> +
> +    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 == 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 = 1;
>      }
>  }
> +#endif
>  
>  void x86_cpu_realize(Object *obj, Error **errp)
>  {
>      X86CPU *cpu = X86_CPU(obj);
>  
> +    x86_cpu_apic_init(cpu, errp);
> +    if (error_is_set(errp)) {
> +        return;
> +    }
> +

I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the
sake of bisectability). Or, likely better, let x86_cpu_apic_init do
nothing for usermode emulation.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux