diff mbox

[RFC,v1,3/3] make address_space_map safe

Message ID 1352430870-10129-4-git-send-email-qemulist@gmail.com
State New
Headers show

Commit Message

pingfan liu Nov. 9, 2012, 3:14 a.m. UTC 
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 cpu-common.h      |    8 ++++++--
 cputlb.c          |    4 ++--
 dma-helpers.c     |    4 +++-
 dma.h             |    5 ++++-
 exec.c            |   45 +++++++++++++++++++++++++++++++++++++--------
 memory.h          |    4 +++-
 target-i386/kvm.c |    4 ++--
 7 files changed, 57 insertions(+), 17 deletions(-)

Comments

Vasilis Liaskovitis Feb. 13, 2013, 12:12 p.m. UTC | #1 
Hi,

I am looking at this old ref/unref patchset for safely removing hot-plugged
dimms/MemoryRegions. I am not sure if the set is still actively worked on or
relevant for qemu-master, but I had a small comment below:

On Fri, Nov 09, 2012 at 11:14:30AM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  cpu-common.h      |    8 ++++++--
>  cputlb.c          |    4 ++--
>  dma-helpers.c     |    4 +++-
>  dma.h             |    5 ++++-
>  exec.c            |   45 +++++++++++++++++++++++++++++++++++++--------
>  memory.h          |    4 +++-
>  target-i386/kvm.c |    4 ++--
>  7 files changed, 57 insertions(+), 17 deletions(-)
> 
[snip]
> diff --git a/exec.c b/exec.c
> index e5f1c0f..e9bd695 100644
> --- a/exec.c
> +++ b/exec.c
[snip]
> @@ -3822,7 +3837,8 @@ void address_space_unmap(AddressSpace *as, void *buffer, target_phys_addr_t len,
>  {
>      if (buffer != bounce.buffer) {
>          if (is_write) {
> -            ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer);
> +            /* Will release RAM refcnt */
> +            ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer, true);
>              while (access_len) {
>                  unsigned l;
>                  l = TARGET_PAGE_SIZE;

Since qemu_ram_addr_from_host_nofail(buffer, true) will decrease the reference
counter for this memoryregion, I think is should be called regardless of
read/write i.e.  outside of the "if (is_write)" clause. Otherwise references for
reads are not decreased properly.

thanks,

- Vasilis
pingfan liu March 7, 2013, 1:59 a.m. UTC | #2 
On Wed, Feb 13, 2013 at 8:12 PM, Vasilis Liaskovitis
<vasilis.liaskovitis@profitbricks.com> wrote:
> Hi,
>
> I am looking at this old ref/unref patchset for safely removing hot-plugged
> dimms/MemoryRegions. I am not sure if the set is still actively worked on or
> relevant for qemu-master, but I had a small comment below:
>
> On Fri, Nov 09, 2012 at 11:14:30AM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  cpu-common.h      |    8 ++++++--
>>  cputlb.c          |    4 ++--
>>  dma-helpers.c     |    4 +++-
>>  dma.h             |    5 ++++-
>>  exec.c            |   45 +++++++++++++++++++++++++++++++++++++--------
>>  memory.h          |    4 +++-
>>  target-i386/kvm.c |    4 ++--
>>  7 files changed, 57 insertions(+), 17 deletions(-)
>>
> [snip]
>> diff --git a/exec.c b/exec.c
>> index e5f1c0f..e9bd695 100644
>> --- a/exec.c
>> +++ b/exec.c
> [snip]
>> @@ -3822,7 +3837,8 @@ void address_space_unmap(AddressSpace *as, void *buffer, target_phys_addr_t len,
>>  {
>>      if (buffer != bounce.buffer) {
>>          if (is_write) {
>> -            ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer);
>> +            /* Will release RAM refcnt */
>> +            ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer, true);
>>              while (access_len) {
>>                  unsigned l;
>>                  l = TARGET_PAGE_SIZE;
>
> Since qemu_ram_addr_from_host_nofail(buffer, true) will decrease the reference
> counter for this memoryregion, I think is should be called regardless of
> read/write i.e.  outside of the "if (is_write)" clause. Otherwise references for
> reads are not decreased properly.
>
Yes, you are right. if reference hold map, it need to be released here.

Thanks,
pingfan


> thanks,
>
> - Vasilis
diff mbox

Patch

diff --git a/cpu-common.h b/cpu-common.h
index 69c1d7a..f257dfc 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -45,8 +45,8 @@  void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size);
 void *qemu_safe_ram_ptr(ram_addr_t addr);
 void qemu_put_ram_ptr(void *addr);
 /* This should not be used by devices.  */
-int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
-ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr);
+int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr, bool unmap);
+ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr, bool unmap);
 void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev);
 
 void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
@@ -64,6 +64,10 @@  static inline void cpu_physical_memory_write(target_phys_addr_t addr,
 {
     cpu_physical_memory_rw(addr, (void *)buf, len, 1);
 }
+void *cpu_physical_memory_map_safe(target_phys_addr_t addr,
+                              target_phys_addr_t *plen,
+                              int is_write,
+                              bool *safe);
 void *cpu_physical_memory_map(target_phys_addr_t addr,
                               target_phys_addr_t *plen,
                               int is_write);
diff --git a/cputlb.c b/cputlb.c
index 9027557..a78ba02 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -164,7 +164,7 @@  static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry)
     if (tlb_is_dirty_ram(tlb_entry)) {
         p = (void *)(uintptr_t)((tlb_entry->addr_write & TARGET_PAGE_MASK)
             + tlb_entry->addend);
-        ram_addr = qemu_ram_addr_from_host_nofail(p);
+        ram_addr = qemu_ram_addr_from_host_nofail(p, false);
         if (!cpu_physical_memory_is_dirty(ram_addr)) {
             tlb_entry->addr_write |= TLB_NOTDIRTY;
         }
@@ -338,7 +338,7 @@  tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
 #endif
     }
     p = (void *)((uintptr_t)addr + env1->tlb_table[mmu_idx][page_index].addend);
-    return qemu_ram_addr_from_host_nofail(p);
+    return qemu_ram_addr_from_host_nofail(p, false);
 }
 
 #define MMUSUFFIX _cmmu
