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

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(-)

On Thu, Nov 05, 2015 at 06:15:50PM +0000, Peter Maydell wrote:
> 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.


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  exec.c                | 40 +++++++++++++++++++++++-----------------
>  include/exec/memory.h |  2 ++
>  2 files changed, 25 insertions(+), 17 deletions(-)
> 
> 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;
>  };
>  
> -- 
> 1.9.1
>

On 05/11/2015 19:15, Peter Maydell wrote:
> 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(-)
> 
> 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();
>      }

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).

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.

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

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

On 09/11/2015 11:54, Peter Maydell wrote:
> > 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.)

Yes.  Of course it works well only if the compile-time constant is
small---but again, my guess is that it'll be two.

It would be even better if you can use the attrs instead of the opaque
to compute the asidx.  Then the only change you need is to watch_mem_*.

Paolo