| Message ID | 1361216553-4549-7-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
Andreas Färber <afaerber@suse.de> wrote: > Commit b758aca1f6cdb175634812b79f5560c36c902d00 (target-alpha: Enable > the alpha-softmmu target.) introduced cpu_{save,load}() functions but > didn't define CPU_SAVE_VERSION, so they were never registered. > > Drop cpu_{save,load}() and register the VMStateDescription via CPUClass. > This operates on the AlphaCPU object instead of CPUAlphaState. > > Signed-off-by: Andreas Färber <afaerber@suse.de> Seeing that we are repeating the code all around. Could we change this to something like: > > #define ENV_GET_CPU(e) CPU(alpha_env_get_cpu(e)) > > +#ifdef CONFIG_USER_ONLY > +#define vmstate_alpha_cpu vmstate_dummy > +#else > +extern const struct VMStateDescription vmstate_alpha_cpu; > +#endif > + Change this to: #ifdef CONFIG_USER_ONLY #define vmstate_register_cpu(unused, unused) #else #define vmstate_register_cpu(env, vmstate_cpu) (env->vmsd = vmstate_cpu) #endif > #endif > diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c > index cec9989..0e739ad 100644 > --- a/target-alpha/cpu.c > +++ b/target-alpha/cpu.c > @@ -21,6 +21,7 @@ > > #include "cpu.h" > #include "qemu-common.h" > +#include "migration/vmstate.h" > > > static void alpha_cpu_realizefn(DeviceState *dev, Error **errp) > @@ -263,6 +264,7 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data) > dc->realize = alpha_cpu_realizefn; > > cc->class_by_name = alpha_cpu_class_by_name; > + cc->vmsd = &vmstate_alpha_cpu; vmstate_register_cpu(cc, vmstate_alpha_cpu) And now that I review the 3rd patch that does the same, couldn't be easier to do just: static const VMStateDescription vmstate_cpu = { .name = "cpu", .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, .fields = vmstate_cpu_fields, }; to static const VMStateDescription vmstate_cpu = { .name = "cpu", .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, .fields = vmstate_cpu_fields, }; static const VMStateDescription vmstate_alpha_cpu = { .name = "cpu", .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, .fields = (VMStateField []) { VMSTATE_STRUCT(env, AlphaCPU, 1, &vmstate_cpu, CPUAlphaState), VMSTATE_END_OF_LIST() } }; ???? Only reason that I can think of for all the changes that you have done is if you are expecting to move fields from CPUAlphaState to AlphaCPU. Otherwise the method just exposed is much, much easier.
On Fri, Feb 22, 2013 at 02:22:43PM +0100, Juan Quintela wrote: > Andreas Färber <afaerber@suse.de> wrote: > > Commit b758aca1f6cdb175634812b79f5560c36c902d00 (target-alpha: Enable > > the alpha-softmmu target.) introduced cpu_{save,load}() functions but > > didn't define CPU_SAVE_VERSION, so they were never registered. > > > > Drop cpu_{save,load}() and register the VMStateDescription via CPUClass. > > This operates on the AlphaCPU object instead of CPUAlphaState. > > > > Signed-off-by: Andreas Färber <afaerber@suse.de> > > > Seeing that we are repeating the code all around. Could we change this > to something like: > > > > > #define ENV_GET_CPU(e) CPU(alpha_env_get_cpu(e)) > > > > +#ifdef CONFIG_USER_ONLY > > +#define vmstate_alpha_cpu vmstate_dummy > > +#else > > +extern const struct VMStateDescription vmstate_alpha_cpu; > > +#endif > > + > > Change this to: > > #ifdef CONFIG_USER_ONLY > #define vmstate_register_cpu(unused, unused) > #else > #define vmstate_register_cpu(env, vmstate_cpu) (env->vmsd = vmstate_cpu) > #endif I like this approach. But using a macro is going to cause unexpected "variable is unused" gcc warnings. Can we make it a static inline function instead?
Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Feb 22, 2013 at 02:22:43PM +0100, Juan Quintela wrote: >> Andreas Färber <afaerber@suse.de> wrote: >> > Commit b758aca1f6cdb175634812b79f5560c36c902d00 (target-alpha: Enable >> > the alpha-softmmu target.) introduced cpu_{save,load}() functions but >> > didn't define CPU_SAVE_VERSION, so they were never registered. >> > >> > Drop cpu_{save,load}() and register the VMStateDescription via CPUClass. >> > This operates on the AlphaCPU object instead of CPUAlphaState. >> > >> > Signed-off-by: Andreas Färber <afaerber@suse.de> >> >> >> Seeing that we are repeating the code all around. Could we change this >> to something like: >> >> > >> > #define ENV_GET_CPU(e) CPU(alpha_env_get_cpu(e)) >> > >> > +#ifdef CONFIG_USER_ONLY >> > +#define vmstate_alpha_cpu vmstate_dummy >> > +#else >> > +extern const struct VMStateDescription vmstate_alpha_cpu; >> > +#endif >> > + >> >> Change this to: >> >> #ifdef CONFIG_USER_ONLY >> #define vmstate_register_cpu(unused, unused) >> #else >> #define vmstate_register_cpu(env, vmstate_cpu) (env->vmsd = vmstate_cpu) >> #endif > > I like this approach. But using a macro is going to cause unexpected > "variable is unused" gcc warnings. Can we make it a static inline > function instead? types are different, we can pas a pointer to the vmstate, so (untested)) static inline void vmstate_register_cpu(struct VMStateDescritpion **arg, struct VMStateDescription *vmstate_cpu) { *arg = vmstate_cpu; } Andreas?
Am 22.02.2013 15:14, schrieb Juan Quintela: > Eduardo Habkost <ehabkost@redhat.com> wrote: >> On Fri, Feb 22, 2013 at 02:22:43PM +0100, Juan Quintela wrote: >>> Andreas Färber <afaerber@suse.de> wrote: >>>> Commit b758aca1f6cdb175634812b79f5560c36c902d00 (target-alpha: Enable >>>> the alpha-softmmu target.) introduced cpu_{save,load}() functions but >>>> didn't define CPU_SAVE_VERSION, so they were never registered. >>>> >>>> Drop cpu_{save,load}() and register the VMStateDescription via CPUClass. >>>> This operates on the AlphaCPU object instead of CPUAlphaState. >>>> >>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>> >>> >>> Seeing that we are repeating the code all around. Could we change this >>> to something like: >>> >>>> >>>> #define ENV_GET_CPU(e) CPU(alpha_env_get_cpu(e)) >>>> >>>> +#ifdef CONFIG_USER_ONLY >>>> +#define vmstate_alpha_cpu vmstate_dummy >>>> +#else >>>> +extern const struct VMStateDescription vmstate_alpha_cpu; >>>> +#endif >>>> + >>> >>> Change this to: >>> >>> #ifdef CONFIG_USER_ONLY >>> #define vmstate_register_cpu(unused, unused) >>> #else >>> #define vmstate_register_cpu(env, vmstate_cpu) (env->vmsd = vmstate_cpu) >>> #endif >> >> I like this approach. But using a macro is going to cause unexpected >> "variable is unused" gcc warnings. Can we make it a static inline >> function instead? > > types are different, we can pas a pointer to the vmstate, so (untested)) > > static inline void vmstate_register_cpu(struct VMStateDescritpion **arg, > struct VMStateDescription *vmstate_cpu) > { > *arg = vmstate_cpu; > } > > Andreas? The problem that vmstate_dummy tackles is that vmstate_foo_cpu doesn't exist in the non-softmmu case. I don't see how that would work with an inline function. We could do a redundant cc->vmsd = NULL; in the user-only case. As for the argument type, void cpu_class_set_vmsd(CPUClass *cc, VMStateDescription *vmsd) would work if we find a solution to the extern-referencing problem. I.e., if we have to #define vmstate_foo_cpu for a cpu_class_set_vmsd() function call then it doesn't buy us much. Andreas
Am 22.02.2013 14:22, schrieb Juan Quintela: > And now that I review the 3rd patch that does the same, couldn't be > easier to do just: > > > static const VMStateDescription vmstate_cpu = { > .name = "cpu", > .version_id = 1, > .minimum_version_id = 1, > .minimum_version_id_old = 1, > .fields = vmstate_cpu_fields, > }; > > to > > static const VMStateDescription vmstate_cpu = { > .name = "cpu", > .version_id = 1, > .minimum_version_id = 1, > .minimum_version_id_old = 1, > .fields = vmstate_cpu_fields, > }; > > static const VMStateDescription vmstate_alpha_cpu = { > .name = "cpu", > .version_id = 1, > .minimum_version_id = 1, > .minimum_version_id_old = 1, > .fields = (VMStateField []) { > VMSTATE_STRUCT(env, AlphaCPU, 1, &vmstate_cpu, CPUAlphaState), > VMSTATE_END_OF_LIST() > } > }; > > ???? > > Only reason that I can think of for all the changes that you have done > is if you are expecting to move fields from CPUAlphaState to AlphaCPU. > Otherwise the method just exposed is much, much easier. Thanks for your review of this series! Did you actually read the cover letter though? :) Short-term I do need to access the halted field outside of CPUX86State in the referenced pending series, mid-term I would like to be able to move more non-TCG fields into FooCPU and not be limited by VMState only being able to access CPUArchState fields. Question: Is the VMSTATE_STRUCT() usage above version-compatible for previously-migratable targets such as lm32? We could leave x86 as is (modulo the vmsd registration discussion) and do the struct indirection for lm32 for now to keep changes minimal as long as non-CPULM32State fields are not yet needed. For alpha and openrisc RFCs my big question was rather whether we want to use some VMSTATE_ macro to embed "cpu_common" as first field of "cpu" and assign it to DeviceClass::vmsd? Not sure which of the macros would allow us to register a VMStateDescription standalone for the "legacy" targets and as part of another VMStateDescription without sub-field such as 'env' above or whether we would need to duplicate the fields into a subsection definition then? More generally it would be a some-targets-do-it-differently-than-others kind of issue. But we already have some targets using VMState and others not, so you could say we have the issue today already. ;) In short: My pressing need is a version-compatible solution for x86 registering VMState for X86CPU (CPUState) rather than CPUX86State, anything else could be postponed pending further discussions. Regards, Andreas
Andreas Färber <afaerber@suse.de> wrote: > Am 22.02.2013 14:22, schrieb Juan Quintela: >> And now that I review the 3rd patch that does the same, couldn't be >> easier to do just: >> >> >> static const VMStateDescription vmstate_cpu = { >> .name = "cpu", >> .version_id = 1, >> .minimum_version_id = 1, >> .minimum_version_id_old = 1, >> .fields = vmstate_cpu_fields, >> }; >> >> to >> >> static const VMStateDescription vmstate_cpu = { >> .name = "cpu", >> .version_id = 1, >> .minimum_version_id = 1, >> .minimum_version_id_old = 1, >> .fields = vmstate_cpu_fields, >> }; >> >> static const VMStateDescription vmstate_alpha_cpu = { >> .name = "cpu", >> .version_id = 1, >> .minimum_version_id = 1, >> .minimum_version_id_old = 1, >> .fields = (VMStateField []) { >> VMSTATE_STRUCT(env, AlphaCPU, 1, &vmstate_cpu, CPUAlphaState), >> VMSTATE_END_OF_LIST() >> } >> }; >> >> ???? >> >> Only reason that I can think of for all the changes that you have done >> is if you are expecting to move fields from CPUAlphaState to AlphaCPU. >> Otherwise the method just exposed is much, much easier. > > Thanks for your review of this series! > > Did you actually read the cover letter though? :) Obviously not O:-) /me puts a beer in the "debt" part to Andreas O:-) > Short-term I do need to access the halted field outside of CPUX86State > in the referenced pending series, mid-term I would like to be able to > move more non-TCG fields into FooCPU and not be limited by VMState only > being able to access CPUArchState fields. > > Question: Is the VMSTATE_STRUCT() usage above version-compatible for > previously-migratable targets such as lm32? We could leave x86 as is > (modulo the vmsd registration discussion) and do the struct indirection > for lm32 for now to keep changes minimal as long as non-CPULM32State > fields are not yet needed. It is compatible for all as they are Today. You stop being able to use the "trick" as soon as you want to access things that are not in env. > For alpha and openrisc RFCs my big question was rather whether we want > to use some VMSTATE_ macro to embed "cpu_common" as first field of "cpu" > and assign it to DeviceClass::vmsd? Not sure which of the macros would > allow us to register a VMStateDescription standalone for the "legacy" > targets and as part of another VMStateDescription without sub-field such > as 'env' above or whether we would need to duplicate the fields into a > subsection definition then? > > More generally it would be a some-targets-do-it-differently-than-others > kind of issue. But we already have some targets using VMState and others > not, so you could say we have the issue today already. ;) > > In short: My pressing need is a version-compatible solution for x86 > registering VMState for X86CPU (CPUState) rather than CPUX86State, > anything else could be postponed pending further discussions. If you do the VMSTATE_STRUCT for all targets, you get half of the path done. And then we can study how to do it better for x86. About cpu_common. I think that it is better that you embbed it for new architectures, and just do it the other way for x86. static const VMStateDescription vmstate_alpha_cpu = { .name = "cpu", .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, .fields = (VMStateField []) { VMSTATE_STRUCT(common, AlphaCPU, 1, &vmstate_cpu_common, CPUArchState), VMSTATE_STRUCT(env, AlphaCPU, 1, &vmstate_cpu, CPUAlphaState), VMSTATE_END_OF_LIST() } }; Should work. Notice that I haven't tested that the types are right, I am assuming that the cpu_common field is called common, etc. Later, Juan.
Am 22.02.2013 16:27, schrieb Andreas Färber: > Am 22.02.2013 15:14, schrieb Juan Quintela: >> Eduardo Habkost <ehabkost@redhat.com> wrote: >>> On Fri, Feb 22, 2013 at 02:22:43PM +0100, Juan Quintela wrote: >>>> Andreas Färber <afaerber@suse.de> wrote: >>>>> Commit b758aca1f6cdb175634812b79f5560c36c902d00 (target-alpha: Enable >>>>> the alpha-softmmu target.) introduced cpu_{save,load}() functions but >>>>> didn't define CPU_SAVE_VERSION, so they were never registered. >>>>> >>>>> Drop cpu_{save,load}() and register the VMStateDescription via CPUClass. >>>>> This operates on the AlphaCPU object instead of CPUAlphaState. >>>>> >>>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>> >>>> >>>> Seeing that we are repeating the code all around. Could we change this >>>> to something like: >>>> >>>>> >>>>> #define ENV_GET_CPU(e) CPU(alpha_env_get_cpu(e)) >>>>> >>>>> +#ifdef CONFIG_USER_ONLY >>>>> +#define vmstate_alpha_cpu vmstate_dummy >>>>> +#else >>>>> +extern const struct VMStateDescription vmstate_alpha_cpu; >>>>> +#endif >>>>> + >>>> >>>> Change this to: >>>> >>>> #ifdef CONFIG_USER_ONLY >>>> #define vmstate_register_cpu(unused, unused) >>>> #else >>>> #define vmstate_register_cpu(env, vmstate_cpu) (env->vmsd = vmstate_cpu) >>>> #endif >>> >>> I like this approach. But using a macro is going to cause unexpected >>> "variable is unused" gcc warnings. Can we make it a static inline >>> function instead? >> >> types are different, we can pas a pointer to the vmstate, so (untested)) >> >> static inline void vmstate_register_cpu(struct VMStateDescritpion **arg, >> struct VMStateDescription *vmstate_cpu) >> { >> *arg = vmstate_cpu; >> } >> >> Andreas? > > The problem that vmstate_dummy tackles is that vmstate_foo_cpu doesn't > exist in the non-softmmu case. I don't see how that would work with an > inline function. We could do a redundant cc->vmsd = NULL; in the > user-only case. > > As for the argument type, void cpu_class_set_vmsd(CPUClass *cc, > VMStateDescription *vmsd) would work if we find a solution to the > extern-referencing problem. I.e., if we have to #define vmstate_foo_cpu > for a cpu_class_set_vmsd() function call then it doesn't buy us much. Sent out v3 series with half function, half macro solution. Would be great if you could look at it this week. ;) Regards, Andreas
diff --git a/target-alpha/cpu-qom.h b/target-alpha/cpu-qom.h index c0f6c6d..2f9f78f 100644 --- a/target-alpha/cpu-qom.h +++ b/target-alpha/cpu-qom.h @@ -72,5 +72,11 @@ static inline AlphaCPU *alpha_env_get_cpu(CPUAlphaState *env) #define ENV_GET_CPU(e) CPU(alpha_env_get_cpu(e)) +#ifdef CONFIG_USER_ONLY +#define vmstate_alpha_cpu vmstate_dummy +#else +extern const struct VMStateDescription vmstate_alpha_cpu; +#endif + #endif diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c index cec9989..0e739ad 100644 --- a/target-alpha/cpu.c +++ b/target-alpha/cpu.c @@ -21,6 +21,7 @@ #include "cpu.h" #include "qemu-common.h" +#include "migration/vmstate.h" static void alpha_cpu_realizefn(DeviceState *dev, Error **errp) @@ -263,6 +264,7 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data) dc->realize = alpha_cpu_realizefn; cc->class_by_name = alpha_cpu_class_by_name; + cc->vmsd = &vmstate_alpha_cpu; } static const TypeInfo alpha_cpu_type_info = { diff --git a/target-alpha/machine.c b/target-alpha/machine.c index 1c9edd1..bdad91e 100644 --- a/target-alpha/machine.c +++ b/target-alpha/machine.c @@ -3,14 +3,18 @@ static int get_fpcr(QEMUFile *f, void *opaque, size_t size) { - CPUAlphaState *env = opaque; + AlphaCPU *cpu = opaque; + CPUAlphaState *env = &cpu->env; + cpu_alpha_store_fpcr(env, qemu_get_be64(f)); return 0; } static void put_fpcr(QEMUFile *f, void *opaque, size_t size) { - CPUAlphaState *env = opaque; + AlphaCPU *cpu = opaque; + CPUAlphaState *env = &cpu->env; + qemu_put_be64(f, cpu_alpha_load_fpcr(env)); } @@ -21,8 +25,8 @@ static const VMStateInfo vmstate_fpcr = { }; static VMStateField vmstate_cpu_fields[] = { - VMSTATE_UINTTL_ARRAY(ir, CPUAlphaState, 31), - VMSTATE_UINTTL_ARRAY(fir, CPUAlphaState, 31), + VMSTATE_UINTTL_ARRAY(env.ir, AlphaCPU, 31), + VMSTATE_UINTTL_ARRAY(env.fir, AlphaCPU, 31), /* Save the architecture value of the fpcr, not the internally expanded version. Since this architecture value does not exist in memory to be stored, this requires a but of hoop @@ -37,51 +41,41 @@ static VMStateField vmstate_cpu_fields[] = { .flags = VMS_SINGLE, .offset = 0 }, - VMSTATE_UINTTL(pc, CPUAlphaState), - VMSTATE_UINTTL(unique, CPUAlphaState), - VMSTATE_UINTTL(lock_addr, CPUAlphaState), - VMSTATE_UINTTL(lock_value, CPUAlphaState), + VMSTATE_UINTTL(env.pc, AlphaCPU), + VMSTATE_UINTTL(env.unique, AlphaCPU), + VMSTATE_UINTTL(env.lock_addr, AlphaCPU), + VMSTATE_UINTTL(env.lock_value, AlphaCPU), /* Note that lock_st_addr is not saved; it is a temporary used during the execution of the st[lq]_c insns. */ - VMSTATE_UINT8(ps, CPUAlphaState), - VMSTATE_UINT8(intr_flag, CPUAlphaState), - VMSTATE_UINT8(pal_mode, CPUAlphaState), - VMSTATE_UINT8(fen, CPUAlphaState), + VMSTATE_UINT8(env.ps, AlphaCPU), + VMSTATE_UINT8(env.intr_flag, AlphaCPU), + VMSTATE_UINT8(env.pal_mode, AlphaCPU), + VMSTATE_UINT8(env.fen, AlphaCPU), - VMSTATE_UINT32(pcc_ofs, CPUAlphaState), + VMSTATE_UINT32(env.pcc_ofs, AlphaCPU), - VMSTATE_UINTTL(trap_arg0, CPUAlphaState), - VMSTATE_UINTTL(trap_arg1, CPUAlphaState), - VMSTATE_UINTTL(trap_arg2, CPUAlphaState), + VMSTATE_UINTTL(env.trap_arg0, AlphaCPU), + VMSTATE_UINTTL(env.trap_arg1, AlphaCPU), + VMSTATE_UINTTL(env.trap_arg2, AlphaCPU), - VMSTATE_UINTTL(exc_addr, CPUAlphaState), - VMSTATE_UINTTL(palbr, CPUAlphaState), - VMSTATE_UINTTL(ptbr, CPUAlphaState), - VMSTATE_UINTTL(vptptr, CPUAlphaState), - VMSTATE_UINTTL(sysval, CPUAlphaState), - VMSTATE_UINTTL(usp, CPUAlphaState), + VMSTATE_UINTTL(env.exc_addr, AlphaCPU), + VMSTATE_UINTTL(env.palbr, AlphaCPU), + VMSTATE_UINTTL(env.ptbr, AlphaCPU), + VMSTATE_UINTTL(env.vptptr, AlphaCPU), + VMSTATE_UINTTL(env.sysval, AlphaCPU), + VMSTATE_UINTTL(env.usp, AlphaCPU), - VMSTATE_UINTTL_ARRAY(shadow, CPUAlphaState, 8), - VMSTATE_UINTTL_ARRAY(scratch, CPUAlphaState, 24), + VMSTATE_UINTTL_ARRAY(env.shadow, AlphaCPU, 8), + VMSTATE_UINTTL_ARRAY(env.scratch, AlphaCPU, 24), VMSTATE_END_OF_LIST() }; -static const VMStateDescription vmstate_cpu = { +const VMStateDescription vmstate_alpha_cpu = { .name = "cpu", .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, .fields = vmstate_cpu_fields, }; - -void cpu_save(QEMUFile *f, void *opaque) -{ - vmstate_save_state(f, &vmstate_cpu, opaque); -} - -int cpu_load(QEMUFile *f, void *opaque, int version_id) -{ - return vmstate_load_state(f, &vmstate_cpu, opaque, version_id); -}
Commit b758aca1f6cdb175634812b79f5560c36c902d00 (target-alpha: Enable the alpha-softmmu target.) introduced cpu_{save,load}() functions but didn't define CPU_SAVE_VERSION, so they were never registered. Drop cpu_{save,load}() and register the VMStateDescription via CPUClass. This operates on the AlphaCPU object instead of CPUAlphaState. Signed-off-by: Andreas Färber <afaerber@suse.de> --- target-alpha/cpu-qom.h | 6 +++++ target-alpha/cpu.c | 2 ++ target-alpha/machine.c | 64 ++++++++++++++++++++++-------------------------- 3 Dateien geändert, 37 Zeilen hinzugefügt(+), 35 Zeilen entfernt(-)