| Message ID | 1432569199-24306-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 25 May 2015 at 16:53, Peter Maydell <peter.maydell@linaro.org> wrote: > From: Juan Quintela <quintela@redhat.com> > > Update the CRIS CPU state save/load to use a VMStateDescription struct > rather than cpu_save/cpu_load functions. > > Have to define TLBSet struct. > Multidimensional arrays in C are a mess, just unroll them. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > [PMM: > * expand commit message a little since it's no longer one patch in > a 35-patch series > * add header/copyright comment to machine.c; credited copyright is > Red Hat and author is Juan, since this commit gives the file all-new > contents; license is LGPL-2-or-later, to match other target-cris code > * remove hardcoded tab > * add fields for locked_irq, interrupt_vector, fault_vector, trap_vector > * drop minimum_version_id_old fields > * bump version_id to 2 as we are not compatible with old state format > * remove unnecessary hw/boards.h include] > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This is a patch of Juan's from way back in 2012, which I am resurrecting > because we now have only two CPUs which still use old-style non-VMState > save/load (CRIS and SPARC). If we can update them both we can drop the > machinery in the common code which supports this. > > Notes: > * CPUCRISState indent style is somewhat mismatched (cf load_info); > I took the "minimal make checkpatch happy" approach, but am > happy to do something else with the line changed here > * I have added a copyright header to target-cris/machine.c, because it > did not have one at all, and this commit effectively gives the > file all-new contents. I have set it up as LGPLv2-or-later, > copyright Red Hat, author Juan. Please let me know if you would > prefer something else! > * I added vmstate entries for four fields which did not previously > get saved and restored, which is presumably a bug fix... > * vmsave/vmload on the axis-dev88 board does not currently seem to > work (among other obvious problems, there is no vmstate support > in the interrupt controller), so we're limited to "looks good > on code review" here. Oops, this has a couple of issues I only noticed when I started looking at the SPARC vmstate: * forgot to register vmstate by setting cc->vmsd * vmstate should be of CRISCPU, not CPUCRISState This is why it's a shame the board doesn't have vmsave/load support for testing. Will send out a v2. -- PMM
On 25 May 2015 at 17:35, Peter Maydell <peter.maydell@linaro.org> wrote: > On 25 May 2015 at 16:53, Peter Maydell <peter.maydell@linaro.org> wrote: >> From: Juan Quintela <quintela@redhat.com> >> >> Update the CRIS CPU state save/load to use a VMStateDescription struct >> rather than cpu_save/cpu_load functions. > Oops, this has a couple of issues I only noticed when I started > looking at the SPARC vmstate: > * forgot to register vmstate by setting cc->vmsd > * vmstate should be of CRISCPU, not CPUCRISState In looking at this I found that we currently have: CPUs that set cc->vmsd: arm, i386, lm32, mips, moxie, ppc, s390x CPUs that set dc->vmsd: alpha, m68k, microblaze, openrisc, sh4, unicore32, xtensa ...an exactly even split. Which of these is the recommended approach for new conversions? CCing Andreas since this is a QOM CPU question... thanks -- PMM
On Mon, May 25, 2015 at 11:30 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 25 May 2015 at 17:35, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 25 May 2015 at 16:53, Peter Maydell <peter.maydell@linaro.org> wrote: >>> From: Juan Quintela <quintela@redhat.com> >>> >>> Update the CRIS CPU state save/load to use a VMStateDescription struct >>> rather than cpu_save/cpu_load functions. > >> Oops, this has a couple of issues I only noticed when I started >> looking at the SPARC vmstate: >> * forgot to register vmstate by setting cc->vmsd >> * vmstate should be of CRISCPU, not CPUCRISState > > In looking at this I found that we currently have: > CPUs that set cc->vmsd: arm, i386, lm32, mips, moxie, ppc, s390x > CPUs that set dc->vmsd: alpha, m68k, microblaze, openrisc, sh4, > unicore32, xtensa > ...an exactly even split. > > Which of these is the recommended approach for new conversions? Should it be DC? CPU should not have specific support for something that already works for TYPE_DEVICE. This seems similar to SysBusDevice::init where we progressively pushed everything up to the higher level over time. Regards, Peter > CCing Andreas since this is a QOM CPU question... > > thanks > -- PMM >
On 25 May 2015 at 19:46, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Mon, May 25, 2015 at 11:30 AM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> In looking at this I found that we currently have: >> CPUs that set cc->vmsd: arm, i386, lm32, mips, moxie, ppc, s390x >> CPUs that set dc->vmsd: alpha, m68k, microblaze, openrisc, sh4, >> unicore32, xtensa >> ...an exactly even split. >> >> Which of these is the recommended approach for new conversions? > > Should it be DC? CPU should not have specific support for something > that already works for TYPE_DEVICE. This seems similar to > SysBusDevice::init where we progressively pushed everything up to the > higher level over time. I'm tempted to agree with that, except that all of the CPU families I'd trust as "major, actively maintained" are in the "set cc->vmsd" camp... -- PMM
On Mon, May 25, 2015 at 04:53:19PM +0100, Peter Maydell wrote: > From: Juan Quintela <quintela@redhat.com> > > Update the CRIS CPU state save/load to use a VMStateDescription struct > rather than cpu_save/cpu_load functions. > > Have to define TLBSet struct. > Multidimensional arrays in C are a mess, just unroll them. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > [PMM: > * expand commit message a little since it's no longer one patch in > a 35-patch series > * add header/copyright comment to machine.c; credited copyright is > Red Hat and author is Juan, since this commit gives the file all-new > contents; license is LGPL-2-or-later, to match other target-cris code > * remove hardcoded tab > * add fields for locked_irq, interrupt_vector, fault_vector, trap_vector > * drop minimum_version_id_old fields > * bump version_id to 2 as we are not compatible with old state format > * remove unnecessary hw/boards.h include] > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This is a patch of Juan's from way back in 2012, which I am resurrecting > because we now have only two CPUs which still use old-style non-VMState > save/load (CRIS and SPARC). If we can update them both we can drop the > machinery in the common code which supports this. > > Notes: > * CPUCRISState indent style is somewhat mismatched (cf load_info); > I took the "minimal make checkpatch happy" approach, but am > happy to do something else with the line changed here > * I have added a copyright header to target-cris/machine.c, because it > did not have one at all, and this commit effectively gives the > file all-new contents. I have set it up as LGPLv2-or-later, > copyright Red Hat, author Juan. Please let me know if you would > prefer something else! > * I added vmstate entries for four fields which did not previously > get saved and restored, which is presumably a bug fix... > * vmsave/vmload on the axis-dev88 board does not currently seem to > work (among other obvious problems, there is no vmstate support > in the interrupt controller), so we're limited to "looks good > on code review" here. That's all OK I think. We've never used the save/restore/migration on CRIS so you won't be breaking anything for us... Cheers, Edgar > > target-cris/cpu.h | 13 ++-- > target-cris/machine.c | 160 +++++++++++++++++++++++--------------------------- > 2 files changed, 81 insertions(+), 92 deletions(-) > > diff --git a/target-cris/cpu.h b/target-cris/cpu.h > index 677b38c..826af97 100644 > --- a/target-cris/cpu.h > +++ b/target-cris/cpu.h > @@ -108,6 +108,11 @@ > > #define NB_MMU_MODES 2 > > +typedef struct { > + uint32_t hi; > + uint32_t lo; > +} TLBSet; > + > typedef struct CPUCRISState { > uint32_t regs[16]; > /* P0 - P15 are referred to as special registers in the docs. */ > @@ -161,11 +166,7 @@ typedef struct CPUCRISState { > * > * One for I and another for D. > */ > - struct > - { > - uint32_t hi; > - uint32_t lo; > - } tlbsets[2][4][16]; > + TLBSet tlbsets[2][4][16]; > > CPU_COMMON > > @@ -227,8 +228,6 @@ enum { > #define cpu_gen_code cpu_cris_gen_code > #define cpu_signal_handler cpu_cris_signal_handler > > -#define CPU_SAVE_VERSION 1 > - > /* MMU modes definitions */ > #define MMU_MODE0_SUFFIX _kernel > #define MMU_MODE1_SUFFIX _user > diff --git a/target-cris/machine.c b/target-cris/machine.c > index 8f9c0dd..8e65327 100644 > --- a/target-cris/machine.c > +++ b/target-cris/machine.c > @@ -1,90 +1,80 @@ > -#include "hw/hw.h" > -#include "hw/boards.h" > - > -void cpu_save(QEMUFile *f, void *opaque) > -{ > - CPUCRISState *env = opaque; > - int i; > - int s; > - int mmu; > - > - for (i = 0; i < 16; i++) > - qemu_put_be32(f, env->regs[i]); > - for (i = 0; i < 16; i++) > - qemu_put_be32(f, env->pregs[i]); > - > - qemu_put_be32(f, env->pc); > - qemu_put_be32(f, env->ksp); > - > - qemu_put_be32(f, env->dslot); > - qemu_put_be32(f, env->btaken); > - qemu_put_be32(f, env->btarget); > - > - qemu_put_be32(f, env->cc_op); > - qemu_put_be32(f, env->cc_mask); > - qemu_put_be32(f, env->cc_dest); > - qemu_put_be32(f, env->cc_src); > - qemu_put_be32(f, env->cc_result); > - qemu_put_be32(f, env->cc_size); > - qemu_put_be32(f, env->cc_x); > - > - for (s = 0; s < 4; s++) { > - for (i = 0; i < 16; i++) > - qemu_put_be32(f, env->sregs[s][i]); > - } > +/* > + * CRIS virtual CPU state save/load support > + * > + * Copyright (c) 2012 Red Hat, Inc. > + * Written by Juan Quintela <quintela@redhat.com> > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, see <http://www.gnu.org/licenses/>. > + */ > > - qemu_put_be32(f, env->mmu_rand_lfsr); > - for (mmu = 0; mmu < 2; mmu++) { > - for (s = 0; s < 4; s++) { > - for (i = 0; i < 16; i++) { > - qemu_put_be32(f, env->tlbsets[mmu][s][i].lo); > - qemu_put_be32(f, env->tlbsets[mmu][s][i].hi); > - } > - } > - } > -} > - > -int cpu_load(QEMUFile *f, void *opaque, int version_id) > -{ > - CPUCRISState *env = opaque; > - int i; > - int s; > - int mmu; > - > - for (i = 0; i < 16; i++) > - env->regs[i] = qemu_get_be32(f); > - for (i = 0; i < 16; i++) > - env->pregs[i] = qemu_get_be32(f); > - > - env->pc = qemu_get_be32(f); > - env->ksp = qemu_get_be32(f); > - > - env->dslot = qemu_get_be32(f); > - env->btaken = qemu_get_be32(f); > - env->btarget = qemu_get_be32(f); > - > - env->cc_op = qemu_get_be32(f); > - env->cc_mask = qemu_get_be32(f); > - env->cc_dest = qemu_get_be32(f); > - env->cc_src = qemu_get_be32(f); > - env->cc_result = qemu_get_be32(f); > - env->cc_size = qemu_get_be32(f); > - env->cc_x = qemu_get_be32(f); > +#include "hw/hw.h" > > - for (s = 0; s < 4; s++) { > - for (i = 0; i < 16; i++) > - env->sregs[s][i] = qemu_get_be32(f); > +static const VMStateDescription vmstate_tlbset = { > + .name = "cpu/tlbset", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(lo, TLBSet), > + VMSTATE_UINT32(hi, TLBSet), > + VMSTATE_END_OF_LIST() > } > +}; > > - env->mmu_rand_lfsr = qemu_get_be32(f); > - for (mmu = 0; mmu < 2; mmu++) { > - for (s = 0; s < 4; s++) { > - for (i = 0; i < 16; i++) { > - env->tlbsets[mmu][s][i].lo = qemu_get_be32(f); > - env->tlbsets[mmu][s][i].hi = qemu_get_be32(f); > - } > - } > +const VMStateDescription vmstate_cpu = { > + .name = "cpu", > + .version_id = 2, > + .minimum_version_id = 2, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32_ARRAY(regs, CPUCRISState, 16), > + VMSTATE_UINT32_ARRAY(pregs, CPUCRISState, 16), > + VMSTATE_UINT32(pc, CPUCRISState), > + VMSTATE_UINT32(ksp, CPUCRISState), > + VMSTATE_INT32(dslot, CPUCRISState), > + VMSTATE_INT32(btaken, CPUCRISState), > + VMSTATE_UINT32(btarget, CPUCRISState), > + VMSTATE_UINT32(cc_op, CPUCRISState), > + VMSTATE_UINT32(cc_mask, CPUCRISState), > + VMSTATE_UINT32(cc_dest, CPUCRISState), > + VMSTATE_UINT32(cc_src, CPUCRISState), > + VMSTATE_UINT32(cc_result, CPUCRISState), > + VMSTATE_INT32(cc_size, CPUCRISState), > + VMSTATE_INT32(cc_x, CPUCRISState), > + VMSTATE_INT32(locked_irq, CPUCRISState), > + VMSTATE_INT32(interrupt_vector, CPUCRISState), > + VMSTATE_INT32(fault_vector, CPUCRISState), > + VMSTATE_INT32(trap_vector, CPUCRISState), > + VMSTATE_UINT32_ARRAY(sregs[0], CPUCRISState, 16), > + VMSTATE_UINT32_ARRAY(sregs[1], CPUCRISState, 16), > + VMSTATE_UINT32_ARRAY(sregs[2], CPUCRISState, 16), > + VMSTATE_UINT32_ARRAY(sregs[3], CPUCRISState, 16), > + VMSTATE_UINT32(mmu_rand_lfsr, CPUCRISState), > + VMSTATE_STRUCT_ARRAY(tlbsets[0][0], CPUCRISState, 16, 0, > + vmstate_tlbset, TLBSet), > + VMSTATE_STRUCT_ARRAY(tlbsets[0][1], CPUCRISState, 16, 0, > + vmstate_tlbset, TLBSet), > + VMSTATE_STRUCT_ARRAY(tlbsets[0][2], CPUCRISState, 16, 0, > + vmstate_tlbset, TLBSet), > + VMSTATE_STRUCT_ARRAY(tlbsets[0][3], CPUCRISState, 16, 0, > + vmstate_tlbset, TLBSet), > + VMSTATE_STRUCT_ARRAY(tlbsets[1][0], CPUCRISState, 16, 0, > + vmstate_tlbset, TLBSet), > + VMSTATE_STRUCT_ARRAY(tlbsets[1][1], CPUCRISState, 16, 0, > + vmstate_tlbset, TLBSet), > + VMSTATE_STRUCT_ARRAY(tlbsets[1][2], CPUCRISState, 16, 0, > + vmstate_tlbset, TLBSet), > + VMSTATE_STRUCT_ARRAY(tlbsets[1][3], CPUCRISState, 16, 0, > + vmstate_tlbset, TLBSet), > + VMSTATE_END_OF_LIST() > } > - > - return 0; > -} > +}; > -- > 2.2.1 >
On 26 May 2015 at 00:58, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > That's all OK I think. We've never used the save/restore/migration on > CRIS so you won't be breaking anything for us... I would actually recommend fixing it in the board/device models just for debugging purposes. It's really handy to be able to snapshot a system just before it fails, so that your reproduce case for a bug is "run image from snapshot for half a second" rather than "wait a minute or more for an OS to boot and then run some command". -- PMM
diff --git a/target-cris/cpu.h b/target-cris/cpu.h index 677b38c..826af97 100644 --- a/target-cris/cpu.h +++ b/target-cris/cpu.h @@ -108,6 +108,11 @@ #define NB_MMU_MODES 2 +typedef struct { + uint32_t hi; + uint32_t lo; +} TLBSet; + typedef struct CPUCRISState { uint32_t regs[16]; /* P0 - P15 are referred to as special registers in the docs. */ @@ -161,11 +166,7 @@ typedef struct CPUCRISState { * * One for I and another for D. */ - struct - { - uint32_t hi; - uint32_t lo; - } tlbsets[2][4][16]; + TLBSet tlbsets[2][4][16]; CPU_COMMON @@ -227,8 +228,6 @@ enum { #define cpu_gen_code cpu_cris_gen_code #define cpu_signal_handler cpu_cris_signal_handler -#define CPU_SAVE_VERSION 1 - /* MMU modes definitions */ #define MMU_MODE0_SUFFIX _kernel #define MMU_MODE1_SUFFIX _user diff --git a/target-cris/machine.c b/target-cris/machine.c index 8f9c0dd..8e65327 100644 --- a/target-cris/machine.c +++ b/target-cris/machine.c @@ -1,90 +1,80 @@ -#include "hw/hw.h" -#include "hw/boards.h" - -void cpu_save(QEMUFile *f, void *opaque) -{ - CPUCRISState *env = opaque; - int i; - int s; - int mmu; - - for (i = 0; i < 16; i++) - qemu_put_be32(f, env->regs[i]); - for (i = 0; i < 16; i++) - qemu_put_be32(f, env->pregs[i]); - - qemu_put_be32(f, env->pc); - qemu_put_be32(f, env->ksp); - - qemu_put_be32(f, env->dslot); - qemu_put_be32(f, env->btaken); - qemu_put_be32(f, env->btarget); - - qemu_put_be32(f, env->cc_op); - qemu_put_be32(f, env->cc_mask); - qemu_put_be32(f, env->cc_dest); - qemu_put_be32(f, env->cc_src); - qemu_put_be32(f, env->cc_result); - qemu_put_be32(f, env->cc_size); - qemu_put_be32(f, env->cc_x); - - for (s = 0; s < 4; s++) { - for (i = 0; i < 16; i++) - qemu_put_be32(f, env->sregs[s][i]); - } +/* + * CRIS virtual CPU state save/load support + * + * Copyright (c) 2012 Red Hat, Inc. + * Written by Juan Quintela <quintela@redhat.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see <http://www.gnu.org/licenses/>. + */ - qemu_put_be32(f, env->mmu_rand_lfsr); - for (mmu = 0; mmu < 2; mmu++) { - for (s = 0; s < 4; s++) { - for (i = 0; i < 16; i++) { - qemu_put_be32(f, env->tlbsets[mmu][s][i].lo); - qemu_put_be32(f, env->tlbsets[mmu][s][i].hi); - } - } - } -} - -int cpu_load(QEMUFile *f, void *opaque, int version_id) -{ - CPUCRISState *env = opaque; - int i; - int s; - int mmu; - - for (i = 0; i < 16; i++) - env->regs[i] = qemu_get_be32(f); - for (i = 0; i < 16; i++) - env->pregs[i] = qemu_get_be32(f); - - env->pc = qemu_get_be32(f); - env->ksp = qemu_get_be32(f); - - env->dslot = qemu_get_be32(f); - env->btaken = qemu_get_be32(f); - env->btarget = qemu_get_be32(f); - - env->cc_op = qemu_get_be32(f); - env->cc_mask = qemu_get_be32(f); - env->cc_dest = qemu_get_be32(f); - env->cc_src = qemu_get_be32(f); - env->cc_result = qemu_get_be32(f); - env->cc_size = qemu_get_be32(f); - env->cc_x = qemu_get_be32(f); +#include "hw/hw.h" - for (s = 0; s < 4; s++) { - for (i = 0; i < 16; i++) - env->sregs[s][i] = qemu_get_be32(f); +static const VMStateDescription vmstate_tlbset = { + .name = "cpu/tlbset", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32(lo, TLBSet), + VMSTATE_UINT32(hi, TLBSet), + VMSTATE_END_OF_LIST() } +}; - env->mmu_rand_lfsr = qemu_get_be32(f); - for (mmu = 0; mmu < 2; mmu++) { - for (s = 0; s < 4; s++) { - for (i = 0; i < 16; i++) { - env->tlbsets[mmu][s][i].lo = qemu_get_be32(f); - env->tlbsets[mmu][s][i].hi = qemu_get_be32(f); - } - } +const VMStateDescription vmstate_cpu = { + .name = "cpu", + .version_id = 2, + .minimum_version_id = 2, + .fields = (VMStateField[]) { + VMSTATE_UINT32_ARRAY(regs, CPUCRISState, 16), + VMSTATE_UINT32_ARRAY(pregs, CPUCRISState, 16), + VMSTATE_UINT32(pc, CPUCRISState), + VMSTATE_UINT32(ksp, CPUCRISState), + VMSTATE_INT32(dslot, CPUCRISState), + VMSTATE_INT32(btaken, CPUCRISState), + VMSTATE_UINT32(btarget, CPUCRISState), + VMSTATE_UINT32(cc_op, CPUCRISState), + VMSTATE_UINT32(cc_mask, CPUCRISState), + VMSTATE_UINT32(cc_dest, CPUCRISState), + VMSTATE_UINT32(cc_src, CPUCRISState), + VMSTATE_UINT32(cc_result, CPUCRISState), + VMSTATE_INT32(cc_size, CPUCRISState), + VMSTATE_INT32(cc_x, CPUCRISState), + VMSTATE_INT32(locked_irq, CPUCRISState), + VMSTATE_INT32(interrupt_vector, CPUCRISState), + VMSTATE_INT32(fault_vector, CPUCRISState), + VMSTATE_INT32(trap_vector, CPUCRISState), + VMSTATE_UINT32_ARRAY(sregs[0], CPUCRISState, 16), + VMSTATE_UINT32_ARRAY(sregs[1], CPUCRISState, 16), + VMSTATE_UINT32_ARRAY(sregs[2], CPUCRISState, 16), + VMSTATE_UINT32_ARRAY(sregs[3], CPUCRISState, 16), + VMSTATE_UINT32(mmu_rand_lfsr, CPUCRISState), + VMSTATE_STRUCT_ARRAY(tlbsets[0][0], CPUCRISState, 16, 0, + vmstate_tlbset, TLBSet), + VMSTATE_STRUCT_ARRAY(tlbsets[0][1], CPUCRISState, 16, 0, + vmstate_tlbset, TLBSet), + VMSTATE_STRUCT_ARRAY(tlbsets[0][2], CPUCRISState, 16, 0, + vmstate_tlbset, TLBSet), + VMSTATE_STRUCT_ARRAY(tlbsets[0][3], CPUCRISState, 16, 0, + vmstate_tlbset, TLBSet), + VMSTATE_STRUCT_ARRAY(tlbsets[1][0], CPUCRISState, 16, 0, + vmstate_tlbset, TLBSet), + VMSTATE_STRUCT_ARRAY(tlbsets[1][1], CPUCRISState, 16, 0, + vmstate_tlbset, TLBSet), + VMSTATE_STRUCT_ARRAY(tlbsets[1][2], CPUCRISState, 16, 0, + vmstate_tlbset, TLBSet), + VMSTATE_STRUCT_ARRAY(tlbsets[1][3], CPUCRISState, 16, 0, + vmstate_tlbset, TLBSet), + VMSTATE_END_OF_LIST() } - - return 0; -} +};