Patchwork Fix monitor 'info registers' command on multi processor guests

login
register
mail settings
Submitter Erlon Cruz
Date Jan. 30, 2013, 6:24 p.m.
Message ID <AB3B4A2BF00C7341A4ADB28108160A9E02A26C2E@AMSACEX4.americas.ad.flextronics.com>
Download mbox | patch
Permalink /patch/216953/
State New
Headers show

Comments

Erlon Cruz - Jan. 30, 2013, 6:24 p.m.
QEMU monitor command 'info registers' only displays information for the first
CPU. This fix that by show registers information for each CPU in the system

Signed-off-by: erlon.cruz@fit-tecnologia.org.br
---
 monitor.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)
Markus Armbruster - Jan. 31, 2013, 9:51 a.m.
"Erlon Cruz" <Erlon.Cruz@fit-tecnologia.org.br> writes:

> QEMU monitor command 'info registers' only displays information for the first
> CPU. This fix that by show registers information for each CPU in the system

This is incorrect.  It displays information for the *current* CPU.
Monitor command "cpu" selects the current CPU.

I doubt we want to change this command.  But I'm reviewing it regardless
of my doubts.

> Signed-off-by: erlon.cruz@fit-tecnologia.org.br
> ---
>  monitor.c |   19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index c0e32d6..70e9227 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -898,8 +898,14 @@ int monitor_get_cpu_index(void)
>  static void do_info_registers(Monitor *mon)
>  {
>      CPUArchState *env;
> -    env = mon_get_cpu();
> -    cpu_dump_state(env, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
> +
> +    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> +        monitor_fprintf((FILE *)mon, "========================= "
> +                "Registers on CPU %d =========================\n",
> +                env->cpu_index);
> +        cpu_dump_state(env, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
> +        monitor_fprintf((FILE *)mon, "\n");
> +    }
>  }
>  

You lost the cpu_synchronize_state() in mon_get_cpu().  Why is that
okay?

>  static void do_info_jit(Monitor *mon)
> @@ -930,8 +936,13 @@ static void do_info_cpu_stats(Monitor *mon)
>  {
>      CPUArchState *env;
>  
> -    env = mon_get_cpu();
> -    cpu_dump_statistics(env, (FILE *)mon, &monitor_fprintf, 0);
> +    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> +        monitor_fprintf((FILE *)mon, "========================= "
> +                "Statistics for CPU %d =========================\n",
> +                env->cpu_index);
> +        cpu_dump_statistics(env, (FILE *)mon, &monitor_fprintf, 0);
> +        monitor_fprintf((FILE *)mon, "\n");
> +    }
>  }
>  #endif

Same here.

Furthermore, you neglected to update the command's documentation in
hmp-commands.hx, and the reference to it in qmp-commands.hx.
Luiz Capitulino - Jan. 31, 2013, 11:33 a.m.
On Thu, 31 Jan 2013 10:51:10 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> "Erlon Cruz" <Erlon.Cruz@fit-tecnologia.org.br> writes:
> 
> > QEMU monitor command 'info registers' only displays information for the first
> > CPU. This fix that by show registers information for each CPU in the system
> 
> This is incorrect.  It displays information for the *current* CPU.
> Monitor command "cpu" selects the current CPU.
> 
> I doubt we want to change this command.  But I'm reviewing it regardless
> of my doubts.

I think a better way of doing this would be to add two new optional
arguments to 'info registers': -i (cpu index) and -a (print all CPUs). By
default the command would do what it does today.
Markus Armbruster - Jan. 31, 2013, 12:12 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 31 Jan 2013 10:51:10 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> "Erlon Cruz" <Erlon.Cruz@fit-tecnologia.org.br> writes:
>> 
>> > QEMU monitor command 'info registers' only displays information
>> > for the first
>> > CPU. This fix that by show registers information for each CPU in the system
>> 
>> This is incorrect.  It displays information for the *current* CPU.
>> Monitor command "cpu" selects the current CPU.
>> 
>> I doubt we want to change this command.  But I'm reviewing it regardless
>> of my doubts.
>
> I think a better way of doing this would be to add two new optional
> arguments to 'info registers': -i (cpu index) and -a (print all CPUs). By
> default the command would do what it does today.

