----- Original Message ----- > From: "Peter Maydell" <peter.mayd...@linaro.org> > To: "Igor Mammedov" <imamm...@redhat.com> > Cc: aligu...@us.ibm.com, "wei liu2" <wei.l...@citrix.com>, > ehabk...@redhat.com, "stefano stabellini" > <stefano.stabell...@eu.citrix.com>, s...@weilnetz.de, mtosa...@redhat.com, > qemu-devel@nongnu.org, ag...@suse.de, > blauwir...@gmail.com, a...@redhat.com, "jan kiszka" <jan.kis...@siemens.com>, > "anthony perard" > <anthony.per...@citrix.com>, pbonz...@redhat.com, afaer...@suse.de > 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 <imamm...@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?
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