[Qemu-devel] [PULL 0/4] Memory API fixes for soft freeze

[Qemu-devel] [PULL 0/4] Memory API fixes for soft freeze

[Paolo Bonzini]

Anthony,

The following changes since commit 6453a3a69488196f26d12654c6b148446abdf3d6:

  Merge remote-tracking branch 'quintela/migration.next' into staging (2013-07-15 14:49:16 -0500)

are available in the git repository at:

  git://github.com/bonzini/qemu.git iommu-for-anthony

for you to fetch changes up to e1622f4b15391bd44eb0f99a244fdf19a20fd981:

  exec: fix incorrect assumptions in memory_access_size (2013-07-18 06:03:25 +0200)

----------------------------------------------------------------
Jan Kiszka (1):
      memory: Return -1 again on reads from unsigned regions

Paolo Bonzini (2):
      memory: actually set the owner
      exec: fix incorrect assumptions in memory_access_size

Peter Maydell (1):
      exec.c: Pass correct pointer type to qemu_ram_ptr_length

 exec.c   | 11 ++---------
 memory.c |  3 +--
 2 files changed, 3 insertions(+), 11 deletions(-)
-- 
1.8.1.4

[Qemu-devel] [PATCH 1/4] exec.c: Pass correct pointer type to qemu_ram_ptr_length

[Paolo Bonzini]

From: Peter Maydell <peter.maydell@linaro.org>

Commit e3127ae0 introduced a problem where we're passing a
hwaddr* to qemu_ram_ptr_length() but it wants a ram_addr_t*;
this will cause problems on 32 bit hosts and in any case
provokes a clang warning on MacOSX:

  CC    arm-softmmu/exec.o
exec.c:2164:46: warning: incompatible pointer types passing 'hwaddr *'
(aka 'unsigned long long *') to parameter of type 'ram_addr_t *'
(aka 'unsigned long *')
[-Wincompatible-pointer-types]
    return qemu_ram_ptr_length(raddr + base, plen);
                                             ^~~~
exec.c:1392:63: note: passing argument to parameter 'size' here
static void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size)
                                                              ^

Since this function is only used in one place, change its
prototype to pass a hwaddr* rather than a ram_addr_t*,
rather than contorting the calling code to get the type right.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Tested-by: Riku Voipio <riku.voipio@linaro.org>
Tested-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index c99a883..d312bb4 100644
--- a/exec.c
+++ b/exec.c
@@ -1379,7 +1379,7 @@ static void *qemu_safe_ram_ptr(ram_addr_t addr)
 
 /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
  * but takes a size argument */
