| Message ID | 20240715084639.983127-1-chigot@adacore.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/ppc: fix decrementer with BookE timers | expand |
Hi, Gentle ping + CC missing maintainers. Thanks Clément On Mon, Jul 15, 2024 at 10:46 AM Clément Chigot <chigot@adacore.com> wrote: > > The BookE decrementer stops at 0, meaning that it won't decremented > towards "negative" values. > However, the current logic is inverted: decr is updated solely when > the resulting value would be negative. > > Signed-off-by: Clément Chigot <chigot@adacore.com> > Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors") > --- > hw/ppc/ppc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > index e6fa5580c0..9fc85c7de0 100644 > --- a/hw/ppc/ppc.c > +++ b/hw/ppc/ppc.c > @@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState *env, int64_t now, > int64_t decr; > > n = ns_to_tb(tb_env->decr_freq, now); > - if (next > n && tb_env->flags & PPC_TIMER_BOOKE) { > + > + /* BookE timers stop when reaching 0. */ > + if (next < n && tb_env->flags & PPC_TIMER_BOOKE) { > decr = 0; > } else { > decr = next - n; > -- > 2.25.1 >
Hey, Gentle ping Thanks Clément On Mon, Jul 29, 2024 at 10:33 AM Clément Chigot <chigot@adacore.com> wrote: > > Hi, > > Gentle ping + CC missing maintainers. > > Thanks Clément > > On Mon, Jul 15, 2024 at 10:46 AM Clément Chigot <chigot@adacore.com> wrote: > > > > The BookE decrementer stops at 0, meaning that it won't decremented > > towards "negative" values. > > However, the current logic is inverted: decr is updated solely when > > the resulting value would be negative. > > > > Signed-off-by: Clément Chigot <chigot@adacore.com> > > Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors") > > --- > > hw/ppc/ppc.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > > index e6fa5580c0..9fc85c7de0 100644 > > --- a/hw/ppc/ppc.c > > +++ b/hw/ppc/ppc.c > > @@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState *env, int64_t now, > > int64_t decr; > > > > n = ns_to_tb(tb_env->decr_freq, now); > > - if (next > n && tb_env->flags & PPC_TIMER_BOOKE) { > > + > > + /* BookE timers stop when reaching 0. */ > > + if (next < n && tb_env->flags & PPC_TIMER_BOOKE) { > > decr = 0; > > } else { > > decr = next - n; > > -- > > 2.25.1 > >
Adding Cédric On 27/8/24 13:49, Clément Chigot wrote: > Hey, > > Gentle ping > > Thanks Clément > > On Mon, Jul 29, 2024 at 10:33 AM Clément Chigot <chigot@adacore.com> wrote: >> >> Hi, >> >> Gentle ping + CC missing maintainers. >> >> Thanks Clément >> >> On Mon, Jul 15, 2024 at 10:46 AM Clément Chigot <chigot@adacore.com> wrote: >>> >>> The BookE decrementer stops at 0, meaning that it won't decremented >>> towards "negative" values. >>> However, the current logic is inverted: decr is updated solely when >>> the resulting value would be negative. >>> >>> Signed-off-by: Clément Chigot <chigot@adacore.com> >>> Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors") >>> --- >>> hw/ppc/ppc.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c >>> index e6fa5580c0..9fc85c7de0 100644 >>> --- a/hw/ppc/ppc.c >>> +++ b/hw/ppc/ppc.c >>> @@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState *env, int64_t now, >>> int64_t decr; >>> >>> n = ns_to_tb(tb_env->decr_freq, now); >>> - if (next > n && tb_env->flags & PPC_TIMER_BOOKE) { >>> + >>> + /* BookE timers stop when reaching 0. */ >>> + if (next < n && tb_env->flags & PPC_TIMER_BOOKE) { >>> decr = 0; >>> } else { >>> decr = next - n; >>> -- >>> 2.25.1 >>> >
Hello Clément, On 7/15/24 10:46, Clément Chigot wrote: > The BookE decrementer stops at 0, meaning that it won't decremented > towards "negative" values. > However, the current logic is inverted: decr is updated solely when > the resulting value would be negative. How did you hit the issue ? which machine ? I didn't see any error when booting Linux 6.6.3 on mpc8544ds, e500mc, e5500 and e6500. > Signed-off-by: Clément Chigot <chigot@adacore.com> > Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors") LGTM, Reviewed-by: Cédric Le Goater <clg@redhat.com> We have some automated tests with the ppce500 machine which it would be interesting to extend to have a better coverage of booke. Thanks, C. > --- > hw/ppc/ppc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > index e6fa5580c0..9fc85c7de0 100644 > --- a/hw/ppc/ppc.c > +++ b/hw/ppc/ppc.c > @@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState *env, int64_t now, > int64_t decr; > > n = ns_to_tb(tb_env->decr_freq, now); > - if (next > n && tb_env->flags & PPC_TIMER_BOOKE) { > + > + /* BookE timers stop when reaching 0. */ > + if (next < n && tb_env->flags & PPC_TIMER_BOOKE) { > decr = 0; > } else { > decr = next - n;
On Tue, Aug 27, 2024 at 7:40 PM Cédric Le Goater <clg@kaod.org> wrote: > > Hello Clément, > > On 7/15/24 10:46, Clément Chigot wrote: > > The BookE decrementer stops at 0, meaning that it won't decremented > > towards "negative" values. > > However, the current logic is inverted: decr is updated solely when > > the resulting value would be negative. > > How did you hit the issue ? which machine ? I didn't see any error > when booting Linux 6.6.3 on mpc8544ds, e500mc, e5500 and e6500. I hit this issue while running some version of VxWorks on a custom machine: p3041ds (description [1] and our local implementation [2]). So, I'm not that surprised you were not able to reproduce. > > Signed-off-by: Clément Chigot <chigot@adacore.com> > > Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors") > > LGTM, > > Reviewed-by: Cédric Le Goater <clg@redhat.com> > > We have some automated tests with the ppce500 machine which it would be > interesting to extend to have a better coverage of booke. Thanks for the pointer, I'll see if I can extend them. > Thanks, > > C. > [1] https://www.nxp.com/design/design-center/software/qoriq-developer-resources/p3041-qoriq-development-system:P3041DS [2] https://github.com/AdaCore/qemu/blob/qemu-stable-9.0.0/hw/ppc/p3041ds.c > > > --- > > hw/ppc/ppc.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > > index e6fa5580c0..9fc85c7de0 100644 > > --- a/hw/ppc/ppc.c > > +++ b/hw/ppc/ppc.c > > @@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState *env, int64_t now, > > int64_t decr; > > > > n = ns_to_tb(tb_env->decr_freq, now); > > - if (next > n && tb_env->flags & PPC_TIMER_BOOKE) { > > + > > + /* BookE timers stop when reaching 0. */ > > + if (next < n && tb_env->flags & PPC_TIMER_BOOKE) { > > decr = 0; > > } else { > > decr = next - n; > >
On 8/28/24 09:21, Clément Chigot wrote: > On Tue, Aug 27, 2024 at 7:40 PM Cédric Le Goater <clg@kaod.org> wrote: >> >> Hello Clément, >> >> On 7/15/24 10:46, Clément Chigot wrote: >>> The BookE decrementer stops at 0, meaning that it won't decremented >>> towards "negative" values. >>> However, the current logic is inverted: decr is updated solely when >>> the resulting value would be negative. >> >> How did you hit the issue ? which machine ? I didn't see any error >> when booting Linux 6.6.3 on mpc8544ds, e500mc, e5500 and e6500. > > I hit this issue while running some version of VxWorks on a custom > machine: p3041ds (description [1] and our local implementation [2]). > So, I'm not that surprised you were not able to reproduce. > >>> Signed-off-by: Clément Chigot <chigot@adacore.com> >>> Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors") >> >> LGTM, >> >> Reviewed-by: Cédric Le Goater <clg@redhat.com> >> >> We have some automated tests with the ppce500 machine which it would be >> interesting to extend to have a better coverage of booke. > > Thanks for the pointer, I'll see if I can extend them. > >> Thanks, >> >> C. >> > > [1] https://www.nxp.com/design/design-center/software/qoriq-developer-resources/p3041-qoriq-development-system:P3041DS > [2] https://github.com/AdaCore/qemu/blob/qemu-stable-9.0.0/hw/ppc/p3041ds.c Nice. Do you have any plans to upstream the machine ? Thanks, C.
On Thu, Aug 29, 2024 at 2:33 PM Cédric Le Goater <clg@kaod.org> wrote: > > On 8/28/24 09:21, Clément Chigot wrote: > > On Tue, Aug 27, 2024 at 7:40 PM Cédric Le Goater <clg@kaod.org> wrote: > >> > >> Hello Clément, > >> > >> On 7/15/24 10:46, Clément Chigot wrote: > >>> The BookE decrementer stops at 0, meaning that it won't decremented > >>> towards "negative" values. > >>> However, the current logic is inverted: decr is updated solely when > >>> the resulting value would be negative. > >> > >> How did you hit the issue ? which machine ? I didn't see any error > >> when booting Linux 6.6.3 on mpc8544ds, e500mc, e5500 and e6500. > > > > I hit this issue while running some version of VxWorks on a custom > > machine: p3041ds (description [1] and our local implementation [2]). > > So, I'm not that surprised you were not able to reproduce. > > > >>> Signed-off-by: Clément Chigot <chigot@adacore.com> > >>> Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors") > >> > >> LGTM, > >> > >> Reviewed-by: Cédric Le Goater <clg@redhat.com> > >> > >> We have some automated tests with the ppce500 machine which it would be > >> interesting to extend to have a better coverage of booke. > > > > Thanks for the pointer, I'll see if I can extend them. > > > >> Thanks, > >> > >> C. > >> > > > > [1] https://www.nxp.com/design/design-center/software/qoriq-developer-resources/p3041-qoriq-development-system:P3041DS > > [2] https://github.com/AdaCore/qemu/blob/qemu-stable-9.0.0/hw/ppc/p3041ds.c > > Nice. Do you have any plans to upstream the machine ? That was the plan. However, we are using that emulation in order to test VxWorks on PPC. Recent versions have now officially provided a BSP for QEmu using ppce500 board. We are planning to transition to it, mainly to avoid having to maintain that P3041DS home-made implementation. Thus, I'm not sure we'll spend time on upstreaming it :( That being said, if you heard people being interested in that board, please do warn me. Thanks, Clément > Thanks, > > C. >
Hi Cédric, On Tue, Aug 27, 2024 at 7:40 PM Cédric Le Goater <clg@kaod.org> wrote: > > Hello Clément, > > On 7/15/24 10:46, Clément Chigot wrote: > > The BookE decrementer stops at 0, meaning that it won't decremented > > towards "negative" values. > > However, the current logic is inverted: decr is updated solely when > > the resulting value would be negative. > > How did you hit the issue ? which machine ? I didn't see any error > when booting Linux 6.6.3 on mpc8544ds, e500mc, e5500 and e6500. > > > Signed-off-by: Clément Chigot <chigot@adacore.com> > > Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors") > > LGTM, > > Reviewed-by: Cédric Le Goater <clg@redhat.com> Unless I'm wrong this patch has not been queued yet. Is there any reason for this ? I just want to make sure it hasn't been forgotten. Thanks, Clément > We have some automated tests with the ppce500 machine which it would be > interesting to extend to have a better coverage of booke. > > Thanks, > > C. > > > > > --- > > hw/ppc/ppc.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > > index e6fa5580c0..9fc85c7de0 100644 > > --- a/hw/ppc/ppc.c > > +++ b/hw/ppc/ppc.c > > @@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState *env, int64_t now, > > int64_t decr; > > > > n = ns_to_tb(tb_env->decr_freq, now); > > - if (next > n && tb_env->flags & PPC_TIMER_BOOKE) { > > + > > + /* BookE timers stop when reaching 0. */ > > + if (next < n && tb_env->flags & PPC_TIMER_BOOKE) { > > decr = 0; > > } else { > > decr = next - n; > >
Hello Clément, > Unless I'm wrong this patch has not been queued yet. Is there any > reason for this ? I don't think there was a PPC PR yet. We are just starting the QEMU 9.2 cycle [*]. Since this is a fix that applies on older releases, may be we could have a PR in not too long. Thanks, C. [*] https://wiki.qemu.org/Planning/9.2
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c index e6fa5580c0..9fc85c7de0 100644 --- a/hw/ppc/ppc.c +++ b/hw/ppc/ppc.c @@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState *env, int64_t now, int64_t decr; n = ns_to_tb(tb_env->decr_freq, now); - if (next > n && tb_env->flags & PPC_TIMER_BOOKE) { + + /* BookE timers stop when reaching 0. */ + if (next < n && tb_env->flags & PPC_TIMER_BOOKE) { decr = 0; } else { decr = next - n;
The BookE decrementer stops at 0, meaning that it won't decremented towards "negative" values. However, the current logic is inverted: decr is updated solely when the resulting value would be negative. Signed-off-by: Clément Chigot <chigot@adacore.com> Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors") --- hw/ppc/ppc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)