Re: [PATCH v2 3/5] RISC-V: 'info gmem' to show hypervisor guest -> physical address translations

[Alistair Francis]

On Wed, Jan 5, 2022 at 1:55 AM Konrad Schwarz
<konrad.schwarz@siemens.com> wrote:
>
> This is analog to the existing 'info mem' command and is implemented
> using the same machinery.
>
> Signed-off-by: Konrad Schwarz <konrad.schwarz@siemens.com>

Hello and thanks for the patches

> ---
>  hmp-commands-info.hx         |  16 +++++
>  include/monitor/hmp-target.h |   2 +
>  target/riscv/monitor.c       | 135 +++++++++++++++++++++++++----------
>  3 files changed, 117 insertions(+), 36 deletions(-)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 407a1da800..fa519f0129 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -237,6 +237,22 @@ SRST
>      Show the active virtual memory mappings.
>  ERST
>
> +#if defined TARGET_RISCV
> +    {
> +        .name       = "gmem",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show the hypervisor guest's physical address"
> +                   " translation",
> +        .cmd        = hmp_info_gmem,
> +    },
> +#endif

I don't think we want RISC-V specific commands. Could we not just
extend `info mem` instead?

> +
> +SRST
> +  ``info gmem``
> +    Show the hypervisor guest's physical address translation.
> +ERST
> +
>      {
>          .name       = "mtree",
>          .args_type  = "flatview:-f,dispatch_tree:-d,owner:-o,disabled:-D",
> diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
> index ffdc15a34b..9f2dd976f6 100644
> --- a/include/monitor/hmp-target.h
> +++ b/include/monitor/hmp-target.h
> @@ -2,6 +2,7 @@
>   * QEMU monitor
>   *
>   * Copyright (c) 2003-2004 Fabrice Bellard
> + * Copyright (c) 2021 Siemens AG, konrad.schwarz@siemens.com

Generally you would only add a copyright claim for a very large
change. Adding a single function prototype doesn't really cut it.

>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
>   * of this software and associated documentation files (the "Software"), to 
> deal
> @@ -45,6 +46,7 @@ CPUArchState *mon_get_cpu_env(Monitor *mon);
>  CPUState *mon_get_cpu(Monitor *mon);
>
>  void hmp_info_mem(Monitor *mon, const QDict *qdict);
> +void hmp_info_gmem(Monitor *mon, const QDict *qdict);
>  void hmp_info_tlb(Monitor *mon, const QDict *qdict);
>  void hmp_mce(Monitor *mon, const QDict *qdict);
>  void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
> diff --git a/target/riscv/monitor.c b/target/riscv/monitor.c
> index 3f74ea9934..ad58bdf9ca 100644
> --- a/target/riscv/monitor.c
> +++ b/target/riscv/monitor.c
> @@ -25,16 +25,6 @@
>  #include "monitor/monitor.h"
>  #include "monitor/hmp-target.h"
>
> -#ifdef TARGET_RISCV64
> -#define PTE_HEADER_FIELDS       "vaddr            paddr            "\
> -                                "size             attr\n"
> -#define PTE_HEADER_DELIMITER    "---------------- ---------------- "\
> -                                "---------------- -------\n"
> -#else
> -#define PTE_HEADER_FIELDS       "vaddr    paddr            size     attr\n"
> -#define PTE_HEADER_DELIMITER    "-------- ---------------- -------- 
> -------\n"
> -#endif
> -
>  /* Perform linear address sign extension */
>  static target_ulong addr_canonical(int va_bits, target_ulong addr)
>  {
> @@ -47,10 +37,34 @@ static target_ulong addr_canonical(int va_bits, 
> target_ulong addr)
>      return addr;
>  }
>
> -static void print_pte_header(Monitor *mon)
> +static void print_pte_header(Monitor *mon,
> +        char const vaddr_char, char const paddr_char)
>  {
> -    monitor_printf(mon, PTE_HEADER_FIELDS);
> -    monitor_printf(mon, PTE_HEADER_DELIMITER);
> +
> +# define        VIRTUAL_WIDTH\
> +        ((int) ((sizeof "ff" - sizeof "") * sizeof(target_ulong)))
> +# define        PHYSICAL_WIDTH\
> +        ((int) ((sizeof "ff" - sizeof "") * sizeof(hwaddr)))
> +# define        ATTRIBUTE_WIDTH ((int) (sizeof "rwxugad" - sizeof ""))
> +
> +# define        VIRTUAL_COLUMN_WIDTH    (1 + VIRTUAL_WIDTH)
> +# define        PHYSICAL_COLUMN_WIDTH   (1 + PHYSICAL_WIDTH)
> +
> +    static char const dashes[PHYSICAL_WIDTH] = "----------------";
> +
> +    monitor_printf(mon,
> +            "%c%-*s%c%-*s%-*s%-*s\n"
> +            "%-*.*s%-*.*s%-*.*s%-*.*s\n",
> +
> +            vaddr_char, VIRTUAL_COLUMN_WIDTH - 1, "addr",
> +            paddr_char, PHYSICAL_COLUMN_WIDTH - 1, "addr",
> +            VIRTUAL_COLUMN_WIDTH, "size",
> +            ATTRIBUTE_WIDTH, "attr",
> +
> +            VIRTUAL_COLUMN_WIDTH, VIRTUAL_WIDTH, dashes,
> +            PHYSICAL_COLUMN_WIDTH, PHYSICAL_WIDTH, dashes,
> +            VIRTUAL_COLUMN_WIDTH, VIRTUAL_WIDTH, dashes,
> +            ATTRIBUTE_WIDTH, ATTRIBUTE_WIDTH, dashes);
>  }
>
>  static void print_pte(Monitor *mon, int va_bits, target_ulong vaddr,
> @@ -65,21 +79,36 @@ static void print_pte(Monitor *mon, int va_bits, 
> target_ulong vaddr,
>          return;
>      }
>
> -    monitor_printf(mon, TARGET_FMT_lx " " TARGET_FMT_plx " " TARGET_FMT_lx
> -                   " %c%c%c%c%c%c%c\n",
> -                   addr_canonical(va_bits, vaddr),
> -                   paddr, size,
> -                   attr & PTE_R ? 'r' : '-',
> -                   attr & PTE_W ? 'w' : '-',
> -                   attr & PTE_X ? 'x' : '-',
> -                   attr & PTE_U ? 'u' : '-',
> -                   attr & PTE_G ? 'g' : '-',
> -                   attr & PTE_A ? 'a' : '-',
> -                   attr & PTE_D ? 'd' : '-');
> +# if 4 == TARGET_LONG_SIZE
> +#       define  TARGET_xFMT     PRIx32
> +# elif 8 == TARGET_LONG_SIZE
> +#       define  TARGET_xFMT     PRIx64
> +# else
> +#       error TARGET_LONG_SIZE not handled
> +# endif

You can just use TCG_PRIlx instead

Alistair

> +
> +    /* note: RISC-V physical addresses are actually xlen + 2 bits long
> +    OTHO, QEMU wil probably never support addresses longer than 64 bits */
> +    monitor_printf(mon,
> +            "%-*.*" TARGET_xFMT
> +            "%-*.*" PRIx64
> +            "%-*.*" TARGET_xFMT
> +            "%c%c%c%c%c%c%c\n",
> +            VIRTUAL_COLUMN_WIDTH, VIRTUAL_WIDTH, addr_canonical(va_bits, 
> vaddr),
> +            PHYSICAL_COLUMN_WIDTH, PHYSICAL_WIDTH, paddr,
> +            VIRTUAL_COLUMN_WIDTH, VIRTUAL_WIDTH, size,
> +            attr & PTE_R ? 'r' : '-',
> +            attr & PTE_W ? 'w' : '-',
> +            attr & PTE_X ? 'x' : '-',
> +            attr & PTE_U ? 'u' : '-',
> +            attr & PTE_G ? 'g' : '-',
> +            attr & PTE_A ? 'a' : '-',
> +            attr & PTE_D ? 'd' : '-');
>  }
>
>  static void walk_pte(Monitor *mon, hwaddr base, target_ulong start,
>                       int level, int ptidxbits, int ptesize, int va_bits,
> +                     int guest,
>                       target_ulong *vbase, hwaddr *pbase, hwaddr *last_paddr,
>                       target_ulong *last_size, int *last_attr)
>  {
> @@ -89,7 +118,7 @@ static void walk_pte(Monitor *mon, hwaddr base, 
> target_ulong start,
>      target_ulong pte;
>      int ptshift;
>      int attr;
> -    int idx;
> +    int idx, idx_end;
>
>      if (level < 0) {
>          return;
> @@ -98,7 +127,8 @@ static void walk_pte(Monitor *mon, hwaddr base, 
> target_ulong start,
>      ptshift = level * ptidxbits;
>      pgsize = 1UL << (PGSHIFT + ptshift);
>
> -    for (idx = 0; idx < (1UL << ptidxbits); idx++) {
> +    for (idx = 0, idx_end = 1 << (ptidxbits + (guest ? 2 : 0));
> +            idx_end > idx; idx++) {
>          pte_addr = base + idx * ptesize;
>          cpu_physical_memory_read(pte_addr, &pte, ptesize);
>
> @@ -131,7 +161,9 @@ static void walk_pte(Monitor *mon, hwaddr base, 
> target_ulong start,
>              } else {
>                  /* pointer to the next level of the page table */
>                  walk_pte(mon, paddr, start, level - 1, ptidxbits, ptesize,
> -                         va_bits, vbase, pbase, last_paddr,
> +                         va_bits,
> +                         0 /* guest */,
> +                         vbase, pbase, last_paddr,
>                           last_size, last_attr);
>              }
>          }
> @@ -141,7 +173,9 @@ static void walk_pte(Monitor *mon, hwaddr base, 
> target_ulong start,
>
>  }
>
> -static void mem_info_svxx(Monitor *mon, CPUArchState *env)
> +static void mem_info_svxx(Monitor *mon, CPUArchState *env,
> +        target_ulong const atp,
> +        int guest, char const vaddr_char, char const paddr_char)
>  {
>      int levels, ptidxbits, ptesize, vm, va_bits;
>      hwaddr base;
> @@ -152,11 +186,11 @@ static void mem_info_svxx(Monitor *mon, CPUArchState 
> *env)
>      int last_attr;
>
>      if (riscv_cpu_mxl(env) == MXL_RV32) {
> -        base = (hwaddr)get_field(env->satp, SATP32_PPN) << PGSHIFT;
> -        vm = get_field(env->satp, SATP32_MODE);
> +        base = (hwaddr)get_field(atp, SATP32_PPN) << PGSHIFT;
> +        vm = get_field(atp, SATP32_MODE);
>      } else {
> -        base = (hwaddr)get_field(env->satp, SATP64_PPN) << PGSHIFT;
> -        vm = get_field(env->satp, SATP64_MODE);
> +        base = (hwaddr)get_field(atp, SATP64_PPN) << PGSHIFT;
> +        vm = get_field(atp, SATP64_MODE);
>      }
>
>      switch (vm) {
> @@ -189,7 +223,7 @@ static void mem_info_svxx(Monitor *mon, CPUArchState *env)
>      va_bits = PGSHIFT + levels * ptidxbits;
>
>      /* print header */
> -    print_pte_header(mon);
> +    print_pte_header(mon, vaddr_char, paddr_char);
>
>      vbase = -1;
>      pbase = -1;
> @@ -199,6 +233,7 @@ static void mem_info_svxx(Monitor *mon, CPUArchState *env)
>
>      /* walk page tables, starting from address 0 */
>      walk_pte(mon, base, 0, levels - 1, ptidxbits, ptesize, va_bits,
> +             guest,
>               &vbase, &pbase, &last_paddr, &last_size, &last_attr);
>
>      /* don't forget the last one */
> @@ -209,6 +244,7 @@ static void mem_info_svxx(Monitor *mon, CPUArchState *env)
>  void hmp_info_mem(Monitor *mon, const QDict *qdict)
>  {
>      CPUArchState *env;
> +    target_ulong atp;
>
>      env = mon_get_cpu_env(mon);
>      if (!env) {
> @@ -221,19 +257,46 @@ void hmp_info_mem(Monitor *mon, const QDict *qdict)
>          return;
>      }
>
> +    atp = env->satp;
>      if (riscv_cpu_mxl(env) == MXL_RV32) {
> -        if (!(env->satp & SATP32_MODE)) {
> +        if (!(atp & SATP32_MODE)) {
>              monitor_printf(mon, "No translation or protection\n");
>              return;
>          }
>      } else {
> -        if (!(env->satp & SATP64_MODE)) {
> +        if (!(atp & SATP64_MODE)) {
>              monitor_printf(mon, "No translation or protection\n");
>              return;
>          }
>      }
>
> -    mem_info_svxx(mon, env);
> +    mem_info_svxx(mon, env, atp, 0, 'v', 'p');
> +}
> +
> +void hmp_info_gmem(Monitor *mon, const QDict *qdict)
> +{
> +    CPUArchState *env;
> +    target_ulong atp;
> +
> +    env = mon_get_cpu_env(mon);
> +    if (!env) {
> +        monitor_printf(mon, "No CPU available\n");
> +        return;
> +    }
> +
> +    if (!riscv_has_ext(env, RVH)) {
> +        monitor_printf(mon, "hypervisor extension not available\n");
> +        return;
> +    }
> +
> +    atp = env->hgatp;
> +    if (!((MXL_RV32 == riscv_cpu_mxl(env) ? SATP32_MODE : SATP64_MODE)
> +            & atp)) {
> +        monitor_printf(mon, "No translation or protection\n");
> +        return;
> +    }
> +
> +    mem_info_svxx(mon, env, atp, 1, 'g', 'p');
>  }
>
>  static const MonitorDef monitor_defs[] = {
> --
> Konrad Schwarz
>
>