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 <shen...@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);
 }

---