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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Sep 20, 2021 at 10:27:06PM -0700, Dongli Zhang wrote:
> 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/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/

I'll include your patch in the next version of this series
just before this patch, and then update this patch to  take
it into account.

> 
> > +
> > +    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)
> > 
> 

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 :|