diff mbox

[29/29] memory: strengthen assertions on mr->terminates

Message ID 1430152117-100558-30-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini April 27, 2015, 4:28 p.m. UTC 
mr->terminates alone doesn't guarantee that we are looking at a RAM region.
mr->ram_addr also has to be checked, in order to distinguish RAM and I/O
regions.

IOMMU regions were not setting mr->ram_addr to a bogus value, do it now
so that the assertions would fire for IOMMU regions as well.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Fam Zheng May 26, 2015, 12:49 p.m. UTC | #1 
On Mon, 04/27 18:28, Paolo Bonzini wrote:
> mr->terminates alone doesn't guarantee that we are looking at a RAM region.
> mr->ram_addr also has to be checked, in order to distinguish RAM and I/O
> regions.
> 
> IOMMU regions were not setting mr->ram_addr to a bogus value, do it now
> so that the assertions would fire for IOMMU regions as well.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

It would be nicer to introduce a memory_region_is_ram(MemoryRegion *mr), the
~(ram_addr_t) duplications are too many.

Fam

> ---
>  memory.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index bb86b4b..82d9df6 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1242,6 +1242,7 @@ void memory_region_init_iommu(MemoryRegion *mr,
>      memory_region_init(mr, owner, name, size);
>      mr->iommu_ops = ops,
>      mr->terminates = true;  /* then re-forwards */
> +    mr->ram_addr = ~(ram_addr_t)0;
>      notifier_list_init(&mr->iommu_notify);
>  }
>  
> @@ -1382,14 +1383,14 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
>  bool memory_region_get_dirty(MemoryRegion *mr, hwaddr addr,
>                               hwaddr size, unsigned client)
>  {
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>      return cpu_physical_memory_get_dirty(mr->ram_addr + addr, size, client);
>  }
>  
>  void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>                               hwaddr size)
>  {
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>      cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size,
>                                          memory_region_get_dirty_log_mask(mr));
>  }
> @@ -1397,7 +1398,7 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>  bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
>                                          hwaddr size, unsigned client)
>  {
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>      return cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr,
>                                                      size, client);
>  }
> @@ -1442,7 +1443,7 @@ void memory_region_rom_device_set_romd(MemoryRegion *mr, bool romd_mode)
>  void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
>                                 hwaddr size, unsigned client)
>  {
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>      cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr, size,
>                                               client);
>  }
> @@ -1453,7 +1454,7 @@ int memory_region_get_fd(MemoryRegion *mr)
>          return memory_region_get_fd(mr->alias);
>      }
>  
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>  
>      return qemu_get_ram_fd(mr->ram_addr & TARGET_PAGE_MASK);
>  }
> @@ -1464,14 +1465,14 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
>          return memory_region_get_ram_ptr(mr->alias) + mr->alias_offset;
>      }
>  
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>  
>      return qemu_get_ram_ptr(mr->ram_addr & TARGET_PAGE_MASK);
>  }
>  
>  void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp)
>  {
> -    assert(mr->terminates);
> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>  
>      qemu_ram_resize(mr->ram_addr, newsize, errp);
>  }
> -- 
> 1.8.3.1
>
Paolo Bonzini May 26, 2015, 12:52 p.m. UTC | #2 
On 26/05/2015 14:49, Fam Zheng wrote:
> It would be nicer to introduce a memory_region_is_ram(MemoryRegion *mr), the
> ~(ram_addr_t) duplications are too many.

Is RAM_ADDR_INVALID okay too?

Paolo

