Re: [Qemu-devel] [PATCH 14/16] target-i386: move APIC to ICC bus

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 22 Apr 2013 17:18:58 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 16.04.2013 00:12, schrieb Igor Mammedov:
[...]

> >  static void icc_bridge_initfn(Object *obj)
> >  {
> > -    qbus_create(TYPE_ICC_BUS, DEVICE(obj), "icc-bus");
> > +    ICCBridgeState *s = ICC_BRIGDE(obj);
> > +    SysBusDevice *sb = SYS_BUS_DEVICE(obj);
> > +    ICCBus *ibus;
> > +
> > +    ibus = ICC_BUS(qbus_create(TYPE_ICC_BUS, DEVICE(obj), "icc-bus"));
> 
> qbus_create_inplace() as suggested on ICC patch would allow to access it
> through state field.
done

> 
> > +
> > +    /* Do not change order of registering regions,
> > +     * APIC must be first registered region, board maps it by 0 index
> > +     */
> > +    memory_region_init(&s->apic_container, "icc-apic-container",
> > +                       APIC_SPACE_SIZE);
> > +    sysbus_init_mmio(sb, &s->apic_container);
> > +    ibus->apic_address_space = &s->apic_container;
> >  }
> >  
> >  static const TypeInfo icc_bridge_info = {
> > diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> > index 3a10c07..5b558aa 100644
> > --- a/hw/i386/kvmvapic.c
> > +++ b/hw/i386/kvmvapic.c
> > @@ -12,6 +12,7 @@
> >  #include "sysemu/cpus.h"
> >  #include "sysemu/kvm.h"
> >  #include "hw/i386/apic_internal.h"
> > +#include "hw/sysbus.h"
> >  
> >  #define VAPIC_IO_PORT           0x7e
> >  
> 
> I take it, this is necessary due to dropping hw/sysbus.h include in
> cpu.c? Might be split out then since patch is rather large already.
It's due to dropping  hw/sysbus.h from include/hw/i386/apic_internal.h
It's hard to split in meaningful separate patch.

> 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index cb57878..31b5294 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -52,6 +52,7 @@
> >  #include "sysemu/arch_init.h"
> >  #include "qemu/bitmap.h"
> >  #include "qemu/config-file.h"
> > +#include "hw/i386/icc_bus.h"
> >  
> >  /* debug PC/ISA interrupts */
> >  //#define DEBUG_IRQ
> 
> > @@ -889,13 +890,13 @@ void pc_acpi_smi_interrupt(void *opaque, int irq,
> > int level) }
> >  }
> >  
> > -static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error
> > **errp) +static X86CPU *pc_new_cpu(const char *cpu_model, int64_t
> > apic_id, Error **errp) {
> >      X86CPU *cpu;
> >  
> >      cpu = cpu_x86_create(cpu_model, errp);
> >      if (!cpu) {
> > -        return;
> > +        return cpu;
> >      }
> >  
> >      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", errp);
> > @@ -904,14 +905,18 @@ static void pc_new_cpu(const char *cpu_model,
> > int64_t apic_id, Error **errp) if (error_is_set(errp)) {
> >          if (cpu != NULL) {
> >              object_unref(OBJECT(cpu));
> > +            cpu = NULL;
> >          }
> >      }
> > +    return cpu;
> >  }
> >  
> 
> Squash this into the pc_new_cpu() introduction?
sure, it's less obvious why it would be needed but makes this patch smaller.

> 
> >  void pc_cpus_init(const char *cpu_model)
> >  {
> >      int i;
> > +    X86CPU *cpu = NULL;
> >      Error *error = NULL;
> > +    SysBusDevice *ib;
> 
> icc maybe? Anything that more's comprehensible. ;)
icc_bridge then.

[...]
> > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> > index e0ae07a..39396f1 100644
> > --- a/hw/intc/apic_common.c
> > +++ b/hw/intc/apic_common.c
> > @@ -21,6 +21,8 @@
> >  #include "hw/i386/apic_internal.h"
> >  #include "trace.h"
> >  #include "sysemu/kvm.h"
> > +#include "hw/qdev.h"
> > +#include "hw/sysbus.h"
> >  
> >  static int apic_irq_delivered;
> >  bool apic_report_tpr_access;
> > @@ -282,12 +284,14 @@ static int apic_load_old(QEMUFile *f, void *opaque,
> > int version_id) return 0;
> >  }
> >  
> > -static int apic_init_common(SysBusDevice *dev)
> > +static int apic_init_common(ICCDevice *dev)
> 
> Instead of mechanically replacing SysBusDevice with ICCDevice in qdev
> init functions, please change to realize. Could be its own prepended
> patch if you prefer.

IRC with Andreas, agreed to do it only for ICCDevice. And do re-factoring
of init() in APICs/IOAPIC later.

> 
> >  {
> >      APICCommonState *s = APIC_COMMON(dev);
> > +    DeviceState *d = DEVICE(dev);
> >      APICCommonClass *info;
> >      static DeviceState *vapic;
> >      static int apic_no;
> > +    static bool mmio_registered;
> >  
> >      if (apic_no >= MAX_APICS) {
> >          return -1;
[...]

> > @@ -375,7 +382,7 @@ static Property apic_properties_common[] = {
> >  
> >  static void apic_common_class_init(ObjectClass *klass, void *data)
> >  {
> > -    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
> > +    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);
> 
> idc please. :)
done

> 
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> >      dc->vmsd = &vmstate_apic_common;
> > @@ -387,7 +394,7 @@ static void apic_common_class_init(ObjectClass
> > *klass, void *data) 
> >  static const TypeInfo apic_common_type = {
[...]

> > @@ -2120,7 +2121,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error
> > **errp) apic_type = "xen-apic";
> >      }
> >  
> > -    env->apic_state = qdev_try_create(NULL, apic_type);
> > +    env->apic_state = qdev_try_create(dev->parent_bus, apic_type);
> 
> qdev_get_parent_bus() according to header comment.
done

> 
> >      if (env->apic_state == NULL) {
[...]
> Andreas
>