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

[Qemu-devel] [PATCH v2 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.

Changes v1->v2:
 * patches 1, 2 unchanged (and reviewed)
 * patch 3: handle as being NULL (which for the
   load_elf APIs means "use address_space_memory")

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 | 4 +++-
 disas.c              | 5 ++++-
 monitor.c            | 8 +++++++-
 3 files changed, 14 insertions(+), 3 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH v2 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>
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;
 }
 
-- 
2.19.1

[Qemu-devel] [PATCH v2 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>
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

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

[Philippe Mathieu-Daudé]

On 22/11/18 18:26, Peter Maydell 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>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@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");
> 

[Qemu-devel] [PATCH v2 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>
---
v1->v2: handle NULL as
---
 include/hw/elf_ops.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 81cecaf27e2..74679ff8da3 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -482,7 +482,9 @@ 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 ? as : &address_space_memory,
+                                        addr, MEMTXATTRS_UNSPECIFIED,
+                                        data, file_size);
                     g_free(data);
                 }
             }
-- 
2.19.1

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

[Philippe Mathieu-Daudé]

On 22/11/18 18:26, 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>
> ---
> v1->v2: handle NULL as

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

> ---
>  include/hw/elf_ops.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 81cecaf27e2..74679ff8da3 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -482,7 +482,9 @@ 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 ? as : &address_space_memory,
> +                                        addr, MEMTXATTRS_UNSPECIFIED,
> +                                        data, file_size);
>                      g_free(data);
>                  }
>              }
> 

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

[Peter Maydell]

On Thu, 22 Nov 2018 at 17:28, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset takes three places in generic code which
> use cpu_physical_memory_read(), and changes them to use
> address_space_read() instead.
>
> Changes v1->v2:
>  * patches 1, 2 unchanged (and reviewed)
>  * patch 3: handle as being NULL (which for the
>    load_elf APIs means "use address_space_memory")

I'm adding this to a 'misc' pullreq I'm putting together.

thanks
-- PMM