* [PATCH] target/openrisc: fix icount handling for timer instructions
@ 2021-03-29 7:42 Pavel Dovgalyuk
2021-03-30 22:05 ` Stafford Horne
0 siblings, 1 reply; 9+ messages in thread
From: Pavel Dovgalyuk @ 2021-03-29 7:42 UTC (permalink / raw)
To: qemu-devel; +Cc: shorne, pavel.dovgalyuk, proljc
This patch adds icount handling to mfspr/mtspr instructions
that may deal with hardware timers.
Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
---
target/openrisc/translate.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
index c6dce879f1..a9c81f8bd5 100644
--- a/target/openrisc/translate.c
+++ b/target/openrisc/translate.c
@@ -884,6 +884,18 @@ static bool trans_l_mfspr(DisasContext *dc, arg_l_mfspr *a)
gen_illegal_exception(dc);
} else {
TCGv spr = tcg_temp_new();
+
+ if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
+ gen_io_start();
+ if (dc->delayed_branch) {
+ tcg_gen_mov_tl(cpu_pc, jmp_pc);
+ tcg_gen_discard_tl(jmp_pc);
+ } else {
+ tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
+ }
+ dc->base.is_jmp = DISAS_EXIT;
+ }
+
tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
gen_helper_mfspr(cpu_R(dc, a->d), cpu_env, cpu_R(dc, a->d), spr);
tcg_temp_free(spr);
@@ -898,6 +910,9 @@ static bool trans_l_mtspr(DisasContext *dc, arg_l_mtspr *a)
} else {
TCGv spr;
+ if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
+ gen_io_start();
+ }
/* For SR, we will need to exit the TB to recognize the new
* exception state. For NPC, in theory this counts as a branch
* (although the SPR only exists for use by an ICE). Save all
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] target/openrisc: fix icount handling for timer instructions
2021-03-29 7:42 [PATCH] target/openrisc: fix icount handling for timer instructions Pavel Dovgalyuk
@ 2021-03-30 22:05 ` Stafford Horne
2021-03-31 7:27 ` Pavel Dovgalyuk
0 siblings, 1 reply; 9+ messages in thread
From: Stafford Horne @ 2021-03-30 22:05 UTC (permalink / raw)
To: Pavel Dovgalyuk; +Cc: qemu-devel, proljc
Hi Pavel,
Thanks for the patch.
On Mon, Mar 29, 2021 at 10:42:41AM +0300, Pavel Dovgalyuk wrote:
> This patch adds icount handling to mfspr/mtspr instructions
> that may deal with hardware timers.
>
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> ---
> target/openrisc/translate.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
> index c6dce879f1..a9c81f8bd5 100644
> --- a/target/openrisc/translate.c
> +++ b/target/openrisc/translate.c
> @@ -884,6 +884,18 @@ static bool trans_l_mfspr(DisasContext *dc, arg_l_mfspr *a)
> gen_illegal_exception(dc);
> } else {
> TCGv spr = tcg_temp_new();
> +
> + if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> + gen_io_start();
> + if (dc->delayed_branch) {
> + tcg_gen_mov_tl(cpu_pc, jmp_pc);
> + tcg_gen_discard_tl(jmp_pc);
> + } else {
> + tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
> + }
> + dc->base.is_jmp = DISAS_EXIT;
> + }
I don't know alot about how the icount works. But I read this document to help
understand this patch.
https://qemu.readthedocs.io/en/latest/devel/tcg-icount.html
Could you explain why we need to exit the tb on mfspr? This may just be reading
a timer value, but I am not sure why we need it?
> tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
> gen_helper_mfspr(cpu_R(dc, a->d), cpu_env, cpu_R(dc, a->d), spr);
> tcg_temp_free(spr);
> @@ -898,6 +910,9 @@ static bool trans_l_mtspr(DisasContext *dc, arg_l_mtspr *a)
> } else {
> TCGv spr;
>
> + if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> + gen_io_start();
> + }
Here and above, why do we need to call gen_io_start()? This seems to need to be
called before io operations.
This may all be OK, but could you help explain the theory of operation? Also,
have you tested this?
-Stafford
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] target/openrisc: fix icount handling for timer instructions
2021-03-30 22:05 ` Stafford Horne
@ 2021-03-31 7:27 ` Pavel Dovgalyuk
2021-03-31 12:33 ` Stafford Horne
0 siblings, 1 reply; 9+ messages in thread
From: Pavel Dovgalyuk @ 2021-03-31 7:27 UTC (permalink / raw)
To: Stafford Horne; +Cc: qemu-devel, proljc
On 31.03.2021 01:05, Stafford Horne wrote:
> Hi Pavel,
>
> Thanks for the patch.
>
> On Mon, Mar 29, 2021 at 10:42:41AM +0300, Pavel Dovgalyuk wrote:
>> This patch adds icount handling to mfspr/mtspr instructions
>> that may deal with hardware timers.
>>
>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
>> ---
>> target/openrisc/translate.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
>> index c6dce879f1..a9c81f8bd5 100644
>> --- a/target/openrisc/translate.c
>> +++ b/target/openrisc/translate.c
>> @@ -884,6 +884,18 @@ static bool trans_l_mfspr(DisasContext *dc, arg_l_mfspr *a)
>> gen_illegal_exception(dc);
>> } else {
>> TCGv spr = tcg_temp_new();
>> +
>> + if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
>> + gen_io_start();
>> + if (dc->delayed_branch) {
>> + tcg_gen_mov_tl(cpu_pc, jmp_pc);
>> + tcg_gen_discard_tl(jmp_pc);
>> + } else {
>> + tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
>> + }
>> + dc->base.is_jmp = DISAS_EXIT;
>> + }
>
> I don't know alot about how the icount works. But I read this document to help
> understand this patch.
>
> https://qemu.readthedocs.io/en/latest/devel/tcg-icount.html
>
> Could you explain why we need to exit the tb on mfspr? This may just be reading
> a timer value, but I am not sure why we need it?
Because virtual clock in icount mode is correct only at the end of the
block.
Allowing virtual clock reads in other places will make execution
non-deterministic, because icount is updated to the value, which it gets
after the block ends.
>
>> tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
>> gen_helper_mfspr(cpu_R(dc, a->d), cpu_env, cpu_R(dc, a->d), spr);
>> tcg_temp_free(spr);
>> @@ -898,6 +910,9 @@ static bool trans_l_mtspr(DisasContext *dc, arg_l_mtspr *a)
>> } else {
>> TCGv spr;
>>
>> + if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
>> + gen_io_start();
>> + }
>
> Here and above, why do we need to call gen_io_start()? This seems to need to be
> called before io operations.
gen_io_start allows reading icount for the instruction.
It is needed to prevent invalid reads in the middle of the block.
>
> This may all be OK, but could you help explain the theory of operation? Also,
> have you tested this?
I have record/replay tests for openrisc, but I can't submit them without
this patch, because they will fail.
Pavel Dovgalyuk
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] target/openrisc: fix icount handling for timer instructions
2021-03-31 7:27 ` Pavel Dovgalyuk
@ 2021-03-31 12:33 ` Stafford Horne
2021-03-31 12:48 ` Pavel Dovgalyuk
0 siblings, 1 reply; 9+ messages in thread
From: Stafford Horne @ 2021-03-31 12:33 UTC (permalink / raw)
To: Pavel Dovgalyuk; +Cc: qemu-devel, proljc
On Wed, Mar 31, 2021 at 10:27:21AM +0300, Pavel Dovgalyuk wrote:
> On 31.03.2021 01:05, Stafford Horne wrote:
> > Hi Pavel,
> >
> > Thanks for the patch.
> >
> > On Mon, Mar 29, 2021 at 10:42:41AM +0300, Pavel Dovgalyuk wrote:
> > > This patch adds icount handling to mfspr/mtspr instructions
> > > that may deal with hardware timers.
> > >
> > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> > > ---
> > > target/openrisc/translate.c | 15 +++++++++++++++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
> > > index c6dce879f1..a9c81f8bd5 100644
> > > --- a/target/openrisc/translate.c
> > > +++ b/target/openrisc/translate.c
> > > @@ -884,6 +884,18 @@ static bool trans_l_mfspr(DisasContext *dc, arg_l_mfspr *a)
> > > gen_illegal_exception(dc);
> > > } else {
> > > TCGv spr = tcg_temp_new();
> > > +
> > > + if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> > > + gen_io_start();
> > > + if (dc->delayed_branch) {
> > > + tcg_gen_mov_tl(cpu_pc, jmp_pc);
> > > + tcg_gen_discard_tl(jmp_pc);
> > > + } else {
> > > + tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
> > > + }
> > > + dc->base.is_jmp = DISAS_EXIT;
> > > + }
> >
> > I don't know alot about how the icount works. But I read this document to help
> > understand this patch.
> >
> > https://qemu.readthedocs.io/en/latest/devel/tcg-icount.html
> >
> > Could you explain why we need to exit the tb on mfspr? This may just be reading
> > a timer value, but I am not sure why we need it?
>
> Because virtual clock in icount mode is correct only at the end of the
> block.
> Allowing virtual clock reads in other places will make execution
> non-deterministic, because icount is updated to the value, which it gets
> after the block ends.
OK, got it.
> >
> > > tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
> > > gen_helper_mfspr(cpu_R(dc, a->d), cpu_env, cpu_R(dc, a->d), spr);
> > > tcg_temp_free(spr);
> > > @@ -898,6 +910,9 @@ static bool trans_l_mtspr(DisasContext *dc, arg_l_mtspr *a)
> > > } else {
> > > TCGv spr;
> > > + if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> > > + gen_io_start();
> > > + }
> >
> > Here and above, why do we need to call gen_io_start()? This seems to need to be
> > called before io operations.
>
> gen_io_start allows reading icount for the instruction.
> It is needed to prevent invalid reads in the middle of the block.
>
> >
> > This may all be OK, but could you help explain the theory of operation? Also,
> > have you tested this?
>
> I have record/replay tests for openrisc, but I can't submit them without
> this patch, because they will fail.
OK.
Acked-by: Stafford Horne <shorne@gmail.com>
I am not currently maintaining an openrisc queue, but I could start one. Do you
have another way to submit this upstream?
-Stafford
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] target/openrisc: fix icount handling for timer instructions
2021-03-31 12:33 ` Stafford Horne
@ 2021-03-31 12:48 ` Pavel Dovgalyuk
2021-03-31 14:30 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Pavel Dovgalyuk @ 2021-03-31 12:48 UTC (permalink / raw)
To: Stafford Horne, Paolo Bonzini; +Cc: qemu-devel, proljc
CC'ed Paolo.
On 31.03.2021 15:33, Stafford Horne wrote:
> On Wed, Mar 31, 2021 at 10:27:21AM +0300, Pavel Dovgalyuk wrote:
>> On 31.03.2021 01:05, Stafford Horne wrote:
>>> Hi Pavel,
>>>
>>> Thanks for the patch.
>>>
>>> On Mon, Mar 29, 2021 at 10:42:41AM +0300, Pavel Dovgalyuk wrote:
>>>> This patch adds icount handling to mfspr/mtspr instructions
>>>> that may deal with hardware timers.
>>>>
>>>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
>>>> ---
>>>> target/openrisc/translate.c | 15 +++++++++++++++
>>>> 1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
>>>> index c6dce879f1..a9c81f8bd5 100644
>>>> --- a/target/openrisc/translate.c
>>>> +++ b/target/openrisc/translate.c
>>>> @@ -884,6 +884,18 @@ static bool trans_l_mfspr(DisasContext *dc, arg_l_mfspr *a)
>>>> gen_illegal_exception(dc);
>>>> } else {
>>>> TCGv spr = tcg_temp_new();
>>>> +
>>>> + if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
>>>> + gen_io_start();
>>>> + if (dc->delayed_branch) {
>>>> + tcg_gen_mov_tl(cpu_pc, jmp_pc);
>>>> + tcg_gen_discard_tl(jmp_pc);
>>>> + } else {
>>>> + tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
>>>> + }
>>>> + dc->base.is_jmp = DISAS_EXIT;
>>>> + }
>>>
>>> I don't know alot about how the icount works. But I read this document to help
>>> understand this patch.
>>>
>>> https://qemu.readthedocs.io/en/latest/devel/tcg-icount.html
>>>
>>> Could you explain why we need to exit the tb on mfspr? This may just be reading
>>> a timer value, but I am not sure why we need it?
>>
>> Because virtual clock in icount mode is correct only at the end of the
>> block.
>> Allowing virtual clock reads in other places will make execution
>> non-deterministic, because icount is updated to the value, which it gets
>> after the block ends.
>
> OK, got it.
>
>>>
>>>> tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
>>>> gen_helper_mfspr(cpu_R(dc, a->d), cpu_env, cpu_R(dc, a->d), spr);
>>>> tcg_temp_free(spr);
>>>> @@ -898,6 +910,9 @@ static bool trans_l_mtspr(DisasContext *dc, arg_l_mtspr *a)
>>>> } else {
>>>> TCGv spr;
>>>> + if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
>>>> + gen_io_start();
>>>> + }
>>>
>>> Here and above, why do we need to call gen_io_start()? This seems to need to be
>>> called before io operations.
>>
>> gen_io_start allows reading icount for the instruction.
>> It is needed to prevent invalid reads in the middle of the block.
>>
>>>
>>> This may all be OK, but could you help explain the theory of operation? Also,
>>> have you tested this?
>>
>> I have record/replay tests for openrisc, but I can't submit them without
>> this patch, because they will fail.
>
> OK.
>
> Acked-by: Stafford Horne <shorne@gmail.com>
>
> I am not currently maintaining an openrisc queue, but I could start one. Do you
> have another way to submit this upstream?
Paolo, can you queue this one?
Pavel Dovgalyuk
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] target/openrisc: fix icount handling for timer instructions
2021-03-31 12:48 ` Pavel Dovgalyuk
@ 2021-03-31 14:30 ` Paolo Bonzini
0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2021-03-31 14:30 UTC (permalink / raw)
To: Pavel Dovgalyuk, Stafford Horne; +Cc: qemu-devel, proljc
On 31/03/21 14:48, Pavel Dovgalyuk wrote:
>> Acked-by: Stafford Horne <shorne@gmail.com>
>>
>> I am not currently maintaining an openrisc queue, but I could start
>> one. Do you
>> have another way to submit this upstream?
>
> Paolo, can you queue this one?
Sure, done.
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] target/openrisc: fix icount handling for timer instructions
@ 2020-11-05 11:54 Pavel Dovgalyuk
2020-11-05 21:39 ` Richard Henderson
0 siblings, 1 reply; 9+ messages in thread
From: Pavel Dovgalyuk @ 2020-11-05 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: shorne, pavel.dovgalyuk, proljc
This patch adds icount handling to mfspr/mtspr instructions
that may deal with hardware timers.
Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
---
target/openrisc/translate.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
index c6dce879f1..a9c81f8bd5 100644
--- a/target/openrisc/translate.c
+++ b/target/openrisc/translate.c
@@ -884,6 +884,18 @@ static bool trans_l_mfspr(DisasContext *dc, arg_l_mfspr *a)
gen_illegal_exception(dc);
} else {
TCGv spr = tcg_temp_new();
+
+ if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
+ gen_io_start();
+ if (dc->delayed_branch) {
+ tcg_gen_mov_tl(cpu_pc, jmp_pc);
+ tcg_gen_discard_tl(jmp_pc);
+ } else {
+ tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
+ }
+ dc->base.is_jmp = DISAS_EXIT;
+ }
+
tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
gen_helper_mfspr(cpu_R(dc, a->d), cpu_env, cpu_R(dc, a->d), spr);
tcg_temp_free(spr);
@@ -898,6 +910,9 @@ static bool trans_l_mtspr(DisasContext *dc, arg_l_mtspr *a)
} else {
TCGv spr;
+ if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
+ gen_io_start();
+ }
/* For SR, we will need to exit the TB to recognize the new
* exception state. For NPC, in theory this counts as a branch
* (although the SPR only exists for use by an ICE). Save all
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] target/openrisc: fix icount handling for timer instructions
2020-11-05 11:54 Pavel Dovgalyuk
@ 2020-11-05 21:39 ` Richard Henderson
2020-11-06 6:36 ` Pavel Dovgalyuk
0 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2020-11-05 21:39 UTC (permalink / raw)
To: Pavel Dovgalyuk, qemu-devel; +Cc: shorne, proljc
On 11/5/20 3:54 AM, Pavel Dovgalyuk wrote:
> This patch adds icount handling to mfspr/mtspr instructions
> that may deal with hardware timers.
>
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> ---
> target/openrisc/translate.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
Looks correct, but it would be better not to duplicate the code from
trans_l_mtspr, and use an is_jmp code (called DISAS_UPDATE_EXIT in some other
targets).
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] target/openrisc: fix icount handling for timer instructions
2020-11-05 21:39 ` Richard Henderson
@ 2020-11-06 6:36 ` Pavel Dovgalyuk
0 siblings, 0 replies; 9+ messages in thread
From: Pavel Dovgalyuk @ 2020-11-06 6:36 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: shorne, proljc
On 06.11.2020 00:39, Richard Henderson wrote:
> On 11/5/20 3:54 AM, Pavel Dovgalyuk wrote:
>> This patch adds icount handling to mfspr/mtspr instructions
>> that may deal with hardware timers.
>>
>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
>> ---
>> target/openrisc/translate.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>
> Looks correct, but it would be better not to duplicate the code from
> trans_l_mtspr, and use an is_jmp code (called DISAS_UPDATE_EXIT in some other
> targets).
mtspr includes the following comment:
* Save all of the cpu state first, allowing it to be overwritten.
Does it mean, that helper can overwrite the PC? Then the PC can't be
updated in is_jmp handler at the end of instruction translation.
Pavel Dovgalyuk
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-31 14:31 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-29 7:42 [PATCH] target/openrisc: fix icount handling for timer instructions Pavel Dovgalyuk
2021-03-30 22:05 ` Stafford Horne
2021-03-31 7:27 ` Pavel Dovgalyuk
2021-03-31 12:33 ` Stafford Horne
2021-03-31 12:48 ` Pavel Dovgalyuk
2021-03-31 14:30 ` Paolo Bonzini
-- strict thread matches above, loose matches on Subject: below --
2020-11-05 11:54 Pavel Dovgalyuk
2020-11-05 21:39 ` Richard Henderson
2020-11-06 6:36 ` Pavel Dovgalyuk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.