Re: [Qemu-devel] [PATCH] Remove MemoryRegionSection check code from sparc_cpu_get_phys_page_debug()

[Mark Cave-Ayland]

On 22/11/17 06:32, Jean-Christophe Dubois wrote:

> This code is preventing the MMU debug code from displaying virtual
> mappings of IO devices (anything that is not located in the RAM).
> 
> Before this patch, Qemu would output 0xffffffffffffffff (-1) as the
> physical address corresponding to a IO device virtual address.
> 
> With this patch the intended physical adresse is displayed.
> 
> Signed-off-by: Jean-Christophe Dubois <j...@tribudubois.net>
> ---
>  target/sparc/mmu_helper.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
> index d5b6c1e48c..f2d2250e7a 100644
> --- a/target/sparc/mmu_helper.c
> +++ b/target/sparc/mmu_helper.c
> @@ -857,18 +857,12 @@ hwaddr sparc_cpu_get_phys_page_debug(CPUState *cs, 
> vaddr addr)
>      CPUSPARCState *env = &cpu->env;
>      hwaddr phys_addr;
>      int mmu_idx = cpu_mmu_index(env, false);
> -    MemoryRegionSection section;
>  
>      if (cpu_sparc_get_phys_page(env, &phys_addr, addr, 2, mmu_idx) != 0) {
>          if (cpu_sparc_get_phys_page(env, &phys_addr, addr, 0, mmu_idx) != 0) 
> {
>              return -1;
>          }
>      }
> -    section = memory_region_find(get_system_memory(), phys_addr, 1);
> -    memory_region_unref(section.mr);
> -    if (!int128_nz(section.size)) {
> -        return -1;
> -    }
>      return phys_addr;
>  }
>  #endif

Hi Jean-Christophe,

Thanks for looking into this and providing the patch. I asked on IRC to
see if anyone could explain what this code was trying to do and Peter
suggested that it could be related to when cpu_get_phys_page_debug()
used to have to be more resilient to bad conditions such as missing
mappings.

Tracing through the git log I see it was introduced as part of commit
cc4aa830 "sparc: avoid cpu_get_physical_page_desc()":

diff --git a/target-sparc/mmu_helper.c b/target-sparc/mmu_helper.c
index 8cdc224..bdff1c3 100644
--- a/target-sparc/mmu_helper.c
+++ b/target-sparc/mmu_helper.c
@@ -19,6 +19,7 @@

 #include "cpu.h"
 #include "trace.h"
+#include "exec-memory.h"

 /* Sparc MMU emulation */

@@ -839,13 +840,15 @@ target_phys_addr_t
cpu_get_phys_page_debug(CPUState *env, target_ulong addr)
 {
     target_phys_addr_t phys_addr;
     int mmu_idx = cpu_mmu_index(env);
+    MemoryRegionSection section;

     if (cpu_sparc_get_phys_page(env, &phys_addr, addr, 2, mmu_idx) != 0) {
         if (cpu_sparc_get_phys_page(env, &phys_addr, addr, 0, mmu_idx)
!= 0) {
             return -1;
         }
     }
-    if (cpu_get_physical_page_desc(phys_addr) == IO_MEM_UNASSIGNED) {
+    section = memory_region_find(get_system_memory(), phys_addr, 1);
+    if (!section.size) {
         return -1;
     }
     return phys_addr;

and cpu_get_physical_page_desc() used to look like this:

/* XXX: temporary until new memory mapping API */
uint32_t cpu_get_physical_page_desc(target_phys_addr_t addr)
{
    PhysPageDesc *p;

    p = phys_page_find(addr >> TARGET_PAGE_BITS);
    if (!p)
        return IO_MEM_UNASSIGNED;
    return p->phys_offset;
}

i.e. IO_MEM_UNASSIGNED indicates there is no physical mapping for this
virtual address.

Now I've had a look at some of the other *_get_phys_page_debug()
functions and I don't see any similar logic (and in fact the git log for
PPC details the simplification of the ppc_*_get_phys_page_debug()
functions by removing permission checks etc.) so I'm inclined to think
that commit cc4aa830 did the wrong thing by restricting lookups to the
system memory address space and the patch is correct.

I'd like a second opinion if possible before queuing this patch as I'm
not overly familiar with this area of the code (and what the exact
behaviour for debug MMIO accesses should be), but otherwise it looks
good - the only part missing is a "sparc: " prefix to the subject line.


ATB,

Mark.