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

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

[Peter Crosthwaite]

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);
 }
-- 
2.0.1.1.gfbfc394

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

[Paolo Bonzini]

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

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

[Peter Maydell]

On 15 August 2014 08:17, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 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.

Um. I think in general colliding memory regions imply a bug in the
machine model which is creating them. We have the collisions
disabled because nobody's got round to fixing this bug in the
PC model yet and it happens to be harmless there. I'm not
really convinced that yanking out the check code is better than
fixing the PC model...

> 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.

I'm not sure your suggestion here would work, because priorities
are only significant relative to other regions within the same
container, whereas collisions can occur between two regions
which don't have the same parent container and whose priorities are
therefore not comparable. (For instance, consider [ A [ B C ] ]
where A and B end up overlapping.)

thanks
-- PMM

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

[Paolo Bonzini]

Il 18/08/2014 00:23, Peter Maydell ha scritto:
> Um. I think in general colliding memory regions imply a bug in the
> machine model which is creating them. We have the collisions
> disabled because nobody's got round to fixing this bug in the
> PC model yet and it happens to be harmless there. I'm not
> really convinced that yanking out the check code is better than
> fixing the PC model...

I'm not sure I can parse anything in the commit message for a5e1cbc80
(which disabled the warning).  I guess I'll try to revert that one and
see what happens.

Paolo

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

[Peter Crosthwaite]

On Mon, Aug 18, 2014 at 8:23 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 15 August 2014 08:17, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> 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.
>
> Um. I think in general colliding memory regions imply a bug in the
> machine model which is creating them. We have the collisions
> disabled because nobody's got round to fixing this bug in the
> PC model yet and it happens to be harmless there. I'm not
> really convinced that yanking out the check code is better than
> fixing the PC model...
>
>> 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.
>
> I'm not sure your suggestion here would work, because priorities
> are only significant relative to other regions within the same
> container, whereas collisions can occur between two regions
> which don't have the same parent container and whose priorities are
> therefore not comparable. (For instance, consider [ A [ B C ] ]
> where A and B end up overlapping.)
>

But that is not a problem that is solved by the old may_overlap flag
is it? The check deleted here is not hierarchy aware so we have never
been able to detect that case. I think we should take a "clean slate"
approach on the implementation of the collision detection. Big change
is needed to get the check in the right place in code, whether it's
same-priority based or using may_overlap.

Regards,
Peter

> thanks
> -- PMM
>

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

[Peter Maydell]

On 18 August 2014 01:14, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Mon, Aug 18, 2014 at 8:23 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 15 August 2014 08:17, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>> 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.
>>
>> I'm not sure your suggestion here would work, because priorities
>> are only significant relative to other regions within the same
>> container, whereas collisions can occur between two regions
>> which don't have the same parent container and whose priorities are
>> therefore not comparable. (For instance, consider [ A [ B C ] ]
>> where A and B end up overlapping.)

> But that is not a problem that is solved by the old may_overlap flag
> is it? The check deleted here is not hierarchy aware so we have never
> been able to detect that case. I think we should take a "clean slate"
> approach on the implementation of the collision detection. Big change
> is needed to get the check in the right place in code, whether it's
> same-priority based or using may_overlap.

Oops, I mistakenly thought this check was happening at the
flattened-ranges point, but it's done when a subregion is
added to a container. Maybe you're right that we should be
able to allow overlaps if the priorities are different and not
otherwise, then. But I'd rather we actually did that rather than
just removing the check completely.

thanks
-- PMM

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

[Peter Crosthwaite]

On Mon, Aug 18, 2014 at 5:23 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 August 2014 01:14, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> On Mon, Aug 18, 2014 at 8:23 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 15 August 2014 08:17, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>>> 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.
>>>
>>> I'm not sure your suggestion here would work, because priorities
>>> are only significant relative to other regions within the same
>>> container, whereas collisions can occur between two regions
>>> which don't have the same parent container and whose priorities are
>>> therefore not comparable. (For instance, consider [ A [ B C ] ]
>>> where A and B end up overlapping.)
>
>> But that is not a problem that is solved by the old may_overlap flag
>> is it? The check deleted here is not hierarchy aware so we have never
>> been able to detect that case. I think we should take a "clean slate"
>> approach on the implementation of the collision detection. Big change
>> is needed to get the check in the right place in code, whether it's
>> same-priority based or using may_overlap.
>
> Oops, I mistakenly thought this check was happening at the
> flattened-ranges point, but it's done when a subregion is
> added to a container. Maybe you're right that we should be
> able to allow overlaps if the priorities are different and not
> otherwise, then. But I'd rather we actually did that rather than
> just removing the check completely.
>

Right can we call it follow up though and get a merge on this one so
we can start fresh? With your new hierarchy problem we are just
compounding the reasons to get rid of this code.

Regards,
Peter

> thanks
> -- PMM
>

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

[Peter Maydell]

On 18 August 2014 08:26, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Mon, Aug 18, 2014 at 5:23 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Oops, I mistakenly thought this check was happening at the
>> flattened-ranges point, but it's done when a subregion is
>> added to a container. Maybe you're right that we should be
>> able to allow overlaps if the priorities are different and not
>> otherwise, then. But I'd rather we actually did that rather than
>> just removing the check completely.
>>
>
> Right can we call it follow up though and get a merge on this one so
> we can start fresh? With your new hierarchy problem we are just
> compounding the reasons to get rid of this code.

That makes it harder to figure out whether we're still
detecting the same set of problems with the new
check, though.

-- PMM