[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] cpu/apic: drop icc bus/bridge/
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] cpu/apic: drop icc bus/bridge/ |
Date: |
Thu, 19 Mar 2015 11:28:17 +0100 |
On Thu, 19 Mar 2015 18:08:32 +0800
Chen Fan <address@hidden> wrote:
>
> On 03/18/2015 12:29 AM, Igor Mammedov wrote:
> > On Tue, 17 Mar 2015 17:08:30 +0800
> > Chen Fan <address@hidden> 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 <address@hidden>
> >> ---
> > [...]
> >
> >> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> >> index 0858b45..020219e 100644
> >> --- a/hw/intc/apic_common.c
> >> +++ b/hw/intc/apic_common.c
> >> @@ -27,6 +27,8 @@
> >> static int apic_irq_delivered;
> >> bool apic_report_tpr_access;
> >>
> >> +static MemoryRegion *apic_container;
> >> +
> >> void cpu_set_apic_base(DeviceState *dev, uint64_t val)
> >> {
> >> trace_cpu_set_apic_base(val);
> >> @@ -314,8 +316,11 @@ static void apic_common_realize(DeviceState *dev,
> >> Error **errp)
> >> info = APIC_COMMON_GET_CLASS(s);
> >> info->realize(dev, errp);
> >> if (!mmio_registered) {
> >> - ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
> >> - memory_region_add_subregion(b->apic_address_space, 0,
> >> &s->io_memory);
> >> + memory_region_add_subregion(apic_container, 0, &s->io_memory);
> >> + memory_region_add_subregion_overlap(CPU(s->cpu)->as->root,
> >> + APIC_DEFAULT_ADDRESS,
> >> + apic_container,
> >> + 0x1000);
> >> mmio_registered = true;
> >> }
> > Provided per CPU address spaces are used,
> > why do you need static apic_container and mmio_registered thing?
> > Shouldn't it be possible to register &s->io_memory of each APIC
> > in CPU's own AS and avoid using globals?
> it seems only need to do
> memory_region_add_subregion_overlap(CPU(s->cpu)->as->root,
> APIC_DEFAULT_ADDRESS,
> &io_memory,
> 0x1000);
>
> at here, right?
yep, something along theses lines.
I also do not remember if we really need it for every APIC device
that inherits from APIC_COMMON.It might be that kvm_iqrchip don't need it at
all.
You could experiment with -machine kernel_irqchip=on/off options
to switch between QEMU emulated APIC and kernel_irqchip.
Also this change is independent of dropping icc_bridge,
so please split it out into a separate patch.
>
> Thanks,
> Chen
> >
> >>
> >> @@ -431,15 +436,23 @@ static Property apic_properties_common[] = {
> >> DEFINE_PROP_END_OF_LIST(),
> >> };
> >>
> >> +static void apic_common_initfn(Object *obj)
> >> +{
> >> + if (!apic_container) {
> >> + apic_container = g_malloc(sizeof(*apic_container));
> >> + memory_region_init(apic_container, obj, "apic-container",
> >> + APIC_SPACE_SIZE);
> >> + }
> >> +}
> >> +
> >
> >
> > .
> >
>