Message ID | 20220421011729.1148727-1-bmeng.cn@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] target/ppc: Fix BookE debug interrupt generation | expand |
On 4/21/22 03:17, Bin Meng wrote: > From: Bin Meng <bin.meng@windriver.com> > > Per E500 core reference manual [1], chapter 8.4.4 "Branch Taken Debug > Event" and chapter 8.4.5 "Instruction Complete Debug Event": > > "A branch taken debug event occurs if both MSR[DE] and DBCR0[BRT] > are set ... Branch taken debug events are not recognized if MSR[DE] > is cleared when the branch instruction executes." > > "An instruction complete debug event occurs when any instruction > completes execution so long as MSR[DE] and DBCR0[ICMP] are both > set ... Instruction complete debug events are not recognized if > MSR[DE] is cleared at the time of the instruction execution." > > Current codes do not check MSR.DE bit before setting HFLAGS_SE and > HFLAGS_BE flag, which would cause the immediate debug interrupt to > be generated, e.g.: when DBCR0.ICMP bit is set by guest software > and MSR.DE is not set. > > [1] https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf > > Signed-off-by: Bin Meng <bin.meng@windriver.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > > Changes in v2: > - update commit message to use E500CORERM instead of PowerISA 2.07 > > target/ppc/helper_regs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c > index 9a691d6833..77bc57415c 100644 > --- a/target/ppc/helper_regs.c > +++ b/target/ppc/helper_regs.c > @@ -63,10 +63,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env) > > if (ppc_flags & POWERPC_FLAG_DE) { > target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0]; > - if (dbcr0 & DBCR0_ICMP) { > + if ((dbcr0 & DBCR0_ICMP) && msr_de) { > hflags |= 1 << HFLAGS_SE; > } > - if (dbcr0 & DBCR0_BRT) { > + if ((dbcr0 & DBCR0_BRT) && msr_de) { > hflags |= 1 << HFLAGS_BE; > } > } else {
Bin Meng <bmeng.cn@gmail.com> writes: > From: Bin Meng <bin.meng@windriver.com> > > Per E500 core reference manual [1], chapter 8.4.4 "Branch Taken Debug > Event" and chapter 8.4.5 "Instruction Complete Debug Event": > > "A branch taken debug event occurs if both MSR[DE] and DBCR0[BRT] > are set ... Branch taken debug events are not recognized if MSR[DE] > is cleared when the branch instruction executes." > > "An instruction complete debug event occurs when any instruction > completes execution so long as MSR[DE] and DBCR0[ICMP] are both > set ... Instruction complete debug events are not recognized if > MSR[DE] is cleared at the time of the instruction execution." > > Current codes do not check MSR.DE bit before setting HFLAGS_SE and > HFLAGS_BE flag, which would cause the immediate debug interrupt to > be generated, e.g.: when DBCR0.ICMP bit is set by guest software > and MSR.DE is not set. > > [1] https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf > > Signed-off-by: Bin Meng <bin.meng@windriver.com> Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>
On 20/04/2022 22:17, Bin Meng wrote: > From: Bin Meng<bin.meng@windriver.com> > > Per E500 core reference manual [1], chapter 8.4.4 "Branch Taken Debug > Event" and chapter 8.4.5 "Instruction Complete Debug Event": > > "A branch taken debug event occurs if both MSR[DE] and DBCR0[BRT] > are set ... Branch taken debug events are not recognized if MSR[DE] > is cleared when the branch instruction executes." > > "An instruction complete debug event occurs when any instruction > completes execution so long as MSR[DE] and DBCR0[ICMP] are both > set ... Instruction complete debug events are not recognized if > MSR[DE] is cleared at the time of the instruction execution." > > Current codes do not check MSR.DE bit before setting HFLAGS_SE and > HFLAGS_BE flag, which would cause the immediate debug interrupt to > be generated, e.g.: when DBCR0.ICMP bit is set by guest software > and MSR.DE is not set. > > [1]https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf > > Signed-off-by: Bin Meng<bin.meng@windriver.com> > --- > > Changes in v2: > - update commit message to use E500CORERM instead of PowerISA 2.07 > > target/ppc/helper_regs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c > index 9a691d6833..77bc57415c 100644 > --- a/target/ppc/helper_regs.c > +++ b/target/ppc/helper_regs.c > @@ -63,10 +63,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env) > > if (ppc_flags & POWERPC_FLAG_DE) { > target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0]; > - if (dbcr0 & DBCR0_ICMP) { > + if ((dbcr0 & DBCR0_ICMP) && msr_de) { There was a discussion some time ago that was better to avoid hidden uses of *env, so it may be better to change msr_de to ((env->msr >> MSR_DE) & 1) or to (env->msr & BIT_ULL(MSR_DE)) > hflags |= 1 << HFLAGS_SE; > } > - if (dbcr0 & DBCR0_BRT) { > + if ((dbcr0 & DBCR0_BRT) && msr_de) { Here as well > hflags |= 1 << HFLAGS_BE; > } > } else { > -- > 2.25.1 > > Apart from that, Reviewed-by: Lucas Mateus Castro <lucas.araujo@eldorado.org.br>
On 4/25/22 16:16, Lucas Mateus Martins Araujo e Castro wrote: > > On 20/04/2022 22:17, Bin Meng wrote: >> From: Bin Meng<bin.meng@windriver.com> >> >> Per E500 core reference manual [1], chapter 8.4.4 "Branch Taken Debug >> Event" and chapter 8.4.5 "Instruction Complete Debug Event": >> >> "A branch taken debug event occurs if both MSR[DE] and DBCR0[BRT] >> are set ... Branch taken debug events are not recognized if MSR[DE] >> is cleared when the branch instruction executes." >> >> "An instruction complete debug event occurs when any instruction >> completes execution so long as MSR[DE] and DBCR0[ICMP] are both >> set ... Instruction complete debug events are not recognized if >> MSR[DE] is cleared at the time of the instruction execution." >> >> Current codes do not check MSR.DE bit before setting HFLAGS_SE and >> HFLAGS_BE flag, which would cause the immediate debug interrupt to >> be generated, e.g.: when DBCR0.ICMP bit is set by guest software >> and MSR.DE is not set. >> >> [1]https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf >> >> Signed-off-by: Bin Meng<bin.meng@windriver.com> >> --- >> >> Changes in v2: >> - update commit message to use E500CORERM instead of PowerISA 2.07 >> >> target/ppc/helper_regs.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c >> index 9a691d6833..77bc57415c 100644 >> --- a/target/ppc/helper_regs.c >> +++ b/target/ppc/helper_regs.c >> @@ -63,10 +63,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env) >> >> if (ppc_flags & POWERPC_FLAG_DE) { >> target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0]; >> - if (dbcr0 & DBCR0_ICMP) { >> + if ((dbcr0 & DBCR0_ICMP) && msr_de) { > There was a discussion some time ago that was better to avoid hidden uses of *env, so it may be better to change msr_de to ((env->msr >> MSR_DE) & 1) or to (env->msr & BIT_ULL(MSR_DE)) I think the "target/ppc: Remove hidden usages of *env" patchset : https://patchwork.ozlabs.org/project/qemu-ppc/list/?series=296440 will take some time to merge and there will certainly be followups. This patch can go first. Thanks, C. >> hflags |= 1 << HFLAGS_SE; >> } >> - if (dbcr0 & DBCR0_BRT) { >> + if ((dbcr0 & DBCR0_BRT) && msr_de) { > Here as well >> hflags |= 1 << HFLAGS_BE; >> } >> } else { >> -- >> 2.25.1 >> >> > Apart from that, > Reviewed-by: Lucas Mateus Castro <lucas.araujo@eldorado.org.br> > -- > Lucas Mateus M. Araujo e Castro > Instituto de Pesquisas ELDORADO <https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station> > Departamento Computação Embarcada > Analista de Software Trainee > Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks, Daniel On 4/20/22 22:17, Bin Meng wrote: > From: Bin Meng <bin.meng@windriver.com> > > Per E500 core reference manual [1], chapter 8.4.4 "Branch Taken Debug > Event" and chapter 8.4.5 "Instruction Complete Debug Event": > > "A branch taken debug event occurs if both MSR[DE] and DBCR0[BRT] > are set ... Branch taken debug events are not recognized if MSR[DE] > is cleared when the branch instruction executes." > > "An instruction complete debug event occurs when any instruction > completes execution so long as MSR[DE] and DBCR0[ICMP] are both > set ... Instruction complete debug events are not recognized if > MSR[DE] is cleared at the time of the instruction execution." > > Current codes do not check MSR.DE bit before setting HFLAGS_SE and > HFLAGS_BE flag, which would cause the immediate debug interrupt to > be generated, e.g.: when DBCR0.ICMP bit is set by guest software > and MSR.DE is not set. > > [1] https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > Changes in v2: > - update commit message to use E500CORERM instead of PowerISA 2.07 > > target/ppc/helper_regs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c > index 9a691d6833..77bc57415c 100644 > --- a/target/ppc/helper_regs.c > +++ b/target/ppc/helper_regs.c > @@ -63,10 +63,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env) > > if (ppc_flags & POWERPC_FLAG_DE) { > target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0]; > - if (dbcr0 & DBCR0_ICMP) { > + if ((dbcr0 & DBCR0_ICMP) && msr_de) { > hflags |= 1 << HFLAGS_SE; > } > - if (dbcr0 & DBCR0_BRT) { > + if ((dbcr0 & DBCR0_BRT) && msr_de) { > hflags |= 1 << HFLAGS_BE; > } > } else {
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c index 9a691d6833..77bc57415c 100644 --- a/target/ppc/helper_regs.c +++ b/target/ppc/helper_regs.c @@ -63,10 +63,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env) if (ppc_flags & POWERPC_FLAG_DE) { target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0]; - if (dbcr0 & DBCR0_ICMP) { + if ((dbcr0 & DBCR0_ICMP) && msr_de) { hflags |= 1 << HFLAGS_SE; } - if (dbcr0 & DBCR0_BRT) { + if ((dbcr0 & DBCR0_BRT) && msr_de) { hflags |= 1 << HFLAGS_BE; } } else {