[Qemu-devel] [PATCH for-1.6 0/3] Memory API fixes

[Qemu-devel] [PATCH for-1.6 0/3] Memory API fixes

[Paolo Bonzini]

Three fixes from Peter, Jan and me.  I'm still recovering from the email
backlog, hence I don't trust myself sending a pull request...  In any
case, they are available also from

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

Paolo

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

Paolo Bonzini (1):
  memory: actually set the owner

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

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

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/3] 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 80ee2ab..ffd611a 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/3] memory: actually set the owner

[Paolo Bonzini]

Brown paper bag for me.  Commit 2c9b15c added a line setting mr->owner,
but there was already one that initialized it to NULL (added in the
earlier commit 803c0816).

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

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

[Hu Tao]

On Tue, Jul 16, 2013 at 05:31:01PM +0200, Paolo Bonzini wrote:
> Brown paper bag for me.  Commit 2c9b15c added a line setting mr->owner,
> but there was already one that initialized it to NULL (added in the
> earlier commit 803c0816).

What I read from log is the reverse, Commit 2c9b15c added a line setting
mr->owner, which was correct, but 803c0816 accidentally set it to NULL later.

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/3] 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

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

[Andreas Färber]

Am 16.07.2013 17:31, schrieb 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>

Unfortunately this negatively affects PReP: OpenHack'Ware booting Debian
Etch prints:

ERROR: WIN_READ_NATIVE_MAX : status 50 != 0x40

It does continue if one is patient enough.
No problems before this commit.

Any ideas, except that OHW may be doing Bad Things?

Andreas

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

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

[Jan Kiszka]

On 2013-07-29 16:59, Andreas Färber wrote:
> Am 16.07.2013 17:31, schrieb 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>
> 
> Unfortunately this negatively affects PReP: OpenHack'Ware booting Debian
> Etch prints:
> 
> ERROR: WIN_READ_NATIVE_MAX : status 50 != 0x40
> 
> It does continue if one is patient enough.
> No problems before this commit.
> 
> Any ideas, except that OHW may be doing Bad Things?

Which accesses return different results now, PIO or MMIO? What did they
return prior to b018ddf633?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

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

[Andreas Färber]

Am 29.07.2013 17:01, schrieb Jan Kiszka:
> On 2013-07-29 16:59, Andreas Färber wrote:
>> Am 16.07.2013 17:31, schrieb 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>
>>
>> Unfortunately this negatively affects PReP: OpenHack'Ware booting Debian
>> Etch prints:
>>
>> ERROR: WIN_READ_NATIVE_MAX : status 50 != 0x40
>>
>> It does continue if one is patient enough.
>> No problems before this commit.
>>
>> Any ideas, except that OHW may be doing Bad Things?
> 
> Which accesses return different results now, PIO or MMIO? What did they
> return prior to b018ddf633?

Would I have to locally apply Paolo's MMIO tracing patches to find out?
My guess is as good as yours otherwise... ;)

Andreas