what are the requirements on target/ code for -icount to work correctly?

what are the requirements on target/ code for -icount to work correctly?

[Peter Maydell]

For -icount mode to work, there are requirements on the target/
code (notably around marking up "I/O" instructions). Unfortunately
we've never documented what these are, which makes it pretty rough
for people writing new targets or reviewing changes to existing ones.
Does anybody understand what they actually are?

Some more specific questions on the general theme:

Q1: the comment on gen_io_end() says:
/*
 * cpu->can_do_io is cleared automatically at the beginning of
 * each translation block.  The cost is minimal and only paid
 * for -icount, plus it would be very easy to forget doing it
 * in the translator.  Therefore, backends only need to call
 * gen_io_start.
 */
but in fact multiple backends *do* call gen_io_end(). When
does a backend have to call this, and when not? Or are those
all legacy useless calls we should delete? (If so, can we
just get rid of this function entirely ?)

Q2: is it a requirement that after an insn which is a "known
to be an I/O insn" one (like x86 in/out) and which is marked
up with gen_io_start()/gen_io_end() that we also end the TB?
Or is it OK to generate more insns after that one? If the former,
is there somewhere we can assert() that this is done ?

Q3: why does gen_tb_start() call gen_io_end()? This is the
*start* of the TB so by definition we haven't started doing
any IO yet...

thanks
-- PMM

Re: what are the requirements on target/ code for -icount to work correctly?

[Pavel Dovgalyuk]

On 18.06.2020 20:38, Peter Maydell wrote:
> For -icount mode to work, there are requirements on the target/
> code (notably around marking up "I/O" instructions). Unfortunately
> we've never documented what these are, which makes it pretty rough
> for people writing new targets or reviewing changes to existing ones.
> Does anybody understand what they actually are?
>
> Some more specific questions on the general theme:
>
> Q1: the comment on gen_io_end() says:
> /*
>   * cpu->can_do_io is cleared automatically at the beginning of
>   * each translation block.  The cost is minimal and only paid
>   * for -icount, plus it would be very easy to forget doing it
>   * in the translator.  Therefore, backends only need to call
>   * gen_io_start.
>   */
> but in fact multiple backends *do* call gen_io_end(). When
> does a backend have to call this, and when not? Or are those
> all legacy useless calls we should delete? (If so, can we
> just get rid of this function entirely ?)

That was my refactoring patch for removing gen_io_end calls.

But in some cases I wasn't sure that translation is stopped after that. 
In such cases gen_io_end wasn't removed.

I think, that we need some efforts from target maintainers to remove all 
such calls.

> Q2: is it a requirement that after an insn which is a "known
> to be an I/O insn" one (like x86 in/out) and which is marked
> up with gen_io_start()/gen_io_end() that we also end the TB?

It is a requirement for instructions that access virtual clock/icount 
value (directly or not).

There is also an assertion that can_do_io is enabled while generating an 
interrupt. I believe, that it doesn't affect RR, but is useful for 
deterministic icount mode.

> Or is it OK to generate more insns after that one? If the former,
> is there somewhere we can assert() that this is done ?

Sounds reasonable.

> Q3: why does gen_tb_start() call gen_io_end()? This is the
> *start* of the TB so by definition we haven't started doing
> any IO yet...

This is an artifact of gen_io_end refactoring.


Pavel Dovgalyuk

Re: what are the requirements on target/ code for -icount to work correctly?

[Paolo Bonzini]

On 19/06/20 07:46, Pavel Dovgalyuk wrote:
> I think, that we need some efforts from target maintainers to remove all such calls. 

I'll take care of target/i386 (which does need one of the three
gen_io_end calls that are left).

>> Q2: is it a requirement that after an insn which is a "known
>> to be an I/O insn" one (like x86 in/out) and which is marked
>> up with gen_io_start()/gen_io_end() that we also end the TB?
> 
> It is a requirement for instructions that access virtual clock/icount
> value (directly or not).
> 
> There is also an assertion that can_do_io is enabled while generating an
> interrupt. I believe, that it doesn't affect RR, but is useful for
> deterministic icount mode.

As I understand it, the definition of "I/O insn" is anything that can
either:

- affect the icount deadline (e.g. by setting or removing a
QEMU_CLOCK_VIRTUAL timer)

- interrupt the current translation block with cpu_loop_exit,
cpu_restore_state or similar.

Paolo

Re: what are the requirements on target/ code for -icount to work correctly?

[Peter Maydell]

On Fri, 19 Jun 2020 at 12:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 19/06/20 07:46, Pavel Dovgalyuk wrote:
> > I think, that we need some efforts from target maintainers to remove all such calls.
>
> I'll take care of target/i386 (which does need one of the three
> gen_io_end calls that are left).

So why does it need it ? Why can't it just rely on "TB going to
end anyway which will clear the can_do_io flag" ?

