[PATCH-for-9.0 0/2] target/monitor: Deprecate 'info tlb/mem' in favor of 'info mmu'

[PATCH-for-9.0 0/2] target/monitor: Deprecate 'info tlb/mem' in favor of 'info mmu'

[Philippe Mathieu-Daudé]

'info tlb' and 'info mem' commands don't scale in heterogeneous
emulation. They will be reworked after the next release, hidden
behind the 'info mmu' command. It is not too late to deprecate
commands, so add the 'info mmu' command as wrapper to the other
ones, but already deprecate them.

Philippe Mathieu-Daudé (2):
  target/monitor: Introduce 'info mmu' command
  target/monitor: Deprecate 'info tlb' and 'info mem' commands

 docs/about/deprecated.rst    | 10 ++++++++
 include/monitor/hmp-target.h |  3 +++
 monitor/hmp-cmds-target.c    | 49 ++++++++++++++++++++++++++++++++++++
 target/i386/monitor.c        |  4 +--
 target/m68k/monitor.c        |  2 +-
 target/nios2/monitor.c       |  2 +-
 target/ppc/ppc-qmp-cmds.c    |  2 +-
 target/riscv/monitor.c       |  2 +-
 target/sh4/monitor.c         |  2 +-
 target/sparc/monitor.c       |  2 +-
 target/xtensa/monitor.c      |  2 +-
 hmp-commands-info.hx         | 22 +++++++++++++---
 12 files changed, 89 insertions(+), 13 deletions(-)

-- 
2.41.0

[PATCH-for-9.0 1/2] target/monitor: Introduce 'info mmu' command

[Philippe Mathieu-Daudé]

Introduce the 'info mmu' command. For now it only
forward to the 'info tlb' and 'info mem' commands,
which will be deprecated.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/monitor/hmp-target.h |  1 +
 monitor/hmp-cmds-target.c    | 37 ++++++++++++++++++++++++++++++++++++
 hmp-commands-info.hx         | 14 ++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index d78e979f05..2af84b1915 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -44,6 +44,7 @@ int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
 CPUArchState *mon_get_cpu_env(Monitor *mon);
 CPUState *mon_get_cpu(Monitor *mon);
 
+void hmp_info_mmu(Monitor *mon, const QDict *qdict);
 void hmp_info_mem(Monitor *mon, const QDict *qdict);
 void hmp_info_tlb(Monitor *mon, const QDict *qdict);
 void hmp_mce(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
index 9338ae8440..71bce4870a 100644
--- a/monitor/hmp-cmds-target.c
+++ b/monitor/hmp-cmds-target.c
@@ -31,6 +31,7 @@
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
 #include "sysemu/hw_accel.h"
+#include "sysemu/tcg.h"
 
 /* Set the current CPU defined by the user. Callers must hold BQL. */
 int monitor_set_cpu(Monitor *mon, int cpu_index)
@@ -379,3 +380,39 @@ void hmp_gpa2hpa(Monitor *mon, const QDict *qdict)
     memory_region_unref(mr);
 }
 #endif
+
+__attribute__((weak))
+void hmp_info_mem(Monitor *mon, const QDict *qdict)
+{
+    monitor_puts(mon,
+                 "No per-CPU mapping information available on this target\n");
+}
+
+__attribute__((weak))
+void hmp_info_tlb(Monitor *mon, const QDict *qdict)
+{
+    monitor_puts(mon,
+                 "No per-CPU TLB information available on this target\n");
+}
+
+void hmp_info_mmu(Monitor *mon, const QDict *qdict)
+{
+    bool tlb = qdict_get_try_bool(qdict, "tlb", false);
+    bool mem = qdict_get_try_bool(qdict, "mem", false);
+
+    if (!tcg_enabled()) {
+        monitor_puts(mon, "This command is specific to TCG accelerator\n");
+        return;
+    }
+
+    if (!tlb && !mem) {
+        tlb = mem = true;
+    }
+
+    if (mem) {
+        hmp_info_mem(mon, qdict);
+    }
+    if (tlb) {
+        hmp_info_tlb(mon, qdict);
+    }
+}
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ad1b1306e3..e31f2467fb 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -208,6 +208,20 @@ SRST
     Show PCI information.
 ERST
 
