Patchwork [v2,7/7] apic: qdev conversion cleanup

login
register
mail settings
Submitter Blue Swirl
Date June 12, 2010, 9:15 p.m.
Message ID <AANLkTil4CwbISuS_1rjpjF5zFgwGPkxU7WLrNv98q0kk@mail.gmail.com>
Download mbox | patch
Permalink /patch/55406/
State New
Headers show

Comments

Blue Swirl - June 12, 2010, 9:15 p.m.
Make APICState completely private to apic.c by using DeviceState
in external APIs.

Move apic_init() to pc.c.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 hw/apic.c         |   86 +++++++++++++++++++++++-----------------------------
 hw/apic.h         |   24 +++++++-------
 hw/pc.c           |   32 +++++++++++++++++++-
 qemu-common.h     |    2 +-
 target-i386/cpu.h |    2 +-
 5 files changed, 83 insertions(+), 63 deletions(-)
Markus Armbruster - June 14, 2010, 9:36 a.m.
Blue Swirl <blauwirbel@gmail.com> writes:

> Make APICState completely private to apic.c by using DeviceState
> in external APIs.

Could you explain why this is an improvement?
Blue Swirl - June 14, 2010, 5:52 p.m.
On Mon, Jun 14, 2010 at 9:36 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> Make APICState completely private to apic.c by using DeviceState
>> in external APIs.
>
> Could you explain why this is an improvement?

Outside of apic.c, there is no need to access APICState fields so we
can remove that privilege. We can move the device instantiation to the
board level where it belongs.
Markus Armbruster - June 15, 2010, 8:17 a.m.
Blue Swirl <blauwirbel@gmail.com> writes:

> On Mon, Jun 14, 2010 at 9:36 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Blue Swirl <blauwirbel@gmail.com> writes:
>>
>>> Make APICState completely private to apic.c by using DeviceState
>>> in external APIs.
>>
>> Could you explain why this is an improvement?
>
> Outside of apic.c, there is no need to access APICState fields so we
> can remove that privilege. We can move the device instantiation to the
> board level where it belongs.

Moving the definition of struct APICState into apic.c is a clear win.
But what does widening argument types from APICState to DeviceState
accomplish?  The compiler won't be able to catch certain stupid type
errors anymore; what do we gain for that loss?
Blue Swirl - June 16, 2010, 5:26 p.m.
On Tue, Jun 15, 2010 at 8:17 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> On Mon, Jun 14, 2010 at 9:36 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Blue Swirl <blauwirbel@gmail.com> writes:
>>>
>>>> Make APICState completely private to apic.c by using DeviceState
>>>> in external APIs.
>>>
>>> Could you explain why this is an improvement?
>>
>> Outside of apic.c, there is no need to access APICState fields so we
>> can remove that privilege. We can move the device instantiation to the
>> board level where it belongs.
>
> Moving the definition of struct APICState into apic.c is a clear win.
> But what does widening argument types from APICState to DeviceState
> accomplish?  The compiler won't be able to catch certain stupid type
> errors anymore; what do we gain for that loss?

More beautiful architecture. This is how for example Sparc devices
work: there are almost no global functions, all but a few are static.
Markus Armbruster - June 16, 2010, 6:19 p.m.
Blue Swirl <blauwirbel@gmail.com> writes:

> On Tue, Jun 15, 2010 at 8:17 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Blue Swirl <blauwirbel@gmail.com> writes:
>>
>>> On Mon, Jun 14, 2010 at 9:36 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Blue Swirl <blauwirbel@gmail.com> writes:
>>>>
>>>>> Make APICState completely private to apic.c by using DeviceState
>>>>> in external APIs.
>>>>
>>>> Could you explain why this is an improvement?
>>>
>>> Outside of apic.c, there is no need to access APICState fields so we
>>> can remove that privilege. We can move the device instantiation to the
>>> board level where it belongs.
>>
>> Moving the definition of struct APICState into apic.c is a clear win.
>> But what does widening argument types from APICState to DeviceState
>> accomplish?  The compiler won't be able to catch certain stupid type
>> errors anymore; what do we gain for that loss?
>
> More beautiful architecture. This is how for example Sparc devices
> work: there are almost no global functions, all but a few are static.

I'd take the static type checking any day.  But it's your funeral, I
guess :)

