[Qemu-devel] new memory api: offsets?

[Qemu-devel] new memory api: offsets?

[Michael Walle]

Hi Avi,

while debugging, i noticed, that mr->offset is never set, expect in the 
initializer. (The subregion collision warning is printed although the regions 
are not overlapping.) Is this intended?

btw. you may include the memory region name in the warning.

-- 
Michael

Re: [Qemu-devel] new memory api: offsets?

[Avi Kivity]

On 09/01/2011 01:54 AM, Michael Walle wrote:
> Hi Avi,
>
> while debugging, i noticed, that mr->offset is never set, expect in the
> initializer. (The subregion collision warning is printed although the regions
> are not overlapping.) Is this intended?

Did you miss memory_region_set_offset()?

The subregion collision warning is unrelated?

> btw. you may include the memory region name in the warning.
>

sure, patches welcome.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] new memory api: offsets?

[Michael Walle]

Am Donnerstag 01 September 2011, 07:54:28 schrieb Avi Kivity:
> On 09/01/2011 01:54 AM, Michael Walle wrote:
> > Hi Avi,
> > 
> > while debugging, i noticed, that mr->offset is never set, expect in the
> > initializer. (The subregion collision warning is printed although the
> > regions are not overlapping.) Is this intended?
> 
> Did you miss memory_region_set_offset()?
I saw that function but it's never called. Now i noticed that this is a 
deprecated public function.

> The subregion collision warning is unrelated?
you are walking along the subregions and use that offset property to detect 
collisions. Shouldn't you use the addr property instead?

@@ -1190,16 +1190,18 @@ static void 
memory_region_add_subregion_common(MemoryRegion *mr,
         if (subregion->may_overlap || other->may_overlap) {
             continue;
         }
-        if (offset >= other->offset + other->size
-            || offset + subregion->size <= other->offset) {
+        if (offset >= other->addr + other->size
+            || offset + subregion->size <= other->addr) {
             continue;
         }

> > btw. you may include the memory region name in the warning.
> 
> sure, patches welcome.

-        printf("warning: subregion collision %llx/%llx vs %llx/%llx\n",
+        printf("warning: subregion collision %llx/%llx (%s) "
+               "vs %llx/%llx (%s)\n",
                (unsigned long long)offset,
                (unsigned long long)subregion->size,
-               (unsigned long long)other->offset,
-               (unsigned long long)other->size);
+               subregion->name,
+               (unsigned long long)other->addr,
+               (unsigned long long)other->size,
+               other->name);

let me know if i should post a git patch instead :)

-- 
Michael

Re: [Qemu-devel] new memory api: offsets?

[Avi Kivity]

On 09/01/2011 01:00 PM, Michael Walle wrote:
> Am Donnerstag 01 September 2011, 07:54:28 schrieb Avi Kivity:
> >  On 09/01/2011 01:54 AM, Michael Walle wrote:
> >  >  Hi Avi,
> >  >
> >  >  while debugging, i noticed, that mr->offset is never set, expect in the
> >  >  initializer. (The subregion collision warning is printed although the
> >  >  regions are not overlapping.) Is this intended?
> >
> >  Did you miss memory_region_set_offset()?
> I saw that function but it's never called. Now i noticed that this is a
> deprecated public function.
>
> >  The subregion collision warning is unrelated?
> you are walking along the subregions and use that offset property to detect
> collisions. Shouldn't you use the addr property instead?

Ah, of course.

>
> @@ -1190,16 +1190,18 @@ static void
> memory_region_add_subregion_common(MemoryRegion *mr,
>           if (subregion->may_overlap || other->may_overlap) {
>               continue;
>           }
> -        if (offset>= other->offset + other->size
> -            || offset + subregion->size<= other->offset) {
> +        if (offset>= other->addr + other->size
> +            || offset + subregion->size<= other->addr) {
>               continue;
>           }

Right.  Please post with a changelog and signoff.

>
> >  >  btw. you may include the memory region name in the warning.
> >
> >  sure, patches welcome.
>
> -        printf("warning: subregion collision %llx/%llx vs %llx/%llx\n",
> +        printf("warning: subregion collision %llx/%llx (%s) "
> +               "vs %llx/%llx (%s)\n",
>                  (unsigned long long)offset,
>                  (unsigned long long)subregion->size,
> -               (unsigned long long)other->offset,
> -               (unsigned long long)other->size);
> +               subregion->name,
> +               (unsigned long long)other->addr,
> +               (unsigned long long)other->size,
> +               other->name);
>
> let me know if i should post a git patch instead :)
>

Yes, with a signoff.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH] memory: fix subregion collision warning

[Michael Walle]

Instead of the offset property use the proper addr property to calculate
the offsets.

Additionally, be a little more verbose on the warning and print the
subregion name.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 memory.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/memory.c b/memory.c
index 101b67c..ba74435 100644
--- a/memory.c
+++ b/memory.c
@@ -1190,16 +1190,19 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
         if (subregion->may_overlap || other->may_overlap) {
             continue;
         }
-        if (offset >= other->offset + other->size
-            || offset + subregion->size <= other->offset) {
+        if (offset >= other->addr + other->size
+            || offset + subregion->size <= other->addr) {
             continue;
         }
 #if 0
-        printf("warning: subregion collision %llx/%llx vs %llx/%llx\n",
+        printf("warning: subregion collision %llx/%llx (%s) "
+               "vs %llx/%llx (%s)\n",
                (unsigned long long)offset,
                (unsigned long long)subregion->size,
-               (unsigned long long)other->offset,
-               (unsigned long long)other->size);
+               subregion->name,
+               (unsigned long long)other->addr,
+               (unsigned long long)other->size,
+               other->name);
 #endif
     }
     QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH] memory: fix subregion collision warning

[Avi Kivity]

On 09/16/2011 12:16 AM, Michael Walle wrote:
> Instead of the offset property use the proper addr property to calculate
> the offsets.
>
> Additionally, be a little more verbose on the warning and print the
> subregion name.
>
>

Thanks, applied to memory/core.

-- 
error compiling committee.c: too many arguments to function