+    {
+        .name       = "mmu",
+        .args_type  = "tlb:-t,mem:-m",
+        .params     = "[-t][-m]",
+        .help       = "show virtual to physical memory "
+                      "(-t: TLB; -m: active mapping)",
+        .cmd        = hmp_info_mmu,
+    },
+
+SRST
+  ``info mmu``
+    Show virtual to physical memory mappings.
+ERST
+
 #if defined(TARGET_I386) || defined(TARGET_SH4) || defined(TARGET_SPARC) || \
     defined(TARGET_PPC) || defined(TARGET_XTENSA) || defined(TARGET_M68K)
     {
-- 
2.41.0

[PATCH-for-9.0 2/2] target/monitor: Deprecate 'info tlb' and 'info mem' commands

[Philippe Mathieu-Daudé]

'info tlb' has been replaced by 'info mmu -t', and
'info mem' by 'info mmu -m'.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/about/deprecated.rst    | 10 ++++++++++
 include/monitor/hmp-target.h |  2 ++
 monitor/hmp-cmds-target.c    | 20 ++++++++++++++++----
 target/i386/monitor.c        |  4 ++--
 target/m68k/monitor.c        |  2 +-
 target/nios2/monitor.c       |  2 +-
 target/ppc/ppc-qmp-cmds.c    |  2 +-
 target/riscv/monitor.c       |  2 +-
 target/sh4/monitor.c         |  2 +-
 target/sparc/monitor.c       |  2 +-
 target/xtensa/monitor.c      |  2 +-
 hmp-commands-info.hx         |  8 ++++----
 12 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 7b548519b5..4f5f4becbe 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -158,6 +158,16 @@ points was removed in 7.0. However QMP still exposed the vcpu
 parameter. This argument has now been deprecated and the remaining
 remaining trace points that used it are selected just by name.
 
+Human Monitor Protocol (HMP) commands
+-------------------------------------
+
+``info tlb`` and ``info mem`` (since 9.0)
+'''''''''''''''''''''''''''''''''''''''''
+
+The ``info tlb`` and ``info mem`` commands have been replaced by
+the ``info mmu`` command, which has the same behaviour but a less
+misleading name.
+
 Host Architectures
 ------------------
 
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 2af84b1915..057f7c6841 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -46,7 +46,9 @@ CPUState *mon_get_cpu(Monitor *mon);
 
 void hmp_info_mmu(Monitor *mon, const QDict *qdict);
 void hmp_info_mem(Monitor *mon, const QDict *qdict);
+void hmp_info_mem_deprecated(Monitor *mon, const QDict *qdict);
 void hmp_info_tlb(Monitor *mon, const QDict *qdict);
+void hmp_info_tlb_deprecated(Monitor *mon, const QDict *qdict);
 void hmp_mce(Monitor *mon, const QDict *qdict);
 void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
 void hmp_info_sev(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
index 71bce4870a..086b58b8d6 100644
--- a/monitor/hmp-cmds-target.c
+++ b/monitor/hmp-cmds-target.c
@@ -382,19 +382,31 @@ void hmp_gpa2hpa(Monitor *mon, const QDict *qdict)
 #endif
 
 __attribute__((weak))
-void hmp_info_mem(Monitor *mon, const QDict *qdict)
+void hmp_info_mem_deprecated(Monitor *mon, const QDict *qdict)
 {
     monitor_puts(mon,
                  "No per-CPU mapping information available on this target\n");
 }
 
 __attribute__((weak))
-void hmp_info_tlb(Monitor *mon, const QDict *qdict)
+void hmp_info_tlb_deprecated(Monitor *mon, const QDict *qdict)
 {
     monitor_puts(mon,
                  "No per-CPU TLB information available on this target\n");
 }
 
+void hmp_info_mem(Monitor *mon, const QDict *qdict)
+{
+    monitor_puts(mon, "This command is deprecated, please use 'info mmu -m'\n");
+    hmp_info_mem_deprecated(mon, qdict);
+}
+
+void hmp_info_tlb(Monitor *mon, const QDict *qdict)
+{
+    monitor_puts(mon, "This command is deprecated, please use 'info mmu -t'\n");
+    hmp_info_tlb_deprecated(mon, qdict);
+}
+
 void hmp_info_mmu(Monitor *mon, const QDict *qdict)
 {
     bool tlb = qdict_get_try_bool(qdict, "tlb", false);
@@ -410,9 +422,9 @@ void hmp_info_mmu(Monitor *mon, const QDict *qdict)
     }
 
     if (mem) {
-        hmp_info_mem(mon, qdict);
+        hmp_info_mem_deprecated(mon, qdict);
     }
     if (tlb) {
-        hmp_info_tlb(mon, qdict);
+        hmp_info_tlb_deprecated(mon, qdict);
     }
 }
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 3a281dab02..5da77b6b22 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -217,7 +217,7 @@ static void tlb_info_la57(Monitor *mon, CPUArchState *env)
 }
 #endif /* TARGET_X86_64 */
 
-void hmp_info_tlb(Monitor *mon, const QDict *qdict)
+void hmp_info_tlb_deprecated(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env;
 
@@ -545,7 +545,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env)
 }
 #endif /* TARGET_X86_64 */
 
-void hmp_info_mem(Monitor *mon, const QDict *qdict)
+void hmp_info_mem_deprecated(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env;
 
diff --git a/target/m68k/monitor.c b/target/m68k/monitor.c
index 2bdf6acae0..ea303805c4 100644
--- a/target/m68k/monitor.c
+++ b/target/m68k/monitor.c
@@ -10,7 +10,7 @@
 #include "monitor/hmp-target.h"
 #include "monitor/monitor.h"
 
-void hmp_info_tlb(Monitor *mon, const QDict *qdict)
+void hmp_info_tlb_deprecated(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env1 = mon_get_cpu_env(mon);
 
diff --git a/target/nios2/monitor.c b/target/nios2/monitor.c
index 0152dec3fa..2e4efee1aa 100644
--- a/target/nios2/monitor.c
+++ b/target/nios2/monitor.c
@@ -27,7 +27,7 @@
 #include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 
-void hmp_info_tlb(Monitor *mon, const QDict *qdict)
+void hmp_info_tlb_deprecated(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env1 = mon_get_cpu_env(mon);
 
diff --git a/target/ppc/ppc-qmp-cmds.c b/target/ppc/ppc-qmp-cmds.c
index a25d86a8d1..891fdc44ef 100644
--- a/target/ppc/ppc-qmp-cmds.c
+++ b/target/ppc/ppc-qmp-cmds.c
@@ -80,7 +80,7 @@ static target_long monitor_get_tbl(Monitor *mon, const struct MonitorDef *md,
     return cpu_ppc_load_tbl(env);
 }
 
-void hmp_info_tlb(Monitor *mon, const QDict *qdict)
+void hmp_info_tlb_deprecated(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env1 = mon_get_cpu_env(mon);
 
diff --git a/target/riscv/monitor.c b/target/riscv/monitor.c
index f5b1ffe6c3..00f2a22f11 100644
--- a/target/riscv/monitor.c
+++ b/target/riscv/monitor.c
@@ -208,7 +208,7 @@ static void mem_info_svxx(Monitor *mon, CPUArchState *env)
               last_paddr + last_size - pbase, last_attr);
 }
 
-void hmp_info_mem(Monitor *mon, const QDict *qdict)
+void hmp_info_mem_deprecated(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env;
 
diff --git a/target/sh4/monitor.c b/target/sh4/monitor.c
index 2da6a5426e..ad6f21defe 100644
--- a/target/sh4/monitor.c
+++ b/target/sh4/monitor.c
@@ -39,7 +39,7 @@ static void print_tlb(Monitor *mon, int idx, tlb_t *tlb)
                    tlb->d, tlb->wt);
 }
 
-void hmp_info_tlb(Monitor *mon, const QDict *qdict)
+void hmp_info_tlb_deprecated(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env = mon_get_cpu_env(mon);
     int i;
diff --git a/target/sparc/monitor.c b/target/sparc/monitor.c
index 73f15aa272..95e6217568 100644
--- a/target/sparc/monitor.c
+++ b/target/sparc/monitor.c
@@ -28,7 +28,7 @@
 #include "monitor/hmp.h"
 
 
-void hmp_info_tlb(Monitor *mon, const QDict *qdict)
+void hmp_info_tlb_deprecated(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env1 = mon_get_cpu_env(mon);
 
diff --git a/target/xtensa/monitor.c b/target/xtensa/monitor.c
index fbf60d5553..92bfad8129 100644
--- a/target/xtensa/monitor.c
+++ b/target/xtensa/monitor.c
@@ -27,7 +27,7 @@
 #include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 
-void hmp_info_tlb(Monitor *mon, const QDict *qdict)
+void hmp_info_tlb_deprecated(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env1 = mon_get_cpu_env(mon);
 
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index e31f2467fb..0066b49ceb 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -228,14 +228,14 @@ ERST
         .name       = "tlb",
         .args_type  = "",
         .params     = "",
-        .help       = "show virtual to physical memory mappings",
+        .help       = "deprecated synonym for 'info mmu -t'",
         .cmd        = hmp_info_tlb,
     },
 #endif
 
 SRST
   ``info tlb``
-    Show virtual to physical memory mappings.
+    This is a deprecated synonym for the mmu command.
 ERST
 
 #if defined(TARGET_I386) || defined(TARGET_RISCV)
@@ -243,14 +243,14 @@ ERST
         .name       = "mem",
         .args_type  = "",
         .params     = "",
-        .help       = "show the active virtual memory mappings",
+        .help       = "deprecated synonym for 'info mmu -m'",
         .cmd        = hmp_info_mem,
     },
 #endif
 
 SRST
   ``info mem``
-    Show the active virtual memory mappings.
+    This is a deprecated synonym for the mmu command.
 ERST
 
     {
-- 
2.41.0

Re: [PATCH-for-9.0 0/2] target/monitor: Deprecate 'info tlb/mem' in favor of 'info mmu'

[Peter Maydell]

On Wed, 20 Mar 2024 at 16:40, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> 'info tlb' and 'info mem' commands don't scale in heterogeneous
> emulation. They will be reworked after the next release, hidden
> behind the 'info mmu' command. It is not too late to deprecate
> commands, so add the 'info mmu' command as wrapper to the other
> ones, but already deprecate them.
>
> Philippe Mathieu-Daudé (2):
>   target/monitor: Introduce 'info mmu' command
>   target/monitor: Deprecate 'info tlb' and 'info mem' commands

This seems to replace "info tlb" and "info mem" with "info mmu -t"
and "info mmu -m", but it doesn't really say anything about:
 * what the difference is between these two things
 * which targets implement which and why
 * what the plan is for the future

I am definitely not a fan of either of these commands, because
(as we currently implement them) they effectively require each
target architecture to implement a second copy of the page table
walking code. But before we can deprecate them we need to be
pretty sure that "info mmu" is what we want to replace them with.

thanks
-- PMM

Re: [PATCH-for-9.0 0/2] target/monitor: Deprecate 'info tlb/mem' in favor of 'info mmu'

[Philippe Mathieu-Daudé]

+Alex/Daniel

On 20/3/24 17:53, Peter Maydell wrote:
> On Wed, 20 Mar 2024 at 16:40, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> 'info tlb' and 'info mem' commands don't scale in heterogeneous
>> emulation. They will be reworked after the next release, hidden
>> behind the 'info mmu' command. It is not too late to deprecate
>> commands, so add the 'info mmu' command as wrapper to the other
>> ones, but already deprecate them.
>>
>> Philippe Mathieu-Daudé (2):
>>    target/monitor: Introduce 'info mmu' command
>>    target/monitor: Deprecate 'info tlb' and 'info mem' commands
> 
> This seems to replace "info tlb" and "info mem" with "info mmu -t"
> and "info mmu -m", but it doesn't really say anything about:
>   * what the difference is between these two things

I really don't know; I'm only trying to keep the monitor interface
identical.

>   * which targets implement which and why

This one is easy to answer:

#if defined(TARGET_I386) || defined(TARGET_SH4) || defined(TARGET_SPARC) 
|| \
     defined(TARGET_PPC) || defined(TARGET_XTENSA) || defined(TARGET_M68K)
     {
         .name       = "tlb",

#if defined(TARGET_I386) || defined(TARGET_RISCV)
     {
         .name       = "mem",

>   * what the plan is for the future

My problem is with linking a single QEMU binary, as these two symbols
(hmp_info_mem and hmp_info_tlb) clash.

Luckily for me these are the only 2 implemented by more then one target:

$ git grep TARGET_ -- hmp-commands*
hmp-commands-info.hx:116:#if defined(TARGET_I386)
hmp-commands-info.hx:225:#if defined(TARGET_I386) || defined(TARGET_SH4) 
|| defined(TARGET_SPARC) || \
hmp-commands-info.hx:226:    defined(TARGET_PPC) || 
defined(TARGET_XTENSA) || defined(TARGET_M68K)
hmp-commands-info.hx:241:#if defined(TARGET_I386) || defined(TARGET_RISCV)
hmp-commands-info.hx:729:#if defined(TARGET_S390X)
hmp-commands-info.hx:744:#if defined(TARGET_S390X)
hmp-commands-info.hx:828:#if defined(TARGET_I386)
hmp-commands-info.hx:882:#if defined(TARGET_I386)
hmp-commands.hx:1126:#if defined(TARGET_S390X)
hmp-commands.hx:1141:#if defined(TARGET_S390X)
hmp-commands.hx:1489:#if defined(TARGET_I386)

All the other ones are only implemented by a single target, so not a
problem for now.

I'm indeed only postponing the problem, without looking at what
this code does. I did it adding hmp_info_mmu_tlb/mem hooks in
TCGCPUOps ("hw/core/tcg-cpu-ops.h"), so the command can be
dispatched per target vcpu as target-agnostic code in
monitor/hmp-cmds.c:

+#include "hw/core/tcg-cpu-ops.h"
+
+static void hmp_info_mmu_tlb(Monitor *mon, CPUState *cpu)
+{
+    const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops;
+
+    if (tcg_ops->hmp_info_mmu_tlb) {
+        tcg_ops->hmp_info_mmu_tlb(mon, cpu_env(cpu));
+    } else {
+        monitor_puts(mon, "No per-CPU information available on this 
target\n");
+    }
+}

> I am definitely not a fan of either of these commands, because
> (as we currently implement them) they effectively require each
> target architecture to implement a second copy of the page table
> walking code. But before we can deprecate them we need to be
> pretty sure that "info mmu" is what we want to replace them with.

An alternative is to just deprecate them, without adding "info mmu" :)

It is OK to un-deprecate stuff if we realize its usefulness.

Regards,

Phil.

Re: [PATCH-for-9.0 0/2] target/monitor: Deprecate 'info tlb/mem' in favor of 'info mmu'

[Peter Maydell]

On Wed, 20 Mar 2024 at 17:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> +Alex/Daniel
>
> On 20/3/24 17:53, Peter Maydell wrote:
> > On Wed, 20 Mar 2024 at 16:40, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> 'info tlb' and 'info mem' commands don't scale in heterogeneous
> >> emulation. They will be reworked after the next release, hidden
> >> behind the 'info mmu' command. It is not too late to deprecate
> >> commands, so add the 'info mmu' command as wrapper to the other
> >> ones, but already deprecate them.
> >>
> >> Philippe Mathieu-Daudé (2):
> >>    target/monitor: Introduce 'info mmu' command
> >>    target/monitor: Deprecate 'info tlb' and 'info mem' commands
> >
> > This seems to replace "info tlb" and "info mem" with "info mmu -t"
> > and "info mmu -m", but it doesn't really say anything about:
> >   * what the difference is between these two things
>
> I really don't know; I'm only trying to keep the monitor interface
> identical.

You don't, though: you change it from "info tlb" to "info mmu -t" etc.

> >   * which targets implement which and why
>
> This one is easy to answer:
>
> #if defined(TARGET_I386) || defined(TARGET_SH4) || defined(TARGET_SPARC)
> || \
>      defined(TARGET_PPC) || defined(TARGET_XTENSA) || defined(TARGET_M68K)
>      {
>          .name       = "tlb",
>
> #if defined(TARGET_I386) || defined(TARGET_RISCV)
>      {
>          .name       = "mem",
>
> >   * what the plan is for the future
>
> My problem is with linking a single QEMU binary, as these two symbols
> (hmp_info_mem and hmp_info_tlb) clash.

Yes, but they both (implicitly) operate on the current HMP CPU,
so the problem with linking into a single binary is that they're
not indirected through a method on the CPU object, not the syntax
used in the monitor to invoke them, presumably.

> I'm indeed only postponing the problem, without looking at what
> this code does. I did it adding hmp_info_mmu_tlb/mem hooks in
> TCGCPUOps ("hw/core/tcg-cpu-ops.h"), so the command can be
> dispatched per target vcpu as target-agnostic code in
> monitor/hmp-cmds.c:
>
> +#include "hw/core/tcg-cpu-ops.h"
> +
> +static void hmp_info_mmu_tlb(Monitor *mon, CPUState *cpu)
> +{
> +    const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops;
> +
> +    if (tcg_ops->hmp_info_mmu_tlb) {
> +        tcg_ops->hmp_info_mmu_tlb(mon, cpu_env(cpu));
> +    } else {
> +        monitor_puts(mon, "No per-CPU information available on this
> target\n");
> +    }
> +}

These aren't TCG specific though, so why TCGCPUOps ?

> > I am definitely not a fan of either of these commands, because
> > (as we currently implement them) they effectively require each
> > target architecture to implement a second copy of the page table
> > walking code. But before we can deprecate them we need to be
> > pretty sure that "info mmu" is what we want to replace them with.
>
> An alternative is to just deprecate them, without adding "info mmu" :)
>
> It is OK to un-deprecate stuff if we realize its usefulness.

The commands are there because some users find them useful.
I just dislike them because I think they're a bit niche and
annoying to implement and not consistent across target
architectures and not very well documented...

By the way, we have no obligation to follow the deprecate-and-drop
process for HMP commands; unlike QMP, we give ourselves the
license to vary it when we feel like it, because the users are
humans, not programs or scripts.

-- PMM

Re: [PATCH-for-9.0 0/2] target/monitor: Deprecate 'info tlb/mem' in favor of 'info mmu'

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Wed, 20 Mar 2024 at 17:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > +Alex/Daniel
> >
> > On 20/3/24 17:53, Peter Maydell wrote:
> > > On Wed, 20 Mar 2024 at 16:40, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > >>
> > >> 'info tlb' and 'info mem' commands don't scale in heterogeneous
> > >> emulation. They will be reworked after the next release, hidden
> > >> behind the 'info mmu' command. It is not too late to deprecate
> > >> commands, so add the 'info mmu' command as wrapper to the other
> > >> ones, but already deprecate them.
> > >>
> > >> Philippe Mathieu-Daudé (2):
> > >>    target/monitor: Introduce 'info mmu' command
> > >>    target/monitor: Deprecate 'info tlb' and 'info mem' commands
> > >
> > > This seems to replace "info tlb" and "info mem" with "info mmu -t"
> > > and "info mmu -m", but it doesn't really say anything about:
> > >   * what the difference is between these two things
> >
> > I really don't know; I'm only trying to keep the monitor interface
> > identical.
> 
> You don't, though: you change it from "info tlb" to "info mmu -t" etc.
> 
> > >   * which targets implement which and why
> >
> > This one is easy to answer:
> >
> > #if defined(TARGET_I386) || defined(TARGET_SH4) || defined(TARGET_SPARC)
> > || \
> >      defined(TARGET_PPC) || defined(TARGET_XTENSA) || defined(TARGET_M68K)
> >      {
> >          .name       = "tlb",
> >
> > #if defined(TARGET_I386) || defined(TARGET_RISCV)
> >      {
> >          .name       = "mem",
> >
> > >   * what the plan is for the future
> >
> > My problem is with linking a single QEMU binary, as these two symbols
> > (hmp_info_mem and hmp_info_tlb) clash.
> 
> Yes, but they both (implicitly) operate on the current HMP CPU,
> so the problem with linking into a single binary is that they're
> not indirected through a method on the CPU object, not the syntax
> used in the monitor to invoke them, presumably.
> 
> > I'm indeed only postponing the problem, without looking at what
> > this code does. I did it adding hmp_info_mmu_tlb/mem hooks in
> > TCGCPUOps ("hw/core/tcg-cpu-ops.h"), so the command can be
> > dispatched per target vcpu as target-agnostic code in
> > monitor/hmp-cmds.c:
> >
> > +#include "hw/core/tcg-cpu-ops.h"
> > +
> > +static void hmp_info_mmu_tlb(Monitor *mon, CPUState *cpu)
> > +{
> > +    const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops;
> > +
> > +    if (tcg_ops->hmp_info_mmu_tlb) {
> > +        tcg_ops->hmp_info_mmu_tlb(mon, cpu_env(cpu));
> > +    } else {
> > +        monitor_puts(mon, "No per-CPU information available on this
> > target\n");
> > +    }
> > +}
> 
> These aren't TCG specific though, so why TCGCPUOps ?
> 
> > > I am definitely not a fan of either of these commands, because
> > > (as we currently implement them) they effectively require each
> > > target architecture to implement a second copy of the page table
> > > walking code. But before we can deprecate them we need to be
> > > pretty sure that "info mmu" is what we want to replace them with.
> >
> > An alternative is to just deprecate them, without adding "info mmu" :)
> >
> > It is OK to un-deprecate stuff if we realize its usefulness.
> 
> The commands are there because some users find them useful.
> I just dislike them because I think they're a bit niche and
> annoying to implement and not consistent across target
> architectures and not very well documented...
> 
> By the way, we have no obligation to follow the deprecate-and-drop
> process for HMP commands; unlike QMP, we give ourselves the
> license to vary it when we feel like it, because the users are
> humans, not programs or scripts.

Right, so no rush to get the deprecation in; change it when you agree
what you'd like a replacement to look like.

Dave

> -- PMM
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/