Patch

diff --git a/hw/apic.c b/hw/apic.c
index d0cdddb..89374fe 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -18,7 +18,6 @@ 
  */
 #include "hw.h"
 #include "apic.h"
-#include "msix.h"
 #include "qemu-timer.h"
 #include "host-utils.h"
 #include "sysbus.h"
@@ -89,9 +88,10 @@ 
 #define MSI_ADDR_DEST_ID_SHIFT		12
 #define	MSI_ADDR_DEST_ID_MASK		0x00ffff0

-#define MSI_ADDR_BASE                   0xfee00000
 #define MSI_ADDR_SIZE                   0x100000

+typedef struct APICState APICState;
+
 struct APICState {
     SysBusDevice busdev;
     void *cpu_env;
@@ -195,8 +195,10 @@  static void apic_local_deliver(APICState *s, int vector)
     }
 }

-void apic_deliver_pic_intr(APICState *s, int level)
+void apic_deliver_pic_intr(DeviceState *d, int level)
 {
+    APICState *s = container_of(d, APICState, busdev.qdev);
+
     if (level) {
         apic_local_deliver(s, APIC_LVT_LINT0);
     } else {
@@ -306,8 +308,10 @@  void apic_deliver_irq(uint8_t dest, uint8_t dest_mode,
                      trigger_mode);
 }

-void cpu_set_apic_base(APICState *s, uint64_t val)
+void cpu_set_apic_base(DeviceState *d, uint64_t val)
 {
+    APICState *s = container_of(d, APICState, busdev.qdev);
+
     DPRINTF("cpu_set_apic_base: %016" PRIx64 "\n", val);
     if (!s)
         return;
@@ -321,23 +325,29 @@  void cpu_set_apic_base(APICState *s, uint64_t val)
     }
 }

-uint64_t cpu_get_apic_base(APICState *s)
+uint64_t cpu_get_apic_base(DeviceState *d)
 {
+    APICState *s = container_of(d, APICState, busdev.qdev);
+
     DPRINTF("cpu_get_apic_base: %016" PRIx64 "\n",
             s ? (uint64_t)s->apicbase: 0);
     return s ? s->apicbase : 0;
 }

-void cpu_set_apic_tpr(APICState *s, uint8_t val)
+void cpu_set_apic_tpr(DeviceState *d, uint8_t val)
 {
+    APICState *s = container_of(d, APICState, busdev.qdev);
+
     if (!s)
         return;
     s->tpr = (val & 0x0f) << 4;
     apic_update_irq(s);
 }

-uint8_t cpu_get_apic_tpr(APICState *s)
+uint8_t cpu_get_apic_tpr(DeviceState *d)
 {
+    APICState *s = container_of(d, APICState, busdev.qdev);
+
     return s ? s->tpr >> 4 : 0;
 }

@@ -479,9 +489,9 @@  static void apic_get_delivery_bitmask(uint32_t
*deliver_bitmask,
     }
 }

-
-void apic_init_reset(APICState *s)
+void apic_init_reset(DeviceState *d)
 {
+    APICState *s = container_of(d, APICState, busdev.qdev);
     int i;

     if (!s)
@@ -512,8 +522,10 @@  static void apic_startup(APICState *s, int vector_num)
     cpu_interrupt(s->cpu_env, CPU_INTERRUPT_SIPI);
 }

-void apic_sipi(APICState *s)
+void apic_sipi(DeviceState *d)
 {
+    APICState *s = container_of(d, APICState, busdev.qdev);
+
     cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_SIPI);

     if (!s->wait_for_sipi)
@@ -522,10 +534,11 @@  void apic_sipi(APICState *s)
     s->wait_for_sipi = 0;
 }

-static void apic_deliver(APICState *s, uint8_t dest, uint8_t dest_mode,
+static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
                          uint8_t delivery_mode, uint8_t vector_num,
                          uint8_t polarity, uint8_t trigger_mode)
 {
+    APICState *s = container_of(d, APICState, busdev.qdev);
     uint32_t deliver_bitmask[MAX_APIC_WORDS];
     int dest_shorthand = (s->icr[0] >> 18) & 3;
     APICState *apic_iter;
@@ -570,8 +583,9 @@  static void apic_deliver(APICState *s, uint8_t
dest, uint8_t dest_mode,
                      trigger_mode);
 }

