diff mbox

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

Message ID 1337791181-27446-5-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov May 23, 2012, 4:39 p.m. UTC 
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(-)

Comments

Jan Kiszka May 23, 2012, 8:08 p.m. UTC | #1 
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
diff mbox

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