Re: [Qemu-devel] [PATCH 1/1] Introduce a new bus "ICC" to connect APIC

[Blue Swirl]

On Wed, Oct 19, 2011 at 01:55,  <address@hidden> wrote:
> From: Liu Ping Fan <address@hidden>
>
> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
> of sysbus. So we can support APIC hot-plug feature.

Is this ICC bus or APIC hot plugging documented somewhere?

> Signed-off-by: liu ping fan <address@hidden>
> ---
>  Makefile.target |    1 +
>  hw/apic.c       |   25 +++++++++++----
>  hw/apic.h       |    1 +
>  hw/icc_bus.c    |   91 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
>  hw/pc.c         |   11 ++++--
>  6 files changed, 174 insertions(+), 11 deletions(-)
>  create mode 100644 hw/icc_bus.c
>  create mode 100644 hw/icc_bus.h
>
> diff --git a/Makefile.target b/Makefile.target
> index 9011f28..5607c6d 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>  obj-i386-y += testdev.o
>  obj-i386-y += acpi.o acpi_piix4.o
> +obj-i386-y += icc_bus.o
>
>  obj-i386-y += pcspk.o i8254.o
>  obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
> diff --git a/hw/apic.c b/hw/apic.c
> index 69d6ac5..00d2297 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -21,9 +21,10 @@
>  #include "ioapic.h"
>  #include "qemu-timer.h"
>  #include "host-utils.h"
> -#include "sysbus.h"
> +#include "icc_bus.h"
>  #include "trace.h"
>  #include "kvm.h"
> +#include "exec-memory.h"
>
>  /* APIC Local Vector Table */
>  #define APIC_LVT_TIMER   0
> @@ -80,7 +81,7 @@
>  typedef struct APICState APICState;
>
>  struct APICState {
> -    SysBusDevice busdev;
> +    ICCBusDevice busdev;
>     MemoryRegion io_memory;
>     void *cpu_env;
>     uint32_t apicbase;
> @@ -1104,9 +1105,20 @@ static const MemoryRegionOps apic_io_ops = {
>     .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> -static int apic_init1(SysBusDevice *dev)
> +/**/
> +int apic_mmio_map(DeviceState *dev, target_phys_addr_t base)
>  {
> -    APICState *s = FROM_SYSBUS(APICState, dev);
> +    APICState *s = DO_UPCAST(APICState, busdev.qdev, dev);
> +
> +    memory_region_add_subregion(get_system_memory(),
> +                                base,
> +                                &s->io_memory);
> +    return 0;
> +}
> +
> +static int apic_init1(ICCBusDevice *dev)
> +{
> +    APICState *s = DO_UPCAST(APICState, busdev, dev);
>     static int last_apic_idx;
>
>     if (last_apic_idx >= MAX_APICS) {
> @@ -1114,7 +1126,6 @@ static int apic_init1(SysBusDevice *dev)
>     }
>     memory_region_init_io(&s->io_memory, &apic_io_ops, s, "apic",
>                           MSI_ADDR_SIZE);
> -    sysbus_init_mmio_region(dev, &s->io_memory);
>
>     s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
>     s->idx = last_apic_idx++;
> @@ -1122,7 +1133,7 @@ static int apic_init1(SysBusDevice *dev)
>     return 0;
>  }
>
> -static SysBusDeviceInfo apic_info = {
> +static ICCBusDeviceInfo apic_info = {
>     .init = apic_init1,
>     .qdev.name = "apic",
>     .qdev.size = sizeof(APICState),
> @@ -1138,7 +1149,7 @@ static SysBusDeviceInfo apic_info = {
>
>  static void apic_register_devices(void)
>  {
> -    sysbus_register_withprop(&apic_info);
> +    iccbus_register_devinfo(&apic_info);
>  }
>
>  device_init(apic_register_devices)
> diff --git a/hw/apic.h b/hw/apic.h
> index c857d52..e2c0af5 100644
> --- a/hw/apic.h
> +++ b/hw/apic.h
> @@ -20,6 +20,7 @@ void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
>  uint8_t cpu_get_apic_tpr(DeviceState *s);
>  void apic_init_reset(DeviceState *s);
>  void apic_sipi(DeviceState *s);
> +int apic_mmio_map(DeviceState *dev, target_phys_addr_t base);
>
>  /* pc.c */
>  int cpu_is_bsp(CPUState *env);
> diff --git a/hw/icc_bus.c b/hw/icc_bus.c
> new file mode 100644
> index 0000000..61a408e
> --- /dev/null
> +++ b/hw/icc_bus.c
> @@ -0,0 +1,91 @@
> +/* icc_bus.c
> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> + *
> + * 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"
> +
> +static CPUS *dummy_cpus;
> +
> +
> +static ICCBusInfo icc_bus_info = {
> +    .qinfo.name = "icc",
> +    .qinfo.size = sizeof(ICCBus),
> +    .qinfo.props = (Property[]) {
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +};
> +
> +
> +
> +static int iccbus_device_init(DeviceState *dev, DeviceInfo *base)
> +{
> +    ICCBusDeviceInfo *info = container_of(base, ICCBusDeviceInfo, qdev);
> +    ICCBusDevice *idev = DO_UPCAST(ICCBusDevice, qdev, dev);
> +
> +    return info->init(idev);
> +}
> +
> +void iccbus_register_devinfo(ICCBusDeviceInfo *info)
> +{
> +    info->qdev.init = iccbus_device_init;
> +    info->qdev.bus_info = &icc_bus_info.qinfo;
> +    assert(info->qdev.size >= sizeof(ICCBusDevice));
> +    qdev_register(&info->qdev);
> +}
> +
> +
> +
> +BusState *get_icc_bus(void)
> +{
> +    return &dummy_cpus->icc_bus->qbus;
> +}
> +
> +static void cpus_reset(DeviceState *d)
> +{
> +}
> +
> +/*Must be called before vcpu's creation*/
> +static int cpus_init(SysBusDevice *dev)
> +{
> +    CPUS *cpus = DO_UPCAST(CPUS, sysdev, dev);
> +    BusState *b = qbus_create(&icc_bus_info.qinfo,
> +                              &dummy_cpus->sysdev.qdev,
> +                              "icc");
> +    if (b == NULL) {
> +        return -1;
> +    }
> +    b->allow_hotplug = 1; /* Yes, we can */
> +    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
> +    dummy_cpus = cpus;
> +    return 0;
> +
> +}
> +
> +
> +static SysBusDeviceInfo cpus_info = {
> +    .init = cpus_init,
> +    .qdev.name = "cpus",
> +    .qdev.size = sizeof(CPUS),
> +    /*.qdev.vmsd = &vmstate_apic,*/
> +    .qdev.reset = cpus_reset,
> +    .qdev.no_user = 1,
> +};
> +
> +static void cpus_register_devices(void)
> +{
> +    sysbus_register_withprop(&cpus_info);
> +}
> +
> +device_init(cpus_register_devices)
> diff --git a/hw/icc_bus.h b/hw/icc_bus.h
> new file mode 100644
> index 0000000..64ac153
> --- /dev/null
> +++ b/hw/icc_bus.h
> @@ -0,0 +1,56 @@
> +/* ICCBus.h
> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> + *
> + * 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 QEMU_ICC_H
> +#define QEMU_ICC_H
> +
> +#include "qdev.h"
> +#include "sysbus.h"
> +
> +typedef struct CPUS CPUS;
> +typedef struct ICCBus ICCBus;
> +typedef struct ICCBusInfo ICCBusInfo;
> +typedef struct ICCBusDevice ICCBusDevice;
> +typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
> +
> +struct CPUS {
> +    SysBusDevice sysdev;
> +    ICCBus *icc_bus;
> +};
> +
> +struct ICCBus {
> +    BusState qbus;
> +};
> +
> +struct ICCBusInfo {
> +    BusInfo qinfo;
> +};
> +struct ICCBusDevice {
> +    DeviceState qdev;
> +};
> +
> +typedef int (*iccbus_initfn)(ICCBusDevice *dev);
> +
> +struct ICCBusDeviceInfo {
> +    DeviceInfo qdev;
> +    iccbus_initfn init;
> +};
> +
> +
> +void iccbus_register_devinfo(ICCBusDeviceInfo *info);
> +BusState *get_icc_bus(void);
> +
> +#endif
> diff --git a/hw/pc.c b/hw/pc.c
> index 6b3662e..78b7826 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -24,6 +24,7 @@
>  #include "hw.h"
>  #include "pc.h"
>  #include "apic.h"
> +#include "icc_bus.h"
>  #include "fdc.h"
>  #include "ide.h"
>  #include "pci.h"
> @@ -875,21 +876,21 @@ DeviceState *cpu_get_current_apic(void)
>  static DeviceState *apic_init(void *env, uint8_t apic_id)
>  {
>     DeviceState *dev;
> -    SysBusDevice *d;
> +    BusState *b;
>     static int apic_mapped;
>
> -    dev = qdev_create(NULL, "apic");
> +    b = get_icc_bus();
> +    dev = qdev_create(b, "apic");
>     qdev_prop_set_uint8(dev, "id", apic_id);
>     qdev_prop_set_ptr(dev, "cpu_env", env);
>     qdev_init_nofail(dev);
> -    d = sysbus_from_qdev(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(d, 0, MSI_ADDR_BASE);
> +        apic_mmio_map(dev, MSI_ADDR_BASE);
>         apic_mapped = 1;
>     }
>
> @@ -955,6 +956,8 @@ CPUState *pc_new_cpu(const char *cpu_model)
>  void pc_cpus_init(const char *cpu_model)
>  {
>     int i;
> +    DeviceState *d = qdev_create(NULL, "cpus");
> +    qdev_init_nofail(d);

This makes the 'cpus' separate device from CPUs which only makes sense
now that LAPICs are in a way shared.

I think the final model should be that PC board should create the CPUs
first, specifying also if ICC should be created (no for machines
without APIC, ISA or i386). Then each CPU should create a per-CPU ICC
bus as needed. Near APIC init, the CPUs are queried if an ICC bus
exists, if yes, an APIC will be attached to this bus (for each CPU).
If the function get_icc_bus() is still needed, it should grab the bus
from CPUState. The APIC base can be changed per CPU.

I'm not sure that a full bus is needed for now, even if it could match
real HW better, since the memory API already provides the separation
needed. Perhaps this would be needed later to make IRQs per-CPU also,
or to put IOAPIC also to the bus?

>
>     /* init CPUs */
>     for(i = 0; i < smp_cpus; i++) {
> --
> 1.7.4.4
>
>
>