| Message ID | 4F609652.9070701@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 14, 2012 at 05:00:02PM +0400, Alexey Starikovskiy wrote: > Signed-off-by: Alexey Starikovskiy<aystarik@gmail.com> > --- > target-arm/cpu.h | 10 ++++------ > target-arm/helper.c | 14 +++++++------- > target-arm/machine.c | 16 ++++++---------- > 3 files changed, 17 insertions(+), 23 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 0d9b39c..0298a98 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -117,11 +117,9 @@ typedef struct CPUARMState { > uint32_t c1_coproc; /* Coprocessor access register. */ > uint32_t c1_xscaleauxcr; /* XScale auxiliary control register. */ > uint32_t c1_scr; /* secure config register. */ > - uint32_t c2_base0; /* MMU translation table base 0. */ > - uint32_t c2_base1; /* MMU translation table base 1. */ > + uint64_t c2_base0; /* MMU translation table base 0. */ > + uint64_t c2_base1; /* MMU translation table base 1. */ > uint32_t c2_control; /* MMU translation table base control. */ > - uint32_t c2_mask; /* MMU translation table base selection mask. */ > - uint32_t c2_base_mask; /* MMU translation table base 0 mask. */ > uint32_t c2_data; /* MPU data cachable bits. */ > uint32_t c2_insn; /* MPU instruction cachable bits. */ > uint32_t c3; /* MMU domain access control register > @@ -131,7 +129,7 @@ typedef struct CPUARMState { > uint32_t c6_region[8]; /* MPU base/size registers. */ > uint32_t c6_insn; /* Fault address registers. */ > uint32_t c6_data; > - uint32_t c7_par; /* Translation result. */ > + uint64_t c7_par; /* Translation result. */ > uint32_t c9_insn; /* Cache lockdown registers. */ > uint32_t c9_data; > uint32_t c9_pmcr; /* performance monitor control register */ > @@ -455,7 +453,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum, > #define cpu_signal_handler cpu_arm_signal_handler > #define cpu_list arm_cpu_list > > -#define CPU_SAVE_VERSION 6 > +#define CPU_SAVE_VERSION 7 > > /* MMU modes definitions */ > #define MMU_MODE0_SUFFIX _kernel > diff --git a/target-arm/helper.c b/target-arm/helper.c > index abe1c30..d190104 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -325,7 +325,6 @@ void cpu_reset(CPUARMState *env) > } > } > env->vfp.xregs[ARM_VFP_FPEXC] = 0; > - env->cp15.c2_base_mask = 0xffffc000u; > /* v7 performance monitor control register: same implementor > * field as main ID register, and we implement no event counters. > */ > @@ -1050,12 +1049,15 @@ static inline int check_ap(CPUState *env, int ap, int domain_prot, > static uint32_t get_level1_table_address(CPUState *env, uint32_t address) > { > uint32_t table; > + int t0size = env->cp15.c2_control& 0x7; > + uint32_t mask = ~(((uint32_t)0xffffffffu)>> t0size); > > - if (address& env->cp15.c2_mask) > + if (address& mask) { > table = env->cp15.c2_base1& 0xffffc000; > - else > - table = env->cp15.c2_base0& env->cp15.c2_base_mask; > - > + } else { > + mask = ~((uint32_t)0x3fffu>> t0size); > + table = env->cp15.c2_base0& mask; > + } > table |= (address>> 18)& 0x3ffc; > return table; > } > @@ -1531,8 +1533,6 @@ void HELPER(set_cp15)(CPUState *env, uint32_t insn, uint32_t val) > case 2: > val&= 7; > env->cp15.c2_control = val; > - env->cp15.c2_mask = ~(((uint32_t)0xffffffffu)>> val); > - env->cp15.c2_base_mask = ~((uint32_t)0x3fffu>> val); > break; > default: > goto bad_reg; > diff --git a/target-arm/machine.c b/target-arm/machine.c > index f66b8df..8fa738e 100644 > --- a/target-arm/machine.c > +++ b/target-arm/machine.c > @@ -27,11 +27,9 @@ void cpu_save(QEMUFile *f, void *opaque) > qemu_put_be32(f, env->cp15.c1_coproc); > qemu_put_be32(f, env->cp15.c1_xscaleauxcr); > qemu_put_be32(f, env->cp15.c1_scr); > - qemu_put_be32(f, env->cp15.c2_base0); > - qemu_put_be32(f, env->cp15.c2_base1); > + qemu_put_be64(f, env->cp15.c2_base0); > + qemu_put_be64(f, env->cp15.c2_base1); > qemu_put_be32(f, env->cp15.c2_control); > - qemu_put_be32(f, env->cp15.c2_mask); > - qemu_put_be32(f, env->cp15.c2_base_mask); > qemu_put_be32(f, env->cp15.c2_data); > qemu_put_be32(f, env->cp15.c2_insn); > qemu_put_be32(f, env->cp15.c3); > @@ -42,7 +40,7 @@ void cpu_save(QEMUFile *f, void *opaque) > } > qemu_put_be32(f, env->cp15.c6_insn); > qemu_put_be32(f, env->cp15.c6_data); > - qemu_put_be32(f, env->cp15.c7_par); > + qemu_put_be64(f, env->cp15.c7_par); > qemu_put_be32(f, env->cp15.c9_insn); > qemu_put_be32(f, env->cp15.c9_data); > qemu_put_be32(f, env->cp15.c9_pmcr); > @@ -145,11 +143,9 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id) > env->cp15.c1_coproc = qemu_get_be32(f); > env->cp15.c1_xscaleauxcr = qemu_get_be32(f); > env->cp15.c1_scr = qemu_get_be32(f); > - env->cp15.c2_base0 = qemu_get_be32(f); > - env->cp15.c2_base1 = qemu_get_be32(f); > + env->cp15.c2_base0 = qemu_get_be64(f); > + env->cp15.c2_base1 = qemu_get_be64(f); This breaks old -> new migration. Should CPU_SAVE_VERSION a do this conditionally on version_id >= CPU_SAVE_VERSION > env->cp15.c2_control = qemu_get_be32(f); > - env->cp15.c2_mask = qemu_get_be32(f); > - env->cp15.c2_base_mask = qemu_get_be32(f); > env->cp15.c2_data = qemu_get_be32(f); > env->cp15.c2_insn = qemu_get_be32(f); > env->cp15.c3 = qemu_get_be32(f); > @@ -160,7 +156,7 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id) > } > env->cp15.c6_insn = qemu_get_be32(f); > env->cp15.c6_data = qemu_get_be32(f); > - env->cp15.c7_par = qemu_get_be32(f); > + env->cp15.c7_par = qemu_get_be64(f); Here too. > env->cp15.c9_insn = qemu_get_be32(f); > env->cp15.c9_data = qemu_get_be32(f); > env->cp15.c9_pmcr = qemu_get_be32(f); > > >
On Wed, Mar 14, 2012 at 01:57:47PM -0500, Michael Roth wrote: > On Wed, Mar 14, 2012 at 05:00:02PM +0400, Alexey Starikovskiy wrote: > > Signed-off-by: Alexey Starikovskiy<aystarik@gmail.com> > > --- > > target-arm/cpu.h | 10 ++++------ > > target-arm/helper.c | 14 +++++++------- > > target-arm/machine.c | 16 ++++++---------- > > 3 files changed, 17 insertions(+), 23 deletions(-) > > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > > index 0d9b39c..0298a98 100644 > > --- a/target-arm/cpu.h > > +++ b/target-arm/cpu.h > > @@ -117,11 +117,9 @@ typedef struct CPUARMState { > > uint32_t c1_coproc; /* Coprocessor access register. */ > > uint32_t c1_xscaleauxcr; /* XScale auxiliary control register. */ > > uint32_t c1_scr; /* secure config register. */ > > - uint32_t c2_base0; /* MMU translation table base 0. */ > > - uint32_t c2_base1; /* MMU translation table base 1. */ > > + uint64_t c2_base0; /* MMU translation table base 0. */ > > + uint64_t c2_base1; /* MMU translation table base 1. */ > > uint32_t c2_control; /* MMU translation table base control. */ > > - uint32_t c2_mask; /* MMU translation table base selection mask. */ > > - uint32_t c2_base_mask; /* MMU translation table base 0 mask. */ > > uint32_t c2_data; /* MPU data cachable bits. */ > > uint32_t c2_insn; /* MPU instruction cachable bits. */ > > uint32_t c3; /* MMU domain access control register > > @@ -131,7 +129,7 @@ typedef struct CPUARMState { > > uint32_t c6_region[8]; /* MPU base/size registers. */ > > uint32_t c6_insn; /* Fault address registers. */ > > uint32_t c6_data; > > - uint32_t c7_par; /* Translation result. */ > > + uint64_t c7_par; /* Translation result. */ > > uint32_t c9_insn; /* Cache lockdown registers. */ > > uint32_t c9_data; > > uint32_t c9_pmcr; /* performance monitor control register */ > > @@ -455,7 +453,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum, > > #define cpu_signal_handler cpu_arm_signal_handler > > #define cpu_list arm_cpu_list > > > > -#define CPU_SAVE_VERSION 6 > > +#define CPU_SAVE_VERSION 7 > > > > /* MMU modes definitions */ > > #define MMU_MODE0_SUFFIX _kernel > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index abe1c30..d190104 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -325,7 +325,6 @@ void cpu_reset(CPUARMState *env) > > } > > } > > env->vfp.xregs[ARM_VFP_FPEXC] = 0; > > - env->cp15.c2_base_mask = 0xffffc000u; > > /* v7 performance monitor control register: same implementor > > * field as main ID register, and we implement no event counters. > > */ > > @@ -1050,12 +1049,15 @@ static inline int check_ap(CPUState *env, int ap, int domain_prot, > > static uint32_t get_level1_table_address(CPUState *env, uint32_t address) > > { > > uint32_t table; > > + int t0size = env->cp15.c2_control& 0x7; > > + uint32_t mask = ~(((uint32_t)0xffffffffu)>> t0size); > > > > - if (address& env->cp15.c2_mask) > > + if (address& mask) { > > table = env->cp15.c2_base1& 0xffffc000; > > - else > > - table = env->cp15.c2_base0& env->cp15.c2_base_mask; > > - > > + } else { > > + mask = ~((uint32_t)0x3fffu>> t0size); > > + table = env->cp15.c2_base0& mask; > > + } > > table |= (address>> 18)& 0x3ffc; > > return table; > > } > > @@ -1531,8 +1533,6 @@ void HELPER(set_cp15)(CPUState *env, uint32_t insn, uint32_t val) > > case 2: > > val&= 7; > > env->cp15.c2_control = val; > > - env->cp15.c2_mask = ~(((uint32_t)0xffffffffu)>> val); > > - env->cp15.c2_base_mask = ~((uint32_t)0x3fffu>> val); > > break; > > default: > > goto bad_reg; > > diff --git a/target-arm/machine.c b/target-arm/machine.c > > index f66b8df..8fa738e 100644 > > --- a/target-arm/machine.c > > +++ b/target-arm/machine.c > > @@ -27,11 +27,9 @@ void cpu_save(QEMUFile *f, void *opaque) > > qemu_put_be32(f, env->cp15.c1_coproc); > > qemu_put_be32(f, env->cp15.c1_xscaleauxcr); > > qemu_put_be32(f, env->cp15.c1_scr); > > - qemu_put_be32(f, env->cp15.c2_base0); > > - qemu_put_be32(f, env->cp15.c2_base1); > > + qemu_put_be64(f, env->cp15.c2_base0); > > + qemu_put_be64(f, env->cp15.c2_base1); > > qemu_put_be32(f, env->cp15.c2_control); > > - qemu_put_be32(f, env->cp15.c2_mask); > > - qemu_put_be32(f, env->cp15.c2_base_mask); > > qemu_put_be32(f, env->cp15.c2_data); > > qemu_put_be32(f, env->cp15.c2_insn); > > qemu_put_be32(f, env->cp15.c3); > > @@ -42,7 +40,7 @@ void cpu_save(QEMUFile *f, void *opaque) > > } > > qemu_put_be32(f, env->cp15.c6_insn); > > qemu_put_be32(f, env->cp15.c6_data); > > - qemu_put_be32(f, env->cp15.c7_par); > > + qemu_put_be64(f, env->cp15.c7_par); > > qemu_put_be32(f, env->cp15.c9_insn); > > qemu_put_be32(f, env->cp15.c9_data); > > qemu_put_be32(f, env->cp15.c9_pmcr); > > @@ -145,11 +143,9 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id) > > env->cp15.c1_coproc = qemu_get_be32(f); > > env->cp15.c1_xscaleauxcr = qemu_get_be32(f); > > env->cp15.c1_scr = qemu_get_be32(f); > > - env->cp15.c2_base0 = qemu_get_be32(f); > > - env->cp15.c2_base1 = qemu_get_be32(f); > > + env->cp15.c2_base0 = qemu_get_be64(f); > > + env->cp15.c2_base1 = qemu_get_be64(f); > > This breaks old -> new migration. Should CPU_SAVE_VERSION a do this > conditionally on version_id >= CPU_SAVE_VERSION Heh, let's try that again. Should *increment* CPU_SAVE_VERSION *and* Also, historically I guess arm just bails on old->new but that seems wrong. Either we need to increment though. > > > env->cp15.c2_control = qemu_get_be32(f); > > - env->cp15.c2_mask = qemu_get_be32(f); > > - env->cp15.c2_base_mask = qemu_get_be32(f); > > env->cp15.c2_data = qemu_get_be32(f); > > env->cp15.c2_insn = qemu_get_be32(f); > > env->cp15.c3 = qemu_get_be32(f); > > @@ -160,7 +156,7 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id) > > } > > env->cp15.c6_insn = qemu_get_be32(f); > > env->cp15.c6_data = qemu_get_be32(f); > > - env->cp15.c7_par = qemu_get_be32(f); > > + env->cp15.c7_par = qemu_get_be64(f); > > Here too. > > > env->cp15.c9_insn = qemu_get_be32(f); > > env->cp15.c9_data = qemu_get_be32(f); > > env->cp15.c9_pmcr = qemu_get_be32(f); > > > > > >
Do I need to do anything beside following or not? Thanks, Alex. On Wed, Mar 14, 2012 at 11:06 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote: >> > -#define CPU_SAVE_VERSION 6 >> > +#define CPU_SAVE_VERSION 7
On Wed, Mar 14, 2012 at 10:09:09PM +0300, Alexey Starikovskiy wrote: > Do I need to do anything beside following or not? Not sure, ARM folks? My suggestion would be to lose the the catch-all -EINVAL error we throw in machine.c:cpu_load() when version_id != CPU_SAVE_VERSION and only conditionally load 64-bit registers if the source's version_id >= CPU_SAVE_VERSION. But we've never done that in the past, and we broke old->new as recently as 2 months ago so I'm not sure it's worth it since there hasn't been a release since then. > > Thanks, > Alex. > > On Wed, Mar 14, 2012 at 11:06 PM, Michael Roth > <mdroth@linux.vnet.ibm.com> wrote: > >> > -#define CPU_SAVE_VERSION 6 > >> > +#define CPU_SAVE_VERSION 7 >
On 14 March 2012 19:38, Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > On Wed, Mar 14, 2012 at 10:09:09PM +0300, Alexey Starikovskiy wrote: >> Do I need to do anything beside following or not? > > Not sure, ARM folks? > > My suggestion would be to lose the the catch-all -EINVAL error we > throw in machine.c:cpu_load() when version_id != CPU_SAVE_VERSION > and only conditionally load 64-bit registers if the source's > version_id >= CPU_SAVE_VERSION. But we've never done that in the > past, and we broke old->new as recently as 2 months ago so I'm not > sure it's worth it since there hasn't been a release since then. My position at the moment is that old->new migration on ARM is not supported and further that we make no attempt to avoid version bumps. At some point as KVM-on-ARM gets towards being complete we'll have to start worrying about compatibility, but at the moment I really don't think that either (a) all the devices have tested and working migration state or (b) we have a CPUState that's actually in good enough shape[*] that we can reasonably start to insist on not breaking cross-version migration. So migration-wise I have no problem with the patch as it stands. (I have other issues with it I suspect but haven't got to reviewing it yet.) [*] as an example of the kind of problem I'd like to see a solution for, there's no reason that adding support for a new feature like LPAE should require the migration state for non-LPAE CPUs to change, but as things stand it does. -- PMM
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 0d9b39c..0298a98 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -117,11 +117,9 @@ typedef struct CPUARMState { uint32_t c1_coproc; /* Coprocessor access register. */ uint32_t c1_xscaleauxcr; /* XScale auxiliary control register. */ uint32_t c1_scr; /* secure config register. */ - uint32_t c2_base0; /* MMU translation table base 0. */ - uint32_t c2_base1; /* MMU translation table base 1. */ + uint64_t c2_base0; /* MMU translation table base 0. */ + uint64_t c2_base1; /* MMU translation table base 1. */ uint32_t c2_control; /* MMU translation table base control. */ - uint32_t c2_mask; /* MMU translation table base selection mask. */ - uint32_t c2_base_mask; /* MMU translation table base 0 mask. */ uint32_t c2_data; /* MPU data cachable bits. */ uint32_t c2_insn; /* MPU instruction cachable bits. */ uint32_t c3; /* MMU domain access control register @@ -131,7 +129,7 @@ typedef struct CPUARMState { uint32_t c6_region[8]; /* MPU base/size registers. */ uint32_t c6_insn; /* Fault address registers. */ uint32_t c6_data; - uint32_t c7_par; /* Translation result. */ + uint64_t c7_par; /* Translation result. */ uint32_t c9_insn; /* Cache lockdown registers. */ uint32_t c9_data; uint32_t c9_pmcr; /* performance monitor control register */ @@ -455,7 +453,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum, #define cpu_signal_handler cpu_arm_signal_handler #define cpu_list arm_cpu_list -#define CPU_SAVE_VERSION 6 +#define CPU_SAVE_VERSION 7 /* MMU modes definitions */ #define MMU_MODE0_SUFFIX _kernel diff --git a/target-arm/helper.c b/target-arm/helper.c index abe1c30..d190104 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -325,7 +325,6 @@ void cpu_reset(CPUARMState *env) } } env->vfp.xregs[ARM_VFP_FPEXC] = 0; - env->cp15.c2_base_mask = 0xffffc000u; /* v7 performance monitor control register: same implementor * field as main ID register, and we implement no event counters. */ @@ -1050,12 +1049,15 @@ static inline int check_ap(CPUState *env, int ap, int domain_prot, static uint32_t get_level1_table_address(CPUState *env, uint32_t address) { uint32_t table; + int t0size = env->cp15.c2_control& 0x7; + uint32_t mask = ~(((uint32_t)0xffffffffu)>> t0size); - if (address& env->cp15.c2_mask) + if (address& mask) { table = env->cp15.c2_base1& 0xffffc000; - else - table = env->cp15.c2_base0& env->cp15.c2_base_mask; - + } else { + mask = ~((uint32_t)0x3fffu>> t0size); + table = env->cp15.c2_base0& mask; + } table |= (address>> 18)& 0x3ffc; return table; } @@ -1531,8 +1533,6 @@ void HELPER(set_cp15)(CPUState *env, uint32_t insn, uint32_t val) case 2: val&= 7; env->cp15.c2_control = val; - env->cp15.c2_mask = ~(((uint32_t)0xffffffffu)>> val); - env->cp15.c2_base_mask = ~((uint32_t)0x3fffu>> val); break; default: goto bad_reg; diff --git a/target-arm/machine.c b/target-arm/machine.c index f66b8df..8fa738e 100644 --- a/target-arm/machine.c +++ b/target-arm/machine.c @@ -27,11 +27,9 @@ void cpu_save(QEMUFile *f, void *opaque) qemu_put_be32(f, env->cp15.c1_coproc); qemu_put_be32(f, env->cp15.c1_xscaleauxcr); qemu_put_be32(f, env->cp15.c1_scr); - qemu_put_be32(f, env->cp15.c2_base0); - qemu_put_be32(f, env->cp15.c2_base1); + qemu_put_be64(f, env->cp15.c2_base0); + qemu_put_be64(f, env->cp15.c2_base1); qemu_put_be32(f, env->cp15.c2_control); - qemu_put_be32(f, env->cp15.c2_mask); - qemu_put_be32(f, env->cp15.c2_base_mask); qemu_put_be32(f, env->cp15.c2_data); qemu_put_be32(f, env->cp15.c2_insn); qemu_put_be32(f, env->cp15.c3); @@ -42,7 +40,7 @@ void cpu_save(QEMUFile *f, void *opaque) } qemu_put_be32(f, env->cp15.c6_insn); qemu_put_be32(f, env->cp15.c6_data); - qemu_put_be32(f, env->cp15.c7_par); + qemu_put_be64(f, env->cp15.c7_par); qemu_put_be32(f, env->cp15.c9_insn); qemu_put_be32(f, env->cp15.c9_data); qemu_put_be32(f, env->cp15.c9_pmcr); @@ -145,11 +143,9 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id) env->cp15.c1_coproc = qemu_get_be32(f); env->cp15.c1_xscaleauxcr = qemu_get_be32(f); env->cp15.c1_scr = qemu_get_be32(f); - env->cp15.c2_base0 = qemu_get_be32(f); - env->cp15.c2_base1 = qemu_get_be32(f); + env->cp15.c2_base0 = qemu_get_be64(f); + env->cp15.c2_base1 = qemu_get_be64(f); env->cp15.c2_control = qemu_get_be32(f); - env->cp15.c2_mask = qemu_get_be32(f); - env->cp15.c2_base_mask = qemu_get_be32(f); env->cp15.c2_data = qemu_get_be32(f); env->cp15.c2_insn = qemu_get_be32(f); env->cp15.c3 = qemu_get_be32(f); @@ -160,7 +156,7 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id) } env->cp15.c6_insn = qemu_get_be32(f); env->cp15.c6_data = qemu_get_be32(f); - env->cp15.c7_par = qemu_get_be32(f); + env->cp15.c7_par = qemu_get_be64(f); env->cp15.c9_insn = qemu_get_be32(f); env->cp15.c9_data = qemu_get_be32(f); env->cp15.c9_pmcr = qemu_get_be32(f);
Signed-off-by: Alexey Starikovskiy<aystarik@gmail.com> --- target-arm/cpu.h | 10 ++++------ target-arm/helper.c | 14 +++++++------- target-arm/machine.c | 16 ++++++---------- 3 files changed, 17 insertions(+), 23 deletions(-)