[v3] Cleanup patches, mostly PC-related

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

Posted by Bernhard Beschow 3 months, 3 weeks ago

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}

Posted by Philippe Mathieu-Daudé 3 months, 2 weeks ago

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

---

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

Posted by Philippe Mathieu-Daudé 3 months, 2 weeks ago

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>