Re: [Qemu-devel] [PATCH v3] apic: bump emulated lapic version to 0x14 on pc machines >= 2.1

[Gabriel L. Somlo]

On Thu, May 01, 2014 at 05:43:23PM -0400, Don Slutz wrote:
> On 05/01/14 14:52, Alexander Graf wrote:
> >With qdev we basically had an array of constructor parameters in the qdev 
> >definition. You could set these from the outside between create and init, 
> >basically:
> >
> >  dev = dev_create()
> >  set_prop(dev, "foo", bar);
> >  dev_init(dev)
> >
> >which semantically translated to
> >
> >  dev = new dev(foo = bar);
> >
> >The way to do this with QOM is similar, but I keep forgetting the details. 
> >I'm sure you'll easily find out :).
> >
> >
> 
> It looks like
> 
> http://permalink.gmane.org/gmane.comp.emulators.qemu/268337

So, after a bit more digging, it appears qdev would be the more
appropriate fit:

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 7ecce2d..9cb418f 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -380,6 +380,7 @@ static const VMStateDescription vmstate_apic_common = {
 
 static Property apic_properties_common[] = {
     DEFINE_PROP_UINT8("id", APICCommonState, id, -1),
+    DEFINE_PROP_UINT32("version", APICCommonState, version, 0x14),
     DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT,
                     true),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 70542a6..0ac1462 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -98,6 +98,7 @@ struct APICCommonState {
     X86CPU *cpu;
     uint32_t apicbase;
     uint8_t id;
+    uint32_t version;
     uint8_t arb_id;
     uint8_t tpr;
     uint32_t spurious_vec;
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 2f40cba..ef19e55 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -675,7 +675,7 @@ static uint32_t apic_mem_readl(void *opaque, hwaddr addr)
         val = s->id << 24;
         break;
     case 0x03: /* version */
-        val = 0x11 | ((APIC_LVT_NB - 1) << 16); /* version 0x11 */
+        val = s->version | ((APIC_LVT_NB - 1) << 16);
         break;
     case 0x08:
         apic_sync_vapic(s, SYNC_FROM_VAPIC);

However, at this point we hit a (IMHO, huge) snag (using pc_q35.c as
an example, pc_piix.c would have the exact same issue).

Modifying the "version" property on an apic using
object_property_set_uint32() would require its "APICCommonState *",
which doesn't exist before pc_q35_init() calls pc_cpus_init(), and
results in an error (modifying property on already realized apic) after
pc_cpus_init() completes.

I could pass the "bool soft_apic_compat" value from pc_q35_init()
all the way down into the bowels of target-i386/cpu.c, but many of
those calls are also made from cpu hotplug, and things get complicated
really fast. More complicated than just setting a global apic version...

I'm not sure the timing of the various constructors works out in a way
that would allow us to avoid a global variable :(

Did I miss anything ? Is there a way to override the default for all
apics, which I set in DEFINE_PROP_UINT32, *before* anything gets
initialized/realized/constructed/whatever ? :)

Thanks,
--Gabriel