[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH qom-cpu-next] cpus: Replace open-coded CPU loop
From: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCH qom-cpu-next] cpus: Replace open-coded CPU loop in qmp_memsave() with qemu_get_cpu() |
Date: |
Mon, 18 Feb 2013 16:09:22 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130105 Thunderbird/17.0.2 |
Am 18.02.2013 13:57, schrieb Luiz Capitulino:
> On Fri, 15 Feb 2013 15:47:41 +0100
> Andreas Färber <address@hidden> wrote:
>
>> No functional change, just less usages of first_cpu and next_cpu fields.
>>
>> Signed-off-by: Andreas Färber <address@hidden>
>
> Reviewed-by: Luiz Capitulino <address@hidden>
>
> 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) {
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg