Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2012-05-23 13:44, Peter Maydell wrote:
> On 23 May 2012 17:39, Igor Mammedov <imammedo@redhat.com> wrote:
>> @@ -295,6 +297,15 @@ static int apic_init_common(SysBusDevice *dev)
>>
>>     sysbus_init_mmio(dev, &s->io_memory);
>>
>> +    /* XXX: mapping more APICs at the same memory location */
>> +    if (apic_mapped == 0) {
>> +        /* NOTE: the APIC is directly connected to the CPU - it is not
>> +           on the global memory bus. */
>> +        /* XXX: what if the base changes? */
>> +        sysbus_mmio_map(sysbus_from_qdev(&s->busdev.qdev), 0, MSI_ADDR_BASE);
>> +        apic_mapped = 1;
>> +    }
>> +
>>     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK) {
>>         vapic = sysbus_create_simple("kvmvapic", -1, NULL);
>>     }
> 
> This looks wrong -- sysbus device init functions shouldn't
> be mapping MMIO regions themselves, in general. They should
> expose MMIO regions to be mapped by whichever device or board
> model creates them. Which is what the code before this patch
> was doing -- why do you want to move this code?

I fact, the CPU normally decides about where the APIC is mapped,
specifically when it is remapped via the MSR (which QEMU cannot support
yet). Well, unless we are talking about an external APIC and a 486 CPU.
Then the board decides. But I guess we could live with ignoring that
corner case and stick the mapping setup into the CPU initialization code.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux