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