Re: [Qemu-devel] [PATCH 07/12] target-i386: Add ICC_BUS and attach apic, kvmvapic and cpu to it

[Igor Mammedov]

On Wed, 27 Mar 2013 11:57:53 +0100
Paolo Bonzini <address@hidden> wrote:

> Il 21/03/2013 15:28, Igor Mammedov ha scritto:  
> > Introduce hot-pluggable ICC_BUS and convert APIC devices from SysBusDevice
> > to ICCDevice wich has ICC_BUS as default one.
> > 
> >  * attach APIC and kvmvapic to ICC_BUS during creation
> >  * use memory API directly instead of using SysBus proxy functions
> >  * introduce get_icc_bus() for getting access to system wide ICC_BUS
> >  * make ICC_BUS default one for X86CPU class, so that device_add would
> >    set it for CPU instead of SysBus
> >  * attach cold-pluged CPUs to icc_bus in softmmu mode, so they could be
> >    uplugged in future
> >  * if kvmvapic init() fails return -1 instead of aborting.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  hw/apic_common.c      |   14 ++++++---
> >  hw/apic_internal.h    |    6 ++--
> >  hw/i386/Makefile.objs |    2 +-
> >  hw/i386/kvmvapic.c    |   15 +++++----
> >  hw/icc_bus.c          |   75 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/icc_bus.h          |   47 ++++++++++++++++++++++++++++++
> >  stubs/Makefile.objs   |    1 +
> >  stubs/get_icc_bus.c   |    6 ++++
> >  target-i386/cpu.c     |   16 ++++++++--
> >  9 files changed, 162 insertions(+), 20 deletions(-)
> >  create mode 100644 hw/icc_bus.c
> >  create mode 100644 hw/icc_bus.h
> >  create mode 100644 stubs/get_icc_bus.c
> > 
> > diff --git a/hw/apic_common.c b/hw/apic_common.c
> > index d0c2616..61599d4 100644
> > --- a/hw/apic_common.c
> > +++ b/hw/apic_common.c
> > @@ -21,6 +21,7 @@
> >  #include "hw/apic_internal.h"
> >  #include "trace.h"
> >  #include "sysemu/kvm.h"
> > +#include "qdev.h"
> >  
> >  static int apic_irq_delivered;
> >  bool apic_report_tpr_access;
> > @@ -282,9 +283,10 @@ 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)
> >  {
> >      APICCommonState *s = APIC_COMMON(dev);
> > +    DeviceState *d = DEVICE(dev);
> >      APICCommonClass *info;
> >      static DeviceState *vapic;
> >      static int apic_no;
> > @@ -297,12 +299,14 @@ static int apic_init_common(SysBusDevice *dev)
> >      info = APIC_COMMON_GET_CLASS(s);
> >      info->init(s);
> >  
> > -    sysbus_init_mmio(dev, &s->io_memory);
> >  
> >      /* Note: We need at least 1M to map the VAPIC option ROM */
> >      if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
> >          ram_size >= 1024 * 1024) {
> > -        vapic = sysbus_create_simple("kvmvapic", -1, NULL);
> > +        vapic = qdev_try_create(d->parent_bus, "kvmvapic");
> > +        if ((vapic == NULL) || (qdev_init(vapic) != 0)) {
> > +            return -1;
> > +        }
> >      }
> >      s->vapic = vapic;
> >      if (apic_report_tpr_access && info->enable_tpr_reporting) {
> > @@ -375,7 +379,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);
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> >      dc->vmsd = &vmstate_apic_common;
> > @@ -387,7 +391,7 @@ static void apic_common_class_init(ObjectClass *klass, 
> > void *data)
> >  
> >  static const TypeInfo apic_common_type = {
> >      .name = TYPE_APIC_COMMON,
> > -    .parent = TYPE_SYS_BUS_DEVICE,
> > +    .parent = TYPE_ICC_DEVICE,
> >      .instance_size = sizeof(APICCommonState),
> >      .class_size = sizeof(APICCommonClass),
> >      .class_init = apic_common_class_init,
> > diff --git a/hw/apic_internal.h b/hw/apic_internal.h
> > index 578241f..e7b378e 100644
> > --- a/hw/apic_internal.h
> > +++ b/hw/apic_internal.h
> > @@ -21,7 +21,7 @@
> >  #define QEMU_APIC_INTERNAL_H
> >  
> >  #include "exec/memory.h"
> > -#include "hw/sysbus.h"
> > +#include "hw/icc_bus.h"
> >  #include "qemu/timer.h"
> >  
> >  /* APIC Local Vector Table */
> > @@ -80,7 +80,7 @@ typedef struct APICCommonState APICCommonState;
> >  
> >  typedef struct APICCommonClass
> >  {
> > -    SysBusDeviceClass parent_class;
> > +    ICCDeviceClass parent_class;
> >  
> >      void (*init)(APICCommonState *s);
> >      void (*set_base)(APICCommonState *s, uint64_t val);
> > @@ -94,7 +94,7 @@ typedef struct APICCommonClass
> >  } APICCommonClass;
> >  
> >  struct APICCommonState {
> > -    SysBusDevice busdev;
> > +    ICCDevice busdev;
> >  
> >      MemoryRegion io_memory;
> >      X86CPU *cpu;
> > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> > index a78c0b2..316f999 100644
> > --- a/hw/i386/Makefile.objs
> > +++ b/hw/i386/Makefile.objs
> > @@ -1,5 +1,5 @@
> >  obj-y += mc146818rtc.o
> > -obj-y += apic_common.o apic.o
> > +obj-y += apic_common.o apic.o icc_bus.o
> >  obj-y += sga.o ioapic_common.o ioapic.o piix_pci.o
> >  obj-y += vmport.o
> >  obj-y += pci/pci-hotplug.o wdt_ib700.o
> > diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> > index 21551a5..7de75db 100644
> > --- a/hw/i386/kvmvapic.c
> > +++ b/hw/i386/kvmvapic.c
> > @@ -12,6 +12,8 @@
> >  #include "sysemu/cpus.h"
> >  #include "sysemu/kvm.h"
> >  #include "hw/apic_internal.h"
> > +#include "migration/vmstate.h"
> > +#include "exec/address-spaces.h"
> >  
> >  #define APIC_DEFAULT_ADDRESS    0xfee00000
> >  
> > @@ -49,7 +51,7 @@ typedef struct GuestROMState {
> >  } QEMU_PACKED GuestROMState;
> >  
> >  typedef struct VAPICROMState {
> > -    SysBusDevice busdev;
> > +    ICCDevice busdev;
> >      MemoryRegion io;
> >      MemoryRegion rom;
> >      uint32_t state;
> > @@ -581,7 +583,7 @@ static void vapic_map_rom_writable(VAPICROMState *s)
> >      size_t rom_size;
> >      uint8_t *ram;
> >  
> > -    as = sysbus_address_space(&s->busdev);
> > +    as = get_system_memory();
> >  
> >      if (s->rom_mapped_writable) {
> >          memory_region_del_subregion(as, &s->rom);
> > @@ -693,13 +695,12 @@ static const MemoryRegionOps vapic_ops = {
> >      .endianness = DEVICE_NATIVE_ENDIAN,
> >  };
> >  
> > -static int vapic_init(SysBusDevice *dev)
> > +static int vapic_init(ICCDevice *dev)
> >  {
> >      VAPICROMState *s = VAPIC_DEVICE(dev);
> >  
> >      memory_region_init_io(&s->io, &vapic_ops, s, "kvmvapic", 2);
> > -    sysbus_add_io(dev, VAPIC_IO_PORT, &s->io);
> > -    sysbus_init_ioports(dev, VAPIC_IO_PORT, 2);
> > +    memory_region_add_subregion(get_system_io(), VAPIC_IO_PORT, &s->io);
> >  
> >      option_rom[nb_option_roms].name = "kvmvapic.bin";
> >      option_rom[nb_option_roms].bootindex = -1;
> > @@ -801,7 +802,7 @@ static const VMStateDescription vmstate_vapic = {
> >  
> >  static void vapic_class_init(ObjectClass *klass, void *data)
> >  {
> > -    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
> > +    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> >      dc->no_user = 1;
> > @@ -812,7 +813,7 @@ static void vapic_class_init(ObjectClass *klass, void 
> > *data)
> >  
> >  static const TypeInfo vapic_type = {
> >      .name          = TYPE_VAPIC_DEVICE,
> > -    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .parent        = TYPE_ICC_DEVICE,
> >      .instance_size = sizeof(VAPICROMState),
> >      .class_init    = vapic_class_init,
> >  };
> > diff --git a/hw/icc_bus.c b/hw/icc_bus.c
> > new file mode 100644
> > index 0000000..b170648
> > --- /dev/null
> > +++ b/hw/icc_bus.c
> > @@ -0,0 +1,75 @@
> > +/* icc_bus.c
> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> > + *
> > + * Copyright (c) 2013 Red Hat, Inc
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see 
> > <http://www.gnu.org/licenses/>
> > + */
> > +#include "icc_bus.h"
> > +#include "qemu/module.h"
> > +
> > +static const TypeInfo icc_bus_info = {
> > +    .name = TYPE_ICC_BUS,
> > +    .parent = TYPE_BUS,
> > +    .instance_size = sizeof(ICCBus),
> > +};
> > +
> > +static int icc_device_init(DeviceState *dev)
> > +{
> > +    ICCDevice *id = ICC_DEVICE(dev);
> > +    ICCDeviceClass *k = ICC_DEVICE_GET_CLASS(id);
> > +
> > +    return k->init(id);
> > +}
> > +
> > +static void icc_device_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *k = DEVICE_CLASS(klass);
> > +
> > +    k->init = icc_device_init;
> > +    k->bus_type = TYPE_ICC_BUS;
> > +}
> > +
> > +static const TypeInfo icc_device_info = {
> > +    .name = TYPE_ICC_DEVICE,
> > +    .parent = TYPE_DEVICE,
> > +    .abstract = true,
> > +    .instance_size = sizeof(ICCDevice),
> > +    .class_size = sizeof(ICCDeviceClass),
> > +    .class_init = icc_device_class_init,
> > +};
> > +
> > +static BusState *icc_bus;
> > +
> > +BusState *get_icc_bus(void)
> > +{
> > +    if (icc_bus == NULL) {
> > +        icc_bus = g_malloc0(icc_bus_info.instance_size);
> > +        qbus_create_inplace(icc_bus, TYPE_ICC_BUS, NULL, "icc-bus");
> > +        icc_bus->allow_hotplug = 1;
> > +        OBJECT(icc_bus)->free = g_free;  
> 
> You can just use qbus_create.
>   
> > +        object_property_add_child(container_get(qdev_get_machine(),
> > +                                                "/unattached"),  
> 
> Please put it straight under /machine, not /unattached.  

sure

> 
> But actually, you lack a device that instantiates the bus.  That device
> would be a simple container device similar hw/a15mpcore.c, and would  

About a year ago something like that device was proposed "cpu_sockets", but
there was suggestion to drop it:
http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02157.html
So I've opted in favor of parent-less bus, but I could create it
explicitly at board level as you suggest.

However having parent sysbus device for icc-bus will allow to see it via
monitor info qtree and reset on SysBus could be propagated through it as well,
it may be good to add it later.

> also take care of registering the MSI memory region.  Create that device  

it probably impossible to do, because [kvm]apic sets its private callbacks
to it when initializing memory region, and CPU just maps it at specified
address, or this region and APIC might not even exists at all if qemu started
with -apic feature flag.

> in the boards, and you won't need a special object_property_add_child.
> 
> get_icc_bus() would then become simply
> object_resolve_path_type("icc-bus", TYPE_ICC_BUS, NULL) or something
> like that.
> 
> Paolo
>   
> > +                                  "icc-bus", OBJECT(icc_bus), NULL);
> > +    }
> > +    return BUS(icc_bus);
> > +}
> > +
> > +static void icc_bus_register_types(void)
> > +{
> > +    type_register_static(&icc_bus_info);
> > +    type_register_static(&icc_device_info);
> > +}
> > +
> > +type_init(icc_bus_register_types)
> > diff --git a/hw/icc_bus.h b/hw/icc_bus.h
> > new file mode 100644
> > index 0000000..a00fef5
> > --- /dev/null
> > +++ b/hw/icc_bus.h
> > @@ -0,0 +1,47 @@
> > +/* icc_bus.h
> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> > + *
> > + * Copyright (c) 2013 Red Hat, Inc
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see 
> > <http://www.gnu.org/licenses/>
> > + */
> > +#ifndef ICC_BUS_H
> > +#define ICC_BUS_H
> > +
> > +#include "hw/qdev-core.h"
> > +
> > +typedef struct ICCBus {
> > +    BusState qbus;
> > +} ICCBus;
> > +#define TYPE_ICC_BUS "ICC"
> > +#define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
> > +
> > +typedef struct ICCDevice {
> > +    DeviceState qdev;
> > +} ICCDevice;
> > +
> > +typedef struct ICCDeviceClass {
> > +    DeviceClass parent_class;
> > +    int (*init)(ICCDevice *dev);
> > +} ICCDeviceClass;
> > +#define TYPE_ICC_DEVICE "icc-device"
> > +#define ICC_DEVICE(obj) OBJECT_CHECK(ICCDevice, (obj), TYPE_ICC_DEVICE)
> > +#define ICC_DEVICE_CLASS(klass) \
> > +     OBJECT_CLASS_CHECK(ICCDeviceClass, (klass), TYPE_ICC_DEVICE)
> > +#define ICC_DEVICE_GET_CLASS(obj) \
> > +     OBJECT_GET_CLASS(ICCDeviceClass, (obj), TYPE_ICC_DEVICE)
> > +
> > +BusState *get_icc_bus(void);
> > +
> > +#endif
> > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > index 28fb4f8..9741e16 100644
> > --- a/stubs/Makefile.objs
> > +++ b/stubs/Makefile.objs
> > @@ -24,3 +24,4 @@ stub-obj-y += vm-stop.o
> >  stub-obj-y += vmstate.o
> >  stub-obj-$(CONFIG_WIN32) += fd-register.o
> >  stub-obj-y += resume_vcpu.o
> > +stub-obj-y += get_icc_bus.o
> > diff --git a/stubs/get_icc_bus.c b/stubs/get_icc_bus.c
> > new file mode 100644
> > index 0000000..43db181
> > --- /dev/null
> > +++ b/stubs/get_icc_bus.c
> > @@ -0,0 +1,6 @@
> > +#include "hw/icc_bus.h"
> > +
> > +BusState *get_icc_bus(void)
> > +{
> > +    return NULL;
> > +}
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 631bcd8..ae46f81 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -42,10 +42,11 @@
> >  
> >  #include "sysemu/sysemu.h"
> >  #include "hw/qdev-properties.h"
> > +#include "hw/icc_bus.h"
> >  #ifndef CONFIG_USER_ONLY
> >  #include "hw/xen.h"
> > -#include "hw/sysbus.h"
> >  #include "hw/apic_internal.h"
> > +#include "exec/address-spaces.h"
> >  #endif
> >  
> >  static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> > @@ -1640,6 +1641,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error 
> > **errp)
> >      CPUX86State *env;
> >      gchar **model_pieces;
> >      char *name, *features;
> > +    BusState *icc_bus = get_icc_bus();
> >  
> >      model_pieces = g_strsplit(cpu_model, ",", 2);
> >      if (!model_pieces[0]) {
> > @@ -1650,6 +1652,10 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error 
> > **errp)
> >      features = model_pieces[1];
> >  
> >      cpu = X86_CPU(object_new(TYPE_X86_CPU));
> > +    if (icc_bus) {
> > +        qdev_set_parent_bus(DEVICE(cpu), icc_bus);
> > +        object_unref(OBJECT(cpu));
> > +    }
> >      env = &cpu->env;
> >      env->cpu_model_str = cpu_model;
> >  
> > @@ -2145,7 +2151,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(get_icc_bus(), apic_type);
> >      if (env->apic_state == NULL) {
> >          error_setg(errp, "APIC device '%s' could not be created", 
> > apic_type);
> >          return;
> > @@ -2176,11 +2182,12 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error 
> > **errp)
> >  
> >      /* XXX: mapping more APICs at the same memory location */
> >      if (apic_mapped == 0) {
> > +        APICCommonState *apic = APIC_COMMON(env->apic_state);
> >          /* 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_overlap(SYS_BUS_DEVICE(env->apic_state), 0,
> > -                                MSI_ADDR_BASE, 0x1000);
> > +        memory_region_add_subregion_overlap(get_system_memory(), 
> > MSI_ADDR_BASE,
> > +                                            &apic->io_memory, 0x1000);
> >          apic_mapped = 1;
> >      }
> >  }
> > @@ -2356,6 +2363,7 @@ static void x86_cpu_common_class_init(ObjectClass 
> > *oc, void *data)
> >      xcc->parent_realize = dc->realize;
> >      dc->props = cpu_x86_properties;
> >      dc->realize = x86_cpu_realizefn;
> > +    dc->bus_type = TYPE_ICC_BUS;
> >  
> >      xcc->parent_reset = cc->reset;
> >      cc->reset = x86_cpu_reset;
> >   
>   


-- 
Regards,
  Igor