[PATCH v3 00/10] Cleanup patches, mostly PC-related

[PATCH v3 00/10] Cleanup patches, mostly PC-related

[Bernhard Beschow]

This series mostly contains PC-related patches I came up with when doing
"virtual retrocomputing" with my via-apollo-pro-133t branch [1]. It includes
improved tracing and reduced usage of cpu_get_current_apic(). The remaining
patch resolves duplicate code in the test of DS1338 RTC which is used in e500
machines.

v3:
* Don't remove cpu_get_current_apic() since it is needed in
  apic_mem_{read,write} (the local APIC memory regions aren't per CPU)

v2:
* Remove some redundant APIC_COMMON(cpu->apic_state) casts
* Resolve cpu_get_current_apic()

Testing done:
* make check
* Work with recent x86_64 Linux distribution running on WHPX

[1] https://github.com/shentok/qemu/tree/via-apollo-pro-133t

Bernhard Beschow (10):
  hw/timer/i8254: Add I/O trace events
  hw/audio/pcspk: Add I/O trace events
  hw/rtc/mc146818rtc: Convert CMOS_DPRINTF() into trace events
  hw/rtc/mc146818rtc: Use ARRAY_SIZE macro
  hw/rtc/mc146818rtc: Assert correct usage of
    mc146818rtc_set_cmos_data()
  hw/ide/ide-internal: Move dma_buf_commit() into ide "namespace"
  hw/i386/apic: Prefer APICCommonState over DeviceState
  hw/i386/apic: Ensure own APIC use in apic_msr_{read,write}
  hw/intc/apic: Pass APICCommonState to apic_register_{read,write}
  tests/qtest/ds1338-test: Reuse from_bcd()

 hw/ide/ide-internal.h                |   2 +-
 include/hw/i386/apic.h               |  37 ++++----
 include/hw/i386/apic_internal.h      |   7 +-
 target/i386/cpu.h                    |   4 +-
 target/i386/kvm/kvm_i386.h           |   2 +-
 target/i386/whpx/whpx-internal.h     |   2 +-
 hw/audio/pcspk.c                     |  10 ++-
 hw/i386/kvm/apic.c                   |   3 +-
 hw/i386/vapic.c                      |   2 +-
 hw/i386/x86-cpu.c                    |   2 +-
 hw/ide/ahci.c                        |   8 +-
 hw/ide/core.c                        |  10 +--
 hw/intc/apic.c                       | 126 ++++++++++-----------------
 hw/intc/apic_common.c                |  56 ++++--------
 hw/rtc/mc146818rtc.c                 |  20 ++---
 hw/timer/i8254.c                     |   6 ++
 target/i386/cpu-apic.c               |  18 ++--
 target/i386/cpu-dump.c               |   2 +-
 target/i386/cpu.c                    |   2 +-
 target/i386/hvf/hvf.c                |   4 +-
 target/i386/kvm/kvm.c                |   2 +-
 target/i386/tcg/system/misc_helper.c |   4 +-
 target/i386/whpx/whpx-apic.c         |   3 +-
 tests/qtest/ds1338-test.c            |  12 +--
 hw/audio/trace-events                |   4 +
 hw/rtc/trace-events                  |   4 +
 hw/timer/trace-events                |   4 +
 27 files changed, 158 insertions(+), 198 deletions(-)

-- 
2.51.1.dirty

[PATCH v3 01/10] hw/timer/i8254: Add I/O trace events

[Bernhard Beschow]

Allows to see how the guest interacts with the device.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/timer/i8254.c      | 6 ++++++
 hw/timer/trace-events | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/hw/timer/i8254.c b/hw/timer/i8254.c
index 4b25c487f7..7033ebf50d 100644
--- a/hw/timer/i8254.c
+++ b/hw/timer/i8254.c
@@ -29,6 +29,7 @@
 #include "hw/timer/i8254.h"
 #include "hw/timer/i8254_internal.h"
 #include "qom/object.h"
+#include "trace.h"
 
 //#define DEBUG_PIT
 
@@ -130,6 +131,8 @@ static void pit_ioport_write(void *opaque, hwaddr addr,
     int channel, access;
     PITChannelState *s;
 
+    trace_pit_ioport_write(addr, val);
+
     addr &= 3;
     if (addr == 3) {
         channel = val >> 6;
@@ -248,6 +251,9 @@ static uint64_t pit_ioport_read(void *opaque, hwaddr addr,
             break;
         }
     }
+
+    trace_pit_ioport_read(addr, ret);
+
     return ret;
 }
 
diff --git a/hw/timer/trace-events b/hw/timer/trace-events
index c5b6db49f5..2bb51f95ea 100644
--- a/hw/timer/trace-events
+++ b/hw/timer/trace-events
@@ -49,6 +49,10 @@ cmsdk_apb_dualtimer_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK A
 cmsdk_apb_dualtimer_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB dualtimer write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 cmsdk_apb_dualtimer_reset(void) "CMSDK APB dualtimer: reset"
 
+# i8254.c
+pit_ioport_read(uint8_t addr, uint32_t value) "[0x%" PRIx8 "] -> 0x%" PRIx32
+pit_ioport_write(uint8_t addr, uint32_t value) "[0x%" PRIx8 "] <- 0x%" PRIx32
+
 # imx_gpt.c
 imx_gpt_set_freq(uint32_t clksrc, uint32_t freq) "Setting clksrc %u to %u Hz"
 imx_gpt_read(const char *name, uint64_t value) "%s -> 0x%08" PRIx64
-- 
2.51.1.dirty

Re: [PATCH v3 01/10] hw/timer/i8254: Add I/O trace events

[Philippe Mathieu-Daudé]

On 19/10/25 23:02, Bernhard Beschow wrote:
> Allows to see how the guest interacts with the device.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/timer/i8254.c      | 6 ++++++
>   hw/timer/trace-events | 4 ++++
>   2 files changed, 10 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 02/10] hw/audio/pcspk: Add I/O trace events

[Bernhard Beschow]

Allows to see how the guest interacts with the device.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/audio/pcspk.c      | 10 +++++++++-
 hw/audio/trace-events |  4 ++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
index a419161b5b..f8020593b0 100644
--- a/hw/audio/pcspk.c
+++ b/hw/audio/pcspk.c
@@ -34,6 +34,7 @@
 #include "hw/audio/pcspk.h"
 #include "qapi/error.h"
 #include "qom/object.h"
+#include "trace.h"
 
 #define PCSPK_BUF_LEN 1792
 #define PCSPK_SAMPLE_RATE 32000
