| Message ID | 20190526072826.32956-1-lucienmp_antispam@yahoo.com |
|---|---|
| State | New |
| Headers | show |
| Series | Incorrect Stack Pointer shadow register support on some m68k CPUs | expand |
Patchew URL: https://patchew.org/QEMU/20190526072826.32956-1-lucienmp_antispam@yahoo.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20190526072826.32956-1-lucienmp_antispam@yahoo.com Type: series Subject: [Qemu-devel] [PATCH] Incorrect Stack Pointer shadow register support on some m68k CPUs === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === From https://github.com/patchew-project/qemu * [new tag] patchew/20190526072826.32956-1-lucienmp_antispam@yahoo.com -> patchew/20190526072826.32956-1-lucienmp_antispam@yahoo.com Switched to a new branch 'test' 2fa92e8d86 Incorrect Stack Pointer shadow register support on some m68k CPUs === OUTPUT BEGIN === ERROR: Author email address is mangled by the mailing list #2: Author: Lucien Murray-Pitts via Qemu-devel <qemu-devel@nongnu.org> WARNING: Block comments use a leading /* on a separate line #46: FILE: target/m68k/cpu.h:465: +/* The ColdFire core ISA is a RISC-style reduction of the 68000 series WARNING: Block comments use * on subsequent lines #47: FILE: target/m68k/cpu.h:466: +/* The ColdFire core ISA is a RISC-style reduction of the 68000 series + Whilst the 68000 flourished by adding extended stack/instructions in total: 1 errors, 2 warnings, 44 lines checked Commit 2fa92e8d8620 (Incorrect Stack Pointer shadow register support on some m68k CPUs) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190526072826.32956-1-lucienmp_antispam@yahoo.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On 26/05/2019 09:28, Lucien Murray-Pitts wrote: > On CPU32 and the early 68000 and 68010 the ISP doesnt exist. > These CPUs only have SSP/USP. > > The availability of this feature is determined by the > implementation of Master mode bit in the SR register. > > Those with the master-mode bit have ISP. > > Additional comments added to the features set to claify > exactly what differentiates each class. (m68k_features) > > The movec instruction when accessing these shadow registers > in some configurations should issue a TRAP. This patch does not > add this funcitonality to the helpers. > I think it's better to also update movec in the same patch. Could you also update the comment about sp in CPUM68KState structure definition? And, if possible, could you fix style problem reported by patchew. Thanks, LAurent > Signed-off-by: Lucien Murray-Pitts <lucienmp_antispam@yahoo.com> > --- > target/m68k/cpu.c | 1 + > target/m68k/cpu.h | 12 ++++++++++-- > target/m68k/helper.c | 3 ++- > 3 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c > index b16957934a..61368d1a9a 100644 > --- a/target/m68k/cpu.c > +++ b/target/m68k/cpu.c > @@ -121,6 +121,7 @@ static void m68020_cpu_initfn(Object *obj) > CPUM68KState *env = &cpu->env; > > m68k_set_feature(env, M68K_FEATURE_M68000); > + m68k_set_feature(env, M68K_FEATURE_MSP); > m68k_set_feature(env, M68K_FEATURE_USP); > m68k_set_feature(env, M68K_FEATURE_WORD_INDEX); > m68k_set_feature(env, M68K_FEATURE_QUAD_MULDIV); > diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h > index 9deff9e234..8be68e5e4f 100644 > --- a/target/m68k/cpu.h > +++ b/target/m68k/cpu.h > @@ -462,12 +462,19 @@ void m68k_switch_sp(CPUM68KState *env); > > void do_m68k_semihosting(CPUM68KState *env, int nr); > > -/* There are 4 ColdFire core ISA revisions: A, A+, B and C. > +/* The ColdFire core ISA is a RISC-style reduction of the 68000 series > + Whilst the 68000 flourished by adding extended stack/instructions in > + five main flavors original 68000, 680010/20/30/40, and a CPU32/CPU32+ > + > + CPU32/32+ are basically 68000/10 compatible, with and 68020. Mostly > + Supervisor state differences. > + > + There are 4 ColdFire core ISA revisions: A, A+, B and C. > Each feature covers the subset of instructions common to the > ISA revisions mentioned. */ > > enum m68k_features { > - M68K_FEATURE_M68000, > + M68K_FEATURE_M68000, /* Base m68k set, as opposed to ColdFire */ > M68K_FEATURE_CF_ISA_A, > M68K_FEATURE_CF_ISA_B, /* (ISA B or C). */ > M68K_FEATURE_CF_ISA_APLUSC, /* BIT/BITREV, FF1, STRLDSR (ISA A+ or C). */ > @@ -477,6 +484,7 @@ enum m68k_features { > M68K_FEATURE_CF_EMAC, > M68K_FEATURE_CF_EMAC_B, /* Revision B EMAC (dual accumulate). */ > M68K_FEATURE_USP, /* User Stack Pointer. (ISA A+, B or C). */ > + M68K_FEATURE_MSP, /* Master Stack Pointer. Not 68000/10,Coldfire,CPU32 */ > M68K_FEATURE_EXT_FULL, /* 68020+ full extension word. */ > M68K_FEATURE_WORD_INDEX, /* word sized address index registers. */ > M68K_FEATURE_SCALED_INDEX, /* scaled address index registers. */ > diff --git a/target/m68k/helper.c b/target/m68k/helper.c > index 6db93bdd81..64c8a82a92 100644 > --- a/target/m68k/helper.c > +++ b/target/m68k/helper.c > @@ -337,7 +337,8 @@ void m68k_switch_sp(CPUM68KState *env) > env->sp[env->current_sp] = env->aregs[7]; > if (m68k_feature(env, M68K_FEATURE_M68000)) { > if (env->sr & SR_S) { > - if (env->sr & SR_M) { > + /* SR:Master-Mode bit unimplemented then ISP is not available */ > + if (!m68k_feature(env, M68K_FEATURE_MSP) || env->sr & SR_M) { > new_sp = M68K_SSP; > } else { > new_sp = M68K_ISP; >
> On Sunday, May 26, 2019, 10:10:39 PM GMT+9, Laurent Vivier <laurent@vivier.eu> wrote: > On 26/05/2019 09:28, Lucien Murray-Pitts wrote: >> On CPU32 and the early 68000 and 68010 the ISP doesnt exist. >> These CPUs only have SSP/USP. >> [SNIP] >> The movec instruction when accessing these shadow registers >> in some configurations should issue a TRAP. This patch does not >> add this funcitonality to the helpers. >> >I think it's better to also update movec in the same patch.[LMP] Movec should be undefined (coldfire manual) for registers it doesnt know. The MC680X0 manual is less clear.Technically this could be just leaving the operation of the instruction alone and allowing it to pass back MSP/ISP/USP as it currently does. My thinking is this is less likely to break anything So I will only add a comment, to state that on 68000/10/CF/CPU32 it should be "undefined" >Could you also update the comment about sp in CPUM68KState structure definition? [LMP] Done >And, if possible, could you fix style problem reported by patchew. [LMP] Done
On 27/05/2019 05:43, Lucien Anti-Spam wrote: > > > On Sunday, May 26, 2019, 10:10:39 PM GMT+9, Laurent Vivier > <laurent@vivier.eu> wrote: > > On 26/05/2019 09:28, Lucien Murray-Pitts wrote: > >> On CPU32 and the early 68000 and 68010 the ISP doesnt exist. > >> These CPUs only have SSP/USP. > >> > [SNIP] > >> The movec instruction when accessing these shadow registers > >> in some configurations should issue a TRAP. This patch does not > >> add this funcitonality to the helpers. > >> > > >I think it's better to also update movec in the same patch. > [LMP] Movec should be undefined (coldfire manual) for registers it > doesnt know. The MC680X0 manual is less clear. > Technically this could be just leaving the operation of the instruction > alone and allowing it to pass back MSP/ISP/USP as it currently does. My > thinking is this is less likely to break anything In fact, code in m68k_movec_from()/m68k_movec_to() need rework because they trigger a cpu_abort() with unknown code, they need rework too. So I think we can just do as you propose for the moment. Thanks, Laurent
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c index b16957934a..61368d1a9a 100644 --- a/target/m68k/cpu.c +++ b/target/m68k/cpu.c @@ -121,6 +121,7 @@ static void m68020_cpu_initfn(Object *obj) CPUM68KState *env = &cpu->env; m68k_set_feature(env, M68K_FEATURE_M68000); + m68k_set_feature(env, M68K_FEATURE_MSP); m68k_set_feature(env, M68K_FEATURE_USP); m68k_set_feature(env, M68K_FEATURE_WORD_INDEX); m68k_set_feature(env, M68K_FEATURE_QUAD_MULDIV); diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h index 9deff9e234..8be68e5e4f 100644 --- a/target/m68k/cpu.h +++ b/target/m68k/cpu.h @@ -462,12 +462,19 @@ void m68k_switch_sp(CPUM68KState *env); void do_m68k_semihosting(CPUM68KState *env, int nr); -/* There are 4 ColdFire core ISA revisions: A, A+, B and C. +/* The ColdFire core ISA is a RISC-style reduction of the 68000 series + Whilst the 68000 flourished by adding extended stack/instructions in + five main flavors original 68000, 680010/20/30/40, and a CPU32/CPU32+ + + CPU32/32+ are basically 68000/10 compatible, with and 68020. Mostly + Supervisor state differences. + + There are 4 ColdFire core ISA revisions: A, A+, B and C. Each feature covers the subset of instructions common to the ISA revisions mentioned. */ enum m68k_features { - M68K_FEATURE_M68000, + M68K_FEATURE_M68000, /* Base m68k set, as opposed to ColdFire */ M68K_FEATURE_CF_ISA_A, M68K_FEATURE_CF_ISA_B, /* (ISA B or C). */ M68K_FEATURE_CF_ISA_APLUSC, /* BIT/BITREV, FF1, STRLDSR (ISA A+ or C). */ @@ -477,6 +484,7 @@ enum m68k_features { M68K_FEATURE_CF_EMAC, M68K_FEATURE_CF_EMAC_B, /* Revision B EMAC (dual accumulate). */ M68K_FEATURE_USP, /* User Stack Pointer. (ISA A+, B or C). */ + M68K_FEATURE_MSP, /* Master Stack Pointer. Not 68000/10,Coldfire,CPU32 */ M68K_FEATURE_EXT_FULL, /* 68020+ full extension word. */ M68K_FEATURE_WORD_INDEX, /* word sized address index registers. */ M68K_FEATURE_SCALED_INDEX, /* scaled address index registers. */ diff --git a/target/m68k/helper.c b/target/m68k/helper.c index 6db93bdd81..64c8a82a92 100644 --- a/target/m68k/helper.c +++ b/target/m68k/helper.c @@ -337,7 +337,8 @@ void m68k_switch_sp(CPUM68KState *env) env->sp[env->current_sp] = env->aregs[7]; if (m68k_feature(env, M68K_FEATURE_M68000)) { if (env->sr & SR_S) { - if (env->sr & SR_M) { + /* SR:Master-Mode bit unimplemented then ISP is not available */ + if (!m68k_feature(env, M68K_FEATURE_MSP) || env->sr & SR_M) { new_sp = M68K_SSP; } else { new_sp = M68K_ISP;
On CPU32 and the early 68000 and 68010 the ISP doesnt exist. These CPUs only have SSP/USP. The availability of this feature is determined by the implementation of Master mode bit in the SR register. Those with the master-mode bit have ISP. Additional comments added to the features set to claify exactly what differentiates each class. (m68k_features) The movec instruction when accessing these shadow registers in some configurations should issue a TRAP. This patch does not add this funcitonality to the helpers. Signed-off-by: Lucien Murray-Pitts <lucienmp_antispam@yahoo.com> --- target/m68k/cpu.c | 1 + target/m68k/cpu.h | 12 ++++++++++-- target/m68k/helper.c | 3 ++- 3 files changed, 13 insertions(+), 3 deletions(-)