diff --git a/dma-helpers.c b/dma-helpers.c
index 3f09dcb..6f6e3b7 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -394,6 +394,7 @@  void *iommu_dma_memory_map(DMAContext *dma, dma_addr_t addr, dma_addr_t *len,
     int err;
     target_phys_addr_t paddr, plen;
     void *buf;
+    bool safe;
 
     if (dma->map) {
         return dma->map(dma, addr, len, dir);
@@ -414,7 +415,8 @@  void *iommu_dma_memory_map(DMAContext *dma, dma_addr_t addr, dma_addr_t *len,
         *len = plen;
     }
 
-    buf = address_space_map(dma->as, paddr, &plen, dir == DMA_DIRECTION_FROM_DEVICE);
+    buf = address_space_map(dma->as, paddr, &plen,
+                        dir == DMA_DIRECTION_FROM_DEVICE, &safe);
     *len = plen;
 
     return buf;
diff --git a/dma.h b/dma.h
index 1bd6f4a..9d67dab 100644
--- a/dma.h
+++ b/dma.h
@@ -176,11 +176,14 @@  static inline void *dma_memory_map(DMAContext *dma,
                                    dma_addr_t addr, dma_addr_t *len,
                                    DMADirection dir)
 {
+    bool safe;
+
     if (!dma_has_iommu(dma)) {
         target_phys_addr_t xlen = *len;
         void *p;
 
-        p = address_space_map(dma->as, addr, &xlen, dir == DMA_DIRECTION_FROM_DEVICE);
+        p = address_space_map(dma->as, addr, &xlen,
+                        dir == DMA_DIRECTION_FROM_DEVICE, &safe);
         *len = xlen;
         return p;
     } else {
diff --git a/exec.c b/exec.c
index e5f1c0f..e9bd695 100644
--- a/exec.c
+++ b/exec.c
@@ -2847,7 +2847,14 @@  void qemu_put_ram_ptr(void *addr)
     trace_qemu_put_ram_ptr(addr);
 }
 
-int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
+static void memory_region_unref(MemoryRegion *mr)
+{
+    if (mr->ops && mr->ops->unref) {
+        mr->ops->unref(mr);
+    }
+}
+
+int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr, bool unmap)
 {
     RAMBlock *block;
     uint8_t *host = ptr;
@@ -2866,6 +2873,9 @@  int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
         if (host - block->host < block->length) {
             *ram_addr = block->offset + (host - block->host);
             qemu_mutex_unlock(&ram_list.lock);
+            if (unmap) {
+                memory_region_unref(block->mr);
+            }
             return 0;
         }
     }
@@ -2876,11 +2886,11 @@  int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
 
 /* Some of the softmmu routines need to translate from a host pointer
    (typically a TLB entry) back to a ram offset.  */
-ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
+ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr, bool unmap)
 {
     ram_addr_t ram_addr;
 
-    if (qemu_ram_addr_from_host(ptr, &ram_addr)) {
+    if (qemu_ram_addr_from_host(ptr, &ram_addr, unmap)) {
         fprintf(stderr, "Bad ram pointer %p\n", ptr);
         abort();
     }
@@ -3762,7 +3772,8 @@  static void cpu_notify_map_clients(void)
 void *address_space_map(AddressSpace *as,
                         target_phys_addr_t addr,
                         target_phys_addr_t *plen,
-                        bool is_write)
+                        bool is_write,
+                        bool *safe)
 {
     target_phys_addr_t len = *plen;
     target_phys_addr_t todo = 0;
@@ -3778,7 +3789,8 @@  void *address_space_map(AddressSpace *as,
         l = (page + TARGET_PAGE_SIZE) - addr;
         if (l > len)
             l = len;
-        address_space_section_lookup_ref(as, page >> TARGET_PAGE_BITS, &mr_obj);
+        *safe = address_space_section_lookup_ref(as, page >> TARGET_PAGE_BITS,
+                &mr_obj);
         section = &mr_obj;
 
         if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
@@ -3805,7 +3817,10 @@  void *address_space_map(AddressSpace *as,
         len -= l;
         addr += l;
         todo += l;
-        memory_region_section_unref(&mr_obj);
+        /* len <= 0,release when unmapped */
+        if (len > 0) {
+            memory_region_section_unref(&mr_obj);
+        }
     }
     rlen = todo;
     ret = qemu_ram_ptr_length(raddr, &rlen);
@@ -3822,7 +3837,8 @@  void address_space_unmap(AddressSpace *as, void *buffer, target_phys_addr_t len,
 {
     if (buffer != bounce.buffer) {
         if (is_write) {
-            ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer);
+            /* Will release RAM refcnt */
+            ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer, true);
             while (access_len) {
                 unsigned l;
                 l = TARGET_PAGE_SIZE;
@@ -3847,11 +3863,24 @@  void address_space_unmap(AddressSpace *as, void *buffer, target_phys_addr_t len,
     cpu_notify_map_clients();
 }
 
+void *cpu_physical_memory_map_safe(target_phys_addr_t addr,
+                              target_phys_addr_t *plen,
+                              int is_write,
+                              bool *safe)
+{
+
+    return address_space_map(&address_space_memory, addr, plen, is_write,
+           safe);
+}
+
 void *cpu_physical_memory_map(target_phys_addr_t addr,
                               target_phys_addr_t *plen,
                               int is_write)
 {
-    return address_space_map(&address_space_memory, addr, plen, is_write);
+    bool safe;
+
+    return address_space_map(&address_space_memory, addr, plen, is_write,
+           &safe);
 }
 
 void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
diff --git a/memory.h b/memory.h
index 704d014..c9d8a04 100644
--- a/memory.h
+++ b/memory.h
@@ -863,9 +863,11 @@  void address_space_read(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
  * @addr: address within that address space
  * @plen: pointer to length of buffer; updated on return
  * @is_write: indicates the transfer direction
+ * @safe: indicates the mapped addr can be safely across big lock or not
  */
 void *address_space_map(AddressSpace *as, target_phys_addr_t addr,
-                        target_phys_addr_t *plen, bool is_write);
+                        target_phys_addr_t *plen, bool is_write,
+                        bool *safe);
 
 /* address_space_unmap: Unmaps a memory region previously mapped by address_space_map()
  *
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 5b18383..40f40c4 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -261,7 +261,7 @@  int kvm_arch_on_sigbus_vcpu(CPUX86State *env, int code, void *addr)
 
     if ((env->mcg_cap & MCG_SER_P) && addr
         && (code == BUS_MCEERR_AR || code == BUS_MCEERR_AO)) {
-        if (qemu_ram_addr_from_host(addr, &ram_addr) ||
+        if (qemu_ram_addr_from_host(addr, &ram_addr, false) ||
             !kvm_physical_memory_addr_from_host(env->kvm_state, addr, &paddr)) {
             fprintf(stderr, "Hardware memory error for memory used by "
                     "QEMU itself instead of guest system!\n");
@@ -293,7 +293,7 @@  int kvm_arch_on_sigbus(int code, void *addr)
         target_phys_addr_t paddr;
 
         /* Hope we are lucky for AO MCE */
-        if (qemu_ram_addr_from_host(addr, &ram_addr) ||
+        if (qemu_ram_addr_from_host(addr, &ram_addr, false) ||
             !kvm_physical_memory_addr_from_host(first_cpu->kvm_state, addr,
                                                 &paddr)) {
             fprintf(stderr, "Hardware memory error for memory used by "