* Re: [Qemu-devel] [PATCH v3] apic: bump emulated lapic version to 0x14 on pc machines >= 2.1 [not found] ` <536152A0.6010104@suse.de> @ 2014-05-01 17:22 ` Gabriel L. Somlo 2014-05-01 18:52 ` Alexander Graf 0 siblings, 1 reply; 6+ messages in thread From: Gabriel L. Somlo @ 2014-05-01 17:22 UTC (permalink / raw) To: Alexander Graf; +Cc: pbonzini, qemu-devel, afaerber, mst On Wed, Apr 30, 2014 at 09:44:32PM +0200, Alexander Graf wrote: > >diff --git a/hw/intc/apic.c b/hw/intc/apic.c > >index 2f40cba..4480bc4 100644 > >--- a/hw/intc/apic.c > >+++ b/hw/intc/apic.c > >@@ -32,6 +32,8 @@ > > #define SYNC_TO_VAPIC 0x2 > > #define SYNC_ISR_IRR_TO_VAPIC 0x4 > >+uint8_t apic_version = 0x14; > > Is there any way to make this a qdev/qom device property rather than > a global? If there is, and anyone with a better understanding of qom/qdev has an example I could follow, that would be much appreciated! As far as I could comprehend it since I started looking at it last night, the apic_class_init() functions run before pci_init() in pc_[q35|piix].c knows which machine type we have, but the apic_realize() functions (which appear to be the actual apic "constructors") run after that. The obvious alternative to having one global apic version would be to add a field to APICCommonClass or APICCommonState, and then somehow modify the default (set in apic_class_init()) from pci_init() according to the machine version; After that, each apic may refer to its private data member "version" when needed. So, is qom/qdev basically boiling down to a set of macros that can translate something like "qom_set_property(apic_instance, version, 0x14);" into "apic_instance.version = 0x14;" ? I'll keep digging, but in case anyone more experienced can tell us why this WON'T work, I'd appreciate being put out of my misery and allowed to go back to using the global :) :) Thanks, --Gabriel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] apic: bump emulated lapic version to 0x14 on pc machines >= 2.1 2014-05-01 17:22 ` [Qemu-devel] [PATCH v3] apic: bump emulated lapic version to 0x14 on pc machines >= 2.1 Gabriel L. Somlo @ 2014-05-01 18:52 ` Alexander Graf 2014-05-01 21:43 ` Don Slutz 0 siblings, 1 reply; 6+ messages in thread From: Alexander Graf @ 2014-05-01 18:52 UTC (permalink / raw) To: Gabriel L. Somlo; +Cc: pbonzini, qemu-devel, afaerber, mst On 01.05.14 19:22, Gabriel L. Somlo wrote: > On Wed, Apr 30, 2014 at 09:44:32PM +0200, Alexander Graf wrote: >>> diff --git a/hw/intc/apic.c b/hw/intc/apic.c >>> index 2f40cba..4480bc4 100644 >>> --- a/hw/intc/apic.c >>> +++ b/hw/intc/apic.c >>> @@ -32,6 +32,8 @@ >>> #define SYNC_TO_VAPIC 0x2 >>> #define SYNC_ISR_IRR_TO_VAPIC 0x4 >>> +uint8_t apic_version = 0x14; >> Is there any way to make this a qdev/qom device property rather than >> a global? > If there is, and anyone with a better understanding of qom/qdev > has an example I could follow, that would be much appreciated! > > As far as I could comprehend it since I started looking at it last > night, the apic_class_init() functions run before pci_init() in > pc_[q35|piix].c knows which machine type we have, but the > apic_realize() functions (which appear to be the actual apic > "constructors") run after that. > > The obvious alternative to having one global apic version would be > to add a field to APICCommonClass or APICCommonState, and then somehow > modify the default (set in apic_class_init()) from pci_init() > according to the machine version; After that, each apic may refer to > its private data member "version" when needed. > > So, is qom/qdev basically boiling down to a set of macros that can > translate something like "qom_set_property(apic_instance, version, 0x14);" > into "apic_instance.version = 0x14;" ? 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 :). Alex ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] apic: bump emulated lapic version to 0x14 on pc machines >= 2.1 2014-05-01 18:52 ` Alexander Graf @ 2014-05-01 21:43 ` Don Slutz 2014-05-02 14:23 ` Gabriel L. Somlo 0 siblings, 1 reply; 6+ messages in thread From: Don Slutz @ 2014-05-01 21:43 UTC (permalink / raw) To: Alexander Graf, Gabriel L. Somlo; +Cc: pbonzini, mst, qemu-devel, afaerber On 05/01/14 14:52, Alexander Graf wrote: > > On 01.05.14 19:22, Gabriel L. Somlo wrote: >> On Wed, Apr 30, 2014 at 09:44:32PM +0200, Alexander Graf wrote: >>>> diff --git a/hw/intc/apic.c b/hw/intc/apic.c >>>> index 2f40cba..4480bc4 100644 >>>> --- a/hw/intc/apic.c >>>> +++ b/hw/intc/apic.c >>>> @@ -32,6 +32,8 @@ >>>> #define SYNC_TO_VAPIC 0x2 >>>> #define SYNC_ISR_IRR_TO_VAPIC 0x4 >>>> +uint8_t apic_version = 0x14; >>> Is there any way to make this a qdev/qom device property rather than >>> a global? >> If there is, and anyone with a better understanding of qom/qdev >> has an example I could follow, that would be much appreciated! >> >> As far as I could comprehend it since I started looking at it last >> night, the apic_class_init() functions run before pci_init() in >> pc_[q35|piix].c knows which machine type we have, but the >> apic_realize() functions (which appear to be the actual apic >> "constructors") run after that. >> >> The obvious alternative to having one global apic version would be >> to add a field to APICCommonClass or APICCommonState, and then somehow >> modify the default (set in apic_class_init()) from pci_init() >> according to the machine version; After that, each apic may refer to >> its private data member "version" when needed. >> >> So, is qom/qdev basically boiling down to a set of macros that can >> translate something like "qom_set_property(apic_instance, version, 0x14);" >> into "apic_instance.version = 0x14;" ? > > 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 (which is a reply to a change I am working on that is in the same place) Hope this helps. -Don Slutz > Alex > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] apic: bump emulated lapic version to 0x14 on pc machines >= 2.1 2014-05-01 21:43 ` Don Slutz @ 2014-05-02 14:23 ` Gabriel L. Somlo 2014-05-02 14:26 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Gabriel L. Somlo @ 2014-05-02 14:23 UTC (permalink / raw) To: Don Slutz; +Cc: pbonzini, mst, Alexander Graf, afaerber, qemu-devel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] apic: bump emulated lapic version to 0x14 on pc machines >= 2.1 2014-05-02 14:23 ` Gabriel L. Somlo @ 2014-05-02 14:26 ` Paolo Bonzini 2014-05-02 17:13 ` Gabriel L. Somlo 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2014-05-02 14:26 UTC (permalink / raw) To: Gabriel L. Somlo, Don Slutz; +Cc: mst, Alexander Graf, afaerber, qemu-devel Il 02/05/2014 16:23, Gabriel L. Somlo ha scritto: > > 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 ? :) Yes, there is. :) It's compat_props. You can set 0x14 in the default, and 0x11 for 2.0 and earlier. Paolo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] apic: bump emulated lapic version to 0x14 on pc machines >= 2.1 2014-05-02 14:26 ` Paolo Bonzini @ 2014-05-02 17:13 ` Gabriel L. Somlo 0 siblings, 0 replies; 6+ messages in thread From: Gabriel L. Somlo @ 2014-05-02 17:13 UTC (permalink / raw) To: Paolo Bonzini; +Cc: afaerber, mst, Alexander Graf, Don Slutz, qemu-devel On Fri, May 02, 2014 at 04:26:41PM +0200, Paolo Bonzini wrote: > Il 02/05/2014 16:23, Gabriel L. Somlo ha scritto: > > > >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 ? :) > > Yes, there is. :) It's compat_props. You can set 0x14 in the > default, and 0x11 for 2.0 and earlier. Aaah, thank you !!! :) So, the patch below should do the trick. One more question before I generate a formal v4: would you prefer I split it into two parts, one which adds blank PC[_Q35]_COMPAT_2_0 compat_props to pc_piix/pc_q35 (and which has to go in after http://lists.nongnu.org/archive/html/qemu-devel/2014-04/msg04728.html, and a separate one adding DEFINE_PROP_UINT32("version") to the apic and an actual payload for PC_COMPAT_2_0 ? Or leave it as one single patch ? Thanks, --Gabriel diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 5b3594b..7c672d7 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -415,6 +415,10 @@ static QEMUMachine pc_i440fx_machine_v2_0 = { PC_I440FX_2_0_MACHINE_OPTIONS, .name = "pc-i440fx-2.0", .init = pc_init_pci_2_0, + .compat_props = (GlobalProperty[]) { + PC_COMPAT_2_0, + { /* end of list */ } + }, }; #define PC_I440FX_1_7_MACHINE_OPTIONS PC_I440FX_MACHINE_OPTIONS diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 5b48231..1fe2213 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -328,6 +328,10 @@ static QEMUMachine pc_q35_machine_v2_0 = { PC_Q35_2_0_MACHINE_OPTIONS, .name = "pc-q35-2.0", .init = pc_q35_init_2_0, + .compat_props = (GlobalProperty[]) { + PC_Q35_COMPAT_2_0, + { /* end of list */ } + }, }; #define PC_Q35_1_7_MACHINE_OPTIONS PC_Q35_MACHINE_OPTIONS 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); 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/include/hw/i386/pc.h b/include/hw/i386/pc.h index 9f26e14..32a7687 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -242,8 +242,12 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t); int e820_get_num_entries(void); bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); +#define PC_Q35_COMPAT_2_0 \ + PC_COMPAT_2_0 + #define PC_Q35_COMPAT_1_7 \ PC_COMPAT_1_7, \ + PC_Q35_COMPAT_2_0, \ {\ .driver = "hpet",\ .property = HPET_INTCAP,\ @@ -262,7 +266,15 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); PC_COMPAT_1_4, \ PC_Q35_COMPAT_1_5 +#define PC_COMPAT_2_0 \ + {\ + .driver = "apic",\ + .property = "version",\ + .value = stringify(0x11),\ + } + #define PC_COMPAT_1_7 \ + PC_COMPAT_2_0, \ {\ .driver = TYPE_USB_DEVICE,\ .property = "msos-desc",\ ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-02 17:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20140430194108.GB16023@ERROL.INI.CMU.EDU>
[not found] ` <536152A0.6010104@suse.de>
2014-05-01 17:22 ` [Qemu-devel] [PATCH v3] apic: bump emulated lapic version to 0x14 on pc machines >= 2.1 Gabriel L. Somlo
2014-05-01 18:52 ` Alexander Graf
2014-05-01 21:43 ` Don Slutz
2014-05-02 14:23 ` Gabriel L. Somlo
2014-05-02 14:26 ` Paolo Bonzini
2014-05-02 17:13 ` Gabriel L. Somlo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).