[Qemu-devel] [PATCH for-4.0 0/3] Avoid cpu_physical_memory_read() in generic code

[Qemu-devel] [PATCH for-4.0 0/3] Avoid cpu_physical_memory_read() in generic code

[Peter Maydell]

This patchset takes three places in generic code which
use cpu_physical_memory_read(), and changes them to use
address_space_read() instead.

cpu_physical_memory_{read,rw,write} all implicitly assume
that there is exactly one view of physical memory. This
is sort-of true today, but we'd like to be able to move
to having heterogenous systems where not all CPUs share
the same view of physical memory.

In disas.c we are disassembling for a particular CPU, so
use that CPU's primary address space (cs->as).

In monitor.c we are reading physical memory for a
particular CPU, so again use that CPU's primary address
space; we fall back to address_space_memory for the case
where there are no CPUs in the system (-machine none).

In elf_ops.h the function was passed an address space to
use, so just use it.

Other places in generic code that use these functions are:
 * dump.c -- the whole UI here seems to assume that there
   is only one view of memory and that is what is being dumped
 * cpu.c:qmp_pmemsave() -- again, the UI assumption is that
   there's only one view of memory
So I've left those alone.

NB: git grep command line for finding callsites:
 git grep '\<cpu_physical_memory_\(read\|write\|rw\)\>'

thanks
-- PMM

Peter Maydell (3):
  disas.c: Use address_space_read() to read memory
  monitor: Use address_space_read() to read memory
  elf_ops.h: Use address_space_write() to write memory

 include/hw/elf_ops.h | 3 ++-
 disas.c              | 5 ++++-
 monitor.c            | 8 +++++++-
 3 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH for-4.0 1/3] disas.c: Use address_space_read() to read memory

[Peter Maydell]

Currently disas.c reads physical memory using
cpu_physical_memory_read(). This effectively hard-codes
assuming that all CPUs have the same view of physical
memory. Switch to address_space_read() instead, which
lets us use the AddressSpace for the CPU we're
disassembling for.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 disas.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/disas.c b/disas.c
index 5325b7e6be6..f9c517b3588 100644
--- a/disas.c
+++ b/disas.c
@@ -588,7 +588,10 @@ static int
 physical_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length,
                      struct disassemble_info *info)
 {
-    cpu_physical_memory_read(memaddr, myaddr, length);
+    CPUDebug *s = container_of(info, CPUDebug, info);
+
+    address_space_read(s->cpu->as, memaddr, MEMTXATTRS_UNSPECIFIED,
+                       myaddr, length);
     return 0;
 }
 
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-4.0 1/3] disas.c: Use address_space_read() to read memory

[Philippe Mathieu-Daudé]

On 22/11/18 12:29, Peter Maydell wrote:
> Currently disas.c reads physical memory using
> cpu_physical_memory_read(). This effectively hard-codes
> assuming that all CPUs have the same view of physical
> memory. Switch to address_space_read() instead, which
> lets us use the AddressSpace for the CPU we're
> disassembling for.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  disas.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/disas.c b/disas.c
> index 5325b7e6be6..f9c517b3588 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -588,7 +588,10 @@ static int
>  physical_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length,
>                       struct disassemble_info *info)
>  {
> -    cpu_physical_memory_read(memaddr, myaddr, length);
> +    CPUDebug *s = container_of(info, CPUDebug, info);
> +
> +    address_space_read(s->cpu->as, memaddr, MEMTXATTRS_UNSPECIFIED,
> +                       myaddr, length);
>      return 0;
>  }
>  
> 

[Qemu-devel] [PATCH for-4.0 2/3] monitor: Use address_space_read() to read memory

[Peter Maydell]

