Re: [Qemu-devel] [PATCH memory v1 1/1] memory: remove may_overlap property

[Paolo Bonzini <pbonzini@redhat.com>]

Il 15/08/2014 09:17, Peter Crosthwaite ha scritto:
> In a5e1cbc80e88ed7d73b3fcb46053a3ba167293fc the enforcement of Memory
> collisions was disabled. This means that the MemoryRegion map_overlap
> state is unused. Remove it completely.
> 
> The commit mentions that it should be fixed, but we have been living
> happily-every-after since removal of the check so it's probably
> unneeded complication.
> 
> If we were to repair this, a simpler and more effective check would be
> to only assert collisions between same-priority regions. The fact that
> colliding memory regions may-overlap is then left as implicit by the
> fact that they have different priorities.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  include/exec/memory.h |  1 -
>  memory.c              | 35 -----------------------------------
>  2 files changed, 36 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e2c8e3e..a8e9707 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -158,7 +158,6 @@ struct MemoryRegion {
>      MemoryRegion *alias;
>      hwaddr alias_offset;
>      int32_t priority;
> -    bool may_overlap;
>      QTAILQ_HEAD(subregions, MemoryRegion) subregions;
>      QTAILQ_ENTRY(MemoryRegion) subregions_link;
>      QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
> diff --git a/memory.c b/memory.c
> index 64d7176..be4cf76 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -964,13 +964,6 @@ static void memory_region_get_priority(Object *obj, Visitor *v, void *opaque,
>      visit_type_int32(v, &value, name, errp);
>  }
>  
> -static bool memory_region_get_may_overlap(Object *obj, Error **errp)
> -{
> -    MemoryRegion *mr = MEMORY_REGION(obj);
> -
> -    return mr->may_overlap;
> -}
> -
>  static void memory_region_get_size(Object *obj, Visitor *v, void *opaque,
>                                     const char *name, Error **errp)
>  {
> @@ -1007,10 +1000,6 @@ static void memory_region_initfn(Object *obj)
>                          memory_region_get_priority,
>                          NULL, /* memory_region_set_priority */
>                          NULL, NULL, &error_abort);
> -    object_property_add_bool(OBJECT(mr), "may-overlap",
> -                             memory_region_get_may_overlap,
> -                             NULL, /* memory_region_set_may_overlap */
> -                             &error_abort);
>      object_property_add(OBJECT(mr), "size", "uint64",
>                          memory_region_get_size,
>                          NULL, /* memory_region_set_size, */
> @@ -1623,7 +1612,6 @@ void memory_region_del_eventfd(MemoryRegion *mr,
>  
>  static void memory_region_update_container_subregions(MemoryRegion *subregion)
>  {
> -    hwaddr offset = subregion->addr;
>      MemoryRegion *mr = subregion->container;
>      MemoryRegion *other;
>  
> @@ -1631,27 +1619,6 @@ static void memory_region_update_container_subregions(MemoryRegion *subregion)
>  
>      memory_region_ref(subregion);
>      QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
> -        if (subregion->may_overlap || other->may_overlap) {
> -            continue;
> -        }
> -        if (int128_ge(int128_make64(offset),
> -                      int128_add(int128_make64(other->addr), other->size))
> -            || int128_le(int128_add(int128_make64(offset), subregion->size),
> -                         int128_make64(other->addr))) {
> -            continue;
> -        }
> -#if 0
> -        printf("warning: subregion collision %llx/%llx (%s) "
> -               "vs %llx/%llx (%s)\n",
> -               (unsigned long long)offset,
> -               (unsigned long long)int128_get64(subregion->size),
> -               subregion->name,
> -               (unsigned long long)other->addr,
> -               (unsigned long long)int128_get64(other->size),
> -               other->name);
> -#endif
> -    }
> -    QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
>          if (subregion->priority >= other->priority) {
>              QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
>              goto done;
> @@ -1677,7 +1644,6 @@ void memory_region_add_subregion(MemoryRegion *mr,
>                                   hwaddr offset,
>                                   MemoryRegion *subregion)
>  {
> -    subregion->may_overlap = false;
>      subregion->priority = 0;
>      memory_region_add_subregion_common(mr, offset, subregion);
>  }
> @@ -1687,7 +1653,6 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
>                                           MemoryRegion *subregion,
>                                           int priority)
>  {
> -    subregion->may_overlap = true;
>      subregion->priority = priority;
>      memory_region_add_subregion_common(mr, offset, subregion);
>  }
> 

Thanks, applied to memory branch.

Paolo