> Fam
> 
>> ---
>>  memory.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/memory.c b/memory.c
>> index bb86b4b..82d9df6 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1242,6 +1242,7 @@ void memory_region_init_iommu(MemoryRegion *mr,
>>      memory_region_init(mr, owner, name, size);
>>      mr->iommu_ops = ops,
>>      mr->terminates = true;  /* then re-forwards */
>> +    mr->ram_addr = ~(ram_addr_t)0;
>>      notifier_list_init(&mr->iommu_notify);
>>  }
>>  
>> @@ -1382,14 +1383,14 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
>>  bool memory_region_get_dirty(MemoryRegion *mr, hwaddr addr,
>>                               hwaddr size, unsigned client)
>>  {
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>      return cpu_physical_memory_get_dirty(mr->ram_addr + addr, size, client);
>>  }
>>  
>>  void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>>                               hwaddr size)
>>  {
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>      cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size,
>>                                          memory_region_get_dirty_log_mask(mr));
>>  }
>> @@ -1397,7 +1398,7 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>>  bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
>>                                          hwaddr size, unsigned client)
>>  {
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>      return cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr,
>>                                                      size, client);
>>  }
>> @@ -1442,7 +1443,7 @@ void memory_region_rom_device_set_romd(MemoryRegion *mr, bool romd_mode)
>>  void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
>>                                 hwaddr size, unsigned client)
>>  {
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>      cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr, size,
>>                                               client);
>>  }
>> @@ -1453,7 +1454,7 @@ int memory_region_get_fd(MemoryRegion *mr)
>>          return memory_region_get_fd(mr->alias);
>>      }
>>  
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>  
>>      return qemu_get_ram_fd(mr->ram_addr & TARGET_PAGE_MASK);
>>  }
>> @@ -1464,14 +1465,14 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
>>          return memory_region_get_ram_ptr(mr->alias) + mr->alias_offset;
>>      }
>>  
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>  
>>      return qemu_get_ram_ptr(mr->ram_addr & TARGET_PAGE_MASK);
>>  }
>>  
>>  void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp)
>>  {
>> -    assert(mr->terminates);
>> +    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
>>  
>>      qemu_ram_resize(mr->ram_addr, newsize, errp);
>>  }
>> -- 
>> 1.8.3.1
>>
Fam Zheng May 26, 2015, 12:56 p.m. UTC | #3 
On Tue, 05/26 14:52, Paolo Bonzini wrote:
> 
> 
> On 26/05/2015 14:49, Fam Zheng wrote:
> > It would be nicer to introduce a memory_region_is_ram(MemoryRegion *mr), the
> > ~(ram_addr_t) duplications are too many.
> 
> Is RAM_ADDR_INVALID okay too?

I think so. If you replace every ~(ram_addr_t)0 with RAM_ADDR_INVALID, you can
add my

Reviewed-by: Fam Zheng <famz@redhat.com>
diff mbox

Patch

diff --git a/memory.c b/memory.c
index bb86b4b..82d9df6 100644
--- a/memory.c
+++ b/memory.c
@@ -1242,6 +1242,7 @@  void memory_region_init_iommu(MemoryRegion *mr,
     memory_region_init(mr, owner, name, size);
     mr->iommu_ops = ops,
     mr->terminates = true;  /* then re-forwards */
+    mr->ram_addr = ~(ram_addr_t)0;
     notifier_list_init(&mr->iommu_notify);
 }
 
@@ -1382,14 +1383,14 @@  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
 bool memory_region_get_dirty(MemoryRegion *mr, hwaddr addr,
                              hwaddr size, unsigned client)
 {
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
     return cpu_physical_memory_get_dirty(mr->ram_addr + addr, size, client);
 }
 
 void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
                              hwaddr size)
 {
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
     cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size,
                                         memory_region_get_dirty_log_mask(mr));
 }
@@ -1397,7 +1398,7 @@  void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
 bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
                                         hwaddr size, unsigned client)
 {
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
     return cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr,
                                                     size, client);
 }
@@ -1442,7 +1443,7 @@  void memory_region_rom_device_set_romd(MemoryRegion *mr, bool romd_mode)
 void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
                                hwaddr size, unsigned client)
 {
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
     cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr, size,
                                              client);
 }
@@ -1453,7 +1454,7 @@  int memory_region_get_fd(MemoryRegion *mr)
         return memory_region_get_fd(mr->alias);
     }
 
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
 
     return qemu_get_ram_fd(mr->ram_addr & TARGET_PAGE_MASK);
 }
@@ -1464,14 +1465,14 @@  void *memory_region_get_ram_ptr(MemoryRegion *mr)
         return memory_region_get_ram_ptr(mr->alias) + mr->alias_offset;
     }
 
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
 
     return qemu_get_ram_ptr(mr->ram_addr & TARGET_PAGE_MASK);
 }
 
 void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp)
 {
-    assert(mr->terminates);
+    assert(mr->terminates && mr->ram_addr != ~(ram_addr_t)0);
 
     qemu_ram_resize(mr->ram_addr, newsize, errp);
 }