| Message ID | 20230401045106.3885562-3-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | last minute tcg fixes | expand |
On 1/4/23 06:51, Richard Henderson wrote: > From: Weiwei Li <liweiwei@iscas.ac.cn> > > CPUs often set CF_PCREL in tcg_cflags before qemu_init_vcpu(), in which > tcg_cflags will be overwrited by tcg_cpu_init_cflags(). The description makes sense, but I couldn't reproduce using: -- >8 -- diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c index af35e0d092..23ebf7de21 100644 --- a/accel/tcg/tcg-accel-ops.c +++ b/accel/tcg/tcg-accel-ops.c @@ -59,6 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel) cflags |= parallel ? CF_PARALLEL : 0; cflags |= icount_enabled() ? CF_USE_ICOUNT : 0; + tcg_debug_assert(!cpu->tcg_cflags); cpu->tcg_cflags = cflags; } --- Li and Junqiang, what is your use case? > Fixes: 4be790263ffc ("accel/tcg: Replace `TARGET_TB_PCREL` with `CF_PCREL`") > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > Message-Id: <20230331150609.114401-6-liweiwei@iscas.ac.cn> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > accel/tcg/tcg-accel-ops.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c > index af35e0d092..58c8e64096 100644 > --- a/accel/tcg/tcg-accel-ops.c > +++ b/accel/tcg/tcg-accel-ops.c > @@ -59,7 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel) > > cflags |= parallel ? CF_PARALLEL : 0; > cflags |= icount_enabled() ? CF_USE_ICOUNT : 0; > - cpu->tcg_cflags = cflags; > + cpu->tcg_cflags |= cflags; This could be acceptable as a release kludge, but semantically tcg_cpu_init_cflags() is meant to *initialize* all flags, not set few of them. Either we aren't calling it from the proper place, or we need to rename it. > } > > void tcg_cpus_destroy(CPUState *cpu)
On 2023/4/3 16:09, Philippe Mathieu-Daudé wrote: > On 1/4/23 06:51, Richard Henderson wrote: >> From: Weiwei Li <liweiwei@iscas.ac.cn> >> >> CPUs often set CF_PCREL in tcg_cflags before qemu_init_vcpu(), in which >> tcg_cflags will be overwrited by tcg_cpu_init_cflags(). > > The description makes sense, but I couldn't reproduce using: > > -- >8 -- > diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c > index af35e0d092..23ebf7de21 100644 > --- a/accel/tcg/tcg-accel-ops.c > +++ b/accel/tcg/tcg-accel-ops.c > @@ -59,6 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel) > > cflags |= parallel ? CF_PARALLEL : 0; > cflags |= icount_enabled() ? CF_USE_ICOUNT : 0; > + tcg_debug_assert(!cpu->tcg_cflags); > cpu->tcg_cflags = cflags; > } > --- > > Li and Junqiang, what is your use case? Only few CPUs support CF_PCREL currently. I found this problem when I tried to introduce PC-relative translation into RISC-V. Maybe It also can be reproduced in system mode for ARM and X86. Regards, Weiwei Li > >> Fixes: 4be790263ffc ("accel/tcg: Replace `TARGET_TB_PCREL` with >> `CF_PCREL`") >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >> Message-Id: <20230331150609.114401-6-liweiwei@iscas.ac.cn> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> accel/tcg/tcg-accel-ops.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c >> index af35e0d092..58c8e64096 100644 >> --- a/accel/tcg/tcg-accel-ops.c >> +++ b/accel/tcg/tcg-accel-ops.c >> @@ -59,7 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel) >> cflags |= parallel ? CF_PARALLEL : 0; >> cflags |= icount_enabled() ? CF_USE_ICOUNT : 0; >> - cpu->tcg_cflags = cflags; >> + cpu->tcg_cflags |= cflags; > > This could be acceptable as a release kludge, but semantically > tcg_cpu_init_cflags() is meant to *initialize* all flags, not > set few of them. Either we aren't calling it from the proper place, > or we need to rename it. > >> } >> void tcg_cpus_destroy(CPUState *cpu)
On 4/3/23 11:09, liweiwei wrote: > > On 2023/4/3 16:09, Philippe Mathieu-Daudé wrote: >> cflags |= parallel ? CF_PARALLEL : 0; >> cflags |= icount_enabled() ? CF_USE_ICOUNT : 0; >> + tcg_debug_assert(!cpu->tcg_cflags); >> cpu->tcg_cflags = cflags; >> } >> --- >> >> Li and Junqiang, what is your use case? > > Only few CPUs support CF_PCREL currently. I found this problem when I > tried to introduce PC-relative translation into RISC-V. > > Maybe It also can be reproduced in system mode for ARM and X86. Yes, this can be reproduced on arm-softmmu with --enable-debug-tcg and the above assertion. On 4/3/23 10:09, Philippe Mathieu-Daudé wrote: >> >> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c >> index af35e0d092..58c8e64096 100644 >> --- a/accel/tcg/tcg-accel-ops.c >> +++ b/accel/tcg/tcg-accel-ops.c >> @@ -59,7 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel) >> cflags |= parallel ? CF_PARALLEL : 0; >> cflags |= icount_enabled() ? CF_USE_ICOUNT : 0; >> - cpu->tcg_cflags = cflags; >> + cpu->tcg_cflags |= cflags; > > This could be acceptable as a release kludge, but semantically > tcg_cpu_init_cflags() is meant to *initialize* all flags, not > set few of them. Either we aren't calling it from the proper place, > or we need to rename it. Agree, this sounds reasonable. Also, maybe setting cpu->tcg_cflags from *_cpu_realizefn was not the right call and we should have some other way of providing target specific cflags.
On 3/4/23 12:44, Anton Johansson wrote: > > On 4/3/23 11:09, liweiwei wrote: >> >> On 2023/4/3 16:09, Philippe Mathieu-Daudé wrote: >>> cflags |= parallel ? CF_PARALLEL : 0; >>> cflags |= icount_enabled() ? CF_USE_ICOUNT : 0; >>> + tcg_debug_assert(!cpu->tcg_cflags); >>> cpu->tcg_cflags = cflags; >>> } >>> --- >>> >>> Li and Junqiang, what is your use case? >> >> Only few CPUs support CF_PCREL currently. I found this problem when I >> tried to introduce PC-relative translation into RISC-V. >> >> Maybe It also can be reproduced in system mode for ARM and X86. > > Yes, this can be reproduced on arm-softmmu with --enable-debug-tcg and > the above assertion. Ah OK. Then... Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c index af35e0d092..58c8e64096 100644 --- a/accel/tcg/tcg-accel-ops.c +++ b/accel/tcg/tcg-accel-ops.c @@ -59,7 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel) cflags |= parallel ? CF_PARALLEL : 0; cflags |= icount_enabled() ? CF_USE_ICOUNT : 0; - cpu->tcg_cflags = cflags; + cpu->tcg_cflags |= cflags; } void tcg_cpus_destroy(CPUState *cpu)