[v5,05/18] mcdstub: memory helper functions added

---
 include/exec/cpu-common.h |  3 +++
 include/exec/memory.h     |  9 +++++++++
 system/memory.c           | 11 +++++++++++
 system/physmem.c          | 26 ++++++++++++++++++++++++++
 4 files changed, 49 insertions(+)

Nicolas Eder <nicolas.eder@lauterbach.com> writes:

> ---
>  include/exec/cpu-common.h |  3 +++
>  include/exec/memory.h     |  9 +++++++++
>  system/memory.c           | 11 +++++++++++
>  system/physmem.c          | 26 ++++++++++++++++++++++++++
>  4 files changed, 49 insertions(+)
>
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 41115d8919..dd989b5ab2 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -182,6 +182,9 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length);
>  int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
>                          void *ptr, size_t len, bool is_write);
>  
> +int cpu_memory_get_physical_address(CPUState *cpu, vaddr *addr, size_t *len);
> +
> +
>  /* vl.c */
>  void list_cpus(void);
>  
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 831f7c996d..174de807d5 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -3142,6 +3142,15 @@ bool ram_block_discard_is_disabled(void);
>   */
>  bool ram_block_discard_is_required(void);
>  
> +/*
> + * mcd_find_address_space() - Find the address spaces with the corresponding
> + * name.
> + *
> + * Currently only used by the mcd debugger.
> + * @as_name: Name to look for.
> + */
> +AddressSpace *mcd_find_address_space(const char *as_name);
> +

Don't hard code this for mcd - maybe address_space_find_byname()?

>  #endif
>  
>  #endif
> diff --git a/system/memory.c b/system/memory.c
> index 798b6c0a17..9a8fa79e0c 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -3562,6 +3562,17 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
>      }
>  }
>  
> +AddressSpace *mcd_find_address_space(const char *as_name)
> +{
> +    AddressSpace *as;
> +    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> +        if (strcmp(as->name, as_name) == 0) {
> +            return as;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  void memory_region_init_ram(MemoryRegion *mr,
>                              Object *owner,
>                              const char *name,
> diff --git a/system/physmem.c b/system/physmem.c
> index a63853a7bc..70733c67c7 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3422,6 +3422,32 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
>      return 0;
>  }
>  
> +int cpu_memory_get_physical_address(CPUState *cpu, vaddr *addr, size_t *len)
> +{
> +    hwaddr phys_addr;
> +    vaddr l, page;
> +
> +    cpu_synchronize_state(cpu);
> +    MemTxAttrs attrs;
> +
> +    page = *addr & TARGET_PAGE_MASK;
> +    phys_addr = cpu_get_phys_page_attrs_debug(cpu, page, &attrs);
> +    /* if no physical page mapped, return an error */
> +    if (phys_addr == -1) {
> +        return -1;
> +    }
> +    l = (page + TARGET_PAGE_SIZE) - *addr;
> +    if (l > *len) {
> +        l = *len;
> +    }
> +    phys_addr += (*addr & ~TARGET_PAGE_MASK);
> +
> +    /* set output values */
> +    *addr = phys_addr;
> +    *len = l;
> +    return 0;
> +}

I think there is some scope to re-factor some code that is shared with
cpu_memory_rw_debug. In fact rather than using this to feed
mcd_read_write_memory() maybe you really just want a
cpu_physical_memory_rw_debug()?

Although as you are going from vaddr anyway where does
cpu_memory_rw_debug() fail for you?

> +
>  /*
>   * Allows code that needs to deal with migration bitmaps etc to still be built
>   * target independent.

On 29/02/2024 17:56, Alex Bennée wrote:

> I think there is some scope to re-factor some code that is shared with
> cpu_memory_rw_debug. In fact rather than using this to feed
> mcd_read_write_memory() maybe you really just want a
> cpu_physical_memory_rw_debug()?
>
> Although as you are going from vaddr anyway where does
> cpu_memory_rw_debug() fail for you?

Hi Alex, thanks for your feedback!

The reason I wrote the memory access the way I did is the following: 
Regardless of the state of the CPU, MCD always wants to be able to 
access all memory spaces (here secure and non-secure).

For example, if the CPU is currently in secure mode I still want to be 
able to perform a memory access via the non-secure memory space. This 
access should be successful, if a non-secure memory address is requested 
and unsuccessful, if a secure memory address is requested.

I could not get this decoupled memory access to work using the existing 
cpu_memory_rw_debug(). It only works properly when using the current 
main memory space (e.g. secure when the CPU is in secure mode).

I did not want to make changes to the existing memory functions because 
I think I could not have checked for all repercussions.

Best regards

Nicolas