@@ -137,13 +138,18 @@ static uint64_t pcspk_io_read(void *opaque, hwaddr addr,
 {
     PCSpkState *s = opaque;
     PITChannelInfo ch;
+    uint8_t val;
 
     pit_get_channel_info(s->pit, 2, &ch);
 
     s->dummy_refresh_clock ^= (1 << 4);
 
-    return ch.gate | (s->data_on << 1) | s->dummy_refresh_clock |
+    val = ch.gate | (s->data_on << 1) | s->dummy_refresh_clock |
        (ch.out << 5);
+
+    trace_pcspk_io_read(s->iobase, val);
+
+    return val;
 }
 
 static void pcspk_io_write(void *opaque, hwaddr addr, uint64_t val,
@@ -152,6 +158,8 @@ static void pcspk_io_write(void *opaque, hwaddr addr, uint64_t val,
     PCSpkState *s = opaque;
     const int gate = val & 1;
 
+    trace_pcspk_io_write(s->iobase, val);
+
     s->data_on = (val >> 1) & 1;
     pit_set_gate(s->pit, 2, gate);
     if (s->voice) {
diff --git a/hw/audio/trace-events b/hw/audio/trace-events
index b8ef572767..30f5921545 100644
--- a/hw/audio/trace-events
+++ b/hw/audio/trace-events
@@ -23,6 +23,10 @@ hda_audio_format(const char *stream, int chan, const char *fmt, int freq) "st %s
 hda_audio_adjust(const char *stream, int pos) "st %s, pos %d"
 hda_audio_overrun(const char *stream) "st %s"
 
+# pcspk.c
+pcspk_io_read(uint16_t addr, uint8_t val) "[0x%"PRIx16"] -> 0x%"PRIx8
+pcspk_io_write(uint16_t addr, uint8_t val) "[0x%"PRIx16"] <- 0x%"PRIx8
+
 #via-ac97.c
 via_ac97_codec_write(uint8_t addr, uint16_t val) "0x%x <- 0x%x"
 via_ac97_sgd_fetch(uint32_t curr, uint32_t addr, char stop, char eol, char flag, uint32_t len) "curr=0x%x addr=0x%x %c%c%c len=%d"
-- 
2.51.1.dirty

Re: [PATCH v3 02/10] hw/audio/pcspk: Add I/O trace events

[Philippe Mathieu-Daudé]

On 19/10/25 23:02, Bernhard Beschow wrote:
> Allows to see how the guest interacts with the device.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/audio/pcspk.c      | 10 +++++++++-
>   hw/audio/trace-events |  4 ++++
>   2 files changed, 13 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 03/10] hw/rtc/mc146818rtc: Convert CMOS_DPRINTF() into trace events

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/rtc/mc146818rtc.c | 14 +++-----------
 hw/rtc/trace-events  |  4 ++++
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index f9f5cf396f..61e9c0bf99 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -43,16 +43,10 @@
 #include "qapi/error.h"
 #include "qapi/qapi-events-misc.h"
 #include "qapi/visitor.h"
+#include "trace.h"
 
-//#define DEBUG_CMOS
 //#define DEBUG_COALESCED
 
-#ifdef DEBUG_CMOS
-# define CMOS_DPRINTF(format, ...)      printf(format, ## __VA_ARGS__)
-#else
-# define CMOS_DPRINTF(format, ...)      do { } while (0)
-#endif
-
 #ifdef DEBUG_COALESCED
 # define DPRINTF_C(format, ...)      printf(format, ## __VA_ARGS__)
 #else
@@ -439,8 +433,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
     if ((addr & 1) == 0) {
         s->cmos_index = data & 0x7f;
     } else {
-        CMOS_DPRINTF("cmos: write index=0x%02x val=0x%02" PRIx64 "\n",
-                     s->cmos_index, data);
+        trace_mc146818_rtc_ioport_write(s->cmos_index, data);
         switch(s->cmos_index) {
         case RTC_SECONDS_ALARM:
         case RTC_MINUTES_ALARM:
@@ -726,8 +719,7 @@ static uint64_t cmos_ioport_read(void *opaque, hwaddr addr,
             ret = s->cmos_data[s->cmos_index];
             break;
         }
-        CMOS_DPRINTF("cmos: read index=0x%02x val=0x%02x\n",
-                     s->cmos_index, ret);
+        trace_mc146818_rtc_ioport_read(s->cmos_index, ret);
         return ret;
     }
 }
diff --git a/hw/rtc/trace-events b/hw/rtc/trace-events
index b9f2852d35..d2f36217cb 100644
--- a/hw/rtc/trace-events
+++ b/hw/rtc/trace-events
@@ -32,6 +32,10 @@ m48txx_nvram_io_write(uint64_t addr, uint64_t value) "io write addr:0x%04" PRIx6
 m48txx_nvram_mem_read(uint32_t addr, uint32_t value) "mem read addr:0x%04x value:0x%02x"
 m48txx_nvram_mem_write(uint32_t addr, uint32_t value) "mem write addr:0x%04x value:0x%02x"
 
+# mc146818rtc.c
+mc146818_rtc_ioport_read(uint8_t addr, uint8_t value) "[0x%02" PRIx8 "] -> 0x%02" PRIx8
+mc146818_rtc_ioport_write(uint8_t addr, uint8_t value) "[0x%02" PRIx8 "] <- 0x%02" PRIx8
+
 # goldfish_rtc.c
 goldfish_rtc_read(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
 goldfish_rtc_write(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
-- 
2.51.1.dirty

Re: [PATCH v3 03/10] hw/rtc/mc146818rtc: Convert CMOS_DPRINTF() into trace events

[Philippe Mathieu-Daudé]

On 19/10/25 23:02, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/rtc/mc146818rtc.c | 14 +++-----------
>   hw/rtc/trace-events  |  4 ++++
>   2 files changed, 7 insertions(+), 11 deletions(-)


> diff --git a/hw/rtc/trace-events b/hw/rtc/trace-events
> index b9f2852d35..d2f36217cb 100644
> --- a/hw/rtc/trace-events
> +++ b/hw/rtc/trace-events
> @@ -32,6 +32,10 @@ m48txx_nvram_io_write(uint64_t addr, uint64_t value) "io write addr:0x%04" PRIx6
>   m48txx_nvram_mem_read(uint32_t addr, uint32_t value) "mem read addr:0x%04x value:0x%02x"
>   m48txx_nvram_mem_write(uint32_t addr, uint32_t value) "mem write addr:0x%04x value:0x%02x"
>   
> +# mc146818rtc.c
> +mc146818_rtc_ioport_read(uint8_t addr, uint8_t value) "[0x%02" PRIx8 "] -> 0x%02" PRIx8
> +mc146818_rtc_ioport_write(uint8_t addr, uint8_t value) "[0x%02" PRIx8 "] <- 0x%02" PRIx8

The block could be theorically mapped anywhere in the I/O ISA space,
so 'uint8_t addr' is a bit too restrictive here. Otherwise,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +
>   # goldfish_rtc.c
>   goldfish_rtc_read(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
>   goldfish_rtc_write(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64

[PATCH v3 04/10] hw/rtc/mc146818rtc: Use ARRAY_SIZE macro

[Bernhard Beschow]

Avoids the error-prone repetition of the array size.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/rtc/mc146818rtc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 61e9c0bf99..5a89062b4c 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -726,13 +726,14 @@ static uint64_t cmos_ioport_read(void *opaque, hwaddr addr,
 
 void mc146818rtc_set_cmos_data(MC146818RtcState *s, int addr, int val)
 {
-    if (addr >= 0 && addr <= 127)
+    if (addr >= 0 && addr < ARRAY_SIZE(s->cmos_data)) {
         s->cmos_data[addr] = val;
+    }
 }
 
 int mc146818rtc_get_cmos_data(MC146818RtcState *s, int addr)
 {
-    assert(addr >= 0 && addr <= 127);
+    assert(addr >= 0 && addr < ARRAY_SIZE(s->cmos_data));
     return s->cmos_data[addr];
 }
 
-- 
2.51.1.dirty

Re: [PATCH v3 04/10] hw/rtc/mc146818rtc: Use ARRAY_SIZE macro

[Philippe Mathieu-Daudé]

On 19/10/25 23:02, Bernhard Beschow wrote:
> Avoids the error-prone repetition of the array size.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/rtc/mc146818rtc.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 05/10] hw/rtc/mc146818rtc: Assert correct usage of mc146818rtc_set_cmos_data()

[Bernhard Beschow]

The offset is never controlled by the guest, so any misuse constitutes a
programming error and shouldn't be silently ignored. Fix this by using assert().

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/rtc/mc146818rtc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 5a89062b4c..8631386b9f 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -726,9 +726,8 @@ static uint64_t cmos_ioport_read(void *opaque, hwaddr addr,
 
 void mc146818rtc_set_cmos_data(MC146818RtcState *s, int addr, int val)
 {
-    if (addr >= 0 && addr < ARRAY_SIZE(s->cmos_data)) {
-        s->cmos_data[addr] = val;
-    }
+    assert(addr >= 0 && addr < ARRAY_SIZE(s->cmos_data));
+    s->cmos_data[addr] = val;
 }
 
 int mc146818rtc_get_cmos_data(MC146818RtcState *s, int addr)
-- 
2.51.1.dirty

Re: [PATCH v3 05/10] hw/rtc/mc146818rtc: Assert correct usage of mc146818rtc_set_cmos_data()

[Philippe Mathieu-Daudé]

On 19/10/25 23:02, Bernhard Beschow wrote:
> The offset is never controlled by the guest, so any misuse constitutes a
> programming error and shouldn't be silently ignored. Fix this by using assert().

Would be nice to document the prototype.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/rtc/mc146818rtc.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

[PATCH v3 06/10] hw/ide/ide-internal: Move dma_buf_commit() into ide "namespace"

[Bernhard Beschow]

The identifier suggests that it is a generic DMA function while it is tied
to IDE. Fix this by adding an "ide_" prefix.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ide/ide-internal.h |  2 +-
 hw/ide/ahci.c         |  8 ++++----
 hw/ide/core.c         | 10 +++++-----
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/ide/ide-internal.h b/hw/ide/ide-internal.h
index 0d64805da2..281d07c9d5 100644
--- a/hw/ide/ide-internal.h
+++ b/hw/ide/ide-internal.h
@@ -398,7 +398,7 @@ int64_t ide_get_sector(IDEState *s);
 void ide_set_sector(IDEState *s, int64_t sector_num);
 
 void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
-void dma_buf_commit(IDEState *s, uint32_t tx_bytes);
+void ide_dma_buf_commit(IDEState *s, uint32_t tx_bytes);
 void ide_dma_error(IDEState *s);
 void ide_abort_command(IDEState *s);
 
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1303c21cb7..14bc66fb7f 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1417,7 +1417,7 @@ static void ahci_pio_transfer(const IDEDMA *dma)
     }
 
     /* Update number of transferred bytes, destroy sglist */
-    dma_buf_commit(s, size);
+    ide_dma_buf_commit(s, size);
 
 out:
     /* declare that we processed everything */
@@ -1482,8 +1482,8 @@ static int32_t ahci_dma_prepare_buf(const IDEDMA *dma, int32_t limit)
 
 /**
  * Updates the command header with a bytes-read value.
- * Called via dma_buf_commit, for both DMA and PIO paths.
- * sglist destruction is handled within dma_buf_commit.
+ * Called via ide_dma_buf_commit, for both DMA and PIO paths.
+ * sglist destruction is handled within ide_dma_buf_commit.
  */
 static void ahci_commit_buf(const IDEDMA *dma, uint32_t tx_bytes)
 {
@@ -1511,7 +1511,7 @@ static int ahci_dma_rw_buf(const IDEDMA *dma, bool is_write)
     }
 
     /* free sglist, update byte count */
-    dma_buf_commit(s, l);
+    ide_dma_buf_commit(s, l);
     s->io_buffer_index += l;
 
     trace_ahci_dma_rw_buf(ad->hba, ad->port_no, l);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index b14983ec54..8c380abf7c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -827,7 +827,7 @@ static void ide_sector_read(IDEState *s)
                                       ide_sector_read_cb, s);
 }
 
-void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
+void ide_dma_buf_commit(IDEState *s, uint32_t tx_bytes)
 {
     if (s->bus->dma->ops->commit_buf) {
         s->bus->dma->ops->commit_buf(s->bus->dma, tx_bytes);
@@ -848,7 +848,7 @@ void ide_set_inactive(IDEState *s, bool more)
 
 void ide_dma_error(IDEState *s)
 {
-    dma_buf_commit(s, 0);
+    ide_dma_buf_commit(s, 0);
     ide_abort_command(s);
     ide_set_inactive(s, false);
     ide_bus_set_irq(s->bus);
@@ -893,7 +893,7 @@ static void ide_dma_cb(void *opaque, int ret)
     if (ret < 0) {
         if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
             s->bus->dma->aiocb = NULL;
-            dma_buf_commit(s, 0);
+            ide_dma_buf_commit(s, 0);
             return;
         }
     }
@@ -912,7 +912,7 @@ static void ide_dma_cb(void *opaque, int ret)
     sector_num = ide_get_sector(s);
     if (n > 0) {
         assert(n * 512 == s->sg.size);
-        dma_buf_commit(s, s->sg.size);
+        ide_dma_buf_commit(s, s->sg.size);
         sector_num += n;
         ide_set_sector(s, sector_num);
         s->nsector -= n;
@@ -944,7 +944,7 @@ static void ide_dma_cb(void *opaque, int ret)
          * Reset the Active bit and don't raise the interrupt.
          */
         s->status = READY_STAT | SEEK_STAT;
-        dma_buf_commit(s, 0);
+        ide_dma_buf_commit(s, 0);
         goto eot;
     }
 
-- 
2.51.1.dirty

Re: [PATCH v3 06/10] hw/ide/ide-internal: Move dma_buf_commit() into ide "namespace"

[Philippe Mathieu-Daudé]

On 19/10/25 23:02, Bernhard Beschow wrote:
> The identifier suggests that it is a generic DMA function while it is tied
> to IDE. Fix this by adding an "ide_" prefix.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/ide/ide-internal.h |  2 +-
>   hw/ide/ahci.c         |  8 ++++----
>   hw/ide/core.c         | 10 +++++-----
>   3 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 07/10] hw/i386/apic: Prefer APICCommonState over DeviceState

[Bernhard Beschow]

Makes the APIC API more type-safe by resolving quite a few APIC_COMMON
downcasts.

Like PICCommonState, the APICCommonState is now a public typedef while staying
an abstract datatype.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/apic.h           | 33 +++++------
 include/hw/i386/apic_internal.h  |  7 +--
 target/i386/cpu.h                |  4 +-
 target/i386/kvm/kvm_i386.h       |  2 +-
 target/i386/whpx/whpx-internal.h |  2 +-
 hw/i386/kvm/apic.c               |  3 +-
 hw/i386/vapic.c                  |  2 +-
 hw/i386/x86-cpu.c                |  2 +-
 hw/intc/apic.c                   | 97 +++++++++++++-------------------
 hw/intc/apic_common.c            | 56 +++++++-----------
 target/i386/cpu-apic.c           | 18 +++---
 target/i386/cpu-dump.c           |  2 +-
 target/i386/cpu.c                |  2 +-
 target/i386/kvm/kvm.c            |  2 +-
 target/i386/whpx/whpx-apic.c     |  3 +-
 15 files changed, 95 insertions(+), 140 deletions(-)

diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
index eb606d6076..871f142888 100644
--- a/include/hw/i386/apic.h
+++ b/include/hw/i386/apic.h
@@ -1,28 +1,29 @@
 #ifndef APIC_H
 #define APIC_H
 
+typedef struct APICCommonState APICCommonState;
 
 /* apic.c */
 void apic_set_max_apic_id(uint32_t max_apic_id);
-int apic_accept_pic_intr(DeviceState *s);
-void apic_deliver_pic_intr(DeviceState *s, int level);
-void apic_deliver_nmi(DeviceState *d);
-int apic_get_interrupt(DeviceState *s);
-int cpu_set_apic_base(DeviceState *s, uint64_t val);
-uint64_t cpu_get_apic_base(DeviceState *s);
-bool cpu_is_apic_enabled(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);
-void apic_poll_irq(DeviceState *d);
-void apic_designate_bsp(DeviceState *d, bool bsp);
-int apic_get_highest_priority_irr(DeviceState *dev);
+int apic_accept_pic_intr(APICCommonState *s);
+void apic_deliver_pic_intr(APICCommonState *s, int level);
+void apic_deliver_nmi(APICCommonState *s);
+int apic_get_interrupt(APICCommonState *s);
+int cpu_set_apic_base(APICCommonState *s, uint64_t val);
+uint64_t cpu_get_apic_base(APICCommonState *s);
+bool cpu_is_apic_enabled(APICCommonState *s);
+void cpu_set_apic_tpr(APICCommonState *s, uint8_t val);
+uint8_t cpu_get_apic_tpr(APICCommonState *s);
+void apic_init_reset(APICCommonState *s);
+void apic_sipi(APICCommonState *s);
+void apic_poll_irq(APICCommonState *s);
+void apic_designate_bsp(APICCommonState *s, bool bsp);
+int apic_get_highest_priority_irr(APICCommonState *s);
 int apic_msr_read(int index, uint64_t *val);
 int apic_msr_write(int index, uint64_t val);
-bool is_x2apic_mode(DeviceState *d);
+bool is_x2apic_mode(APICCommonState *s);
 
 /* pc.c */
-DeviceState *cpu_get_current_apic(void);
+APICCommonState *cpu_get_current_apic(void);
 
 #endif
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 429278da61..4a62fdceb4 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -22,6 +22,7 @@
 #define QEMU_APIC_INTERNAL_H
 
 #include "cpu.h"
+#include "hw/i386/apic.h"
 #include "system/memory.h"
 #include "qemu/timer.h"
 #include "target/i386/cpu-qom.h"
@@ -125,8 +126,6 @@
 #define VAPIC_ENABLE_BIT                0
 #define VAPIC_ENABLE_MASK               (1 << VAPIC_ENABLE_BIT)
 
-typedef struct APICCommonState APICCommonState;
-
 #define TYPE_APIC_COMMON "apic-common"
 typedef struct APICCommonClass APICCommonClass;
 DECLARE_OBJ_CHECKERS(APICCommonState, APICCommonClass,
@@ -203,8 +202,8 @@ typedef struct VAPICState {
 extern bool apic_report_tpr_access;
 
 bool apic_next_timer(APICCommonState *s, int64_t current_time);
-void apic_enable_tpr_access_reporting(DeviceState *d, bool enable);
-void apic_enable_vapic(DeviceState *d, hwaddr paddr);
+void apic_enable_tpr_access_reporting(APICCommonState *s, bool enable);
+void apic_enable_vapic(APICCommonState *s, hwaddr paddr);
 
 void vapic_report_tpr_access(DeviceState *dev, CPUState *cpu, target_ulong ip,
                              TPRAccess access);
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index ce948861a7..67ff52a8b4 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2349,7 +2349,7 @@ struct ArchCPU {
 
     /* in order to simplify APIC support, we leave this pointer to the
        user */
-    struct DeviceState *apic_state;
+    struct APICCommonState *apic_state;
     struct MemoryRegion *cpu_as_root, *cpu_as_mem, *smram;
     Notifier machine_done;
 
@@ -2830,7 +2830,7 @@ bool cpu_svm_has_intercept(CPUX86State *env, uint32_t type);
 
 /* apic.c */
 void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
-void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
+void apic_handle_tpr_access_report(APICCommonState *s, target_ulong ip,
                                    TPRAccess access);
 
 /* Special values for X86CPUVersion: */
diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
index 5f83e8850a..5c908fdd6a 100644
--- a/target/i386/kvm/kvm_i386.h
+++ b/target/i386/kvm/kvm_i386.h
@@ -56,7 +56,7 @@ bool kvm_has_adjust_clock_stable(void);
 bool kvm_has_exception_payload(void);
 void kvm_synchronize_all_tsc(void);
 
-void kvm_get_apic_state(DeviceState *d, struct kvm_lapic_state *kapic);
+void kvm_get_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic);
 void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
 
 bool kvm_has_x2apic_api(void);
diff --git a/target/i386/whpx/whpx-internal.h b/target/i386/whpx/whpx-internal.h
index 6633e9c4ca..066e16bd8e 100644
--- a/target/i386/whpx/whpx-internal.h
+++ b/target/i386/whpx/whpx-internal.h
@@ -44,7 +44,7 @@ struct whpx_state {
 };
 
 extern struct whpx_state whpx_global;
-void whpx_apic_get(DeviceState *s);
+void whpx_apic_get(struct APICCommonState *s);
 
 #define WHV_E_UNKNOWN_CAPABILITY 0x80370300L
 
diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index 1be9bfe36e..82355f0463 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -60,9 +60,8 @@ static void kvm_put_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic
     kvm_apic_set_reg(kapic, 0x3e, s->divide_conf);
 }
 
-void kvm_get_apic_state(DeviceState *dev, struct kvm_lapic_state *kapic)
+void kvm_get_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic)
 {
-    APICCommonState *s = APIC_COMMON(dev);
     int i, v;
 
     if (kvm_has_x2apic_api() && s->apicbase & MSR_IA32_APICBASE_EXTD) {
diff --git a/hw/i386/vapic.c b/hw/i386/vapic.c
index 0c1c92c479..f1089f0a7c 100644
--- a/hw/i386/vapic.c
+++ b/hw/i386/vapic.c
@@ -490,7 +490,7 @@ void vapic_report_tpr_access(DeviceState *dev, CPUState *cs, target_ulong ip,
 }
 
 typedef struct VAPICEnableTPRReporting {
-    DeviceState *apic;
+    APICCommonState *apic;
     bool enable;
 } VAPICEnableTPRReporting;
 
diff --git a/hw/i386/x86-cpu.c b/hw/i386/x86-cpu.c
index c876e6709e..1a86a853d5 100644
--- a/hw/i386/x86-cpu.c
+++ b/hw/i386/x86-cpu.c
@@ -86,7 +86,7 @@ int cpu_get_pic_interrupt(CPUX86State *env)
     return intno;
 }
 
-DeviceState *cpu_get_current_apic(void)
+APICCommonState *cpu_get_current_apic(void)
 {
     if (current_cpu) {
         X86CPU *cpu = X86_CPU(current_cpu);
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index c768033856..cb35c80c75 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -181,10 +181,8 @@ static void apic_local_deliver(APICCommonState *s, int vector)
     }
 }
 
-void apic_deliver_pic_intr(DeviceState *dev, int level)
+void apic_deliver_pic_intr(APICCommonState *s, int level)
 {
-    APICCommonState *s = APIC(dev);
-
     if (level) {
         apic_local_deliver(s, APIC_LVT_LINT0);
     } else {
@@ -301,10 +299,8 @@ static void apic_deliver_irq(uint32_t dest, uint8_t dest_mode,
     apic_bus_deliver(deliver_bitmask, delivery_mode, vector_num, trigger_mode);
 }
 
-bool is_x2apic_mode(DeviceState *dev)
+bool is_x2apic_mode(APICCommonState *s)
 {
-    APICCommonState *s = APIC(dev);
-
     return s->apicbase & MSR_IA32_APICBASE_EXTD;
 }
 
@@ -388,15 +384,12 @@ static void apic_set_tpr(APICCommonState *s, uint8_t val)
     }
 }
 
-int apic_get_highest_priority_irr(DeviceState *dev)
+int apic_get_highest_priority_irr(APICCommonState *s)
 {
-    APICCommonState *s;
-
-    if (!dev) {
+    if (!s) {
         /* no interrupts */
         return -1;
     }
-    s = APIC_COMMON(dev);
     return get_highest_priority_int(s->irr);
 }
 
@@ -458,22 +451,19 @@ static int apic_irq_pending(APICCommonState *s)
 static void apic_update_irq(APICCommonState *s)
 {
     CPUState *cpu;
-    DeviceState *dev = (DeviceState *)s;
 
     cpu = CPU(s->cpu);
     if (!qemu_cpu_is_self(cpu)) {
         cpu_interrupt(cpu, CPU_INTERRUPT_POLL);
     } else if (apic_irq_pending(s) > 0) {
         cpu_interrupt(cpu, CPU_INTERRUPT_HARD);
-    } else if (!apic_accept_pic_intr(dev) || !pic_get_output(isa_pic)) {
+    } else if (!apic_accept_pic_intr(s) || !pic_get_output(isa_pic)) {
         cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
     }
 }
 
-void apic_poll_irq(DeviceState *dev)
+void apic_poll_irq(APICCommonState *s)
 {
-    APICCommonState *s = APIC(dev);
-
     apic_sync_vapic(s, SYNC_FROM_VAPIC);
     apic_update_irq(s);
 }
@@ -516,7 +506,7 @@ static void apic_eoi(APICCommonState *s)
 
 static bool apic_match_dest(APICCommonState *apic, uint32_t dest)
 {
-    if (is_x2apic_mode(&apic->parent_obj)) {
+    if (is_x2apic_mode(apic)) {
         return apic->initial_apic_id == dest;
     } else {
         return apic->id == (uint8_t)dest;
@@ -550,7 +540,7 @@ static void apic_get_broadcast_bitmask(uint32_t *deliver_bitmask,
     for (i = 0; i < max_apics; i++) {
         apic_iter = local_apics[i];
         if (apic_iter) {
-            bool apic_in_x2apic = is_x2apic_mode(&apic_iter->parent_obj);
+            bool apic_in_x2apic = is_x2apic_mode(apic_iter);
 
             if (is_x2apic_broadcast && apic_in_x2apic) {
                 apic_set_bit(deliver_bitmask, i);
@@ -642,27 +632,24 @@ static void apic_startup(APICCommonState *s, int vector_num)
     cpu_interrupt(CPU(s->cpu), CPU_INTERRUPT_SIPI);
 }
 
-void apic_sipi(DeviceState *dev)
+void apic_sipi(APICCommonState *s)
 {
-    APICCommonState *s = APIC(dev);
-
     if (!s->wait_for_sipi)
         return;
     cpu_x86_load_seg_cache_sipi(s->cpu, s->sipi_vector);
     s->wait_for_sipi = 0;
 }
 
-static void apic_deliver(DeviceState *dev, uint32_t dest, uint8_t dest_mode,
+static void apic_deliver(APICCommonState *s, uint32_t dest, uint8_t dest_mode,
                          uint8_t delivery_mode, uint8_t vector_num,
                          uint8_t trigger_mode, uint8_t dest_shorthand)
 {
-    APICCommonState *s = APIC(dev);
     APICCommonState *apic_iter;
     uint32_t deliver_bitmask_size = max_apic_words * sizeof(uint32_t);
     g_autofree uint32_t *deliver_bitmask = g_new(uint32_t, max_apic_words);
     uint32_t current_apic_id;
 
-    if (is_x2apic_mode(dev)) {
+    if (is_x2apic_mode(s)) {
         current_apic_id = s->initial_apic_id;
     } else {
         current_apic_id = s->id;
@@ -709,18 +696,15 @@ static void apic_deliver(DeviceState *dev, uint32_t dest, uint8_t dest_mode,
 
 static bool apic_check_pic(APICCommonState *s)
 {
-    DeviceState *dev = (DeviceState *)s;
-
-    if (!apic_accept_pic_intr(dev) || !pic_get_output(isa_pic)) {
+    if (!apic_accept_pic_intr(s) || !pic_get_output(isa_pic)) {
         return false;
     }
-    apic_deliver_pic_intr(dev, 1);
+    apic_deliver_pic_intr(s, 1);
     return true;
 }
 
-int apic_get_interrupt(DeviceState *dev)
+int apic_get_interrupt(APICCommonState *s)
 {
-    APICCommonState *s = APIC(dev);
     int intno;
 
     /* if the APIC is installed or enabled, we let the 8259 handle the
@@ -752,9 +736,8 @@ int apic_get_interrupt(DeviceState *dev)
     return intno;
 }
 
-int apic_accept_pic_intr(DeviceState *dev)
+int apic_accept_pic_intr(APICCommonState *s)
 {
-    APICCommonState *s = APIC(dev);
     uint32_t lvt0;
 
     if (!s)
@@ -788,20 +771,18 @@ static void apic_timer(void *opaque)
 
 static int apic_register_read(int index, uint64_t *value)
 {
-    DeviceState *dev;
     APICCommonState *s;
     uint32_t val;
     int ret = 0;
 
-    dev = cpu_get_current_apic();
-    if (!dev) {
+    s = cpu_get_current_apic();
+    if (!s) {
         return -1;
     }
-    s = APIC(dev);
 
     switch(index) {
     case 0x02: /* id */
-        if (is_x2apic_mode(dev)) {
+        if (is_x2apic_mode(s)) {
             val = s->initial_apic_id;
         } else {
             val = s->id << 24;
@@ -828,14 +809,14 @@ static int apic_register_read(int index, uint64_t *value)
         val = 0;
         break;
     case 0x0d:
-        if (is_x2apic_mode(dev)) {
+        if (is_x2apic_mode(s)) {
             val = s->extended_log_dest;
         } else {
             val = s->log_dest << 24;
         }
         break;
     case 0x0e:
-        if (is_x2apic_mode(dev)) {
+        if (is_x2apic_mode(s)) {
             val = 0;
             ret = -1;
         } else {
@@ -902,14 +883,14 @@ static uint64_t apic_mem_read(void *opaque, hwaddr addr, unsigned size)
 
 int apic_msr_read(int index, uint64_t *val)
 {
-    DeviceState *dev;
+    APICCommonState *s;
 
-    dev = cpu_get_current_apic();
-    if (!dev) {
+    s = cpu_get_current_apic();
+    if (!s) {
         return -1;
     }
 
-    if (!is_x2apic_mode(dev)) {
+    if (!is_x2apic_mode(s)) {
         return -1;
     }
 
@@ -943,20 +924,18 @@ static void apic_send_msi(MSIMessage *msi)
 
 static int apic_register_write(int index, uint64_t val)
 {
-    DeviceState *dev;
     APICCommonState *s;
 
-    dev = cpu_get_current_apic();
-    if (!dev) {
+    s = cpu_get_current_apic();
+    if (!s) {
         return -1;
     }
-    s = APIC(dev);
 
     trace_apic_register_write(index, val);
 
     switch(index) {
     case 0x02:
-        if (is_x2apic_mode(dev)) {
+        if (is_x2apic_mode(s)) {
             return -1;
         }
 
@@ -979,14 +958,14 @@ static int apic_register_write(int index, uint64_t val)
         apic_eoi(s);
         break;
     case 0x0d:
-        if (is_x2apic_mode(dev)) {
+        if (is_x2apic_mode(s)) {
             return -1;
         }
 
         s->log_dest = val >> 24;
         break;
     case 0x0e:
-        if (is_x2apic_mode(dev)) {
+        if (is_x2apic_mode(s)) {
             return -1;
         }
 
@@ -1005,20 +984,20 @@ static int apic_register_write(int index, uint64_t val)
         uint32_t dest;
 
         s->icr[0] = val;
-        if (is_x2apic_mode(dev)) {
+        if (is_x2apic_mode(s)) {
             s->icr[1] = val >> 32;
             dest = s->icr[1];
         } else {
             dest = (s->icr[1] >> 24) & 0xff;
         }
 
-        apic_deliver(dev, dest, (s->icr[0] >> 11) & 1,
+        apic_deliver(s, dest, (s->icr[0] >> 11) & 1,
                      (s->icr[0] >> 8) & 7, (s->icr[0] & 0xff),
                      (s->icr[0] >> 15) & 1, (s->icr[0] >> 18) & 3);
         break;
     }
     case 0x31:
-        if (is_x2apic_mode(dev)) {
+        if (is_x2apic_mode(s)) {
             return -1;
         }
 
@@ -1053,7 +1032,7 @@ static int apic_register_write(int index, uint64_t val)
     case 0x3f: {
         int vector = val & 0xff;
 
-        if (!is_x2apic_mode(dev)) {
+        if (!is_x2apic_mode(s)) {
             return -1;
         }
 
@@ -1063,7 +1042,7 @@ static int apic_register_write(int index, uint64_t val)
          * - Trigger mode: 0 (Edge)
          * - Delivery mode: 0 (Fixed)
          */
-        apic_deliver(dev, 0, 0, APIC_DM_FIXED, vector, 0, 1);
+        apic_deliver(s, 0, 0, APIC_DM_FIXED, vector, 0, 1);
 
         break;
     }
@@ -1102,14 +1081,14 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
 
 int apic_msr_write(int index, uint64_t val)
 {
-    DeviceState *dev;
+    APICCommonState *s;
 
-    dev = cpu_get_current_apic();
-    if (!dev) {
+    s = cpu_get_current_apic();
+    if (!s) {
         return -1;
     }
 
-    if (!is_x2apic_mode(dev)) {
+    if (!is_x2apic_mode(s)) {
         return -1;
     }
 
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 394fe02013..ec9e978b0b 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -35,12 +35,11 @@
 
 bool apic_report_tpr_access;
 
-int cpu_set_apic_base(DeviceState *dev, uint64_t val)
+int cpu_set_apic_base(APICCommonState *s, uint64_t val)
 {
     trace_cpu_set_apic_base(val);
 
-    if (dev) {
-        APICCommonState *s = APIC_COMMON(dev);
+    if (s) {
         APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
         /* Reset possibly modified xAPIC ID */
         s->id = s->initial_apic_id;
@@ -50,10 +49,9 @@ int cpu_set_apic_base(DeviceState *dev, uint64_t val)
     return 0;
 }
 
-uint64_t cpu_get_apic_base(DeviceState *dev)
+uint64_t cpu_get_apic_base(APICCommonState *s)
 {
-    if (dev) {
-        APICCommonState *s = APIC_COMMON(dev);
+    if (s) {
         trace_cpu_get_apic_base((uint64_t)s->apicbase);
         return s->apicbase;
     } else {
@@ -62,52 +60,43 @@ uint64_t cpu_get_apic_base(DeviceState *dev)
     }
 }
 
-bool cpu_is_apic_enabled(DeviceState *dev)
+bool cpu_is_apic_enabled(APICCommonState *s)
 {
-    APICCommonState *s;
-
-    if (!dev) {
+    if (!s) {
         return false;
     }
 
-    s = APIC_COMMON(dev);
-
     return s->apicbase & MSR_IA32_APICBASE_ENABLE;
 }
 
-void cpu_set_apic_tpr(DeviceState *dev, uint8_t val)
+void cpu_set_apic_tpr(APICCommonState *s, uint8_t val)
 {
-    APICCommonState *s;
     APICCommonClass *info;
 
-    if (!dev) {
+    if (!s) {
         return;
     }
 
-    s = APIC_COMMON(dev);
     info = APIC_COMMON_GET_CLASS(s);
 
     info->set_tpr(s, val);
 }
 
-uint8_t cpu_get_apic_tpr(DeviceState *dev)
+uint8_t cpu_get_apic_tpr(APICCommonState *s)
 {
-    APICCommonState *s;
     APICCommonClass *info;
 
-    if (!dev) {
+    if (!s) {
         return 0;
     }
 
-    s = APIC_COMMON(dev);
     info = APIC_COMMON_GET_CLASS(s);
 
     return info->get_tpr(s);
 }
 
-void apic_enable_tpr_access_reporting(DeviceState *dev, bool enable)
+void apic_enable_tpr_access_reporting(APICCommonState *s, bool enable)
 {
-    APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
 
     apic_report_tpr_access = enable;
@@ -116,26 +105,22 @@ void apic_enable_tpr_access_reporting(DeviceState *dev, bool enable)
     }
 }
 
-void apic_enable_vapic(DeviceState *dev, hwaddr paddr)
+void apic_enable_vapic(APICCommonState *s, hwaddr paddr)
 {
-    APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
 
     s->vapic_paddr = paddr;
     info->vapic_base_update(s);
 }
 
-void apic_handle_tpr_access_report(DeviceState *dev, target_ulong ip,
+void apic_handle_tpr_access_report(APICCommonState *s, target_ulong ip,
                                    TPRAccess access)
 {
-    APICCommonState *s = APIC_COMMON(dev);
-
     vapic_report_tpr_access(s->vapic, CPU(s->cpu), ip, access);
 }
 
-void apic_deliver_nmi(DeviceState *dev)
+void apic_deliver_nmi(APICCommonState *s)
 {
-    APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
 
     info->external_nmi(s);
@@ -193,16 +178,14 @@ uint32_t apic_get_current_count(APICCommonState *s)
     return val;
 }
 
-void apic_init_reset(DeviceState *dev)
+void apic_init_reset(APICCommonState *s)
 {
-    APICCommonState *s;
     APICCommonClass *info;
     int i;
 
-    if (!dev) {
+    if (!s) {
         return;
     }
-    s = APIC_COMMON(dev);
     s->tpr = 0;
     s->spurious_vec = 0xff;
     s->log_dest = 0;
@@ -233,13 +216,12 @@ void apic_init_reset(DeviceState *dev)
     }
 }
 
-void apic_designate_bsp(DeviceState *dev, bool bsp)
+void apic_designate_bsp(APICCommonState *s, bool bsp)
 {
-    if (dev == NULL) {
+    if (s == NULL) {
         return;
     }
 
-    APICCommonState *s = APIC_COMMON(dev);
     if (bsp) {
         s->apicbase |= MSR_IA32_APICBASE_BSP;
     } else {
@@ -262,7 +244,7 @@ static void apic_reset_common(DeviceState *dev)
     s->vapic_paddr = 0;
     info->vapic_base_update(s);
 
-    apic_init_reset(dev);
+    apic_init_reset(s);
 }
 
 static const VMStateDescription vmstate_apic_common;
diff --git a/target/i386/cpu-apic.c b/target/i386/cpu-apic.c
index 242a05fdbe..564c1288e4 100644
--- a/target/i386/cpu-apic.c
+++ b/target/i386/cpu-apic.c
@@ -41,34 +41,31 @@ APICCommonClass *apic_get_class(Error **errp)
 
 void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
-    APICCommonState *apic;
     APICCommonClass *apic_class = apic_get_class(errp);
 
     if (!apic_class) {
         return;
     }
 
-    cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
+    cpu->apic_state = APIC_COMMON(object_new_with_class(OBJECT_CLASS(apic_class)));
     object_property_add_child(OBJECT(cpu), "lapic",
                               OBJECT(cpu->apic_state));
     object_unref(OBJECT(cpu->apic_state));
 
     /* TODO: convert to link<> */
-    apic = APIC_COMMON(cpu->apic_state);
-    apic->cpu = cpu;
-    apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
+    cpu->apic_state->cpu = cpu;
+    cpu->apic_state->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
 
     /*
      * apic_common_set_id needs to check if the CPU has x2APIC
-     * feature in case APIC ID >= 255, so we need to set apic->cpu
+     * feature in case APIC ID >= 255, so we need to set cpu->apic_state->cpu
      * before setting APIC ID
      */
-    qdev_prop_set_uint32(cpu->apic_state, "id", cpu->apic_id);
+    qdev_prop_set_uint32(DEVICE(cpu->apic_state), "id", cpu->apic_id);
 }
 
 void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 {
-    APICCommonState *apic;
     static bool apic_mmio_map_once;
 
     if (cpu->apic_state == NULL) {
@@ -77,12 +74,11 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
     qdev_realize(DEVICE(cpu->apic_state), NULL, errp);
 
     /* Map APIC MMIO area */
-    apic = APIC_COMMON(cpu->apic_state);
     if (!apic_mmio_map_once) {
         memory_region_add_subregion_overlap(get_system_memory(),
-                                            apic->apicbase &
+                                            cpu->apic_state->apicbase &
                                             MSR_IA32_APICBASE_BASE,
-                                            &apic->io_memory,
+                                            &cpu->apic_state->io_memory,
                                             0x1000);
         apic_mmio_map_once = true;
      }
diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
index a72ed93bd2..67bf31e0ca 100644
--- a/target/i386/cpu-dump.c
+++ b/target/i386/cpu-dump.c
@@ -291,7 +291,7 @@ static void dump_apic_interrupt(const char *name, uint32_t *ireg_tab,
 void x86_cpu_dump_local_apic_state(CPUState *cs, int flags)
 {
     X86CPU *cpu = X86_CPU(cs);
-    APICCommonState *s = APIC_COMMON(cpu->apic_state);
+    APICCommonState *s = cpu->apic_state;
     if (!s) {
         qemu_printf("local apic state not available\n");
         return;
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 455caff6b2..0a66e1fec9 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -8789,7 +8789,7 @@ void x86_cpu_after_reset(X86CPU *cpu)
     }
 
     if (cpu->apic_state) {
-        device_cold_reset(cpu->apic_state);
+        device_cold_reset(DEVICE(cpu->apic_state));
     }
 #endif
 }
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 309f043373..f7a6ef650a 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5029,7 +5029,7 @@ static int kvm_get_mp_state(X86CPU *cpu)
 
 static int kvm_get_apic(X86CPU *cpu)
 {
-    DeviceState *apic = cpu->apic_state;
+    APICCommonState *apic = cpu->apic_state;
     struct kvm_lapic_state kapic;
     int ret;
 
diff --git a/target/i386/whpx/whpx-apic.c b/target/i386/whpx/whpx-apic.c
index e1ef6d4e6d..afcb25843b 100644
--- a/target/i386/whpx/whpx-apic.c
+++ b/target/i386/whpx/whpx-apic.c
@@ -151,9 +151,8 @@ static void whpx_apic_put(CPUState *cs, run_on_cpu_data data)
     }
 }
 
-void whpx_apic_get(DeviceState *dev)
+void whpx_apic_get(APICCommonState *s)
 {
-    APICCommonState *s = APIC_COMMON(dev);
     CPUState *cpu = CPU(s->cpu);
     struct whpx_lapic_state kapic;
 
-- 
2.51.1.dirty

Re: [PATCH v3 07/10] hw/i386/apic: Prefer APICCommonState over DeviceState

[Philippe Mathieu-Daudé]

On 19/10/25 23:03, Bernhard Beschow wrote:
> Makes the APIC API more type-safe by resolving quite a few APIC_COMMON
> downcasts.

Nice!

> Like PICCommonState, the APICCommonState is now a public typedef while staying
> an abstract datatype.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/i386/apic.h           | 33 +++++------
>   include/hw/i386/apic_internal.h  |  7 +--
>   target/i386/cpu.h                |  4 +-
>   target/i386/kvm/kvm_i386.h       |  2 +-
>   target/i386/whpx/whpx-internal.h |  2 +-
>   hw/i386/kvm/apic.c               |  3 +-
>   hw/i386/vapic.c                  |  2 +-
>   hw/i386/x86-cpu.c                |  2 +-
>   hw/intc/apic.c                   | 97 +++++++++++++-------------------
>   hw/intc/apic_common.c            | 56 +++++++-----------
>   target/i386/cpu-apic.c           | 18 +++---
>   target/i386/cpu-dump.c           |  2 +-
>   target/i386/cpu.c                |  2 +-
>   target/i386/kvm/kvm.c            |  2 +-
>   target/i386/whpx/whpx-apic.c     |  3 +-
>   15 files changed, 95 insertions(+), 140 deletions(-)


> -int apic_get_highest_priority_irr(DeviceState *dev)
> +int apic_get_highest_priority_irr(APICCommonState *s)
>   {
> -    APICCommonState *s;
> -
> -    if (!dev) {
> +    if (!s) {
>           /* no interrupts */

Pre-existing dubious check.

>           return -1;
>       }
> -    s = APIC_COMMON(dev);
>       return get_highest_priority_int(s->irr);
>   }

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 08/10] hw/i386/apic: Ensure own APIC use in apic_msr_{read, write}

[Bernhard Beschow]

Avoids the `current_cpu` global and seems more robust by not "forgetting" the
own APIC and then re-determining it by cpu_get_current_apic() which uses the
global.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/apic.h               |  4 ++--
 hw/intc/apic.c                       | 10 ++--------
 target/i386/hvf/hvf.c                |  4 ++--
 target/i386/tcg/system/misc_helper.c |  4 ++--
 4 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
index 871f142888..6a0933f401 100644
--- a/include/hw/i386/apic.h
+++ b/include/hw/i386/apic.h
@@ -19,8 +19,8 @@ void apic_sipi(APICCommonState *s);
 void apic_poll_irq(APICCommonState *s);
 void apic_designate_bsp(APICCommonState *s, bool bsp);
 int apic_get_highest_priority_irr(APICCommonState *s);
-int apic_msr_read(int index, uint64_t *val);
-int apic_msr_write(int index, uint64_t val);
+int apic_msr_read(APICCommonState *s, int index, uint64_t *val);
+int apic_msr_write(APICCommonState *s, int index, uint64_t val);
 bool is_x2apic_mode(APICCommonState *s);
 
 /* pc.c */
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index cb35c80c75..ba0eda3921 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -881,11 +881,8 @@ static uint64_t apic_mem_read(void *opaque, hwaddr addr, unsigned size)
     return val;
 }
 
-int apic_msr_read(int index, uint64_t *val)
+int apic_msr_read(APICCommonState *s, int index, uint64_t *val)
 {
-    APICCommonState *s;
-
-    s = cpu_get_current_apic();
     if (!s) {
         return -1;
     }
@@ -1079,11 +1076,8 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
     apic_register_write(index, val);
 }
 
-int apic_msr_write(int index, uint64_t val)
+int apic_msr_write(APICCommonState *s, int index, uint64_t val)
 {
-    APICCommonState *s;
-
-    s = cpu_get_current_apic();
     if (!s) {
         return -1;
     }
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 8445cadece..33f723a76a 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -527,7 +527,7 @@ void hvf_simulate_rdmsr(CPUState *cs)
         int ret;
         int index = (uint32_t)env->regs[R_ECX] - MSR_APIC_START;
 
-        ret = apic_msr_read(index, &val);
+        ret = apic_msr_read(cpu->apic_state, index, &val);
         if (ret < 0) {
             x86_emul_raise_exception(env, EXCP0D_GPF, 0);
         }
@@ -638,7 +638,7 @@ void hvf_simulate_wrmsr(CPUState *cs)
         int ret;
         int index = (uint32_t)env->regs[R_ECX] - MSR_APIC_START;
 
-        ret = apic_msr_write(index, data);
+        ret = apic_msr_write(cpu->apic_state, index, data);
         if (ret < 0) {
             x86_emul_raise_exception(env, EXCP0D_GPF, 0);
         }
diff --git a/target/i386/tcg/system/misc_helper.c b/target/i386/tcg/system/misc_helper.c
index 9c3f5cc99b..0c32424d36 100644
--- a/target/i386/tcg/system/misc_helper.c
+++ b/target/i386/tcg/system/misc_helper.c
@@ -299,7 +299,7 @@ void helper_wrmsr(CPUX86State *env)
         int index = (uint32_t)env->regs[R_ECX] - MSR_APIC_START;
 
         bql_lock();
-        ret = apic_msr_write(index, val);
+        ret = apic_msr_write(env_archcpu(env)->apic_state, index, val);
         bql_unlock();
         if (ret < 0) {
             goto error;
@@ -477,7 +477,7 @@ void helper_rdmsr(CPUX86State *env)
         int index = (uint32_t)env->regs[R_ECX] - MSR_APIC_START;
 
         bql_lock();
-        ret = apic_msr_read(index, &val);
+        ret = apic_msr_read(x86_cpu->apic_state, index, &val);
         bql_unlock();
         if (ret < 0) {
             raise_exception_err_ra(env, EXCP0D_GPF, 0, GETPC());
-- 
2.51.1.dirty

Re: [PATCH v3 08/10] hw/i386/apic: Ensure own APIC use in apic_msr_{read, write}

[Philippe Mathieu-Daudé]

On 19/10/25 23:03, Bernhard Beschow wrote:
> Avoids the `current_cpu` global and seems more robust by not "forgetting" the
> own APIC and then re-determining it by cpu_get_current_apic() which uses the
> global.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/i386/apic.h               |  4 ++--
>   hw/intc/apic.c                       | 10 ++--------
>   target/i386/hvf/hvf.c                |  4 ++--
>   target/i386/tcg/system/misc_helper.c |  4 ++--
>   4 files changed, 8 insertions(+), 14 deletions(-)

Good cleanup!

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 08/10] hw/i386/apic: Ensure own APIC use in apic_msr_{read, write}

[Bernhard Beschow]

Am 20. Oktober 2025 06:09:22 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 19/10/25 23:03, Bernhard Beschow wrote:
>> Avoids the `current_cpu` global and seems more robust by not "forgetting" the
>> own APIC and then re-determining it by cpu_get_current_apic() which uses the
>> global.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/hw/i386/apic.h               |  4 ++--
>>   hw/intc/apic.c                       | 10 ++--------
>>   target/i386/hvf/hvf.c                |  4 ++--
>>   target/i386/tcg/system/misc_helper.c |  4 ++--
>>   4 files changed, 8 insertions(+), 14 deletions(-)
>
>Good cleanup!
>
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks! I think it would be possible to remove cpu_get_current_apic() entirely if each local APIC's memory region could be bound to the address space of each CPU. However, it seems that the respective root memory regions aren't prepared for that and I didn't want to go into this rabbit hole here in this context.

Best regards,
Bernhard

[PATCH v3 09/10] hw/intc/apic: Pass APICCommonState to apic_register_{read, write}

[Bernhard Beschow]

As per the previous patch, the APIC instance is already available in
apic_msr_{read,write}, so it can be passed along. It turns out that
the call to cpu_get_current_apic() is only required in
apic_mem_{read,write}, so it has been moved there. Longer term,
cpu_get_current_apic() could be removed entirely if
apic_mem_{read,write} is tied to a CPU's local address space.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/intc/apic.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index ba0eda3921..077ef18686 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -769,17 +769,11 @@ static void apic_timer(void *opaque)
     apic_timer_update(s, s->next_time);
 }
 
-static int apic_register_read(int index, uint64_t *value)
+static int apic_register_read(APICCommonState *s, int index, uint64_t *value)
 {
-    APICCommonState *s;
     uint32_t val;
     int ret = 0;
 
-    s = cpu_get_current_apic();
-    if (!s) {
-        return -1;
-    }
-
     switch(index) {
     case 0x02: /* id */
         if (is_x2apic_mode(s)) {
@@ -868,6 +862,7 @@ static int apic_register_read(int index, uint64_t *value)
 
 static uint64_t apic_mem_read(void *opaque, hwaddr addr, unsigned size)
 {
+    APICCommonState *s = cpu_get_current_apic();
     uint64_t val;
     int index;
 
@@ -875,8 +870,12 @@ static uint64_t apic_mem_read(void *opaque, hwaddr addr, unsigned size)
         return 0;
     }
 
+    if (!s) {
+        return -1;
+    }
+
     index = (addr >> 4) & 0xff;
-    apic_register_read(index, &val);
+    apic_register_read(s, index, &val);
 
     return val;
 }
@@ -891,7 +890,7 @@ int apic_msr_read(APICCommonState *s, int index, uint64_t *val)
         return -1;
     }
 
-    return apic_register_read(index, val);
+    return apic_register_read(s, index, val);
 }
 
 static void apic_send_msi(MSIMessage *msi)
@@ -919,15 +918,8 @@ static void apic_send_msi(MSIMessage *msi)
     apic_deliver_irq(dest, dest_mode, delivery, vector, trigger_mode);
 }
 
-static int apic_register_write(int index, uint64_t val)
+static int apic_register_write(APICCommonState *s, int index, uint64_t val)
 {
-    APICCommonState *s;
-
-    s = cpu_get_current_apic();
-    if (!s) {
-        return -1;
-    }
-
     trace_apic_register_write(index, val);
 
     switch(index) {
@@ -1054,12 +1046,17 @@ static int apic_register_write(int index, uint64_t val)
 static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
                            unsigned size)
 {
+    APICCommonState *s = cpu_get_current_apic();
     int index = (addr >> 4) & 0xff;
 
     if (size < 4) {
         return;
     }
 
+    if (!s) {
+        return;
+    }
+
     if (addr > 0xfff || !index) {
         /*
          * MSI and MMIO APIC are at the same memory location,
@@ -1073,7 +1070,7 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
         return;
     }
 
-    apic_register_write(index, val);
+    apic_register_write(s, index, val);
 }
 
 int apic_msr_write(APICCommonState *s, int index, uint64_t val)
@@ -1086,7 +1083,7 @@ int apic_msr_write(APICCommonState *s, int index, uint64_t val)
         return -1;
     }
 
-    return apic_register_write(index, val);
+    return apic_register_write(s, index, val);
 }
 
 static void apic_pre_save(APICCommonState *s)
-- 
2.51.1.dirty

Re: [PATCH v3 09/10] hw/intc/apic: Pass APICCommonState to apic_register_{read, write}

[Philippe Mathieu-Daudé]

On 19/10/25 23:03, Bernhard Beschow wrote:
> As per the previous patch, the APIC instance is already available in
> apic_msr_{read,write}, so it can be passed along. It turns out that
> the call to cpu_get_current_apic() is only required in
> apic_mem_{read,write}, so it has been moved there. Longer term,
> cpu_get_current_apic() could be removed entirely if
> apic_mem_{read,write} is tied to a CPU's local address space.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/intc/apic.c | 35 ++++++++++++++++-------------------
>   1 file changed, 16 insertions(+), 19 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 09/10] hw/intc/apic: Pass APICCommonState to apic_register_{read, write}

[Philippe Mathieu-Daudé]

Hi Bernhard,

On 19/10/25 23:03, Bernhard Beschow wrote:
> As per the previous patch, the APIC instance is already available in
> apic_msr_{read,write}, so it can be passed along. It turns out that
> the call to cpu_get_current_apic() is only required in
> apic_mem_{read,write}, so it has been moved there. Longer term,
> cpu_get_current_apic() could be removed entirely if
> apic_mem_{read,write} is tied to a CPU's local address space.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/intc/apic.c | 35 ++++++++++++++++-------------------
>   1 file changed, 16 insertions(+), 19 deletions(-)


> @@ -1054,12 +1046,17 @@ static int apic_register_write(int index, uint64_t val)
>   static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
>                              unsigned size)
>   {
> +    APICCommonState *s = cpu_get_current_apic();
>       int index = (addr >> 4) & 0xff;
>   
>       if (size < 4) {
>           return;
>       }
>   
> +    if (!s) {
> +        return;
> +    }

This is not the correct place to return...

>       if (addr > 0xfff || !index) {
>           /*
>            * MSI and MMIO APIC are at the same memory location,

... because of this comment. See the (squashed) fix below.

> @@ -1073,7 +1070,7 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
>           return;
>       }
>   
> -    apic_register_write(index, val);
> +    apic_register_write(s, index, val);
>   }

-- >8 --
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 077ef18686b..aad253af158 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -1046,30 +1046,30 @@ static int apic_register_write(APICCommonState 
*s, int index, uint64_t val)
  static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
                             unsigned size)
  {
      APICCommonState *s = cpu_get_current_apic();
      int index = (addr >> 4) & 0xff;

      if (size < 4) {
          return;
      }

-    if (!s) {
-        return;
-    }
-
      if (addr > 0xfff || !index) {
          /*
           * MSI and MMIO APIC are at the same memory location,
           * but actually not on the global bus: MSI is on PCI bus
           * APIC is connected directly to the CPU.
           * Mapping them on the global bus happens to work because
           * MSI registers are reserved in APIC MMIO and vice versa.
           */
          MSIMessage msi = { .address = addr, .data = val };
          apic_send_msi(&msi);
          return;
      }

+    if (!s) {
+        return;
+    }
+
      apic_register_write(s, index, val);
  }

---

[PATCH v3 10/10] tests/qtest/ds1338-test: Reuse from_bcd()

[Bernhard Beschow]

from_bcd() is a public API function which can be unit-tested. Reuse it to avoid
code duplication.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 tests/qtest/ds1338-test.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tests/qtest/ds1338-test.c b/tests/qtest/ds1338-test.c
index d12424d27f..b8d0e65ec4 100644
--- a/tests/qtest/ds1338-test.c
+++ b/tests/qtest/ds1338-test.c
@@ -18,16 +18,12 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/bcd.h"
 #include "libqtest.h"
 #include "libqos/i2c.h"
 
 #define DS1338_ADDR 0x68
 
-static inline uint8_t bcd2bin(uint8_t x)
-{
-    return ((x) & 0x0f) + ((x) >> 4) * 10;
-}
-
 static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
 {
     QI2CDevice *i2cdev = (QI2CDevice *)obj;
@@ -39,9 +35,9 @@ static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
     i2c_read_block(i2cdev, 0, resp, sizeof(resp));
 
     /* check retrieved time against local time */
-    g_assert_cmpuint(bcd2bin(resp[4]), == , tm_ptr->tm_mday);
-    g_assert_cmpuint(bcd2bin(resp[5]), == , 1 + tm_ptr->tm_mon);
-    g_assert_cmpuint(2000 + bcd2bin(resp[6]), == , 1900 + tm_ptr->tm_year);
+    g_assert_cmpuint(from_bcd(resp[4]), == , tm_ptr->tm_mday);
+    g_assert_cmpuint(from_bcd(resp[5]), == , 1 + tm_ptr->tm_mon);
+    g_assert_cmpuint(2000 + from_bcd(resp[6]), == , 1900 + tm_ptr->tm_year);
 }
 
 static void ds1338_register_nodes(void)
-- 
2.51.1.dirty

Re: [PATCH v3 10/10] tests/qtest/ds1338-test: Reuse from_bcd()

[Philippe Mathieu-Daudé]

On 19/10/25 23:03, Bernhard Beschow wrote:
> from_bcd() is a public API function which can be unit-tested. Reuse it to avoid
> code duplication.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   tests/qtest/ds1338-test.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 00/10] Cleanup patches, mostly PC-related

[Philippe Mathieu-Daudé]

On 19/10/25 23:02, Bernhard Beschow wrote:

> Bernhard Beschow (10):
>    hw/timer/i8254: Add I/O trace events
>    hw/audio/pcspk: Add I/O trace events
>    hw/rtc/mc146818rtc: Convert CMOS_DPRINTF() into trace events
>    hw/rtc/mc146818rtc: Use ARRAY_SIZE macro
>    hw/rtc/mc146818rtc: Assert correct usage of
>      mc146818rtc_set_cmos_data()
>    hw/ide/ide-internal: Move dma_buf_commit() into ide "namespace"
>    hw/i386/apic: Prefer APICCommonState over DeviceState
>    hw/i386/apic: Ensure own APIC use in apic_msr_{read,write}
>    hw/intc/apic: Pass APICCommonState to apic_register_{read,write}
>    tests/qtest/ds1338-test: Reuse from_bcd()

Thanks, except if Paolo/MST/Igor object, series queued squashing:

-- >8 --
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 67ff52a8b40..d981ca05977 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -29,2 +29,3 @@
  #include "exec/memop.h"
+#include "hw/i386/apic.h"
  #include "hw/i386/topology.h"
@@ -2352,3 +2352,3 @@ struct ArchCPU {
         user */
-    struct APICCommonState *apic_state;
+    APICCommonState *apic_state;
      struct MemoryRegion *cpu_as_root, *cpu_as_mem, *smram;
diff --git a/target/i386/whpx/whpx-internal.h 
b/target/i386/whpx/whpx-internal.h
index 066e16bd8e2..2dcad1f5650 100644
--- a/target/i386/whpx/whpx-internal.h
+++ b/target/i386/whpx/whpx-internal.h
@@ -7,2 +7,4 @@

+#include "hw/i386/apic.h"
+
  typedef enum WhpxBreakpointState {
@@ -46,3 +48,3 @@ struct whpx_state {
  extern struct whpx_state whpx_global;
-void whpx_apic_get(struct APICCommonState *s);
+void whpx_apic_get(APICCommonState *s);

---

Re: [PATCH v3 00/10] Cleanup patches, mostly PC-related

[Philippe Mathieu-Daudé]

On 21/10/25 18:07, Philippe Mathieu-Daudé wrote:
> On 19/10/25 23:02, Bernhard Beschow wrote:
> 
>> Bernhard Beschow (10):
>>    hw/timer/i8254: Add I/O trace events
>>    hw/audio/pcspk: Add I/O trace events
>>    hw/rtc/mc146818rtc: Convert CMOS_DPRINTF() into trace events
>>    hw/rtc/mc146818rtc: Use ARRAY_SIZE macro
>>    hw/rtc/mc146818rtc: Assert correct usage of
>>      mc146818rtc_set_cmos_data()
>>    hw/ide/ide-internal: Move dma_buf_commit() into ide "namespace"
>>    hw/i386/apic: Prefer APICCommonState over DeviceState
>>    hw/i386/apic: Ensure own APIC use in apic_msr_{read,write}
>>    hw/intc/apic: Pass APICCommonState to apic_register_{read,write}
>>    tests/qtest/ds1338-test: Reuse from_bcd()
> 
> Thanks, except if Paolo/MST/Igor object, series queued squashing:
> 
> -- >8 --
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 67ff52a8b40..d981ca05977 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -29,2 +29,3 @@
>   #include "exec/memop.h"
> +#include "hw/i386/apic.h"
>   #include "hw/i386/topology.h"
> @@ -2352,3 +2352,3 @@ struct ArchCPU {
>          user */
> -    struct APICCommonState *apic_state;
> +    APICCommonState *apic_state;
>       struct MemoryRegion *cpu_as_root, *cpu_as_mem, *smram;
> diff --git a/target/i386/whpx/whpx-internal.h b/target/i386/whpx/whpx- 
> internal.h
> index 066e16bd8e2..2dcad1f5650 100644
> --- a/target/i386/whpx/whpx-internal.h
> +++ b/target/i386/whpx/whpx-internal.h
> @@ -7,2 +7,4 @@
> 
> +#include "hw/i386/apic.h"
> +
>   typedef enum WhpxBreakpointState {
> @@ -46,3 +48,3 @@ struct whpx_state {
>   extern struct whpx_state whpx_global;
> -void whpx_apic_get(struct APICCommonState *s);
> +void whpx_apic_get(APICCommonState *s);
> 
> ---

Also squashing:

-- >8 --
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 077ef18686b..aad253af158 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -1056,4 +1055,0 @@ static void apic_mem_write(void *opaque, hwaddr 
addr, uint64_t val,
-    if (!s) {
-        return;
-    }
-
@@ -1072,0 +1069,4 @@ static void apic_mem_write(void *opaque, hwaddr 
addr, uint64_t val,
+    if (!s) {
+        return;
+    }
+
---