Re: [Qemu-devel] [PATCH 1/2] cpu/apic: drop icc bus/bridge/

[Chen Fan]

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?

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);
> > +    }
> > +}
> > +
>
>
> .