Patchwork [qom-next,4/6] pc: move apic_mapped initialization into common apic init code

login
register
mail settings
Submitter Igor Mammedov
Date May 23, 2012, 4:39 p.m.
Message ID <1337791181-27446-5-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/160972/
State New
Headers show

Comments

Igor Mammedov - May 23, 2012, 4:39 p.m.
Move from apic_init in pc.c, the code that belongs to apic_init_common.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/apic_common.c |   11 +++++++++++
 hw/msi.h         |    2 ++
 hw/pc.c          |   12 ------------
 3 files changed, 13 insertions(+), 12 deletions(-)
Jan Kiszka - May 23, 2012, 8:08 p.m.
On 2012-05-23 13:44, Peter Maydell wrote:
> On 23 May 2012 17:39, Igor Mammedov <imammedo@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?

I fact, the CPU normally decides about where the APIC is mapped,
specifically when it is remapped via the MSR (which QEMU cannot support
yet). Well, unless we are talking about an external APIC and a 486 CPU.
Then the board decides. But I guess we could live with ignoring that
corner case and stick the mapping setup into the CPU initialization code.

Jan

Patch

diff --git a/hw/apic_common.c b/hw/apic_common.c
index 23d51e8..c3fa66b 100644
--- a/hw/apic_common.c
+++ b/hw/apic_common.c
@@ -21,6 +21,7 @@ 
 #include "apic_internal.h"
 #include "trace.h"
 #include "kvm.h"
+#include "msi.h"
 
 static int apic_irq_delivered;
 bool apic_report_tpr_access;
@@ -284,6 +285,7 @@  static int apic_init_common(SysBusDevice *dev)
     APICCommonClass *info;
     static DeviceState *vapic;
     static int apic_no;
+    static int apic_mapped;
 
     if (apic_no >= MAX_APICS) {
         return -1;
@@ -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);
     }
diff --git a/hw/msi.h b/hw/msi.h
index 3040bb0..abd52b6 100644
--- a/hw/msi.h
+++ b/hw/msi.h
@@ -40,4 +40,6 @@  static inline bool msi_present(const PCIDevice *dev)
     return dev->cap_present & QEMU_PCI_CAP_MSI;
 }
 
+#define MSI_ADDR_BASE 0xfee00000
+
 #endif /* QEMU_MSI_H */
diff --git a/hw/pc.c b/hw/pc.c
index 1aa90a2..1ccfc6e 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -70,8 +70,6 @@ 
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
 #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
-#define MSI_ADDR_BASE 0xfee00000
-
 #define E820_NR_ENTRIES		16
 
 struct e820_entry {
@@ -882,7 +880,6 @@  DeviceState *cpu_get_current_apic(void)
 static DeviceState *apic_init(void *env, uint8_t apic_id)
 {
     DeviceState *dev;
-    static int apic_mapped;
 
     if (kvm_irqchip_in_kernel()) {
         dev = qdev_create(NULL, "kvm-apic");
@@ -896,15 +893,6 @@  static DeviceState *apic_init(void *env, uint8_t apic_id)
     qdev_prop_set_ptr(dev, "cpu_env", env);
     qdev_init_nofail(dev);
 
-    /* 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(dev), 0, MSI_ADDR_BASE);
-        apic_mapped = 1;
-    }
-
     return dev;
 }