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

[Igor Mammedov]

----- Original Message -----
> From: "Peter Maydell" <address@hidden>
> To: "Igor Mammedov" <address@hidden>
> Cc: address@hidden, "wei liu2" <address@hidden>, address@hidden, "stefano 
> stabellini"
> <address@hidden>, address@hidden, address@hidden, address@hidden, 
> address@hidden,
> address@hidden, address@hidden, "jan kiszka" <address@hidden>, "anthony 
> perard"
> <address@hidden>, address@hidden, address@hidden
> Sent: Wednesday, May 23, 2012 6:44:30 PM
> Subject: Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped 
> initialization into common apic init code
>
> On 23 May 2012 17:39, Igor Mammedov <address@hidden> 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?

Why:
For cpu-hotplug it was suggested to use device_add/del
interface for it. To do so in a generalized way hot-plugged cpu
should follow general QOM object creation sequence, i.e.
  - create new cpu instance
  - set properties
  - realize instance
without creating precedent of special case for cpus in device_add/del
if possible. So goal is to have a self-sufficient cpu object that
doesn't require external hooks to create/initialize it. It looks
possible do so for target-i386 at least.

Some thoughts why mapping should be inside of apic object initfn:

LAPIC is a part of cpu so it's created and initialized by cpu. (not
true for 486 but for later cpus should be)
I've looked in Intel SDM and chapter "10.4.4 Local APIC Status and Location"
says:

"APIC Base field, bits 12 through 35 ⎯ Specifies the base address of the APIC
registers. ---cut--- Following a power-up or reset, the field is set to FEE0 
0000H."

it suggests that apic base is mapped right after cpu power-on and power-on 
could be
roughly mapped to object_new(),set_prop,realize sequence for cpu. So when cpu
creates child object "apic", then mapping of default apic base inside apic/cpu
objects looks more appropriate. (depending on cpu model, but we could chose a 
more
convenient case to implement).

Thanks,
  Igor