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.