| Message ID | 1386060535-15908-6-git-send-email-s.fedorov@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote: > From: Svetlana Fedoseeva <s.fedoseeva@samsung.com> > > Define CPU monitor mode. Adjust core registers banking. Adjust CPU VM > state info. Provide CPU mode name for monitor mode. > > Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com> > Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> > --- > target-arm/cpu.h | 7 ++++--- > target-arm/helper.c | 3 +++ > target-arm/machine.c | 12 ++++++------ > target-arm/translate.c | 2 +- > 4 files changed, 14 insertions(+), 10 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 0b93e39..94d8bd1 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -124,9 +124,9 @@ typedef struct CPUARMState { > uint32_t spsr; > > /* Banked registers. */ > - uint32_t banked_spsr[6]; > - uint32_t banked_r13[6]; > - uint32_t banked_r14[6]; > + uint32_t banked_spsr[7]; > + uint32_t banked_r13[7]; > + uint32_t banked_r14[7]; > Are there any more modes yet to be implemented? It might save on future VMSD version bumps if we just pad this out to its ultimate value now. > /* These hold r8-r12. */ > uint32_t usr_regs[5]; > @@ -402,6 +402,7 @@ enum arm_cpu_mode { > ARM_CPU_MODE_FIQ = 0x11, > ARM_CPU_MODE_IRQ = 0x12, > ARM_CPU_MODE_SVC = 0x13, > + ARM_CPU_MODE_MON = 0x16, > ARM_CPU_MODE_ABT = 0x17, > ARM_CPU_MODE_UND = 0x1b, > ARM_CPU_MODE_SYS = 0x1f > diff --git a/target-arm/helper.c b/target-arm/helper.c > index d7922ad..d4407cf 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -2018,6 +2018,7 @@ static int bad_mode_switch(CPUARMState *env, int mode) > case ARM_CPU_MODE_USR: > case ARM_CPU_MODE_SYS: > case ARM_CPU_MODE_SVC: > + case ARM_CPU_MODE_MON: > case ARM_CPU_MODE_ABT: > case ARM_CPU_MODE_UND: > case ARM_CPU_MODE_IRQ: > @@ -2202,6 +2203,8 @@ int bank_number(int mode) > return 4; > case ARM_CPU_MODE_FIQ: > return 5; > + case ARM_CPU_MODE_MON: > + return 6; > } > hw_error("bank number requested for bad CPSR mode value 0x%x\n", mode); > } > diff --git a/target-arm/machine.c b/target-arm/machine.c > index 74f010f..51d0c79 100644 > --- a/target-arm/machine.c > +++ b/target-arm/machine.c > @@ -222,9 +222,9 @@ static int cpu_post_load(void *opaque, int version_id) > > const VMStateDescription vmstate_arm_cpu = { > .name = "cpu", > - .version_id = 13, > - .minimum_version_id = 13, > - .minimum_version_id_old = 13, > + .version_id = 14, > + .minimum_version_id = 14, > + .minimum_version_id_old = 14, > .pre_save = cpu_pre_save, > .post_load = cpu_post_load, > .fields = (VMStateField[]) { > @@ -238,9 +238,9 @@ const VMStateDescription vmstate_arm_cpu = { > .offset = 0, > }, > VMSTATE_UINT32(env.spsr, ARMCPU), > - VMSTATE_UINT32_ARRAY(env.banked_spsr, ARMCPU, 6), > - VMSTATE_UINT32_ARRAY(env.banked_r13, ARMCPU, 6), > - VMSTATE_UINT32_ARRAY(env.banked_r14, ARMCPU, 6), > + VMSTATE_UINT32_ARRAY(env.banked_spsr, ARMCPU, 7), > + VMSTATE_UINT32_ARRAY(env.banked_r13, ARMCPU, 7), > + VMSTATE_UINT32_ARRAY(env.banked_r14, ARMCPU, 7), > VMSTATE_UINT32_ARRAY(env.usr_regs, ARMCPU, 5), > VMSTATE_UINT32_ARRAY(env.fiq_regs, ARMCPU, 5), > /* The length-check must come before the arrays to avoid > diff --git a/target-arm/translate.c b/target-arm/translate.c > index 5f003e7..665c8ac 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c > @@ -10295,7 +10295,7 @@ void gen_intermediate_code_pc(CPUARMState *env, TranslationBlock *tb) > } > > static const char *cpu_mode_names[16] = { > - "usr", "fiq", "irq", "svc", "???", "???", "???", "abt", > + "usr", "fiq", "irq", "svc", "???", "???", "mon", "abt", > "???", "???", "???", "und", "???", "???", "???", "sys" > }; > > -- > 1.7.9.5 > >
On 3 December 2013 12:20, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote: >> From: Svetlana Fedoseeva <s.fedoseeva@samsung.com> >> >> Define CPU monitor mode. Adjust core registers banking. Adjust CPU VM >> state info. Provide CPU mode name for monitor mode. >> >> Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com> >> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> >> --- >> target-arm/cpu.h | 7 ++++--- >> target-arm/helper.c | 3 +++ >> target-arm/machine.c | 12 ++++++------ >> target-arm/translate.c | 2 +- >> 4 files changed, 14 insertions(+), 10 deletions(-) >> >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index 0b93e39..94d8bd1 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -124,9 +124,9 @@ typedef struct CPUARMState { >> uint32_t spsr; >> >> /* Banked registers. */ >> - uint32_t banked_spsr[6]; >> - uint32_t banked_r13[6]; >> - uint32_t banked_r14[6]; >> + uint32_t banked_spsr[7]; >> + uint32_t banked_r13[7]; >> + uint32_t banked_r14[7]; >> > > Are there any more modes yet to be implemented? It might save on > future VMSD version bumps if we just pad this out to its ultimate > value now. The remaining mode defined for AArch32 which we don't implement yet is Hyp mode, which has a banked R13 and SPSR, but not a banked LR. -- PMM
On 12/03/2013 04:51 PM, Peter Maydell wrote: > On 3 December 2013 12:20, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote: >>> From: Svetlana Fedoseeva <s.fedoseeva@samsung.com> >>> >>> Define CPU monitor mode. Adjust core registers banking. Adjust CPU VM >>> state info. Provide CPU mode name for monitor mode. >>> >>> Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com> >>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> >>> --- >>> target-arm/cpu.h | 7 ++++--- >>> target-arm/helper.c | 3 +++ >>> target-arm/machine.c | 12 ++++++------ >>> target-arm/translate.c | 2 +- >>> 4 files changed, 14 insertions(+), 10 deletions(-) >>> >>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >>> index 0b93e39..94d8bd1 100644 >>> --- a/target-arm/cpu.h >>> +++ b/target-arm/cpu.h >>> @@ -124,9 +124,9 @@ typedef struct CPUARMState { >>> uint32_t spsr; >>> >>> /* Banked registers. */ >>> - uint32_t banked_spsr[6]; >>> - uint32_t banked_r13[6]; >>> - uint32_t banked_r14[6]; >>> + uint32_t banked_spsr[7]; >>> + uint32_t banked_r13[7]; >>> + uint32_t banked_r14[7]; >>> >> Are there any more modes yet to be implemented? It might save on >> future VMSD version bumps if we just pad this out to its ultimate >> value now. > The remaining mode defined for AArch32 which we don't > implement yet is Hyp mode, which has a banked R13 and SPSR, > but not a banked LR. > > -- PMM > > So should a number of banked core registers be increased more? Personally, I'd like to keep this patch only TZ-related. Best regards, Sergey Fedorov
On Wed, Dec 4, 2013 at 8:01 PM, Fedorov Sergey <s.fedorov@samsung.com> wrote: > > On 12/03/2013 04:51 PM, Peter Maydell wrote: >> >> On 3 December 2013 12:20, Peter Crosthwaite >> <peter.crosthwaite@xilinx.com> wrote: >>> >>> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> >>> wrote: >>>> >>>> From: Svetlana Fedoseeva <s.fedoseeva@samsung.com> >>>> >>>> Define CPU monitor mode. Adjust core registers banking. Adjust CPU VM >>>> state info. Provide CPU mode name for monitor mode. >>>> >>>> Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com> >>>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> >>>> --- >>>> target-arm/cpu.h | 7 ++++--- >>>> target-arm/helper.c | 3 +++ >>>> target-arm/machine.c | 12 ++++++------ >>>> target-arm/translate.c | 2 +- >>>> 4 files changed, 14 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >>>> index 0b93e39..94d8bd1 100644 >>>> --- a/target-arm/cpu.h >>>> +++ b/target-arm/cpu.h >>>> @@ -124,9 +124,9 @@ typedef struct CPUARMState { >>>> uint32_t spsr; >>>> >>>> /* Banked registers. */ >>>> - uint32_t banked_spsr[6]; >>>> - uint32_t banked_r13[6]; >>>> - uint32_t banked_r14[6]; >>>> + uint32_t banked_spsr[7]; >>>> + uint32_t banked_r13[7]; >>>> + uint32_t banked_r14[7]; >>>> >>> Are there any more modes yet to be implemented? It might save on >>> future VMSD version bumps if we just pad this out to its ultimate >>> value now. >> >> The remaining mode defined for AArch32 which we don't >> implement yet is Hyp mode, which has a banked R13 and SPSR, >> but not a banked LR. >> >> -- PMM >> >> > > So should a number of banked core registers be increased more? Personally, > I'd like to keep this patch only TZ-related. > So what im proposing is just a slightly more general patch. Is it really any more complicated than just applying your change pattern for the hyp mode? Patch subject would be something like: "target-arm: Add all remaining missing modes" The motiviation is less VMSD version bumps in ARM CPU (a place where I expect assume such version bumps to be considerable annoyance). Regards, Peter > Best regards, > Sergey Fedorov >
On 4 December 2013 10:58, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > So what im proposing is just a slightly more general patch. Is it > really any more complicated than just applying your change pattern for > the hyp mode? I think it would be, because of the wrinkle that hyp mode doesn't have a banked LR, so the existing "assume we can just translate the mode into a single index good for both LR and SP" logic would break. The minimal change if we wanted to keep VMSD bumps to a minimum would be to increase the size of the banked_spsr[] and banked_r13[] arrays to allow for Hyp mode but do nothing else (except add a comment about it I guess). > The motiviation is less VMSD version bumps in ARM CPU (a place where I > expect assume such version bumps to be considerable annoyance). Well, they're only a problem at the point where we start trying to support cross-version migration; we're not at that point yet... thanks -- PMM
On 12/04/2013 03:18 PM, Peter Maydell wrote: > On 4 December 2013 10:58, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> So what im proposing is just a slightly more general patch. Is it >> really any more complicated than just applying your change pattern for >> the hyp mode? > I think it would be, because of the wrinkle that hyp mode doesn't > have a banked LR, so the existing "assume we can just translate > the mode into a single index good for both LR and SP" logic > would break. > > The minimal change if we wanted to keep VMSD bumps to a > minimum would be to increase the size of the banked_spsr[] > and banked_r13[] arrays to allow for Hyp mode but do nothing > else (except add a comment about it I guess). If we want to bump VMSD just once for monitor + hypervisor mode then we need to add ELR_hyp register definition too. I think then it would be better to implement hypervisor mode and its special banking scheme, too. But I doubt it worth to combine these two things into one patch. > >> The motiviation is less VMSD version bumps in ARM CPU (a place where I >> expect assume such version bumps to be considerable annoyance). > Well, they're only a problem at the point where we start trying > to support cross-version migration; we're not at that point yet... > > thanks > -- PMM > Best regards, Sergey Fedorov
On 4 December 2013 12:33, Fedorov Sergey <s.fedorov@samsung.com> wrote: > > On 12/04/2013 03:18 PM, Peter Maydell wrote: >> >> On 4 December 2013 10:58, Peter Crosthwaite >> <peter.crosthwaite@xilinx.com> wrote: >>> >>> So what im proposing is just a slightly more general patch. Is it >>> really any more complicated than just applying your change pattern for >>> the hyp mode? >> >> I think it would be, because of the wrinkle that hyp mode doesn't >> have a banked LR, so the existing "assume we can just translate >> the mode into a single index good for both LR and SP" logic >> would break. >> >> The minimal change if we wanted to keep VMSD bumps to a >> minimum would be to increase the size of the banked_spsr[] >> and banked_r13[] arrays to allow for Hyp mode but do nothing >> else (except add a comment about it I guess). > > > If we want to bump VMSD just once for monitor + hypervisor mode then we need > to add ELR_hyp register definition too. I think then it would be better to > implement hypervisor mode and its special banking scheme, too. But I doubt > it worth to combine these two things into one patch. It's possible to add single new fields to the VMState without requiring a compatibility break, by marking the new field as "only present in version X or greater"; new elements on the end of arrays are a little fiddlier. But yes, I think we should just not worry about possible future Hyp mode now. Let's stick with your current patch. thanks -- PMM
On Wed, Dec 4, 2013 at 10:35 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 4 December 2013 12:33, Fedorov Sergey <s.fedorov@samsung.com> wrote: >> >> On 12/04/2013 03:18 PM, Peter Maydell wrote: >>> >>> On 4 December 2013 10:58, Peter Crosthwaite >>> <peter.crosthwaite@xilinx.com> wrote: >>>> >>>> So what im proposing is just a slightly more general patch. Is it >>>> really any more complicated than just applying your change pattern for >>>> the hyp mode? >>> >>> I think it would be, because of the wrinkle that hyp mode doesn't >>> have a banked LR, so the existing "assume we can just translate >>> the mode into a single index good for both LR and SP" logic >>> would break. >>> >>> The minimal change if we wanted to keep VMSD bumps to a >>> minimum would be to increase the size of the banked_spsr[] >>> and banked_r13[] arrays to allow for Hyp mode but do nothing >>> else (except add a comment about it I guess). >> >> >> If we want to bump VMSD just once for monitor + hypervisor mode then we need >> to add ELR_hyp register definition too. I think then it would be better to >> implement hypervisor mode and its special banking scheme, too. But I doubt >> it worth to combine these two things into one patch. > > It's possible to add single new fields to the VMState without > requiring a compatibility break, by marking the new field as > "only present in version X or greater"; new elements on the > end of arrays are a little fiddlier. > > But yes, I think we should just not worry about possible future > Hyp mode now. Let's stick with your current patch. > +1. Patch is good for the moment. > thanks > -- PMM >
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 0b93e39..94d8bd1 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -124,9 +124,9 @@ typedef struct CPUARMState { uint32_t spsr; /* Banked registers. */ - uint32_t banked_spsr[6]; - uint32_t banked_r13[6]; - uint32_t banked_r14[6]; + uint32_t banked_spsr[7]; + uint32_t banked_r13[7]; + uint32_t banked_r14[7]; /* These hold r8-r12. */ uint32_t usr_regs[5]; @@ -402,6 +402,7 @@ enum arm_cpu_mode { ARM_CPU_MODE_FIQ = 0x11, ARM_CPU_MODE_IRQ = 0x12, ARM_CPU_MODE_SVC = 0x13, + ARM_CPU_MODE_MON = 0x16, ARM_CPU_MODE_ABT = 0x17, ARM_CPU_MODE_UND = 0x1b, ARM_CPU_MODE_SYS = 0x1f diff --git a/target-arm/helper.c b/target-arm/helper.c index d7922ad..d4407cf 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -2018,6 +2018,7 @@ static int bad_mode_switch(CPUARMState *env, int mode) case ARM_CPU_MODE_USR: case ARM_CPU_MODE_SYS: case ARM_CPU_MODE_SVC: + case ARM_CPU_MODE_MON: case ARM_CPU_MODE_ABT: case ARM_CPU_MODE_UND: case ARM_CPU_MODE_IRQ: @@ -2202,6 +2203,8 @@ int bank_number(int mode) return 4; case ARM_CPU_MODE_FIQ: return 5; + case ARM_CPU_MODE_MON: + return 6; } hw_error("bank number requested for bad CPSR mode value 0x%x\n", mode); } diff --git a/target-arm/machine.c b/target-arm/machine.c index 74f010f..51d0c79 100644 --- a/target-arm/machine.c +++ b/target-arm/machine.c @@ -222,9 +222,9 @@ static int cpu_post_load(void *opaque, int version_id) const VMStateDescription vmstate_arm_cpu = { .name = "cpu", - .version_id = 13, - .minimum_version_id = 13, - .minimum_version_id_old = 13, + .version_id = 14, + .minimum_version_id = 14, + .minimum_version_id_old = 14, .pre_save = cpu_pre_save, .post_load = cpu_post_load, .fields = (VMStateField[]) { @@ -238,9 +238,9 @@ const VMStateDescription vmstate_arm_cpu = { .offset = 0, }, VMSTATE_UINT32(env.spsr, ARMCPU), - VMSTATE_UINT32_ARRAY(env.banked_spsr, ARMCPU, 6), - VMSTATE_UINT32_ARRAY(env.banked_r13, ARMCPU, 6), - VMSTATE_UINT32_ARRAY(env.banked_r14, ARMCPU, 6), + VMSTATE_UINT32_ARRAY(env.banked_spsr, ARMCPU, 7), + VMSTATE_UINT32_ARRAY(env.banked_r13, ARMCPU, 7), + VMSTATE_UINT32_ARRAY(env.banked_r14, ARMCPU, 7), VMSTATE_UINT32_ARRAY(env.usr_regs, ARMCPU, 5), VMSTATE_UINT32_ARRAY(env.fiq_regs, ARMCPU, 5), /* The length-check must come before the arrays to avoid diff --git a/target-arm/translate.c b/target-arm/translate.c index 5f003e7..665c8ac 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -10295,7 +10295,7 @@ void gen_intermediate_code_pc(CPUARMState *env, TranslationBlock *tb) } static const char *cpu_mode_names[16] = { - "usr", "fiq", "irq", "svc", "???", "???", "???", "abt", + "usr", "fiq", "irq", "svc", "???", "???", "mon", "abt", "???", "???", "???", "und", "???", "???", "???", "sys" };