On Mon, 23 Mar 2015 17:07:29 +0800
Chen Fan <chen.fan.f...@cn.fujitsu.com> wrote:
>
> On 03/23/2015 04:23 PM, Igor Mammedov wrote:
> > On Mon, 23 Mar 2015 13:54:23 +0800
> > Chen Fan <chen.fan.f...@cn.fujitsu.com> wrote:
> >
> >> ICC bus was invented only to provide hotplug capability to
> >> CPU and APIC because at the time being hotplug was available only for
> >> BUS attached devices.
> >>
> >> Now this patch is to drop ICC bus impl, and switch to bus-less
> >> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
> >>
> >> Signed-off-by: Chen Fan <chen.fan.f...@cn.fujitsu.com>
> >> ---
> >> hw/i386/pc.c | 29 +++++++++++------------------
> >> hw/i386/pc_piix.c | 9 +--------
> >> hw/i386/pc_q35.c | 9 +--------
> >> hw/intc/apic.c | 6 +++---
> >> hw/intc/apic_common.c | 11 ++---------
> >> include/hw/i386/apic_internal.h | 5 ++---
> >> include/hw/i386/pc.h | 2 +-
> >> target-i386/cpu.c | 2 --
> >> 8 files changed, 21 insertions(+), 52 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 4b46c29..5d15473 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> > [...]
> >
> >> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char *cpu_model,
> >> DeviceState *icc_bridge)
> >> /* map APIC MMIO area if CPU has APIC */
> >> if (cpu && cpu->apic_state) {
> >> /* XXX: what if the base changes? */
> >> - sysbus_mmio_map_overlap(SYS_BUS_DEVICE(icc_bridge), 0,
> >> - APIC_DEFAULT_ADDRESS, 0x1000);
> >> + apic = APIC_COMMON(cpu->apic_state);
> >> + memory_region_add_subregion_overlap(CPU(cpu)->as->root,
> >> + APIC_DEFAULT_ADDRESS,
> >> + &apic->io_memory,
> >> + 0x1000);
> > Why is it here?
> > Shouldn't it be mapped not once but for each CPU since we are using
> > per CPU address spaces?
> >
> > Split this change out into a separate patch please, with commit message
> > describing what it does.
> Hi Igor,
>
> in your previous mail said, "It might be that kvm_irqchip don't
> need it at all."
> I don't know why kvm_irqchip don't need it ?
That's why it's MIGHT, I'm not sure since I've not look at that code for a
while.
> because I have test that for kernel_irqchip=on, qemu emulator the
> kvm-apic object,
> and sent the MSI to kernel irqchip for pcie devices. it also need map
> the region.
Can we have this test as a patch to qemu/tests? so it would be easier to
discuss it.
>
> Thanks,
> Chen
>
>
> >
> > PS:
> > It should be part of APIC code or at worst case part of CPU's realize.
> >
> > PS2:
> > new cpu tests don't test actual CPU execution, so they can't validate
> > this change. To test it you need to run test in TCG (at least) or
> > TCG + KVM mode, with some guest code that programs and checks APIC
> > of each CPU.
> >
> > PS3:
> > the rest of the patch I'd suggest to merge with 2/2 patch that
> > removes unused icc_bridge code, there isn't point in splitting
> > that from removing icc_bridge from other files.
> >
> > [...]
> >>
> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >> index f01690b..2385e6b 100644
> >> --- a/target-i386/cpu.c
> >> +++ b/target-i386/cpu.c
> >> @@ -42,7 +42,6 @@
> >>
> >> #include "sysemu/sysemu.h"
> >> #include "hw/qdev-properties.h"
> >> -#include "hw/cpu/icc_bus.h"
> >> #ifndef CONFIG_USER_ONLY
> >> #include "hw/xen/xen.h"
> >> #include "hw/i386/apic_internal.h"
> >> @@ -2941,7 +2940,6 @@ static void x86_cpu_common_class_init(ObjectClass
> >> *oc, void *data)
> >>
> >> xcc->parent_realize = dc->realize;
> >> dc->realize = x86_cpu_realizefn;
> >> - dc->bus_type = TYPE_ICC_BUS;
> > that isn't the only place in this file that should be changed.
> >
> > See x86_cpu_apic_create():
> > cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
> >
> > probably it's not right to try get parent bus from bus-less device,
> > qdev_try_create() call should be replaced by object_new()/object_unref()
> > pair.
> >
> >> dc->props = x86_cpu_properties;
> >>
> >> xcc->parent_reset = cc->reset;
> > .
> >
>
