Re: [Qemu-devel] [RESEND PATCH v9 1/4] apic: map APIC's MMIO region at each CPU's address space

[Eduardo Habkost <ehabkost@redhat.com>]

On Wed, Aug 26, 2015 at 11:27:08AM -0400, Paolo Bonzini wrote:
[...]
> > >> > +    if (tcg_enabled()) {
> > >> > +        memory_region_add_subregion_overlap(cpu->cpu_as_root,
> > >> > +                                            apic->apicbase &
> > >> > +                                            MSR_IA32_APICBASE_BASE,
> > >> > +                                            &apic->io_memory,
> > >> > +                                            0x1000);
> > >>
> > >> Why exactly is this necessary? If this is necessary, why don't we need
> > >> to do this for non-TCG accelerators?
> > >
> > > At least KVM and qtest do not support per-CPU address spaces.
> > 
> > Right, but given this restriction why can't we also do whatever
> > we need to work without the per-CPU address spaces with TCG?
> > 
> 
> Because the emulation quality is indeed a bit better with the per-CPU
> address spaces; you could move each APIC's base address independent of
> the others. However, this is not a feature that is actually used by
> anything in practice, so I doubt anyone cares about TCG implementing
> it correctly.

Do we need additional changes in TCG to implement it correctly, or is
this going to work out of the box as soon as we apply this series?

If it's the latter, the patch makes sense to me (but please add a
comment to the code explaining why). If it's the former, I don't see the
point of making the code more complex before that feature is actually
implemented by TCG.

Also, we could make the logic simpler if we just check if
cpu->cpu_as_root is set, e.g.:

    /* Use per-CPU address space if available (TCG supports it, KVM
     * doesn't). This allows the APIC base address of each CPU
     * to be moved independently.
     */
    memory_region_add_subregion_overlap(cpu->cpu_as_root ?:
                                        get_system_memory(),
                                        apic->apicbase &
                                        MSR_IA32_APICBASE_BASE,
                                        &apic->io_memory,
                                        0x1000);

-- 
Eduardo