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   |_______/