Re: [Qemu-devel] [PATCH v6 04/17] Extend HMP command info cpus to display accelerator id and model name

[Luiz Capitulino]

On Wed, 6 May 2015 07:38:53 -0300
Eduardo Habkost <address@hidden> wrote:

> On Wed, May 06, 2015 at 09:32:58AM +0200, Michael Mueller wrote:
> > On Tue, 5 May 2015 10:14:32 -0300
> > Eduardo Habkost <address@hidden> wrote:
> > 
> > > On Mon, Apr 27, 2015 at 04:53:18PM +0200, Michael Mueller wrote:
> > > > The HMP command info cpus now displays the CPU model name and the
> > > > backing accelerator if part of the CPUState.
> > > > 
> > > > (qemu) info cpus
> > > > * CPU #0: (halted) model=2827-ga2 accel=kvm thread_id=1679
> > > > 
> > > > Signed-off-by: Michael Mueller <address@hidden>
> > > > Acked-by: Christian Borntraeger <address@hidden>
> > > 
> > > Do we really need this? I mean: I expect the amount of CPU data we
> > > provide to QMP clients to grow a lot in the near future, but that
> > > doesn't mean HMP users need all that data to be printed by "info cpus".
> > 
> > Where do you see the limit of what is worth to be shown an what not.
> > I personally use "info cpus" less then sporadic but already got a comment
> > internally on that information being worthwhile to be shown.  
> 
> I really don't know, but I think we shouldn't add stuff to HMP unless we
> have a good reason. For each new piece of data in HMP I would like to at
> least see the description of a real use case that justifies adding it to
> HMP and not just implementing a simple script on top of QMP.
> 
> For accel info we already have "info kvm" that is not ideal but is
> enough for current use cases, isn't it? CPU model name information seems
> to be more useful, but if it is just for debugging, people can just run
> QMP query-cpus command.
> 
> Luiz, what do you think?

I don't see a problem with that. HMP is a debugging interface anyways.
Actually, I think it's a good test-case for QMP having a high-level
in-tree client (vs. qmp-shell, which is too low-level).

If the problem is that a command is dumping too much information to
the point of hurting usability, we can split the command or add a '-a'
option or something like that.

> 
> > 
> > > 
> > > 
> > > > ---
> > > >  hmp.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/hmp.c b/hmp.c
> > > > index f142d36..676d821 100644
> > > > --- a/hmp.c
> > > > +++ b/hmp.c
> > > > @@ -290,6 +290,13 @@ void hmp_info_cpus(Monitor *mon, const QDict 
> > > > *qdict)
> > > >              monitor_printf(mon, " (halted)");
> > > >          }
> > > >  
> > > > +        if (cpu->value->has_model) {
> > > > +            monitor_printf(mon, " model=%s", cpu->value->model);
> > > > +        }
> > > > +        if (cpu->value->has_accel) {
> > > > +            monitor_printf(mon, " accel=%s", 
> > > > AccelId_lookup[cpu->value->accel]);
> > > > +        }
> > > > +
> > > >          monitor_printf(mon, " thread_id=%" PRId64 "\n", 
> > > > cpu->value->thread_id);
> > > >      }
> > > >  
> > > > -- 
> > > > 1.8.3.1
> > > > 
> > > 
> > 
>