Re: [Qemu-devel] [PULL 2/5] apic: map APIC's MMIO region at each CPU's address space

[Eduardo Habkost]

On Fri, May 29, 2015 at 09:27:19PM +0200, Paolo Bonzini wrote:
> On 29/05/2015 20:04, Eduardo Habkost wrote:
> >      static int apic_no;
> > -    static bool mmio_registered;
> > +    CPUState *cpu = CPU(s->cpu);
> > +    MemoryRegion *root;
> >  
> >      if (apic_no >= MAX_APICS) {
> >          error_setg(errp, "%s initialization failed.",
> > @@ -307,11 +308,12 @@ 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);
> > -        mmio_registered = true;
> > -    }
> > +
> > +    root = address_space_root_memory_region(cpu->as);
> 
> I think just using cpu->as->root is okay.
> 
> > +    memory_region_add_subregion_overlap(root,
> > +                                        s->apicbase & 
> > MSR_IA32_APICBASE_BASE,
> > +                                        &s->io_memory,
> > +                                        0x1000);
> 
> I think this patch is incorrect, because you do not install a separate
> address space for each CPU.  Also, the CPU address space is only used
> with TCG so it should be guarded by "if (tcg_enabled())".

I am removing this from the queue. I guess I should have waited for some
feedback from the memory API maintainer before committing.

-- 
Eduardo