Re: [Qemu-devel] [PATCH 07/11] exec: add a reference to the region returned by address_space_translate

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2013-06-25 13:13, Paolo Bonzini wrote:
> Once address_space_translate will only be protected by RCU, the returned
> MemoryRegion can disappear as soon as the RCU read-side critical
> section ends.  Avoid this by adding a reference to the region, and
> dropping it in the caller of address_space_translate.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c                | 16 ++++++++++++++--
>  include/exec/memory.h |  3 ++-
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 5454e4d..b7f032d 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -286,6 +286,7 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
>  
>      *plen = len;
>      *xlat = addr;
> +    memory_region_ref(mr);
>      return mr;
>  }
>  
> @@ -1960,6 +1961,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>                  memcpy(buf, ptr, l);
>              }
>          }
> +        memory_region_unref(mr);
>          len -= l;
>          buf += l;
>          addr += l;
> @@ -2125,8 +2127,10 @@ void *address_space_map(AddressSpace *as,
>              raddr = memory_region_get_ram_addr(mr) + xlat;
>          } else {
>              if (memory_region_get_ram_addr(mr) + xlat != raddr + todo) {
> +                memory_region_unref(mr);
>                  break;
>              }
> +            memory_region_unref(mr);
>          }
>  
>          len -= l;

This stage of the implementation looks inconsistent for
memory_region_get_ram_addr. Isn't the idea so far that the mr reference
is dropped again before leaving the function?

> @@ -2226,6 +2230,7 @@ static inline uint32_t ldl_phys_internal(hwaddr addr,
>              break;
>          }
>      }
> +    memory_region_unref(mr);
>      return val;
>  }
>  
> @@ -2285,6 +2290,7 @@ static inline uint64_t ldq_phys_internal(hwaddr addr,
>              break;
>          }
>      }
> +    memory_region_unref(mr);
>      return val;
>  }
>  
> @@ -2352,6 +2358,7 @@ static inline uint32_t lduw_phys_internal(hwaddr addr,
>              break;
>          }
>      }
> +    memory_region_unref(mr);
>      return val;
>  }
>  
> @@ -2399,6 +2406,7 @@ void stl_phys_notdirty(hwaddr addr, uint32_t val)
>              }
>          }
>      }
> +    memory_region_unref(mr);
>  }
>  
>  /* warning: addr must be aligned */
> @@ -2440,6 +2448,7 @@ static inline void stl_phys_internal(hwaddr addr, uint32_t val,
>          }
>          invalidate_and_set_dirty(addr1, 4);
>      }
> +    memory_region_unref(mr);
>  }
>  
>  void stl_phys(hwaddr addr, uint32_t val)
> @@ -2503,6 +2512,7 @@ static inline void stw_phys_internal(hwaddr addr, uint32_t val,
>          }
>          invalidate_and_set_dirty(addr1, 2);
>      }
> +    memory_region_unref(mr);
>  }
>  
>  void stw_phys(hwaddr addr, uint32_t val)
> @@ -2592,11 +2602,13 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr)
>  {
>      MemoryRegion*mr;
>      hwaddr l = 1;
> +    bool res;
>  
>      mr = address_space_translate(&address_space_memory,
>                                   phys_addr, &phys_addr, &l, false);
>  
> -    return !(memory_region_is_ram(mr) ||
> -             memory_region_is_romd(mr));
> +    res = !(memory_region_is_ram(mr) || memory_region_is_romd(mr));
> +    memory_region_unref(mr);
> +    return res;
>  }
>  #endif

Unless I'm missing something ATM, address_space_access_valid,
cpu_physical_memory_write_rom and tb_invalidate_phys_addr also require
unref calls.

> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index c842d48..2a37b2c 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1003,7 +1003,8 @@ bool address_space_write(AddressSpace *as, hwaddr addr,
>  bool address_space_read(AddressSpace *as, hwaddr addr, uint8_t *buf, int len);
>  
>  /* address_space_translate: translate an address range into an address space
> - * into a MemoryRegion and an address range into that section
> + * into a MemoryRegion and an address range into that section.  Add a reference
> + * to that region.
>   *
>   * @as: #AddressSpace to be accessed
>   * @addr: address within that address space
> 

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux