diff mbox

[08/16] exec.c: Have one io_mem_watch per AddressSpace

Message ID 1446747358-18214-9-git-send-email-peter.maydell@linaro.org
State Accepted
Headers show

Commit Message

Peter Maydell Nov. 5, 2015, 6:15 p.m. UTC 
The io_mem_watch MemoryRegion's read and write callbacks pass the
accesses through to an underlying address space. Now that that
might be something other than address_space_memory, we need to
pass the correct AddressSpace in via the opaque pointer. That
means we need to have one io_mem_watch MemoryRegion per address
space, rather than sharing one between all address spaces.

This should also fix gdbstub watchpoints in x86 SMRAM, which would
previously not have worked correctly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 exec.c                | 40 +++++++++++++++++++++++-----------------
 include/exec/memory.h |  2 ++
 2 files changed, 25 insertions(+), 17 deletions(-)

-- 
1.9.1

Comments

Peter Maydell Nov. 9, 2015, 10:54 a.m. UTC | #1 
On 9 November 2015 at 10:49, Paolo Bonzini <pbonzini@redhat.com> wrote:
>

>

> On 05/11/2015 19:15, Peter Maydell wrote:

>> @@ -2045,17 +2037,18 @@ static MemTxResult watch_mem_read(void *opaque, hwaddr addr, uint64_t *pdata,

>>  {

>>      MemTxResult res;

>>      uint64_t data;

>> +    AddressSpace *as = opaque;

>>

>>      check_watchpoint(addr & ~TARGET_PAGE_MASK, size, attrs, BP_MEM_READ);

>>      switch (size) {

>>      case 1:

>> -        data = address_space_ldub(&address_space_memory, addr, attrs, &res);

>> +        data = address_space_ldub(as, addr, attrs, &res);

>>          break;

>>      case 2:

>> -        data = address_space_lduw(&address_space_memory, addr, attrs, &res);

>> +        data = address_space_lduw(as, addr, attrs, &res);

>>          break;

>>      case 4:

>> -        data = address_space_ldl(&address_space_memory, addr, attrs, &res);

>> +        data = address_space_ldl(as, addr, attrs, &res);

>>          break;

>>      default: abort();

>>      }

>

> current_cpu is available here, so it should be possible to have only one

> io_mem_watch per CPU address space index (i.e. two).


So the opaque gives you the asidx and then you look at current_cpu's
cpu_ases[asidx]? Yeah, that works and is simpler. (Good argument for
making "number of ASes per CPU" a compile-time constant I guess.)

> But actually, if the address space can be derived from the attributes,

> you just need to fish the right address space out of current_cpu.

>

>> +    as->io_mem_watch = g_new0(MemoryRegion, 1);

>

> This is leaked when the address space goes away.  You can allocate it

> statically inside AddressSpace if my alternative plan from above doesn't

> work out.


Oh, right, it's not sufficient to unref it, it doesn't auto-free
itself when the refcount drops to 0.

thanks
-- PMM
diff mbox

Patch

diff --git a/exec.c b/exec.c
index bc6ab8a..9998fa0 100644
--- a/exec.c
+++ b/exec.c
@@ -163,8 +163,6 @@  static void io_mem_init(void);
 static void memory_map_init(void);
 static void tcg_commit(MemoryListener *listener);
 
-static MemoryRegion io_mem_watch;
-
 /**
  * CPUAddressSpace: all the information a CPU needs about an AddressSpace
  * @cpu: the CPU whose AddressSpace this is
@@ -334,12 +332,6 @@  static MemoryRegionSection *phys_page_find(PhysPageEntry lp, hwaddr addr,
     }
 }
 
-bool memory_region_is_unassigned(MemoryRegion *mr)
-{
-    return mr != &io_mem_rom && mr != &io_mem_notdirty && !mr->rom_device
-        && mr != &io_mem_watch;
-}
-
 /* Called from RCU critical section */
 static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
                                                         hwaddr addr,
@@ -2045,17 +2037,18 @@  static MemTxResult watch_mem_read(void *opaque, hwaddr addr, uint64_t *pdata,
 {
     MemTxResult res;
     uint64_t data;
+    AddressSpace *as = opaque;
 
     check_watchpoint(addr & ~TARGET_PAGE_MASK, size, attrs, BP_MEM_READ);
     switch (size) {
     case 1:
-        data = address_space_ldub(&address_space_memory, addr, attrs, &res);
+        data = address_space_ldub(as, addr, attrs, &res);
         break;
     case 2:
-        data = address_space_lduw(&address_space_memory, addr, attrs, &res);
+        data = address_space_lduw(as, addr, attrs, &res);
         break;
     case 4:
-        data = address_space_ldl(&address_space_memory, addr, attrs, &res);
+        data = address_space_ldl(as, addr, attrs, &res);
         break;
     default: abort();
     }
@@ -2068,17 +2061,18 @@  static MemTxResult watch_mem_write(void *opaque, hwaddr addr,
                                    MemTxAttrs attrs)
 {
     MemTxResult res;
+    AddressSpace *as = opaque;
 
     check_watchpoint(addr & ~TARGET_PAGE_MASK, size, attrs, BP_MEM_WRITE);
     switch (size) {
     case 1:
-        address_space_stb(&address_space_memory, addr, val, attrs, &res);
+        address_space_stb(as, addr, val, attrs, &res);
         break;
     case 2:
-        address_space_stw(&address_space_memory, addr, val, attrs, &res);
+        address_space_stw(as, addr, val, attrs, &res);
         break;
     case 4:
-        address_space_stl(&address_space_memory, addr, val, attrs, &res);
+        address_space_stl(as, addr, val, attrs, &res);
         break;
     default: abort();
     }
@@ -2091,6 +2085,15 @@  static const MemoryRegionOps watch_mem_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+bool memory_region_is_unassigned(MemoryRegion *mr)
+{
+    /* Checking the ops pointer of the MemoryRegion is strictly
+     * speaking looking at private information of the MemoryRegion :-(
+     */
+    return mr != &io_mem_rom && mr != &io_mem_notdirty && !mr->rom_device
+        && mr->ops != &watch_mem_ops;
+}
+
 static MemTxResult subpage_read(void *opaque, hwaddr addr, uint64_t *data,
                                 unsigned len, MemTxAttrs attrs)
 {
@@ -2251,8 +2254,6 @@  static void io_mem_init(void)
                           NULL, UINT64_MAX);
     memory_region_init_io(&io_mem_notdirty, NULL, &notdirty_mem_ops, NULL,
                           NULL, UINT64_MAX);
-    memory_region_init_io(&io_mem_watch, NULL, &watch_mem_ops, NULL,
-                          NULL, UINT64_MAX);
 }
 
 static void mem_begin(MemoryListener *listener)
@@ -2267,7 +2268,7 @@  static void mem_begin(MemoryListener *listener)
     assert(n == PHYS_SECTION_NOTDIRTY);
     n = dummy_section(&d->map, as, &io_mem_rom);
     assert(n == PHYS_SECTION_ROM);
-    n = dummy_section(&d->map, as, &io_mem_watch);
+    n = dummy_section(&d->map, as, as->io_mem_watch);
     assert(n == PHYS_SECTION_WATCH);
 
     d->phys_map  = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .skip = 1 };
@@ -2315,6 +2316,10 @@  static void tcg_commit(MemoryListener *listener)
 
 void address_space_init_dispatch(AddressSpace *as)
 {
+    as->io_mem_watch = g_new0(MemoryRegion, 1);
+    memory_region_init_io(as->io_mem_watch, NULL, &watch_mem_ops, as,
+                          NULL, UINT64_MAX);
+
     as->dispatch = NULL;
     as->dispatch_listener = (MemoryListener) {
         .begin = mem_begin,
@@ -2339,6 +2344,7 @@  void address_space_destroy_dispatch(AddressSpace *as)
     if (d) {
         call_rcu(d, address_space_dispatch_free, rcu);
     }
+    memory_region_unref(as->io_mem_watch);
 }
 
 static void memory_map_init(void)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 0f07159..e5d98f8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -246,6 +246,8 @@  struct AddressSpace {
     struct AddressSpaceDispatch *next_dispatch;
     MemoryListener dispatch_listener;
 
+    MemoryRegion *io_mem_watch;
+
     QTAILQ_ENTRY(AddressSpace) address_spaces_link;
 };