| Message ID | 1425479753-18349-2-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 4, 2015 at 8:35 AM, Alex Bennée <alex.bennee@linaro.org> wrote: > This adds the saving and restore of the current Multi-Processing state > of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of > potential states for x86 we only use two for ARM. Either the process is > running or not. We then save this state into the cpu_powered TCG state > to avoid changing the serialisation format. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > v2 > - make mpstate field runtime dependant (kvm_enabled()) > - drop initial KVM_CAP_MP_STATE requirement > - re-use cpu_powered instead of new field > > diff --git a/target-arm/machine.c b/target-arm/machine.c > index 9446e5a..185f9a2 100644 > --- a/target-arm/machine.c > +++ b/target-arm/machine.c > @@ -161,6 +161,7 @@ static const VMStateInfo vmstate_cpsr = { > .put = put_cpsr, > }; > > + > remove extraneous space > static void cpu_pre_save(void *opaque) > { > ARMCPU *cpu = opaque; > @@ -170,6 +171,20 @@ static void cpu_pre_save(void *opaque) > /* This should never fail */ > abort(); > } > +#if defined CONFIG_KVM > The convention for ifdefing KVMatures appears to be more around the feature rather than KVM_CONFIG. For instance loo fek at kvm_check_extension in kvm-all.c. I may be missing something but if you follow this convention this should be #ifdef KVM_CAP_MP_STATE > + if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) { > + struct kvm_mp_state mp_state; > + int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, > &mp_state); > + if (ret) { > + fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n", > + __func__, ret, strerror(ret)); > + abort(); > + } > + cpu->powered_off = > + (mp_state.mp_state == KVM_MP_STATE_RUNNABLE) > + ? false : true; > + } > +#endif > } else { > if (!write_cpustate_to_list(cpu)) { > /* This should never fail. */ > @@ -222,6 +237,20 @@ static int cpu_post_load(void *opaque, int version_id) > * we're using it. > */ > write_list_to_cpustate(cpu); > +#if defined CONFIG_KVM > + if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) { > + struct kvm_mp_state mp_state = { > + .mp_state = > + cpu->powered_off ? KVM_MP_STATE_HALTED : > KVM_MP_STATE_RUNNABLE > + }; > + int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, > &mp_state); > + if (ret) { > + fprintf(stderr, "%s: failed to set MP_STATE %d/%s\n", > + __func__, ret, strerror(ret)); > + return -1; > + } > + } > +#endif > } else { > if (!write_list_to_cpustate(cpu)) { > return -1; > -- > 2.3.1 > > > Besides these the above nits... Reviewed-by: Greg Bellows <greg.bellows@linaro.org>
On 4 March 2015 at 14:35, Alex Bennée <alex.bennee@linaro.org> wrote: > This adds the saving and restore of the current Multi-Processing state > of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of > potential states for x86 we only use two for ARM. Either the process is > running or not. We then save this state into the cpu_powered TCG state > to avoid changing the serialisation format. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > v2 > - make mpstate field runtime dependant (kvm_enabled()) > - drop initial KVM_CAP_MP_STATE requirement > - re-use cpu_powered instead of new field > > diff --git a/target-arm/machine.c b/target-arm/machine.c > index 9446e5a..185f9a2 100644 > --- a/target-arm/machine.c > +++ b/target-arm/machine.c > @@ -161,6 +161,7 @@ static const VMStateInfo vmstate_cpsr = { > .put = put_cpsr, > }; > > + > static void cpu_pre_save(void *opaque) > { > ARMCPU *cpu = opaque; > @@ -170,6 +171,20 @@ static void cpu_pre_save(void *opaque) > /* This should never fail */ > abort(); > } > +#if defined CONFIG_KVM > + if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) { > + struct kvm_mp_state mp_state; > + int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state); > + if (ret) { > + fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n", > + __func__, ret, strerror(ret)); > + abort(); > + } > + cpu->powered_off = > + (mp_state.mp_state == KVM_MP_STATE_RUNNABLE) > + ? false : true; Ternary operator to produce a true-or-false result is a bit redundant... > + } > +#endif Why is this in pre-save/post-load rather than in the kvm_arch_get/put_registers functions like all the other syncing code? -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 4 March 2015 at 14:35, Alex Bennée <alex.bennee@linaro.org> wrote: >> This adds the saving and restore of the current Multi-Processing state >> of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of >> potential states for x86 we only use two for ARM. Either the process is >> running or not. We then save this state into the cpu_powered TCG state >> to avoid changing the serialisation format. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> v2 >> - make mpstate field runtime dependant (kvm_enabled()) >> - drop initial KVM_CAP_MP_STATE requirement >> - re-use cpu_powered instead of new field >> >> diff --git a/target-arm/machine.c b/target-arm/machine.c >> index 9446e5a..185f9a2 100644 >> --- a/target-arm/machine.c >> +++ b/target-arm/machine.c >> @@ -161,6 +161,7 @@ static const VMStateInfo vmstate_cpsr = { >> .put = put_cpsr, >> }; >> >> + >> static void cpu_pre_save(void *opaque) >> { >> ARMCPU *cpu = opaque; >> @@ -170,6 +171,20 @@ static void cpu_pre_save(void *opaque) >> /* This should never fail */ >> abort(); >> } >> +#if defined CONFIG_KVM >> + if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) { >> + struct kvm_mp_state mp_state; >> + int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state); >> + if (ret) { >> + fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n", >> + __func__, ret, strerror(ret)); >> + abort(); >> + } >> + cpu->powered_off = >> + (mp_state.mp_state == KVM_MP_STATE_RUNNABLE) >> + ? false : true; > > Ternary operator to produce a true-or-false result is a bit > redundant... > >> + } >> +#endif > > Why is this in pre-save/post-load rather than in the > kvm_arch_get/put_registers functions like all the other > syncing code? Yeah the #ifdefs should have waved the red flag - I'll move it ;-) > > -- PMM
diff --git a/target-arm/machine.c b/target-arm/machine.c index 9446e5a..185f9a2 100644 --- a/target-arm/machine.c +++ b/target-arm/machine.c @@ -161,6 +161,7 @@ static const VMStateInfo vmstate_cpsr = { .put = put_cpsr, }; + static void cpu_pre_save(void *opaque) { ARMCPU *cpu = opaque; @@ -170,6 +171,20 @@ static void cpu_pre_save(void *opaque) /* This should never fail */ abort(); } +#if defined CONFIG_KVM + if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) { + struct kvm_mp_state mp_state; + int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state); + if (ret) { + fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n", + __func__, ret, strerror(ret)); + abort(); + } + cpu->powered_off = + (mp_state.mp_state == KVM_MP_STATE_RUNNABLE) + ? false : true; + } +#endif } else { if (!write_cpustate_to_list(cpu)) { /* This should never fail. */ @@ -222,6 +237,20 @@ static int cpu_post_load(void *opaque, int version_id) * we're using it. */ write_list_to_cpustate(cpu); +#if defined CONFIG_KVM + if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) { + struct kvm_mp_state mp_state = { + .mp_state = + cpu->powered_off ? KVM_MP_STATE_HALTED : KVM_MP_STATE_RUNNABLE + }; + int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state); + if (ret) { + fprintf(stderr, "%s: failed to set MP_STATE %d/%s\n", + __func__, ret, strerror(ret)); + return -1; + } + } +#endif } else { if (!write_list_to_cpustate(cpu)) { return -1;
This adds the saving and restore of the current Multi-Processing state of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of potential states for x86 we only use two for ARM. Either the process is running or not. We then save this state into the cpu_powered TCG state to avoid changing the serialisation format. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- v2 - make mpstate field runtime dependant (kvm_enabled()) - drop initial KVM_CAP_MP_STATE requirement - re-use cpu_powered instead of new field