Re: [PATCH v2 42/53] hw/core: introduce a 'format_tlb' callback

[Daniel P . Berrangé]

On Tue, Sep 14, 2021 at 07:02:19PM +0200, Philippe Mathieu-Daudé wrote:
> On 9/14/21 6:34 PM, Daniel P. Berrangé wrote:
> > On Tue, Sep 14, 2021 at 05:56:09PM +0200, Philippe Mathieu-Daudé wrote:
> >> On 9/14/21 4:20 PM, Daniel P. Berrangé wrote:
> >>> This will allow us to reduce duplication between the different targets
> >>> implementing the 'info tlb' command.
> >>>
> >>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >>> ---
> >>>  hw/core/cpu-common.c  |  9 +++++++++
> >>>  include/hw/core/cpu.h | 11 +++++++++++
> >>>  2 files changed, 20 insertions(+)
> >>
> >>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> >>> index 4c47e1df18..64fc57c8d9 100644
> >>> --- a/include/hw/core/cpu.h
> >>> +++ b/include/hw/core/cpu.h
> >>>   * @has_work: Callback for checking if there is work to do.
> >>>   * @memory_rw_debug: Callback for GDB memory access.
> >>>   * @format_state: Callback for formatting state.
> >>> + * @format_tlb: Callback for formatting memory mappings
> 
> "... for formatting translations of virtual to physical memory mappings"
> 
> >>>   * @get_arch_id: Callback for getting architecture-dependent CPU ID.
> >>>   * @set_pc: Callback for setting the Program Counter register. This
> >>>   *       should have the semantics used by the target architecture when
> >>> @@ -136,6 +137,7 @@ struct CPUClass {
> >>>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
> >>>                             uint8_t *buf, int len, bool is_write);
> >>>      void (*format_state)(CPUState *cpu, GString *buf, int flags);
> >>> +    void (*format_tlb)(CPUState *cpu, GString *buf);
> >>
> >> Doesn't this belong to SysemuCPUOps?
> > 
> > I can't really answer, since my knowledge of this area of QEMU code is
> > fairly mimimal. I put it here because it is basically serving the same
> > purpose as the "format_state" callback immediately above it, which was
> > a rename of the existing "dump_state" callback. I assumed whatever was
> > there already was a good practice to follow[1]...
> 
> Since it involves physical memory, I'm pretty sure this is sysemu
> specific. Beside in the following patches you guard the handlers
> with '#ifndef CONFIG_USER_ONLY'.
> 
> Good news, there is very few changes needed in your patches, for
> example the next patch:

.snip..

yes I see what you mean now, and agree this looks like a better
approach

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|