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

2015-05-06 Thread Michael Mueller
On Tue, 5 May 2015 10:14:32 -0300
Eduardo Habkost ehabk...@redhat.com 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 m...@linux.vnet.ibm.com
  Acked-by: Christian Borntraeger borntrae...@de.ibm.com
 
 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.  

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




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

2015-05-06 Thread 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 ehabk...@redhat.com 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 m...@linux.vnet.ibm.com
   Acked-by: Christian Borntraeger borntrae...@de.ibm.com
  
  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



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

2015-05-06 Thread Eduardo Habkost
On Wed, May 06, 2015 at 08:59:56AM -0400, Luiz Capitulino wrote:
 On Wed, 6 May 2015 07:38:53 -0300
 Eduardo Habkost ehabk...@redhat.com 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 ehabk...@redhat.com 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 m...@linux.vnet.ibm.com
 Acked-by: Christian Borntraeger borntrae...@de.ibm.com

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.

Thanks! If HMP is seen as a debugging interface, my main objections
aren't valid.

That said, I would prefer to keep the command output cleaner and add
only the model field, as people can use info kvm for the accel info
by now.

-- 
Eduardo



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

2015-05-06 Thread Michael Mueller
On Wed, 6 May 2015 10:33:55 -0300
Eduardo Habkost ehabk...@redhat.com wrote:

 On Wed, May 06, 2015 at 08:59:56AM -0400, Luiz Capitulino wrote:
  On Wed, 6 May 2015 07:38:53 -0300
  Eduardo Habkost ehabk...@redhat.com 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 ehabk...@redhat.com 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 m...@linux.vnet.ibm.com
  Acked-by: Christian Borntraeger borntrae...@de.ibm.com
 
 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.
 
 Thanks! If HMP is seen as a debugging interface, my main objections
 aren't valid.
 
 That said, I would prefer to keep the command output cleaner and add
 only the model field, as people can use info kvm for the accel info
 by now.

Ok, I buy that and will kick the accel name out again.

Michael

 




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

2015-05-06 Thread Luiz Capitulino
On Wed, 6 May 2015 07:38:53 -0300
Eduardo Habkost ehabk...@redhat.com 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 ehabk...@redhat.com 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 m...@linux.vnet.ibm.com
Acked-by: Christian Borntraeger borntrae...@de.ibm.com
   
   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

   
  
 




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

2015-05-05 Thread Eduardo Habkost
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 m...@linux.vnet.ibm.com
 Acked-by: Christian Borntraeger borntrae...@de.ibm.com

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.


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