Patchwork [qom-cpu-next] cpus: Replace open-coded CPU loop in qmp_memsave() with qemu_get_cpu()

login
register
mail settings
Submitter Andreas Färber
Date Feb. 15, 2013, 2:47 p.m.
Message ID <1360939661-23623-1-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/220753/
State New
Headers show

Comments

Andreas Färber - Feb. 15, 2013, 2:47 p.m.
No functional change, just less usages of first_cpu and next_cpu fields.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 cpus.c |   11 +++--------
 1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
Luiz Capitulino - Feb. 18, 2013, 12:57 p.m.
On Fri, 15 Feb 2013 15:47:41 +0100
Andreas Färber <afaerber@suse.de> wrote:

> No functional change, just less usages of first_cpu and next_cpu fields.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

One comment below.

> ---
>  cpus.c |   11 +++--------
>  1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 41779eb..845e915 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1262,18 +1262,13 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>          cpu_index = 0;
>      }
>  
> -    for (env = first_cpu; env; env = env->next_cpu) {
> -        cpu = ENV_GET_CPU(env);
> -        if (cpu_index == cpu->cpu_index) {
> -            break;
> -        }
> -    }
> -
> -    if (env == NULL) {
> +    cpu = qemu_get_cpu(cpu_index);
> +    if (cpu == NULL) {
>          error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
>                    "a CPU number");
>          return;
>      }
> +    env = cpu->env_ptr;

I wonder if the callees should be taking a CPUState object instead of
CPUArchState.

>  
>      f = fopen(filename, "wb");
>      if (!f) {
Andreas Färber - Feb. 18, 2013, 3:09 p.m.
Am 18.02.2013 13:57, schrieb Luiz Capitulino:
> On Fri, 15 Feb 2013 15:47:41 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> No functional change, just less usages of first_cpu and next_cpu fields.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
> 
> One comment below.
> 
>> ---
>>  cpus.c |   11 +++--------
>>  1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 41779eb..845e915 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1262,18 +1262,13 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>>          cpu_index = 0;
>>      }
>>  
>> -    for (env = first_cpu; env; env = env->next_cpu) {
>> -        cpu = ENV_GET_CPU(env);
>> -        if (cpu_index == cpu->cpu_index) {
>> -            break;
>> -        }
>> -    }
>> -
>> -    if (env == NULL) {
>> +    cpu = qemu_get_cpu(cpu_index);
>> +    if (cpu == NULL) {
>>          error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
>>                    "a CPU number");
>>          return;
>>      }
>> +    env = cpu->env_ptr;
> 
> I wonder if the callees should be taking a CPUState object instead of
> CPUArchState.

In general yes. However this was a side-product of a bug investigation
for ppc, re-reviewing my cpu_index field movement patch.

The user here is cpu_memory_rw_debug(), which for softmmu calls
cpu_get_phys_page_debug(), which in turn is a target-specific global
function. Both have quite a number of callers, so I'd like to schedule
that after my pending qom-cpu-9 refactorings to avoid conflicts, and
cpu_get_phys_page_debug() feels like a candidate for a CPUState callback
to avoid target-specific globals.

Independently, the function needs to stay in exec.c since it uses
TARGET_PAGE_{SIZE,MASK} for its implementation.

I appreciate you thinking of this though!

Thanks, applied to qom-cpu-next (with extended commit message):
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next

Regards,
Andreas

>>  
>>      f = fopen(filename, "wb");
>>      if (!f) {

Patch

diff --git a/cpus.c b/cpus.c
index 41779eb..845e915 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1262,18 +1262,13 @@  void qmp_memsave(int64_t addr, int64_t size, const char *filename,
         cpu_index = 0;
     }
 
-    for (env = first_cpu; env; env = env->next_cpu) {
-        cpu = ENV_GET_CPU(env);
-        if (cpu_index == cpu->cpu_index) {
-            break;
-        }
-    }
-
-    if (env == NULL) {
+    cpu = qemu_get_cpu(cpu_index);
+    if (cpu == NULL) {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
                   "a CPU number");
         return;
     }
+    env = cpu->env_ptr;
 
     f = fopen(filename, "wb");
     if (!f) {