Re: [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level

[Andreas Färber]

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().
> 
> [1] - all x86 cpus have integrated APIC if we overlook existence of i486,
> and it's more convenient to model after majority of them.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> 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()
> 
> 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(-)
> 
> 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)
>  
> -#define MSI_ADDR_BASE 0xfee00000
> -
>  #define E820_NR_ENTRIES              16
>  
>  struct e820_entry {
> @@ -847,35 +845,6 @@ DeviceState *cpu_get_current_apic(void)
>      }
>  }
>  
> -static DeviceState *apic_init(void *env, uint8_t apic_id)
> -{
> -    DeviceState *dev;
> -    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);
> -    qdev_prop_set_ptr(dev, "cpu_env", env);
> -    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;
> @@ -885,24 +854,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
> level)
>      }
>  }
>  
> -static X86CPU *pc_new_cpu(const char *cpu_model)
> -{
> -    X86CPU *cpu;
> -    CPUX86State *env;
> -
> -    cpu = cpu_x86_init(cpu_model);
> -    if (cpu == NULL) {
> -        fprintf(stderr, "Unable to find x86 CPU definition\n");
> -        exit(1);
> -    }
> -    env = &cpu->env;
> -    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> -        env->apic_state = 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
>      }
>  
> -    for(i = 0; i < smp_cpus; i++) {
> -        pc_new_cpu(cpu_model);
> +    for (i = 0; i < smp_cpus; i++) {
> +        if (!cpu_x86_init(cpu_model)) {
> +            fprintf(stderr, "Unable to find x86 CPU definition\n");
> +            exit(1);
> +        }
>      }
>  }
>  
> 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 <linux/kvm_para.h>
>  #endif
>  
> +#include "sysemu.h"
> +#include "hw/xen.h"

Could also go into the #ifndef below.

> +#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.
> @@ -1870,6 +1876,56 @@ static void mce_init(X86CPU *cpu)
>      }
>  }
>  
> +#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 = &cpu->env;
> +    const char *apic_type = "apic";
> +
> +    if (!(env->cpuid_features & CPUID_APIC || smp_cpus > 1)) {
> +        return;
> +    }

I would prefer to keep the original logic at the call site
(x86_cpu_initfn) rather than silently returning without setting errp.

> +
> +    if (kvm_irqchip_in_kernel()) {
> +        apic_type = "kvm-apic";
> +    } else if (xen_enabled()) {
> +        apic_type = "xen-apic";
> +    }
> +    env->apic_state = qdev_create(NULL, apic_type);
> +
> +    if (env->apic_state == 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);

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?

> +
> +    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;
> +    }
> +
> +    return;
> +#endif
> +}
> +
>  void x86_cpu_realize(Object *obj, Error **errp)
>  {
>      X86CPU *cpu = 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
>  
> +    x86_cpu_apic_init(cpu, errp);

if (error_is_set(errp)) {
    return;
}
?

> +
>      mce_init(cpu);
>      qemu_init_vcpu(&cpu->env);
>      cpu_reset(CPU(cpu));
> 

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg