Patchwork [v2] apic: bump emulated lapic version to 0x14 on pc machines >= 2.1

login
register
mail settings
Submitter Gabriel L. Somlo
Date April 30, 2014, 7:18 p.m.
Message ID <20140430191816.GA16023@ERROL.INI.CMU.EDU>
Download mbox | patch
Permalink /patch/344298/
State New
Headers show

Comments

Gabriel L. Somlo - April 30, 2014, 7:18 p.m.
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 Thu, Mar 06, 2014 at 09:50:48AM +0200, Michael S. Tsirkin wrote:
> On Sat, Mar 01, 2014 at 07:17:07PM -0500, Gabriel L. Somlo wrote:
> > On Sat, Mar 01, 2014 at 03:46:22PM +0100, Paolo Bonzini wrote:
> > > 
> > > Looks good, but you have to make this a qdev property so that older
> > > versions keep using version 0x11.
>
> [...]
>
> No, your patch is fine generally, it's not about users tweaking the
> version.
> It's about users getting compatible behaviour when they specify
> e.g. -M pc-1.6
> 
> Using property here is an implementation detail not exposed to users.
> Look for all the PC_COMPAT macros as an example of that.

OK, in the mean time I figured out how PC_[Q35]_COMPAT_* works,
so here's another iteration, where we only bump the emulated lapic
version to 0x14 for the newest machine types (2.1 and up).

Let me know what you think.

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(-)
Paolo Bonzini - April 30, 2014, 7:20 p.m.
Il 30/04/2014 21:18, Gabriel L. Somlo ha scritto:
> 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 Thu, Mar 06, 2014 at 09:50:48AM +0200, Michael S. Tsirkin wrote:
>> On Sat, Mar 01, 2014 at 07:17:07PM -0500, Gabriel L. Somlo wrote:
>>> On Sat, Mar 01, 2014 at 03:46:22PM +0100, Paolo Bonzini wrote:
>>>>
>>>> Looks good, but you have to make this a qdev property so that older
>>>> versions keep using version 0x11.
>>
>> [...]
>>
>> No, your patch is fine generally, it's not about users tweaking the
>> version.
>> It's about users getting compatible behaviour when they specify
>> e.g. -M pc-1.6
>>
>> Using property here is an implementation detail not exposed to users.
>> Look for all the PC_COMPAT macros as an example of that.
>
> OK, in the mean time I figured out how PC_[Q35]_COMPAT_* works,
> so here's another iteration, where we only bump the emulated lapic
> version to 0x14 for the newest machine types (2.1 and up).
>
> Let me know what you think.
>
> 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..a488ec9 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_v14 = true;
>  /* 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_v14) {
> +        apic_version = 0x14;
> +    }
> +
>      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_v14 = false;
>  }
>
>  static void pc_compat_1_7(QEMUMachineInitArgs *args)
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 5b48231..3779b48 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_v14 = true;
>  /* 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_v14) {
> +        apic_version = 0x14;
> +    }
> +
>      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_v14 = false;
>  }
>
>  static void pc_compat_1_7(QEMUMachineInitArgs *args)
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index 2f40cba..b53e8ff 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 = 0x11;
> +
>  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);

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.

Paolo

>          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);
>

Patch

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5b3594b..a488ec9 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_v14 = true;
 /* 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_v14) {
+        apic_version = 0x14;
+    }
+
     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_v14 = false;
 }
 
 static void pc_compat_1_7(QEMUMachineInitArgs *args)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 5b48231..3779b48 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_v14 = true;
 /* 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_v14) {
+        apic_version = 0x14;
+    }
+
     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_v14 = false;
 }
 
 static void pc_compat_1_7(QEMUMachineInitArgs *args)
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 2f40cba..b53e8ff 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 = 0x11;
+
 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);