[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] Introduce a new bus "ICC" to connect APIC
From: |
liu ping fan |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] Introduce a new bus "ICC" to connect APIC |
Date: |
Tue, 25 Oct 2011 16:55:22 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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?
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;
.......
}
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
> >
> >
> >
Re: [Qemu-devel] [PATCH 1/1] Introduce a new bus "ICC" to connect APIC, Blue Swirl, 2011/10/23
Re: [Qemu-devel] [PATCH 1/1] Introduce a new bus "ICC" to connect APIC,
liu ping fan <=