-int apic_get_interrupt(APICState *s)
+int apic_get_interrupt(DeviceState *d)
 {
+    APICState *s = container_of(d, APICState, busdev.qdev);
     int intno;

     /* if the APIC is installed or enabled, we let the 8259 handle the
@@ -593,8 +607,9 @@  int apic_get_interrupt(APICState *s)
     return intno;
 }

-int apic_accept_pic_intr(APICState *s)
+int apic_accept_pic_intr(DeviceState *d)
 {
+    APICState *s = container_of(d, APICState, busdev.qdev);
     uint32_t lvt0;

     if (!s)
@@ -680,14 +695,16 @@  static void apic_mem_writew(void *opaque,
target_phys_addr_t addr, uint32_t val)

 static uint32_t apic_mem_readl(void *opaque, target_phys_addr_t addr)
 {
+    DeviceState *d;
     APICState *s;
     uint32_t val;
     int index;

-    s = cpu_get_current_apic();
-    if (!s) {
+    d = cpu_get_current_apic();
+    if (!d) {
         return 0;
     }
+    s = container_of(d, APICState, busdev.qdev);

     index = (addr >> 4) & 0xff;
     switch(index) {
@@ -769,6 +786,7 @@  static void apic_send_msi(target_phys_addr_t addr,
uint32 data)

 static void apic_mem_writel(void *opaque, target_phys_addr_t addr,
uint32_t val)
 {
+    DeviceState *d;
     APICState *s;
     int index = (addr >> 4) & 0xff;
     if (addr > 0xfff || !index) {
@@ -781,10 +799,11 @@  static void apic_mem_writel(void *opaque,
target_phys_addr_t addr, uint32_t val)
         return;
     }

-    s = cpu_get_current_apic();
-    if (!s) {
+    d = cpu_get_current_apic();
+    if (!d) {
         return;
     }
+    s = container_of(d, APICState, busdev.qdev);

     DPRINTF("write: " TARGET_FMT_plx " = %08x\n", addr, val);

@@ -821,7 +840,7 @@  static void apic_mem_writel(void *opaque,
target_phys_addr_t addr, uint32_t val)
         break;
     case 0x30:
         s->icr[0] = val;
-        apic_deliver(s, (s->icr[1] >> 24) & 0xff, (s->icr[0] >> 11) & 1,
+        apic_deliver(d, (s->icr[1] >> 24) & 0xff, (s->icr[0] >> 11) & 1,
                      (s->icr[0] >> 8) & 7, (s->icr[0] & 0xff),
                      (s->icr[0] >> 14) & 1, (s->icr[0] >> 15) & 1);
         break;
@@ -935,7 +954,7 @@  static void apic_reset(DeviceState *d)
     s->apicbase = 0xfee00000 |
         (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;

-    apic_init_reset(s);
+    apic_init_reset(d);

     if (bsp) {
         /*
@@ -959,35 +978,6 @@  static CPUWriteMemoryFunc * const apic_mem_write[3] = {
     apic_mem_writel,
 };

-APICState *apic_init(void *env, uint8_t apic_id)
-{
-    DeviceState *dev;
-    SysBusDevice *d;
-    APICState *s;
-    static int apic_mapped;
-
-    dev = qdev_create(NULL, "apic");
-    qdev_prop_set_uint8(dev, "id", apic_id);
-    qdev_prop_set_ptr(dev, "cpu_env", env);
-    qdev_init_nofail(dev);
-    d = sysbus_from_qdev(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(d, 0, MSI_ADDR_BASE);
-        apic_mapped = 1;
-    }
-
-    msix_supported = 1;
-
-    s = container_of(dev, APICState, busdev.qdev);
-
-    return s;
-}
-
 static int apic_init1(SysBusDevice *dev)
 {
     APICState *s = FROM_SYSBUS(APICState, dev);
diff --git a/hw/apic.h b/hw/apic.h
index 77078ca..8a0c9d0 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -1,27 +1,27 @@ 
 #ifndef APIC_H
 #define APIC_H

+#include "qemu-common.h"
+
 /* apic.c */
-typedef struct APICState APICState;
 void apic_deliver_irq(uint8_t dest, uint8_t dest_mode,
                              uint8_t delivery_mode,
                              uint8_t vector_num, uint8_t polarity,
                              uint8_t trigger_mode);
-APICState *apic_init(void *env, uint8_t apic_id);
-int apic_accept_pic_intr(APICState *s);
-void apic_deliver_pic_intr(APICState *s, int level);
-int apic_get_interrupt(APICState *s);
+int apic_accept_pic_intr(DeviceState *s);
+void apic_deliver_pic_intr(DeviceState *s, int level);
+int apic_get_interrupt(DeviceState *s);
 void apic_reset_irq_delivered(void);
 int apic_get_irq_delivered(void);
-void cpu_set_apic_base(APICState *s, uint64_t val);
-uint64_t cpu_get_apic_base(APICState *s);
-void cpu_set_apic_tpr(APICState *s, uint8_t val);
-uint8_t cpu_get_apic_tpr(APICState *s);
-void apic_init_reset(APICState *s);
-void apic_sipi(APICState *s);
+void cpu_set_apic_base(DeviceState *s, uint64_t val);
+uint64_t cpu_get_apic_base(DeviceState *s);
+void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
+uint8_t cpu_get_apic_tpr(DeviceState *s);
+void apic_init_reset(DeviceState *s);
+void apic_sipi(DeviceState *s);

 /* pc.c */
 int cpu_is_bsp(CPUState *env);
-APICState *cpu_get_current_apic(void);
+DeviceState *cpu_get_current_apic(void);

 #endif
diff --git a/hw/pc.c b/hw/pc.c
index 422e273..2ea28c5 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -35,6 +35,8 @@ 
 #include "elf.h"
 #include "multiboot.h"
 #include "mc146818rtc.h"
+#include "msix.h"
+#include "sysbus.h"

 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
@@ -61,6 +63,8 @@ 
 #define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)

+#define MSI_ADDR_BASE 0xfee00000
+
 #define E820_NR_ENTRIES		16

 struct e820_entry {
@@ -749,7 +753,7 @@  int cpu_is_bsp(CPUState *env)
     return env->cpu_index == 0;
 }

-APICState *cpu_get_current_apic(void)
+DeviceState *cpu_get_current_apic(void)
 {
     if (cpu_single_env) {
         return cpu_single_env->apic_state;
@@ -758,6 +762,32 @@  APICState *cpu_get_current_apic(void)
     }
 }

+static DeviceState *apic_init(void *env, uint8_t apic_id)
+{
+    DeviceState *dev;
+    SysBusDevice *d;
+    static int apic_mapped;
+
+    dev = qdev_create(NULL, "apic");
+    qdev_prop_set_uint8(dev, "id", apic_id);
+    qdev_prop_set_ptr(dev, "cpu_env", env);
+    qdev_init_nofail(dev);
+    d = sysbus_from_qdev(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(d, 0, MSI_ADDR_BASE);
+        apic_mapped = 1;
+    }
+
+    msix_supported = 1;
+
+    return dev;
+}
+
 /* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
    BIOS will read it and start S3 resume at POST Entry */
 void pc_cmos_set_s3_resume(void *opaque, int irq, int level)
diff --git a/qemu-common.h b/qemu-common.h
index a4888e5..5918c4c 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -16,6 +16,7 @@ 
 typedef struct QEMUTimer QEMUTimer;
 typedef struct QEMUFile QEMUFile;
 typedef struct QEMUBH QEMUBH;
+typedef struct DeviceState DeviceState;

 /* Hack around the mess dyngen-exec.h causes: We need QEMU_NORETURN
in files that
    cannot include the following headers without conflicts. This condition has
@@ -226,7 +227,6 @@  typedef struct PCMCIACardState PCMCIACardState;
 typedef struct MouseTransformInfo MouseTransformInfo;
 typedef struct uWireSlave uWireSlave;
 typedef struct I2SCodec I2SCodec;
-typedef struct DeviceState DeviceState;
 typedef struct SSIBus SSIBus;
 typedef struct EventNotifier EventNotifier;
 typedef struct VirtIODevice VirtIODevice;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 619a747..8dafa0d 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -705,7 +705,7 @@  typedef struct CPUX86State {

     /* in order to simplify APIC support, we leave this pointer to the
        user */
-    struct APICState *apic_state;
+    struct DeviceState *apic_state;

     uint64 mcg_cap;
     uint64 mcg_status;