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

[Paolo Bonzini]

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.

But actually, you lack a device that instantiates the bus.  That device
would be a simple container device similar hw/a15mpcore.c, and would
also take care of registering the MSI memory region.  Create that device
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;
>