Sounds good to me.
Erlon Cruz - Jan. 31, 2013, 1:12 p.m.
On 01/31/2013 07:51 AM, Markus Armbruster wrote:
> "Erlon Cruz" <Erlon.Cruz@fit-tecnologia.org.br> writes:
>
>> QEMU monitor command 'info registers' only displays information for the first
>> CPU. This fix that by show registers information for each CPU in the system
> This is incorrect.  It displays information for the *current* CPU.
> Monitor command "cpu" selects the current CPU.
That is a bit confusing. May be pointing in the command documentation 
that the current cpu could be changed would help.
>
> I doubt we want to change this command.  But I'm reviewing it regardless
> of my doubts.
>
>> Signed-off-by: erlon.cruz@fit-tecnologia.org.br
>> ---
>>   monitor.c |   19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index c0e32d6..70e9227 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -898,8 +898,14 @@ int monitor_get_cpu_index(void)
>>   static void do_info_registers(Monitor *mon)
>>   {
>>       CPUArchState *env;
>> -    env = mon_get_cpu();
>> -    cpu_dump_state(env, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
>> +
>> +    for (env = first_cpu; env != NULL; env = env->next_cpu) {
>> +        monitor_fprintf((FILE *)mon, "========================= "
>> +                "Registers on CPU %d =========================\n",
>> +                env->cpu_index);
>> +        cpu_dump_state(env, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
>> +        monitor_fprintf((FILE *)mon, "\n");
>> +    }
>>   }
>>   
> You lost the cpu_synchronize_state() in mon_get_cpu().  Why is that
> okay?
I missed that. It will show inconsistent information on KVM guests if we 
dont use cpu_syncronize_state().
>
>>   static void do_info_jit(Monitor *mon)
>> @@ -930,8 +936,13 @@ static void do_info_cpu_stats(Monitor *mon)
>>   {
>>       CPUArchState *env;
>>   
>> -    env = mon_get_cpu();
>> -    cpu_dump_statistics(env, (FILE *)mon, &monitor_fprintf, 0);
>> +    for (env = first_cpu; env != NULL; env = env->next_cpu) {
>> +        monitor_fprintf((FILE *)mon, "========================= "
>> +                "Statistics for CPU %d =========================\n",
>> +                env->cpu_index);
>> +        cpu_dump_statistics(env, (FILE *)mon, &monitor_fprintf, 0);
>> +        monitor_fprintf((FILE *)mon, "\n");
>> +    }
>>   }
>>   #endif
> Same here.
>
> Furthermore, you neglected to update the command's documentation in
> hmp-commands.hx, and the reference to it in qmp-commands.hx.
> .
In this case the command syntax didn't  changed right? What would need 
to be changed in the documentation?

Legal Disclaimer:
The information contained in this message may be privileged and confidential. It is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. If you have received this message in error, please immediately notify the sender and delete or destroy any copy of this message!
Erlon Cruz - Jan. 31, 2013, 1:14 p.m.
On 01/31/2013 09:33 AM, Luiz Capitulino wrote:
> On Thu, 31 Jan 2013 10:51:10 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> "Erlon Cruz" <Erlon.Cruz@fit-tecnologia.org.br> writes:
>>
>>> QEMU monitor command 'info registers' only displays information for the first
>>> CPU. This fix that by show registers information for each CPU in the system
>> This is incorrect.  It displays information for the *current* CPU.
>> Monitor command "cpu" selects the current CPU.
>>
>> I doubt we want to change this command.  But I'm reviewing it regardless
>> of my doubts.
> I think a better way of doing this would be to add two new optional
> arguments to 'info registers': -i (cpu index) and -a (print all CPUs). By
> default the command would do what it does today.
> .
How about also printing the CPU index before the infos?

Legal Disclaimer:
The information contained in this message may be privileged and confidential. It is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. If you have received this message in error, please immediately notify the sender and delete or destroy any copy of this message!
Luiz Capitulino - Jan. 31, 2013, 3:12 p.m.
On Thu, 31 Jan 2013 05:12:25 -0800
"Erlon Cruz" <Erlon.Cruz@fit-tecnologia.org.br> wrote:

> Legal Disclaimer:
> The information contained in this message may be privileged and confidential. It is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. If you have received this message in error, please immediately notify the sender and delete or destroy any copy of this message!

This is not appropriate for a public ML (or for sending patches), please
drop this.

PS: I won't reply to messages containing such disclaimer nor take patch
from you if they have it.
Erlon Cruz - Jan. 31, 2013, 4:47 p.m.
On Thu, Jan 31, 2013 at 1:12 PM, Luiz Capitulino <lcapitulino@redhat.com>wrote:

> On Thu, 31 Jan 2013 05:12:25 -0800
> "Erlon Cruz" <Erlon.Cruz@fit-tecnologia.org.br> wrote:
>
> > Legal Disclaimer:
> > The information contained in this message may be privileged and
> confidential. It is intended to be read only by the individual or entity to
> whom it is addressed or by their designee. If the reader of this message is
> not the intended recipient, you are on notice that any distribution of this
> message, in any form, is strictly prohibited. If you have received this
> message in error, please immediately notify the sender and delete or
> destroy any copy of this message!
>
> This is not appropriate for a public ML (or for sending patches), please
> drop this.
>
> PS: I won't reply to messages containing such disclaimer nor take patch
> from you if they have it.
>

Sorry, I haven't  noticed that my mail server was appending this.

Patch

diff --git a/monitor.c b/monitor.c
index c0e32d6..70e9227 100644
--- a/monitor.c
+++ b/monitor.c
@@ -898,8 +898,14 @@  int monitor_get_cpu_index(void)
 static void do_info_registers(Monitor *mon)
 {
     CPUArchState *env;
-    env = mon_get_cpu();
-    cpu_dump_state(env, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        monitor_fprintf((FILE *)mon, "========================= "
+                "Registers on CPU %d =========================\n",
+                env->cpu_index);
+        cpu_dump_state(env, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
+        monitor_fprintf((FILE *)mon, "\n");
+    }
 }
 
 static void do_info_jit(Monitor *mon)
@@ -930,8 +936,13 @@  static void do_info_cpu_stats(Monitor *mon)
 {
     CPUArchState *env;
 
-    env = mon_get_cpu();
-    cpu_dump_statistics(env, (FILE *)mon, &monitor_fprintf, 0);
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        monitor_fprintf((FILE *)mon, "========================= "
+                "Statistics for CPU %d =========================\n",
+                env->cpu_index);
+        cpu_dump_statistics(env, (FILE *)mon, &monitor_fprintf, 0);
+        monitor_fprintf((FILE *)mon, "\n");
+    }
 }
 #endif