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);