-static void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size)
+static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size)
 {
     if (*size == 0) {
         return NULL;
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/4] memory: actually set the owner

[Paolo Bonzini]

Brown paper bag for me.  Originally commit 803c0816 came before commit
2c9b15c.  When the order was inverted, I left in the NULL initialization
of mr->owner.

Reviewed-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/memory.c b/memory.c
index c8f9a2b..9938b6b 100644
--- a/memory.c
+++ b/memory.c
@@ -805,7 +805,6 @@ void memory_region_init(MemoryRegion *mr,
     mr->owner = owner;
     mr->iommu_ops = NULL;
     mr->parent = NULL;
-    mr->owner = NULL;
     mr->size = int128_make64(size);
     if (size == UINT64_MAX) {
         mr->size = int128_2_64();
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/4] memory: Return -1 again on reads from unsigned regions

[Paolo Bonzini]

From: Jan Kiszka <jan.kiszka@siemens.com>

This restore the behavior prior to b018ddf633 which accidentally changed
the return code to 0. Specifically guests probing for register existence
were affected by this.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/memory.c b/memory.c
index 9938b6b..34a088e 100644
--- a/memory.c
+++ b/memory.c
@@ -840,7 +840,7 @@ static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
     if (current_cpu != NULL) {
         cpu_unassigned_access(current_cpu, addr, false, false, 0, size);
     }
-    return 0;
+    return -1ULL;
 }
 
 static void unassigned_mem_write(void *opaque, hwaddr addr,
-- 
1.8.1.4

[Qemu-devel] [PATCH 4/4] exec: fix incorrect assumptions in memory_access_size

[Paolo Bonzini]

access_size_min can be 1 because erroneous accesses must not crash
QEMU, they should trigger exceptions in the guest or just return
garbage (depending on the CPU).  I am not sure I understand the
comment: placing a 4-byte field at the last byte of a region
makes no sense (unless impl.unaligned is true), and that is
why memory.c:access_with_adjusted_size does not bother with
minimums larger than the remaining length.

access_size_max can be mr->ops->valid.max_access_size because memory.c
can and will still break accesses bigger than
mr->ops->impl.max_access_size.

Reported-by: Markus Armbruster <armbru@redhat.com>
Tested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index d312bb4..c8658c6 100644
--- a/exec.c
+++ b/exec.c
@@ -1898,14 +1898,10 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 
 static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
-    unsigned access_size_min = mr->ops->impl.min_access_size;
-    unsigned access_size_max = mr->ops->impl.max_access_size;
+    unsigned access_size_max = mr->ops->valid.max_access_size;
 
     /* Regions are assumed to support 1-4 byte accesses unless
        otherwise specified.  */
-    if (access_size_min == 0) {
-        access_size_min = 1;
-    }
     if (access_size_max == 0) {
         access_size_max = 4;
     }
@@ -1922,9 +1918,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
     if (l > access_size_max) {
         l = access_size_max;
     }
-    /* ??? The users of this function are wrong, not supporting minimums larger
-       than the remaining length.  C.f. memory.c:access_with_adjusted_size.  */
-    assert(l >= access_size_min);
 
     return l;
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 4/4] exec: fix incorrect assumptions in memory_access_size

[Luiz Capitulino]

On Fri, 19 Jul 2013 22:07:58 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> access_size_min can be 1 because erroneous accesses must not crash
> QEMU, they should trigger exceptions in the guest or just return
> garbage (depending on the CPU).  I am not sure I understand the
> comment: placing a 4-byte field at the last byte of a region
> makes no sense (unless impl.unaligned is true), and that is
> why memory.c:access_with_adjusted_size does not bother with
> minimums larger than the remaining length.
> 
> access_size_max can be mr->ops->valid.max_access_size because memory.c
> can and will still break accesses bigger than
> mr->ops->impl.max_access_size.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Tested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Yeah, works for me now:

Tested-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  exec.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index d312bb4..c8658c6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1898,14 +1898,10 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>  
>  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>  {
> -    unsigned access_size_min = mr->ops->impl.min_access_size;
> -    unsigned access_size_max = mr->ops->impl.max_access_size;
> +    unsigned access_size_max = mr->ops->valid.max_access_size;
>  
>      /* Regions are assumed to support 1-4 byte accesses unless
>         otherwise specified.  */
> -    if (access_size_min == 0) {
> -        access_size_min = 1;
> -    }
>      if (access_size_max == 0) {
>          access_size_max = 4;
>      }
> @@ -1922,9 +1918,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>      if (l > access_size_max) {
>          l = access_size_max;
>      }
> -    /* ??? The users of this function are wrong, not supporting minimums larger
> -       than the remaining length.  C.f. memory.c:access_with_adjusted_size.  */
> -    assert(l >= access_size_min);
>  
>      return l;
>  }

Re: [Qemu-devel] [PULL 0/4] Memory API fixes for soft freeze

[Peter Maydell]

On 19 July 2013 21:07, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Anthony,
>
> The following changes since commit 6453a3a69488196f26d12654c6b148446abdf3d6:
>
>   Merge remote-tracking branch 'quintela/migration.next' into staging (2013-07-15 14:49:16 -0500)
>
> are available in the git repository at:
>
>   git://github.com/bonzini/qemu.git iommu-for-anthony
>
> for you to fetch changes up to e1622f4b15391bd44eb0f99a244fdf19a20fd981:

Ping!

> Peter Maydell (1):
>       exec.c: Pass correct pointer type to qemu_ram_ptr_length

In particular this compile failure fix has now been two weeks
on list and still not in master :-(

thanks
-- PMM