[Qemu-devel] Virtualising qmp_query_cpus() arch specifics

[Qemu-devel] Virtualising qmp_query_cpus() arch specifics

[Peter Crosthwaite]

Hi All,

So for my multi-arch work, one of the eventual requirements is to
remove all #define TARGET_FOO from core code. I came across this in
cpus.c/qmp_query_cpus():

#if defined(TARGET_I386)
        X86CPU *x86_cpu = X86_CPU(cpu);
        CPUX86State *env = &x86_cpu->env;
#elif defined(TARGET_PPC)
        PowerPCCPU *ppc_cpu = POWERPC_CPU(cpu);
        CPUPPCState *env = &ppc_cpu->env;
...

#if defined(TARGET_I386)
        info->value->has_pc = true;
        info->value->pc = env->eip + env->segs[R_CS].base;
#elif defined(TARGET_PPC)
        info->value->has_nip = true;
        info->value->nip = env->nip;
...

This should probably be a QOM CPU virtual function. What makes me
uneasy about it however, is a direct implementation means the QAPI
autogenerated CPUInfoList struct would need to be exposed to
target-foo/cpu.c. Is this ok or an abstraction fail? Do we need some
other other minimal struct to communicate between target-foo and QMP
with these particulars? Any third options?

Regards,
Peter

Re: [Qemu-devel] Virtualising qmp_query_cpus() arch specifics

[Andreas Färber]

Am 09.07.2015 um 08:36 schrieb Peter Crosthwaite:
> Hi All,
> 
> So for my multi-arch work, one of the eventual requirements is to
> remove all #define TARGET_FOO from core code. I came across this in
> cpus.c/qmp_query_cpus():
> 
> #if defined(TARGET_I386)
>         X86CPU *x86_cpu = X86_CPU(cpu);
>         CPUX86State *env = &x86_cpu->env;
> #elif defined(TARGET_PPC)
>         PowerPCCPU *ppc_cpu = POWERPC_CPU(cpu);
>         CPUPPCState *env = &ppc_cpu->env;
> ...
> 
> #if defined(TARGET_I386)
>         info->value->has_pc = true;
>         info->value->pc = env->eip + env->segs[R_CS].base;
> #elif defined(TARGET_PPC)
>         info->value->has_nip = true;
>         info->value->nip = env->nip;
> ...
> 
> This should probably be a QOM CPU virtual function. What makes me
> uneasy about it however, is a direct implementation means the QAPI
> autogenerated CPUInfoList struct would need to be exposed to
> target-foo/cpu.c. Is this ok or an abstraction fail? Do we need some
> other other minimal struct to communicate between target-foo and QMP
> with these particulars? Any third options?

Wouldn't that just be a get_pc() matching your set_pc() hook?

Unfortunately there's name differences above, pc vs. nip. Maybe we can
just use a generic field and keep #ifdef stuff only as legacy compat?

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] Virtualising qmp_query_cpus() arch specifics

[Peter Crosthwaite]

On Thu, Jul 9, 2015 at 5:31 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 09.07.2015 um 08:36 schrieb Peter Crosthwaite:
>> Hi All,
>>
>> So for my multi-arch work, one of the eventual requirements is to
>> remove all #define TARGET_FOO from core code. I came across this in
>> cpus.c/qmp_query_cpus():
>>
>> #if defined(TARGET_I386)
>>         X86CPU *x86_cpu = X86_CPU(cpu);
>>         CPUX86State *env = &x86_cpu->env;
>> #elif defined(TARGET_PPC)
>>         PowerPCCPU *ppc_cpu = POWERPC_CPU(cpu);
>>         CPUPPCState *env = &ppc_cpu->env;
>> ...
>>
>> #if defined(TARGET_I386)
>>         info->value->has_pc = true;
>>         info->value->pc = env->eip + env->segs[R_CS].base;
>> #elif defined(TARGET_PPC)
>>         info->value->has_nip = true;
>>         info->value->nip = env->nip;
>> ...
>>
>> This should probably be a QOM CPU virtual function. What makes me
>> uneasy about it however, is a direct implementation means the QAPI
>> autogenerated CPUInfoList struct would need to be exposed to
>> target-foo/cpu.c. Is this ok or an abstraction fail? Do we need some
>> other other minimal struct to communicate between target-foo and QMP
>> with these particulars? Any third options?
>
> Wouldn't that just be a get_pc() matching your set_pc() hook?
>
> Unfortunately there's name differences above, pc vs. nip. Maybe we can
> just use a generic field and keep #ifdef stuff only as legacy compat?
>

Not sure I follow exactly. But I thought of another option. The
multi-arch work allows you to include multiple cpu.h's from code that
doesn't need the cpu-defs.h particulars. cpus.c should qualify. So it
is entirely possible that instead we keep this all local here and use
QOM RTTI:

#include target-foo/cpu.h
#include target-bar/cpu.h

if (object_dynamic_cast(cpu, TYPE_FOO) {
    FooCpu *foo_cpu = FOO_CPU(cpu);
    ...
} else if (object_dynamic_cast(cpu, TYPE_BAR) {
    BarCpu *bar_cpu = BAR_CPU(cpu);
    ...
}

This would push this cleanup until after the multi-arch main series.

Regards,
Peter

> Cheers,
> Andreas
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
> 21284 (AG Nürnberg)
>