> >> Q2: is it a requirement that after an insn which is a "known
> >> to be an I/O insn" one (like x86 in/out) and which is marked
> >> up with gen_io_start()/gen_io_end() that we also end the TB?
> >
> > It is a requirement for instructions that access virtual clock/icount
> > value (directly or not).
> >
> > There is also an assertion that can_do_io is enabled while generating an
> > interrupt. I believe, that it doesn't affect RR, but is useful for
> > deterministic icount mode.
>
> As I understand it, the definition of "I/O insn" is anything that can
> either:
>
> - affect the icount deadline (e.g. by setting or removing a
> QEMU_CLOCK_VIRTUAL timer)
>
> - interrupt the current translation block with cpu_loop_exit,
> cpu_restore_state or similar.

Right, but really what I'm interested in is what the
requirements are on translate.c code that emits one of these
insns. The exact definition of what an I/O insn seems
more straightforward (and you can always err on the safe side).

thanks
-- PMM

Re: what are the requirements on target/ code for -icount to work correctly?

[Paolo Bonzini]

On 19/06/20 14:18, Peter Maydell wrote:
> On Fri, 19 Jun 2020 at 12:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 19/06/20 07:46, Pavel Dovgalyuk wrote:
>>> I think, that we need some efforts from target maintainers to remove all such calls.
>>
>> I'll take care of target/i386 (which does need one of the three
>> gen_io_end calls that are left).
> 
> So why does it need it ? Why can't it just rely on "TB going to
> end anyway which will clear the can_do_io flag" ?

Because the TB is not always going to end in that case that is left.

>>>> Q2: is it a requirement that after an insn which is a "known
>>>> to be an I/O insn" one (like x86 in/out) and which is marked
>>>> up with gen_io_start()/gen_io_end() that we also end the TB?
>>>
>>> It is a requirement for instructions that access virtual clock/icount
>>> value (directly or not).
>>>
>>> There is also an assertion that can_do_io is enabled while generating an
>>> interrupt. I believe, that it doesn't affect RR, but is useful for
>>> deterministic icount mode.
>>
>> As I understand it, the definition of "I/O insn" is anything that can
>> either:
>>
>> - affect the icount deadline (e.g. by setting or removing a
>> QEMU_CLOCK_VIRTUAL timer)
>>
>> - interrupt the current translation block with cpu_loop_exit,
>> cpu_restore_state or similar.
> 
> Right, but really what I'm interested in is what the
> requirements are on translate.c code that emits one of these
> insns.

I would be interested in a precise definition of that as well (I've not
really done any icount work on the translation side).

Paolo

> The exact definition of what an I/O insn seems
> more straightforward (and you can always err on the safe side).
> 
> thanks
> -- PMM
> 

Re: what are the requirements on target/ code for -icount to work correctly?

[Peter Maydell]

On Fri, 19 Jun 2020 at 13:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 19/06/20 14:18, Peter Maydell wrote:
> > On Fri, 19 Jun 2020 at 12:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >> On 19/06/20 07:46, Pavel Dovgalyuk wrote:
> >>> I think, that we need some efforts from target maintainers to remove all such calls.
> >>
> >> I'll take care of target/i386 (which does need one of the three
> >> gen_io_end calls that are left).
> >
> > So why does it need it ? Why can't it just rely on "TB going to
> > end anyway which will clear the can_do_io flag" ?
>
> Because the TB is not always going to end in that case that is left.

OK, so when is it valid not to end the TB after an IO instruction ?
My initial belief was that the TB should *always* end.

thanks
-- PMM

Re: what are the requirements on target/ code for -icount to work correctly?

[Paolo Bonzini]

On 19/06/20 14:39, Peter Maydell wrote:
> On Fri, 19 Jun 2020 at 13:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 19/06/20 14:18, Peter Maydell wrote:
>>> On Fri, 19 Jun 2020 at 12:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>> On 19/06/20 07:46, Pavel Dovgalyuk wrote:
>>>>> I think, that we need some efforts from target maintainers to remove all such calls.
>>>>
>>>> I'll take care of target/i386 (which does need one of the three
>>>> gen_io_end calls that are left).
>>>
>>> So why does it need it ? Why can't it just rely on "TB going to
>>> end anyway which will clear the can_do_io flag" ?
>>
>> Because the TB is not always going to end in that case that is left.
> 
> OK, so when is it valid not to end the TB after an IO instruction ?
> My initial belief was that the TB should *always* end.

You're right, cpu_io_recompile works only for memory accesses so that
third one has to be fixed.

Paolo

Re: what are the requirements on target/ code for -icount to work correctly?

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 19 Jun 2020 at 13:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 19/06/20 14:18, Peter Maydell wrote:
>> > On Fri, 19 Jun 2020 at 12:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >>
>> >> On 19/06/20 07:46, Pavel Dovgalyuk wrote:
>> >>> I think, that we need some efforts from target maintainers to remove all such calls.
>> >>
>> >> I'll take care of target/i386 (which does need one of the three
>> >> gen_io_end calls that are left).
>> >
>> > So why does it need it ? Why can't it just rely on "TB going to
>> > end anyway which will clear the can_do_io flag" ?
>>
>> Because the TB is not always going to end in that case that is left.
>
> OK, so when is it valid not to end the TB after an IO instruction ?
> My initial belief was that the TB should *always* end.

