On Tue, Oct 4, 2011 at 8:58 PM, Anthony Liguori <anth...@codemonkey.ws>wrote:
> On 10/04/2011 06:13 AM, pingf...@linux.vnet.com wrote:
>
>> From: Liu Ping Fan<pingf...@linux.vnet.ibm.**com<pingf...@linux.vnet.ibm.com>
>> >
>>
>> Separate apic from qbus to icc bus which supports hotplug feature.
>> And make the creation of apic as part of cpu initialization, so
>> apic's state has been ready, before setting kvm_apic.
>>
>> Signed-off-by: Liu Ping
>> Fan<pingf...@linux.vnet.ibm.**com<pingf...@linux.vnet.ibm.com>
>> >
>> ---
>> Makefile.target | 1 +
>> hw/apic.c | 7 ++++-
>> hw/apic.h | 1 +
>> hw/icc_bus.c | 62 ++++++++++++++++++++++++++++++**
>> ++++++++++++++++++++
>> hw/icc_bus.h | 24 +++++++++++++++++++
>> hw/pc.c | 11 ++++-----
>> target-i386/cpu.h | 1 +
>> target-i386/helper.c | 7 +++++-
>> target-i386/kvm.c | 1 -
>> 9 files changed, 105 insertions(+), 10 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..95a1664 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -24,6 +24,7 @@
>> #include "sysbus.h"
>> #include "trace.h"
>> #include "kvm.h"
>> +#include "icc_bus.h"
>>
>> /* APIC Local Vector Table */
>> #define APIC_LVT_TIMER 0
>> @@ -304,11 +305,13 @@ void cpu_set_apic_base(DeviceState *d, uint64_t val)
>>
>> if (!s)
>> return;
>> +
>> if (kvm_enabled()&& kvm_irqchip_in_kernel())
>>
>> s->apicbase = val;
>> else
>> s->apicbase = (val& 0xfffff000) |
>> (s->apicbase& (MSR_IA32_APICBASE_BSP |
>> MSR_IA32_APICBASE_ENABLE));
>>
>> +
>> /* if disabled, cannot be enabled again */
>> if (!(val& MSR_IA32_APICBASE_ENABLE)) {
>> s->apicbase&= ~MSR_IA32_APICBASE_ENABLE;
>>
>> @@ -1075,7 +1078,7 @@ static const VMStateDescription vmstate_apic = {
>> }
>> };
>>
>
> Please don't introduce extra whitespace in unrelated code.
>
> Adopt,
>
>
>> -static void apic_reset(DeviceState *d)
>> +void apic_reset(DeviceState *d)
>> {
>> APICState *s = DO_UPCAST(APICState, busdev.qdev, d);
>> int bsp;
>> @@ -1138,7 +1141,7 @@ static SysBusDeviceInfo apic_info = {
>>
>> static void apic_register_devices(void)
>> {
>> - sysbus_register_withprop(&**apic_info);
>> + iccbus_register(&apic_info);
>> }
>>
>> device_init(apic_register_**devices)
>> diff --git a/hw/apic.h b/hw/apic.h
>> index c857d52..e258efa 100644
>> --- a/hw/apic.h
>> +++ b/hw/apic.h
>> @@ -24,5 +24,6 @@ void apic_sipi(DeviceState *s);
>> /* pc.c */
>> int cpu_is_bsp(CPUState *env);
>> DeviceState *cpu_get_current_apic(void);
>> +void apic_reset(DeviceState *d);
>>
>> #endif
>> diff --git a/hw/icc_bus.c b/hw/icc_bus.c
>> new file mode 100644
>> index 0000000..360ca2a
>> --- /dev/null
>> +++ b/hw/icc_bus.c
>> @@ -0,0 +1,62 @@
>> +/*
>> +*/
>>
>
> New files need to include copyright/licenses.
>
> Adopt,
+#define ICC_BUS_PLUG
>> +#ifdef ICC_BUS_PLUG
>>
>
> Drop these guards here and at the end of the file. We conditionally build
> files by having an:
>
> obj-$(CONFIG_ICC_BUS) += icc_bus.o
>
> In the Makefile.
>
> Adopt,
> +#include "icc_bus.h"
>>
>
> +
>> +
>> +
>> +struct icc_bus_info icc_info = {
>> + .qinfo.name = "icc",
>> + .qinfo.size = sizeof(struct icc_bus),
>> + .qinfo.props = (Property[]) {
>> + DEFINE_PROP_END_OF_LIST(),
>> + }
>> +
>> +};
>>
>
> Structure name is not following Coding Style.
>
I will fix it.
+
>> +static const VMStateDescription vmstate_icc_bus = {
>> + .name = "icc_bus",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .minimum_version_id_old = 1,
>> + .pre_save = NULL,
>> + .post_load = NULL,
>> +};
>> +
>> +struct icc_bus *g_iccbus;
>> +
>> +struct icc_bus *icc_init_bus(DeviceState *parent, const char *name)
>> +{
>> + struct icc_bus *bus;
>> +
>> + bus = FROM_QBUS(icc_bus, qbus_create(&icc_info.qinfo, parent, name));
>> + bus->qbus.allow_hotplug = 1; /* Yes, we can */
>> + bus->qbus.name = "icc";
>> + vmstate_register(NULL, -1,&vmstate_icc_bus, bus);
>>
>> + g_iccbus = bus;
>> + return bus;
>> +}
>> +
>> +
>> +
>> +
>> +
>> +static int iccbus_device_init(DeviceState *dev, DeviceInfo *base)
>> +{
>> + SysBusDeviceInfo *info = container_of(base, SysBusDeviceInfo, qdev);
>> +
>> + return info->init(sysbus_from_qdev(**dev));
>> +}
>> +
>> +void iccbus_register(**SysBusDeviceInfo *info)
>> +{
>> + info->qdev.init = iccbus_device_init;
>> + info->qdev.bus_info =&icc_info.qinfo;
>> +
>> + assert(info->qdev.size>= sizeof(SysBusDevice));
>> + qdev_register(&info->qdev);
>> +}
>> +
>>
>
> It's not obvious to me why you need the g_iccbus variable.
>
> As ICC bus is used by LAPIC and IOAPIC, so I want to export it as a system
wide variable.
> +#endif
>> diff --git a/hw/icc_bus.h b/hw/icc_bus.h
>> new file mode 100644
>> index 0000000..94d9242
>> --- /dev/null
>> +++ b/hw/icc_bus.h
>> @@ -0,0 +1,24 @@
>> +#ifndef QEMU_ICC_H
>> +#define QEMU_ICC_H
>>
>
>
> Needs copyright and a blurb explaining what this file is and what the ICC
> bus is.
>
> Same comments re: whitespace below.
>
> Regards,
>
> Anthony Liguori
>
>
> +#include "qdev.h"
>> +#include "sysbus.h"
>> +
>> +typedef struct icc_bus icc_bus;
>> +typedef struct icc_bus_info icc_bus_info;
>> +
>> +
>> +struct icc_bus {
>> + BusState qbus;
>> +};
>> +
>> +struct icc_bus_info {
>> + BusInfo qinfo;
>> +};
>> +
>> +extern struct icc_bus_info icc_info;
>> +extern struct icc_bus *g_iccbus;
>> +extern struct icc_bus *icc_init_bus(DeviceState *parent, const char
>> *name);
>> +extern void iccbus_register(**SysBusDeviceInfo *info);
>>
>
> Functions don't need an 'extern' modifier. I don't think icc_info needs to
> be exported either.
>
> Instead of exporting g_iccbus, it would be better to have a get_icc_bus()
> method that returned the global iccbus. That way we can more easily hook
> accesses to the bus.
>
> Adopt,
Thanks and regards,
ping fan
+#endif
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 6b3662e..10371d8 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"
>> @@ -91,6 +92,7 @@ struct e820_table {
>> static struct e820_table e820_table;
>> struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>>
>> +
>> void isa_irq_handler(void *opaque, int n, int level)
>> {
>> IsaIrqState *isa = (IsaIrqState *)opaque;
>> @@ -872,13 +874,13 @@ DeviceState *cpu_get_current_apic(void)
>> }
>> }
>>
>> -static DeviceState *apic_init(void *env, uint8_t apic_id)
>> +DeviceState *apic_init(void *env, uint8_t apic_id)
>> {
>> DeviceState *dev;
>> SysBusDevice *d;
>> static int apic_mapped;
>>
>> - dev = qdev_create(NULL, "apic");
>> + dev = qdev_create(&g_iccbus->qbus, "apic");
>> qdev_prop_set_uint8(dev, "id", apic_id);
>> qdev_prop_set_ptr(dev, "cpu_env", env);
>> qdev_init_nofail(dev);
>> @@ -943,10 +945,6 @@ CPUState *pc_new_cpu(const char *cpu_model)
>> fprintf(stderr, "Unable to find x86 CPU definition\n");
>> exit(1);
>> }
>> - if ((env->cpuid_features& CPUID_APIC) || smp_cpus> 1) {
>>
>> - env->cpuid_apic_id = env->cpu_index;
>> - env->apic_state = apic_init(env, env->cpuid_apic_id);
>> - }
>> qemu_register_reset(pc_cpu_**reset, env);
>> pc_cpu_reset(env);
>> return env;
>> @@ -956,6 +954,7 @@ void pc_cpus_init(const char *cpu_model)
>> {
>> int i;
>>
>> + icc_init_bus(NULL, "icc");
>> /* init CPUs */
>> for(i = 0; i< smp_cpus; i++) {
>> pc_new_cpu(cpu_model);
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index 2a071f2..0160c55 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -1062,4 +1062,5 @@ void svm_check_intercept(CPUState *env1, uint32_t
>> type);
>>
>> uint32_t cpu_cc_compute_all(CPUState *env1, int op);
>>
>> +extern DeviceState *apic_init(void *env, uint8_t apic_id);
>> #endif /* CPU_I386_H */
>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>> index 5df40d4..551a8a2 100644
>> --- a/target-i386/helper.c
>> +++ b/target-i386/helper.c
>> @@ -30,6 +30,7 @@
>> #include "monitor.h"
>> #endif
>>
>> +
>> //#define DEBUG_MMU
>>
>> /* NOTE: must be called outside the CPU execute loop */
>> @@ -1257,7 +1258,11 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
>> return NULL;
>> }
>> mce_init(env);
>> -
>> + if ((env->cpuid_features& CPUID_APIC)
>>
>> + || smp_cpus> 1) {
>> + env->cpuid_apic_id = env->cpu_index;
>> + env->apic_state = apic_init(env, env->cpuid_apic_id);
>> + }
>> qemu_init_vcpu(env);
>>
>> return env;
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index 571d792..407dba6 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -880,7 +880,6 @@ static int kvm_put_sregs(CPUState *env)
>>
>> sregs.cr8 = cpu_get_apic_tpr(env->apic_**state);
>> sregs.apic_base = cpu_get_apic_base(env->apic_**state);
>> -
>> sregs.efer = env->efer;
>>
>> return kvm_vcpu_ioctl(env, KVM_SET_SREGS,&sregs);
>>
>
>
>
