Re: [Qemu-devel] [PATCH 08/16] exec.c: Have one io_mem_watch per AddressSpace

[Paolo Bonzini <pbonzini@redhat.com>]

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