It has to be because the value of icount is either assumes:

  - having executed all instructions in the block
  or
  - done a re-compile, re-crediting the execution not done 

if not you could associate an io event with the wrong icount value.

-- 
Alex Bennée

Re: what are the requirements on target/ code for -icount to work correctly?

[Peter Maydell]

On Fri, 19 Jun 2020 at 13:55, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 19/06/20 14:39, Peter Maydell wrote:
> > On Fri, 19 Jun 2020 at 13:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >> On 19/06/20 14:18, Peter Maydell wrote:
> >>> On Fri, 19 Jun 2020 at 12:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>>>
> >>>> On 19/06/20 07:46, Pavel Dovgalyuk wrote:
> >>>>> I think, that we need some efforts from target maintainers to remove all such calls.
> >>>>
> >>>> I'll take care of target/i386 (which does need one of the three
> >>>> gen_io_end calls that are left).
> >>>
> >>> So why does it need it ? Why can't it just rely on "TB going to
> >>> end anyway which will clear the can_do_io flag" ?
> >>
> >> Because the TB is not always going to end in that case that is left.
> >
> > OK, so when is it valid not to end the TB after an IO instruction ?
> > My initial belief was that the TB should *always* end.
>
> You're right, cpu_io_recompile works only for memory accesses so that
> third one has to be fixed.

Cool. This is starting to sound like the right cleanup is going to be:

 * get rid of all the existing callsites in targets (possibly
   including forcing end-of-TB if the code wasn't doing that before)
 * add an assert that the TB really did end (easy for targets using
   the common-translator-loop, would need to go in per-target code
   for the targets that aren't [cris, lm32, microblaze, moxie, nios2,
   tilegx, unicore32])
 * inline gen_io_end into its one remaining callsite in gen_tb_start()

thanks
-- PMM

Re: what are the requirements on target/ code for -icount to work correctly?

[Peter Maydell]

On Fri, 19 Jun 2020 at 12:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 19/06/20 07:46, Pavel Dovgalyuk wrote:
> > I think, that we need some efforts from target maintainers to remove all such calls.
>
> I'll take care of target/i386 (which does need one of the three
> gen_io_end calls that are left).

I've just sent a patch that removes the target/arm gen_io_end() calls.
I had a quick look at sparc, xtensa and ppc, but they were too complicated
for a quick look to be sufficient :-)

-- PMM

Re: what are the requirements on target/ code for -icount to work correctly?

[Max Filippov]

On Fri, Jun 19, 2020 at 10:05 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> I've just sent a patch that removes the target/arm gen_io_end() calls.
> I had a quick look at sparc, xtensa and ppc, but they were too complicated
> for a quick look to be sufficient :-)

I've checked the xtensa translator. The only gen_io_end() is for
opcodes that end TB when a full instruction is translated, because
they can change active interrupt requests. So it can be removed.
I'll send a patch.

One instance of gen_io_start is for the rsr.ccount opcode that reads
current cycle counter that translates to reading
QEMU_CLOCK_VIRTUAL clock.
Looks like this adds one more case to the
following list:

> As I understand it, the definition of "I/O insn" is anything that can
> either:
>
> - affect the icount deadline (e.g. by setting or removing a
> QEMU_CLOCK_VIRTUAL timer)
>
> - interrupt the current translation block with cpu_loop_exit,
> cpu_restore_state or similar.

-- 
Thanks.
-- Max

Re: what are the requirements on target/ code for -icount to work correctly?

[Pavel Dovgalyuk]

On 22.06.2020 08:24, Max Filippov wrote:
> On Fri, Jun 19, 2020 at 10:05 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>> I've just sent a patch that removes the target/arm gen_io_end() calls.
>> I had a quick look at sparc, xtensa and ppc, but they were too complicated
>> for a quick look to be sufficient :-)
> 
> I've checked the xtensa translator. The only gen_io_end() is for
> opcodes that end TB when a full instruction is translated, because
> they can change active interrupt requests. So it can be removed.
> I'll send a patch.
> 
> One instance of gen_io_start is for the rsr.ccount opcode that reads
> current cycle counter that translates to reading
> QEMU_CLOCK_VIRTUAL clock.

Right, this is documented by Alex in the new document recently sent into 
the mailing list:
https://lore.kernel.org/qemu-devel/7611429f-ec38-554d-3aec-e4a2f456a1ea@linaro.org/T/#mff14f2e3dc2cedd7cc6b1356985a0454e4772202

> Looks like this adds one more case to the
> following list:
> 
>> As I understand it, the definition of "I/O insn" is anything that can
>> either:
>>
>> - affect the icount deadline (e.g. by setting or removing a
>> QEMU_CLOCK_VIRTUAL timer)
>>
>> - interrupt the current translation block with cpu_loop_exit,
>> cpu_restore_state or similar.
>