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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 04 Oct 2012 18:15:48 +0200
Andreas Färber <afaerber@suse.de> 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().
> > 
> > [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 <imammedo@redhat.com>
> > ---
> > 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.
sure

> 
> > +#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)
Could you explain it bit more?

> rather than silently returning without setting errp.
and this one too, pls.

> 
> > +
> > +    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?
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 == 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;
> }
> ?
Something like this patch was in CPU properties series. I'll include it here
as well.

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

Thanks,
  Igor