Currently monitor.c reads physical memory using
cpu_physical_memory_read(). This effectively hard-codes
assuming that all CPUs have the same view of physical
memory. Switch to address_space_read() instead, which
lets us use the AddressSpace for the CPU we're
reading memory for (falling back to address_space_memory
if there is no CPU, as happens with the "none" board).
As a bonus, this allows us to detect failures to read memory.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 monitor.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index d39390c2f2f..b0e8f2c490a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1604,7 +1604,13 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
         if (l > line_size)
             l = line_size;
         if (is_physical) {
-            cpu_physical_memory_read(addr, buf, l);
+            AddressSpace *as = cs ? cs->as : &address_space_memory;
+            MemTxResult r = address_space_read(as, addr,
+                                               MEMTXATTRS_UNSPECIFIED, buf, l);
+            if (r != MEMTX_OK) {
+                monitor_printf(mon, " Cannot access memory\n");
+                break;
+            }
         } else {
             if (cpu_memory_rw_debug(cs, addr, buf, l, 0) < 0) {
                 monitor_printf(mon, " Cannot access memory\n");
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-4.0 2/3] monitor: Use address_space_read() to read memory

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> Currently monitor.c reads physical memory using
> cpu_physical_memory_read(). This effectively hard-codes
> assuming that all CPUs have the same view of physical
> memory. Switch to address_space_read() instead, which
> lets us use the AddressSpace for the CPU we're
> reading memory for (falling back to address_space_memory
> if there is no CPU, as happens with the "none" board).
> As a bonus, this allows us to detect failures to read memory.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Yes that's nice; there's probably already a couple of CPUs that show
differences.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  monitor.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/monitor.c b/monitor.c
> index d39390c2f2f..b0e8f2c490a 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1604,7 +1604,13 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>          if (l > line_size)
>              l = line_size;
>          if (is_physical) {
> -            cpu_physical_memory_read(addr, buf, l);
> +            AddressSpace *as = cs ? cs->as : &address_space_memory;
> +            MemTxResult r = address_space_read(as, addr,
> +                                               MEMTXATTRS_UNSPECIFIED, buf, l);
> +            if (r != MEMTX_OK) {
> +                monitor_printf(mon, " Cannot access memory\n");
> +                break;
> +            }
>          } else {
>              if (cpu_memory_rw_debug(cs, addr, buf, l, 0) < 0) {
>                  monitor_printf(mon, " Cannot access memory\n");
> -- 
> 2.19.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH for-4.0 3/3] elf_ops.h: Use address_space_write() to write memory

[Peter Maydell]

Currently the load_elf function in elf_ops.h uses
cpu_physical_memory_write() to write the ELF file to
memory if it is not handling it as a ROM blob. This
means we ignore the AddressSpace that the function
is passed to define where it should be loaded.
Use address_space_write() instead.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/elf_ops.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 81cecaf27e2..793dcb85c2b 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -482,7 +482,8 @@ static int glue(load_elf, SZ)(const char *name, int fd,
                     rom_add_elf_program(label, data, file_size, mem_size,
                                         addr, as);
                 } else {
-                    cpu_physical_memory_write(addr, data, file_size);
+                    address_space_write(as, addr, MEMTXATTRS_UNSPECIFIED,
+                                        data, file_size);
                     g_free(data);
                 }
             }
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-4.0 3/3] elf_ops.h: Use address_space_write() to write memory

[Philippe Mathieu-Daudé]

On 22/11/18 12:29, Peter Maydell wrote:
> Currently the load_elf function in elf_ops.h uses
> cpu_physical_memory_write() to write the ELF file to
> memory if it is not handling it as a ROM blob. This
> means we ignore the AddressSpace that the function
> is passed to define where it should be loaded.
> Use address_space_write() instead.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  include/hw/elf_ops.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 81cecaf27e2..793dcb85c2b 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -482,7 +482,8 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                      rom_add_elf_program(label, data, file_size, mem_size,
>                                          addr, as);
>                  } else {
> -                    cpu_physical_memory_write(addr, data, file_size);
> +                    address_space_write(as, addr, MEMTXATTRS_UNSPECIFIED,
> +                                        data, file_size);
>                      g_free(data);
>                  }
>              }
> 

Re: [Qemu-devel] [PATCH for-4.0 3/3] elf_ops.h: Use address_space_write() to write memory

[Peter Maydell]

On 22 November 2018 at 11:29, Peter Maydell <peter.maydell@linaro.org> wrote:
> Currently the load_elf function in elf_ops.h uses
> cpu_physical_memory_write() to write the ELF file to
> memory if it is not handling it as a ROM blob. This
> means we ignore the AddressSpace that the function
> is passed to define where it should be loaded.
> Use address_space_write() instead.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/elf_ops.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 81cecaf27e2..793dcb85c2b 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -482,7 +482,8 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                      rom_add_elf_program(label, data, file_size, mem_size,
>                                          addr, as);
>                  } else {
> -                    cpu_physical_memory_write(addr, data, file_size);
> +                    address_space_write(as, addr, MEMTXATTRS_UNSPECIFIED,
> +                                        data, file_size);
>                      g_free(data);
>                  }
>              }
> --

This turns out to have a bug which my testing somehow missed.
The 'as' argument to this function can be NULL, which means that
it should use address_space_memory, so we need to handle that.
(The other side of the if() doesn't need to special case NULL
because rom_add_elf_program() and the other loader.c code handle
NULL later on.)

thanks
-- PMM