| Message ID | 20140430194108.GB16023@ERROL.INI.CMU.EDU |
|---|---|
| State | New |
| Headers | show |
On 30.04.14 21:41, Gabriel L. Somlo wrote: > Some guests (e.g. 0S X) more or less arbitrarily insist on a > minimum lapic version of 0x14 in order to successfully boot. > This patch bumps the emulated apic version on piix and q35 > machine types >= 2.1. > > Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu> > --- > > On Wed, Apr 30, 2014 at 09:20:18PM +0200, Paolo Bonzini wrote: >> I think it's preferrable to have apic_version be a device property >> with a default of 0x14, and use the compat_props to make it 0x11 on >> older machine types. > You mean, flip everything around, like below ? > > Thanks, > Gabriel > > hw/i386/pc_piix.c | 7 +++++++ > hw/i386/pc_q35.c | 7 +++++++ > hw/intc/apic.c | 4 +++- > include/hw/i386/apic.h | 2 ++ > 4 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 5b3594b..d90ce5a 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -29,6 +29,7 @@ > #include "hw/i386/pc.h" > #include "hw/i386/apic.h" > #include "hw/i386/smbios.h" > +#include "hw/i386/apic.h" > #include "hw/pci/pci.h" > #include "hw/pci/pci_ids.h" > #include "hw/usb.h" > @@ -62,6 +63,7 @@ static bool has_pci_info; > static bool has_acpi_build = true; > static bool smbios_defaults = true; > static bool smbios_legacy_mode; > +static bool soft_apic_compat; > /* Make sure that guest addresses aligned at 1Gbyte boundaries get mapped to > * host addresses aligned at 1Gbyte boundaries. This way we can use 1GByte > * pages in the host. > @@ -102,6 +104,10 @@ static void pc_init1(QEMUMachineInitArgs *args, > exit(1); > } > > + if (soft_apic_compat) { > + apic_version = 0x11; > + } > + > icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE); > object_property_add_child(qdev_get_machine(), "icc-bridge", > OBJECT(icc_bridge), NULL); > @@ -266,6 +272,7 @@ static void pc_init_pci(QEMUMachineInitArgs *args) > static void pc_compat_2_0(QEMUMachineInitArgs *args) > { > smbios_legacy_mode = true; > + soft_apic_compat = true; > } > > static void pc_compat_1_7(QEMUMachineInitArgs *args) > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 5b48231..8db0edb 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -40,6 +40,7 @@ > #include "exec/address-spaces.h" > #include "hw/i386/ich9.h" > #include "hw/i386/smbios.h" > +#include "hw/i386/apic.h" > #include "hw/ide/pci.h" > #include "hw/ide/ahci.h" > #include "hw/usb.h" > @@ -52,6 +53,7 @@ static bool has_pci_info; > static bool has_acpi_build = true; > static bool smbios_defaults = true; > static bool smbios_legacy_mode; > +static bool soft_apic_compat; > /* Make sure that guest addresses aligned at 1Gbyte boundaries get mapped to > * host addresses aligned at 1Gbyte boundaries. This way we can use 1GByte > * pages in the host. > @@ -89,6 +91,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args) > exit(1); > } > > + if (soft_apic_compat) { > + apic_version = 0x11; > + } > + > icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE); > object_property_add_child(qdev_get_machine(), "icc-bridge", > OBJECT(icc_bridge), NULL); > @@ -244,6 +250,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) > static void pc_compat_2_0(QEMUMachineInitArgs *args) > { > smbios_legacy_mode = true; > + soft_apic_compat = true; > } > > static void pc_compat_1_7(QEMUMachineInitArgs *args) > 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? Alex
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
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
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 > >
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 5b3594b..d90ce5a 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -29,6 +29,7 @@ #include "hw/i386/pc.h" #include "hw/i386/apic.h" #include "hw/i386/smbios.h" +#include "hw/i386/apic.h" #include "hw/pci/pci.h" #include "hw/pci/pci_ids.h" #include "hw/usb.h" @@ -62,6 +63,7 @@ static bool has_pci_info; static bool has_acpi_build = true; static bool smbios_defaults = true; static bool smbios_legacy_mode; +static bool soft_apic_compat; /* Make sure that guest addresses aligned at 1Gbyte boundaries get mapped to * host addresses aligned at 1Gbyte boundaries. This way we can use 1GByte * pages in the host. @@ -102,6 +104,10 @@ static void pc_init1(QEMUMachineInitArgs *args, exit(1); } + if (soft_apic_compat) { + apic_version = 0x11; + } + icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE); object_property_add_child(qdev_get_machine(), "icc-bridge", OBJECT(icc_bridge), NULL); @@ -266,6 +272,7 @@ static void pc_init_pci(QEMUMachineInitArgs *args) static void pc_compat_2_0(QEMUMachineInitArgs *args) { smbios_legacy_mode = true; + soft_apic_compat = true; } static void pc_compat_1_7(QEMUMachineInitArgs *args) diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 5b48231..8db0edb 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -40,6 +40,7 @@ #include "exec/address-spaces.h" #include "hw/i386/ich9.h" #include "hw/i386/smbios.h" +#include "hw/i386/apic.h" #include "hw/ide/pci.h" #include "hw/ide/ahci.h" #include "hw/usb.h" @@ -52,6 +53,7 @@ static bool has_pci_info; static bool has_acpi_build = true; static bool smbios_defaults = true; static bool smbios_legacy_mode; +static bool soft_apic_compat; /* Make sure that guest addresses aligned at 1Gbyte boundaries get mapped to * host addresses aligned at 1Gbyte boundaries. This way we can use 1GByte * pages in the host. @@ -89,6 +91,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args) exit(1); } + if (soft_apic_compat) { + apic_version = 0x11; + } + icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE); object_property_add_child(qdev_get_machine(), "icc-bridge", OBJECT(icc_bridge), NULL); @@ -244,6 +250,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) static void pc_compat_2_0(QEMUMachineInitArgs *args) { smbios_legacy_mode = true; + soft_apic_compat = true; } static void pc_compat_1_7(QEMUMachineInitArgs *args) 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; + static APICCommonState *local_apics[MAX_APICS + 1]; static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode); @@ -675,7 +677,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 = apic_version | ((APIC_LVT_NB - 1) << 16); break; case 0x08: apic_sync_vapic(s, SYNC_FROM_VAPIC); diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h index 1d48e02..4e79240 100644 --- a/include/hw/i386/apic.h +++ b/include/hw/i386/apic.h @@ -4,6 +4,8 @@ #include "qemu-common.h" /* apic.c */ +extern uint8_t apic_version; + void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode, uint8_t vector_num, uint8_t trigger_mode); int apic_accept_pic_intr(DeviceState *s);