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

[Eduardo Habkost]

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?

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

-- 
Eduardo