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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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:

-- >8 --
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ab86224ee23..9d2bd2e2ef4 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6732,6 +6732,7 @@ static Property x86_cpu_properties[] = {
 #include "hw/core/sysemu-cpu-ops.h"

 static const struct SysemuCPUOps i386_sysemu_ops = {
+    .format_tlb = x86_cpu_format_tlb,
     .get_memory_mapping = x86_cpu_get_memory_mapping,
     .get_paging_enabled = x86_cpu_get_paging_enabled,
     .get_phys_page_attrs_debug = x86_cpu_get_phys_page_attrs_debug,
@@ -6765,9 +6766,6 @@ static void x86_cpu_common_class_init(ObjectClass
*oc, void *data)
     cc->parse_features = x86_cpu_parse_featurestr;
     cc->has_work = x86_cpu_has_work;
     cc->format_state = x86_cpu_format_state;
-#ifndef CONFIG_USER_ONLY
-    cc->format_tlb = x86_cpu_format_tlb;
-#endif
     cc->set_pc = x86_cpu_set_pc;
     cc->gdb_read_register = x86_cpu_gdb_read_register;
     cc->gdb_write_register = x86_cpu_gdb_write_register;
---

> 
> Regards,
> Daniel
> 
> [1] yes assuming these things is often foolish in QEMU :-)
>