Re: [PATCH v2 38/53] qapi: introduce x-query-lapic QMP command

[Dongli Zhang]

Hi Daniel,

On 9/14/21 7:20 AM, Daniel P. Berrangé wrote:
> This is a counterpart to the HMP "info lapic" command. It is being
> added with an "x-" prefix because this QMP command is intended as an
> adhoc debugging tool and will thus not be modelled in QAPI as fully
> structured data, nor will it have long term guaranteed stability.
> The existing HMP command is rewritten to call the QMP command.
> 
> This command is unable to use the pre-existing HumanReadableText,
> because if 'common.json' is included into 'machine-target.json'
> the static marshalling method for HumanReadableText will be reported
> as unused by the compiler on all architectures except s390x.
> 
> Possible options were
> 
>  1 Support 'if' conditionals on 'include' statements in QAPI
>  2 Add further commands to 'machine-target.json' that use
>    HumanReadableText, such that it has at least one usage
>    on all architecture targets.
>  3 Duplicate HumanReadableText as TargetHumanReadableText
>    adding conditions
> 
> This patch takes option (3) in the belief that we will eventually
> get to a point where option (2) happens, and TargetHumanReadableText
> can be removed again.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/core/cpu-common.c     |   7 ++
>  include/hw/core/cpu.h    |  10 +++
>  qapi/machine-target.json |  19 ++++-
>  target/i386/cpu-dump.c   | 161 ++++++++++++++++++++-------------------
>  target/i386/cpu.h        |   4 +-
>  target/i386/monitor.c    |  46 +++++++++--
>  6 files changed, 160 insertions(+), 87 deletions(-)
> 
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index c2cd33a817..d1ebc77d1b 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -49,6 +49,13 @@ CPUState *cpu_by_arch_id(int64_t id)
>      return NULL;
>  }
>  
> +int64_t cpu_get_arch_id(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    return cc->get_arch_id(cpu);
> +}
> +
>  bool cpu_exists(int64_t id)
>  {
>      return !!cpu_by_arch_id(id);
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 1599ef9df3..a0913eedaa 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -780,6 +780,16 @@ bool cpu_exists(int64_t id);
>   */
>  CPUState *cpu_by_arch_id(int64_t id);
>  
> +/**
> + * cpu_get_arch_id:
> + * @cpu: the CPU to query
> + *
> + * Get the guest exposed CPU ID for @cpu
> + *
> + * Returns: The guest exposed CPU ID
> + */
> +int64_t cpu_get_arch_id(CPUState *cpu);
> +
>  /**
>   * cpu_interrupt:
>   * @cpu: The CPU to set an interrupt on.
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 9040aff863..62220d1f08 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -353,7 +353,8 @@
>  ##
>  { 'struct': 'TargetHumanReadableText',
>    'data': { 'human-readable-text': 'str' },
> -  'if': 'TARGET_S390X' }
> +  'if': { 'any': ['TARGET_S390X',
> +                  'TARGET_I386' ] } }
>  
>  ##
>  # @x-query-cmma:
> @@ -369,6 +370,22 @@
>    'returns': 'TargetHumanReadableText',
>    'if': 'TARGET_S390X' }
>  
> +##
> +# @x-query-lapic:
> +#
> +# @apic-id: the local APIC ID to report
> +#
> +# Query local APIC state.
> +#
> +# Returns: local APIC state
> +#
> +# Since: 6.2
> +##
> +{ 'command': 'x-query-lapic',
> +  'data': { 'apic-id': 'int' },
> +  'returns': 'TargetHumanReadableText',
> +  'if': 'TARGET_I386' }
> +
>  ##
>  # @x-query-skeys:
>  #
> diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
> index f30fbcb76e..41a1f64138 100644
> --- a/target/i386/cpu-dump.c
> +++ b/target/i386/cpu-dump.c
> @@ -20,6 +20,7 @@
>  #include "qemu/osdep.h"
>  #include "cpu.h"
>  #include "qemu/qemu-print.h"
> +#include "qapi/error.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "hw/i386/apic_internal.h"
>  #endif
> @@ -179,24 +180,26 @@ static inline const char *dm2str(uint32_t dm)
>      return str[dm];
>  }
>  
> -static void dump_apic_lvt(const char *name, uint32_t lvt, bool is_timer)
> +static void format_apic_lvt(const char *name, uint32_t lvt, bool is_timer,
> +                            GString *buf)
>  {
>      uint32_t dm = (lvt & APIC_LVT_DELIV_MOD) >> APIC_LVT_DELIV_MOD_SHIFT;
> -    qemu_printf("%s\t 0x%08x %s %-5s %-6s %-7s %-12s %-6s",
> -                name, lvt,
> -                lvt & APIC_LVT_INT_POLARITY ? "active-lo" : "active-hi",
> -                lvt & APIC_LVT_LEVEL_TRIGGER ? "level" : "edge",
> -                lvt & APIC_LVT_MASKED ? "masked" : "",
> -                lvt & APIC_LVT_DELIV_STS ? "pending" : "",
> -                !is_timer ?
> -                    "" : lvt & APIC_LVT_TIMER_PERIODIC ?
> -                            "periodic" : lvt & APIC_LVT_TIMER_TSCDEADLINE ?
> -                                            "tsc-deadline" : "one-shot",
> +    g_string_append_printf(buf, "%s\t 0x%08x %s %-5s %-6s %-7s %-12s %-6s",
> +                           name, lvt,
> +                           lvt & APIC_LVT_INT_POLARITY ?
> +                           "active-lo" : "active-hi",
> +                           lvt & APIC_LVT_LEVEL_TRIGGER ? "level" : "edge",
> +                           lvt & APIC_LVT_MASKED ? "masked" : "",
> +                           lvt & APIC_LVT_DELIV_STS ? "pending" : "",
> +                           !is_timer ?
> +                           "" : lvt & APIC_LVT_TIMER_PERIODIC ?
> +                           "periodic" : lvt & APIC_LVT_TIMER_TSCDEADLINE ?
> +                           "tsc-deadline" : "one-shot",
>                  dm2str(dm));
>      if (dm != APIC_DM_NMI) {
> -        qemu_printf(" (vec %u)\n", lvt & APIC_VECTOR_MASK);
> +        g_string_append_printf(buf, " (vec %u)\n", lvt & APIC_VECTOR_MASK);
>      } else {
> -        qemu_printf("\n");
> +        g_string_append_printf(buf, "\n");
>      }
>  }
>  
> @@ -228,7 +231,7 @@ static inline void mask2str(char *str, uint32_t val, 
> uint8_t size)
>  
>  #define MAX_LOGICAL_APIC_ID_MASK_SIZE 16
>  
> -static void dump_apic_icr(APICCommonState *s, CPUX86State *env)
> +static void format_apic_icr(APICCommonState *s, CPUX86State *env, GString 
> *buf)
>  {
>      uint32_t icr = s->icr[0], icr2 = s->icr[1];
>      uint8_t dest_shorthand = \
> @@ -238,16 +241,16 @@ static void dump_apic_icr(APICCommonState *s, 
> CPUX86State *env)
>      uint32_t dest_field;
>      bool x2apic;
>  
> -    qemu_printf("ICR\t 0x%08x %s %s %s %s\n",
> -                icr,
> -                logical_mod ? "logical" : "physical",
> -                icr & APIC_ICR_TRIGGER_MOD ? "level" : "edge",
> -                icr & APIC_ICR_LEVEL ? "assert" : "de-assert",
> -                shorthand2str(dest_shorthand));
> +    g_string_append_printf(buf, "ICR\t 0x%08x %s %s %s %s\n",
> +                           icr,
> +                           logical_mod ? "logical" : "physical",
> +                           icr & APIC_ICR_TRIGGER_MOD ? "level" : "edge",
> +                           icr & APIC_ICR_LEVEL ? "assert" : "de-assert",
> +                           shorthand2str(dest_shorthand));
>  
> -    qemu_printf("ICR2\t 0x%08x", icr2);
> +    g_string_append_printf(buf, "ICR2\t 0x%08x", icr2);
>      if (dest_shorthand != 0) {
> -        qemu_printf("\n");
> +        g_string_append_printf(buf, "\n");
>          return;
>      }
>      x2apic = env->features[FEAT_1_ECX] & CPUID_EXT_X2APIC;
> @@ -255,96 +258,100 @@ static void dump_apic_icr(APICCommonState *s, 
> CPUX86State *env)
>  
>      if (!logical_mod) {
>          if (x2apic) {
> -            qemu_printf(" cpu %u (X2APIC ID)\n", dest_field);
> +            g_string_append_printf(buf, " cpu %u (X2APIC ID)\n", dest_field);
>          } else {
> -            qemu_printf(" cpu %u (APIC ID)\n",
> -                        dest_field & APIC_LOGDEST_XAPIC_ID);
> +            g_string_append_printf(buf, " cpu %u (APIC ID)\n",
> +                                   dest_field & APIC_LOGDEST_XAPIC_ID);
>          }
>          return;
>      }
>  
>      if (s->dest_mode == 0xf) { /* flat mode */
>          mask2str(apic_id_str, icr2 >> APIC_ICR_DEST_SHIFT, 8);
> -        qemu_printf(" mask %s (APIC ID)\n", apic_id_str);
> +        g_string_append_printf(buf, " mask %s (APIC ID)\n", apic_id_str);
>      } else if (s->dest_mode == 0) { /* cluster mode */
>          if (x2apic) {
>              mask2str(apic_id_str, dest_field & APIC_LOGDEST_X2APIC_ID, 16);
> -            qemu_printf(" cluster %u mask %s (X2APIC ID)\n",
> -                        dest_field >> APIC_LOGDEST_X2APIC_SHIFT, 
> apic_id_str);
> +            g_string_append_printf(buf, " cluster %u mask %s (X2APIC ID)\n",
> +                                   dest_field >> APIC_LOGDEST_X2APIC_SHIFT,
> +                                   apic_id_str);
>          } else {
>              mask2str(apic_id_str, dest_field & APIC_LOGDEST_XAPIC_ID, 4);
> -            qemu_printf(" cluster %u mask %s (APIC ID)\n",
> -                        dest_field >> APIC_LOGDEST_XAPIC_SHIFT, apic_id_str);
> +            g_string_append_printf(buf, " cluster %u mask %s (APIC ID)\n",
> +                                   dest_field >> APIC_LOGDEST_XAPIC_SHIFT,
> +                                   apic_id_str);
>          }
>      }
>  }
>  
> -static void dump_apic_interrupt(const char *name, uint32_t *ireg_tab,
> -                                uint32_t *tmr_tab)
> +static void format_apic_interrupt(const char *name, uint32_t *ireg_tab,
> +                                  uint32_t *tmr_tab, GString *buf)
>  {
>      int i, empty = true;
>  
> -    qemu_printf("%s\t ", name);
> +    g_string_append_printf(buf, "%s\t ", name);
>      for (i = 0; i < 256; i++) {
>          if (apic_get_bit(ireg_tab, i)) {
> -            qemu_printf("%u%s ", i,
> -                        apic_get_bit(tmr_tab, i) ? "(level)" : "");
> +            g_string_append_printf(buf, "%u%s ", i,
> +                                   apic_get_bit(tmr_tab, i) ? "(level)" : 
> "");
>              empty = false;
>          }
>      }
> -    qemu_printf("%s\n", empty ? "(none)" : "");
> +    g_string_append_printf(buf, "%s\n", empty ? "(none)" : "");
>  }
>  
> -void x86_cpu_dump_local_apic_state(CPUState *cs, int flags)
> +GString *x86_cpu_format_local_apic_state(CPUState *cs, int flags, Error 
> **errp)
>  {
> +    g_autoptr(GString) buf = g_string_new("");
>      X86CPU *cpu = X86_CPU(cs);
>      APICCommonState *s = APIC_COMMON(cpu->apic_state);
>      if (!s) {
> -        qemu_printf("local apic state not available\n");
> -        return;
> +        error_setg(errp, "local apic state not available");
> +        return NULL;
>      }
>      uint32_t *lvt = s->lvt;
>  
> -    qemu_printf("dumping local APIC state for CPU %-2u\n\n",
> -                CPU(cpu)->cpu_index);
> -    dump_apic_lvt("LVT0", lvt[APIC_LVT_LINT0], false);
> -    dump_apic_lvt("LVT1", lvt[APIC_LVT_LINT1], false);
> -    dump_apic_lvt("LVTPC", lvt[APIC_LVT_PERFORM], false);
> -    dump_apic_lvt("LVTERR", lvt[APIC_LVT_ERROR], false);
> -    dump_apic_lvt("LVTTHMR", lvt[APIC_LVT_THERMAL], false);
> -    dump_apic_lvt("LVTT", lvt[APIC_LVT_TIMER], true);
> -
> -    qemu_printf("Timer\t DCR=0x%x (divide by %u) initial_count = %u"
> -                " current_count = %u\n",
> -                s->divide_conf & APIC_DCR_MASK,
> -                divider_conf(s->divide_conf),
> -                s->initial_count, apic_get_current_count(s));
> -
> -    qemu_printf("SPIV\t 0x%08x APIC %s, focus=%s, spurious vec %u\n",
> -                s->spurious_vec,
> -                s->spurious_vec & APIC_SPURIO_ENABLED ? "enabled" : 
> "disabled",
> -                s->spurious_vec & APIC_SPURIO_FOCUS ? "on" : "off",
> -                s->spurious_vec & APIC_VECTOR_MASK);
> -
> -    dump_apic_icr(s, &cpu->env);
> -
> -    qemu_printf("ESR\t 0x%08x\n", s->esr);
> -
> -    dump_apic_interrupt("ISR", s->isr, s->tmr);
> -    dump_apic_interrupt("IRR", s->irr, s->tmr);
> -
> -    qemu_printf("\nAPR 0x%02x TPR 0x%02x DFR 0x%02x LDR 0x%02x",
> -                s->arb_id, s->tpr, s->dest_mode, s->log_dest);
> +    g_string_append_printf(buf, "dumping local APIC state for CPU %-2u\n\n",
> +                           CPU(cpu)->cpu_index);
> +    format_apic_lvt("LVT0", lvt[APIC_LVT_LINT0], false, buf);
> +    format_apic_lvt("LVT1", lvt[APIC_LVT_LINT1], false, buf);
> +    format_apic_lvt("LVTPC", lvt[APIC_LVT_PERFORM], false, buf);
> +    format_apic_lvt("LVTERR", lvt[APIC_LVT_ERROR], false, buf);
> +    format_apic_lvt("LVTTHMR", lvt[APIC_LVT_THERMAL], false, buf);
> +    format_apic_lvt("LVTT", lvt[APIC_LVT_TIMER], true, buf);
> +
> +    g_string_append_printf(buf,
> +                           "Timer\t DCR=0x%x (divide by %u) initial_count = 
> %u"
> +                           " current_count = %u\n",
> +                           s->divide_conf & APIC_DCR_MASK,
> +                           divider_conf(s->divide_conf),
> +                           s->initial_count, apic_get_current_count(s));
> +
> +    g_string_append_printf(buf,
> +                           "SPIV\t 0x%08x APIC %s, focus=%s, spurious vec 
> %u\n",
> +                           s->spurious_vec,
> +                           s->spurious_vec & APIC_SPURIO_ENABLED ?
> +                           "enabled" : "disabled",
> +                           s->spurious_vec & APIC_SPURIO_FOCUS ? "on" : 
> "off",
> +                           s->spurious_vec & APIC_VECTOR_MASK);
> +
> +    format_apic_icr(s, &cpu->env, buf);
> +
> +    g_string_append_printf(buf, "ESR\t 0x%08x\n", s->esr);
> +
> +    format_apic_interrupt("ISR", s->isr, s->tmr, buf);
> +    format_apic_interrupt("IRR", s->irr, s->tmr, buf);
> +
> +    g_string_append_printf(buf, "\nAPR 0x%02x TPR 0x%02x DFR 0x%02x LDR 
> 0x%02x",
> +                           s->arb_id, s->tpr, s->dest_mode, s->log_dest);
>      if (s->dest_mode == 0) {
> -        qemu_printf("(cluster %u: id %u)",
> -                    s->log_dest >> APIC_LOGDEST_XAPIC_SHIFT,
> -                    s->log_dest & APIC_LOGDEST_XAPIC_ID);
> +        g_string_append_printf(buf, "(cluster %u: id %u)",
> +                               s->log_dest >> APIC_LOGDEST_XAPIC_SHIFT,
> +                               s->log_dest & APIC_LOGDEST_XAPIC_ID);
>      }
> -    qemu_printf(" PPR 0x%02x\n", apic_get_ppr(s));
> -}
> -#else
> -void x86_cpu_dump_local_apic_state(CPUState *cs, int flags)
> -{
> +    g_string_append_printf(buf, " PPR 0x%02x\n", apic_get_ppr(s));
> +
> +    return g_steal_pointer(&buf);
>  }
>  #endif /* !CONFIG_USER_ONLY */
>  
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index d87c8808f6..2bcb175da8 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -2201,8 +2201,10 @@ void x86_cpu_set_default_version(X86CPUVersion 
> version);
>  #define APIC_DEFAULT_ADDRESS 0xfee00000
>  #define APIC_SPACE_SIZE      0x100000
>  
> +#ifndef CONFIG_USER_ONLY
>  /* cpu-dump.c */
> -void x86_cpu_dump_local_apic_state(CPUState *cs, int flags);
> +GString *x86_cpu_format_local_apic_state(CPUState *cs, int flags, Error 
> **errp);
> +#endif /* !CONFIG_USER_ONLY */
>  
>  /* cpu.c */
>  bool cpu_is_bsp(X86CPU *cpu);
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 19468c4e85..fc09f90059 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -33,6 +33,7 @@
>  #include "qapi/error.h"
>  #include "sev_i386.h"
>  #include "qapi/qapi-commands-misc-target.h"
> +#include "qapi/qapi-commands-machine-target.h"
>  #include "qapi/qapi-commands-misc.h"
>  #include "hw/i386/pc.h"
>  
> @@ -650,23 +651,52 @@ const MonitorDef *target_monitor_defs(void)
>      return monitor_defs;
>  }
>  
> +TargetHumanReadableText *qmp_x_query_lapic(int64_t apicid,
> +                                           Error **errp)
> +{
> +    TargetHumanReadableText *ret;
> +    g_autoptr(GString) buf = NULL;
> +    CPUState *cs = cpu_by_arch_id(apicid);
> +
> +    if (!cs) {
> +        error_setg(errp, "No CPU with APIC ID %" PRId64 " available", 
> apicid);
> +        return NULL;
> +    }

Suppose the accelerator is KVM, the CPUState (cs) might not be syncrhonized with
KVM kernel space (APIC+PIR). As a result, the data is stale.

I sent below patch long time ago but it never got reviewed.

https://lore.kernel.org/qemu-devel/20210908143803.29191-1-dongli.zhang@oracle.com/

> +
> +    buf = x86_cpu_format_local_apic_state(cs, CPU_DUMP_FPU, errp);
> +    if (!buf) {
> +        return NULL;
> +    }
> +
> +    ret = g_new0(TargetHumanReadableText, 1);
> +    ret->human_readable_text = g_steal_pointer(&buf->str);
> +    return ret;
> +}
> +
>  void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
>  {
> -    CPUState *cs;
> +    Error *err = NULL;
> +    g_autoptr(TargetHumanReadableText) info = NULL;
> +    int64_t apicid;
>  
>      if (qdict_haskey(qdict, "apic-id")) {
> -        int id = qdict_get_try_int(qdict, "apic-id", 0);
> -        cs = cpu_by_arch_id(id);
> +        apicid = qdict_get_try_int(qdict, "apic-id", 0);

Here is where I used to fix with the patch.

Thank you very much!

Dongli Zhang

>      } else {
> -        cs = mon_get_cpu(mon);
> +        CPUState *cs = mon_get_cpu(mon);
> +        if (!cs) {
> +            monitor_printf(mon, "No CPU available\n");
> +            return;
> +        }
> +        apicid = cpu_get_arch_id(cs);
>      }
>  
> -
> -    if (!cs) {
> -        monitor_printf(mon, "No CPU available\n");
> +    info = qmp_x_query_lapic(apicid, &err);
> +    if (err) {
> +        error_report_err(err);
>          return;
>      }
> -    x86_cpu_dump_local_apic_state(cs, CPU_DUMP_FPU);
> +
> +    monitor_printf(mon, "%s", info->human_readable_text);
>  }
>  
>  SevInfo *qmp_query_sev(Error **errp)
>