[PATCH v5 18/20] hw/i386: convert apic access to use MemTxAttrs

["Alex Bennée" <alex.bennee@linaro.org>]

This allows us to correctly model invalid accesses to the interrupt
controller as well as avoiding the use of current_cpu hacks to find
the APIC structure. We have to ensure we check for MSI signals first
which shouldn't arrive from the CPU but are either triggered by PCI or
internal IOAPIC writes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Xu <peterx@redhat.com>

---
v1
  - don't validate requester_id for MTRT_MACHINE, just assume IOPIC
---
 include/hw/i386/apic.h |  2 +-
 hw/i386/x86.c          | 11 +++-----
 hw/intc/apic.c         | 62 ++++++++++++++++++++++++++++--------------
 3 files changed, 46 insertions(+), 29 deletions(-)

diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
index da1d2fe155..064ea5ac1b 100644
--- a/include/hw/i386/apic.h
+++ b/include/hw/i386/apic.h
@@ -22,6 +22,6 @@ void apic_designate_bsp(DeviceState *d, bool bsp);
 int apic_get_highest_priority_irr(DeviceState *dev);
 
 /* pc.c */
-DeviceState *cpu_get_current_apic(void);
+DeviceState *cpu_get_current_apic(int cpu_index);
 
 #endif
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 78cc131926..66645a669c 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -585,14 +585,11 @@ int cpu_get_pic_interrupt(CPUX86State *env)
     return intno;
 }
 
-DeviceState *cpu_get_current_apic(void)
+DeviceState *cpu_get_current_apic(int cpu_index)
 {
-    if (current_cpu) {
-        X86CPU *cpu = X86_CPU(current_cpu);
-        return cpu->apic_state;
-    } else {
-        return NULL;
-    }
+    CPUState *cs = qemu_get_cpu(cpu_index);
+    X86CPU *cpu = X86_CPU(cs);
+    return cpu->apic_state;
 }
 
 void gsi_handler(void *opaque, int n, int level)
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 3df11c34d6..0a9897e64f 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -18,9 +18,11 @@
  */
 #include "qemu/osdep.h"
 #include "qemu/thread.h"
+#include "qemu/log.h"
 #include "hw/i386/apic_internal.h"
 #include "hw/i386/apic.h"
 #include "hw/i386/ioapic.h"
+#include "hw/i386/ioapic_internal.h"
 #include "hw/intc/i8259.h"
 #include "hw/pci/msi.h"
 #include "qemu/host-utils.h"
@@ -634,21 +636,23 @@ static void apic_timer(void *opaque)
     apic_timer_update(s, s->next_time);
 }
 
-static uint64_t apic_mem_read(void *opaque, hwaddr addr, unsigned size)
+static MemTxResult apic_mem_read(void *opaque, hwaddr addr, uint64_t *data,
+                                 unsigned int size, MemTxAttrs attrs)
 {
     DeviceState *dev;
     APICCommonState *s;
     uint32_t val;
     int index;
 
-    if (size < 4) {
-        return 0;
+    if (attrs.requester_type != MTRT_CPU) {
+        return MEMTX_ACCESS_ERROR;
     }
+    dev = cpu_get_current_apic(attrs.requester_id);
 
-    dev = cpu_get_current_apic();
-    if (!dev) {
-        return 0;
+    if (size < 4) {
+        return MEMTX_ERROR;
     }
+
     s = APIC(dev);
 
     index = (addr >> 4) & 0xff;
@@ -719,7 +723,8 @@ static uint64_t apic_mem_read(void *opaque, hwaddr addr, unsigned size)
         break;
     }
     trace_apic_mem_readl(addr, val);
-    return val;
+    *data = val;
+    return MEMTX_OK;
 }
 
 static void apic_send_msi(MSIMessage *msi)
@@ -735,32 +740,45 @@ static void apic_send_msi(MSIMessage *msi)
     apic_deliver_irq(dest, dest_mode, delivery, vector, trigger_mode);
 }
 
-static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
-                           unsigned size)
+static MemTxResult apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
+                                  unsigned int size, MemTxAttrs attrs)
 {
     DeviceState *dev;
     APICCommonState *s;
     int index = (addr >> 4) & 0xff;
 
     if (size < 4) {
-        return;
+        return MEMTX_ERROR;
     }
 
+    /*
+     * 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.
+     *
+     * We can check the MemTxAttrs to check they are coming from where
+     * we expect. Even though the MSI registers are reserved in APIC
+     * MMIO and vice versa they shouldn't respond to CPU writes.
+     */
     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. */
+        switch (attrs.requester_type) {
+        case MTRT_MACHINE: /* MEMTX_IOPIC */
+        case MTRT_PCI:     /* PCI signalled MSI */
+            break;
+        default:
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: rejecting write from %d",
+                          __func__, attrs.requester_id);
+            return MEMTX_ACCESS_ERROR;
+        }
         MSIMessage msi = { .address = addr, .data = val };
         apic_send_msi(&msi);
-        return;
+        return MEMTX_OK;
     }
 
-    dev = cpu_get_current_apic();
-    if (!dev) {
-        return;
+    if (attrs.requester_type != MTRT_CPU) {
+        return MEMTX_ACCESS_ERROR;
     }
+    dev = cpu_get_current_apic(attrs.requester_id);
     s = APIC(dev);
 
     trace_apic_mem_writel(addr, val);
@@ -839,6 +857,8 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
         s->esr |= APIC_ESR_ILLEGAL_ADDRESS;
         break;
     }
+
+    return MEMTX_OK;
 }
 
 static void apic_pre_save(APICCommonState *s)
@@ -856,8 +876,8 @@ static void apic_post_load(APICCommonState *s)
 }
 
 static const MemoryRegionOps apic_io_ops = {
-    .read = apic_mem_read,
-    .write = apic_mem_write,
+    .read_with_attrs = apic_mem_read,
+    .write_with_attrs = apic_mem_write,
     .impl.min_access_size = 1,
     .impl.max_access_size = 4,
     .valid.min_access_size = 1,
-- 
2.34.1