diff mbox

[v7,13/13] hmp: Add "info ppc-cpu-cores" command

Message ID 1453960195-15181-14-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bharata B Rao Jan. 28, 2016, 5:49 a.m. UTC
This is the hmp equivalent of "query ppc-cpu-cores"

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hmp-commands-info.hx | 16 ++++++++++++++++
 hmp.c                | 31 +++++++++++++++++++++++++++++++
 hmp.h                |  1 +
 3 files changed, 48 insertions(+)

Comments

Eric Blake Jan. 28, 2016, 9:56 p.m. UTC | #1
On 01/27/2016 10:49 PM, Bharata B Rao wrote:
> This is the hmp equivalent of "query ppc-cpu-cores"

The QMP command is spelled "query-ppc-cpu-cores".

Most HMP commands prefer '_' over '-'; so this should be 'info
ppc_cpu_cores'.

> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hmp-commands-info.hx | 16 ++++++++++++++++
>  hmp.c                | 31 +++++++++++++++++++++++++++++++
>  hmp.h                |  1 +
>  3 files changed, 48 insertions(+)
> 

> +++ b/hmp.c
> @@ -2375,3 +2375,34 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
>  
>      qapi_free_RockerOfDpaGroupList(list);
>  }
> +
> +void hmp_info_ppc_cpu_cores(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    PPCCPUCoreList *ppc_cpu_core_list = qmp_query_ppc_cpu_cores(&err);
> +    PPCCPUCoreList *s = ppc_cpu_core_list;
> +    CpuInfoList *thread;
> +
> +    while (s) {
> +        monitor_printf(mon, "PowerPC CPU device: \"%s\"\n",
> +                       s->value->id ? s->value->id : "");

This should probably be checking s->value->has_id rather than assuming
that s->value->id will be NULL when not present (well, I'd like to clean
up qapi to avoid the need for has_FOO when FOO  is a pointer, but we're
not there yet).

> +        monitor_printf(mon, "  hotplugged: %s\n",
> +                           s->value->hotplugged ? "true" : "false");
> +        monitor_printf(mon, "  hotpluggable: %s\n",
> +                           s->value->hotpluggable ? "true" : "false");
> +        monitor_printf(mon, "  Threads:\n");
> +        for (thread = s->value->threads; thread; thread = thread->next) {
> +            monitor_printf(mon, "    CPU #%" PRId64 ":", thread->value->CPU);
> +            monitor_printf(mon, " nip=0x%016" PRIx64,
> +                           thread->value->u.ppc->nip);

This uses value->u.ppc without first checking that the discriminator
value->arch is set to CPU_INFO_ARCH_PPC; could that be a problem down
the road?
Bharata B Rao Jan. 29, 2016, 6:49 a.m. UTC | #2
On Thu, Jan 28, 2016 at 02:56:41PM -0700, Eric Blake wrote:
> On 01/27/2016 10:49 PM, Bharata B Rao wrote:
> > This is the hmp equivalent of "query ppc-cpu-cores"
> 
> The QMP command is spelled "query-ppc-cpu-cores".
> 
> Most HMP commands prefer '_' over '-'; so this should be 'info
> ppc_cpu_cores'.

I see that a few commands have '-' but as you note, majority of them use
'_'. Though I personally prefer '-', if HMP convention is to go with '_',
will change in the next iteration.

> 
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hmp-commands-info.hx | 16 ++++++++++++++++
> >  hmp.c                | 31 +++++++++++++++++++++++++++++++
> >  hmp.h                |  1 +
> >  3 files changed, 48 insertions(+)
> > 
> 
> > +++ b/hmp.c
> > @@ -2375,3 +2375,34 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
> >  
> >      qapi_free_RockerOfDpaGroupList(list);
> >  }
> > +
> > +void hmp_info_ppc_cpu_cores(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *err = NULL;
> > +    PPCCPUCoreList *ppc_cpu_core_list = qmp_query_ppc_cpu_cores(&err);
> > +    PPCCPUCoreList *s = ppc_cpu_core_list;
> > +    CpuInfoList *thread;
> > +
> > +    while (s) {
> > +        monitor_printf(mon, "PowerPC CPU device: \"%s\"\n",
> > +                       s->value->id ? s->value->id : "");
> 
> This should probably be checking s->value->has_id rather than assuming
> that s->value->id will be NULL when not present (well, I'd like to clean
> up qapi to avoid the need for has_FOO when FOO  is a pointer, but we're
> not there yet).

Ok, will switch to s->value->has_id ? s->value->id : "")

> 
> > +        monitor_printf(mon, "  hotplugged: %s\n",
> > +                           s->value->hotplugged ? "true" : "false");
> > +        monitor_printf(mon, "  hotpluggable: %s\n",
> > +                           s->value->hotpluggable ? "true" : "false");
> > +        monitor_printf(mon, "  Threads:\n");
> > +        for (thread = s->value->threads; thread; thread = thread->next) {
> > +            monitor_printf(mon, "    CPU #%" PRId64 ":", thread->value->CPU);
> > +            monitor_printf(mon, " nip=0x%016" PRIx64,
> > +                           thread->value->u.ppc->nip);
> 
> This uses value->u.ppc without first checking that the discriminator
> value->arch is set to CPU_INFO_ARCH_PPC; could that be a problem down
> the road?

Can't think of any potential problems as this command is PowerPC
specific.

BTW can you please let me know what else is needed from QEMU end to
drive this PowerPC CPU core device hotplug from libvirt ?

Regards,
Bharata.
diff mbox

Patch

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 9b71351..cd9a42e 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -786,6 +786,22 @@  STEXI
 Display the value of a storage key (s390 only)
 ETEXI
 
+#if defined(TARGET_PPC64)
+    {
+        .name       = "ppc-cpu-cores",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show PowerPC CPU core devices",
+        .mhandler.cmd = hmp_info_ppc_cpu_cores,
+    },
+#endif
+
+STEXI
+@item info ppc-cpu-cores
+@findex ppc-cpu-cores
+Show PowerPC CPU core devices.
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/hmp.c b/hmp.c
index 54f2620..ae75aa1 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2375,3 +2375,34 @@  void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
 
     qapi_free_RockerOfDpaGroupList(list);
 }
+
+void hmp_info_ppc_cpu_cores(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    PPCCPUCoreList *ppc_cpu_core_list = qmp_query_ppc_cpu_cores(&err);
+    PPCCPUCoreList *s = ppc_cpu_core_list;
+    CpuInfoList *thread;
+
+    while (s) {
+        monitor_printf(mon, "PowerPC CPU device: \"%s\"\n",
+                       s->value->id ? s->value->id : "");
+        monitor_printf(mon, "  hotplugged: %s\n",
+                           s->value->hotplugged ? "true" : "false");
+        monitor_printf(mon, "  hotpluggable: %s\n",
+                           s->value->hotpluggable ? "true" : "false");
+        monitor_printf(mon, "  Threads:\n");
+        for (thread = s->value->threads; thread; thread = thread->next) {
+            monitor_printf(mon, "    CPU #%" PRId64 ":", thread->value->CPU);
+            monitor_printf(mon, " nip=0x%016" PRIx64,
+                           thread->value->u.ppc->nip);
+            if (thread->value->halted) {
+                monitor_printf(mon, " (halted)");
+            }
+            monitor_printf(mon, " thread_id=%" PRId64 "\n",
+                           thread->value->thread_id);
+        }
+        s = s->next;
+    }
+
+    qapi_free_PPCCPUCoreList(ppc_cpu_core_list);
+}
diff --git a/hmp.h b/hmp.h
index a8c5b5a..a31e3d2 100644
--- a/hmp.h
+++ b/hmp.h
@@ -131,5 +131,6 @@  void hmp_rocker(Monitor *mon, const QDict *qdict);
 void hmp_rocker_ports(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
+void hmp_info_ppc_cpu_cores(Monitor *mon, const QDict *qdict);
 
 #endif