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

[Blue Swirl]

On Thu, Oct 27, 2011 at 08:41, liu ping fan <address@hidden> wrote:
> Hi,
>
> I want to rework on it according to your comments. Before that, just want to 
> make clear that I understand your meanings exactly :)
>
> According to the previous discussion, I will model the system according the 
> rule -- if there is APIC in the system (including UP and MP), ICC bus will be 
> created, otherwise no.
>
> But there is a special case in UP scene,that is, if we make 8259a connect 
> directly to APIC without using IOAPIC, as showed by Figure 3-3 in intel's 
> "MultiProcessor Specification", I think the rule can also be suitable.

I think this is handled for some part by pic_irq_request() in pc.c.

> So in board level initialization--pc1_init(), I will check _cpuid_features_ & 
> CPUID_APIC to judge whether to create ICC or not.
>
> Any objection?

OK for me.

> Thanks and regards,
> ping fan
>
>
> On Tue, Oct 25, 2011 at 08:24:21PM +0000, Blue Swirl wrote:
>> On Tue, Oct 25, 2011 at 08:55, liu ping fan <address@hidden> wrote:
>> > On Sun, Oct 23, 2011 at 12:40:08PM +0000, Blue Swirl wrote:
>> >> 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.
>> >>
>> > So I will do like this in pc_init1(),
>> >    if (need_icc_bus(global_cpu_model))
>> >        create_icc_bus();
>> >
>> >    pc_cpus_init(cpu_model);
>> > Right?
>>
>> Sorry, my model was not correct. So one ICC should be created at board
>> level if any APICs exist, each LAPIC should connect to it and later
>> also IOAPIC.
>>
>> > But what about the parent bus of APICState? Do we need two models for 
>> > APICState
>> > static SysBusDeviceInfo apic_info and static ICCBusDeviceInfo 
>> > apic_icc_info?
>> > And what about APICState,just insert another bus stub, or create a new one?
>> >    struct APICState {
>> >        SysBusDevice busdev;
>> >       ------------------> ICCBusDevice iccdev;
>> >        MemoryRegion io_memory;
>> >        .......
>> >    }
>>
>> Looks OK.
>>
>> > Thanks,
>> > ping fan
>> >
>> >> 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
>> >> >
>> >> >
>> >> >
>> >
>