----- Mail original -----
> De: "Paolo Bonzini" <pbonz...@redhat.com>
> À: "Sebastian Tanase" <sebastian.tan...@openwide.fr>
> Cc: aligu...@amazon.com, afaer...@suse.de, r...@twiddle.net, "peter maydell"
> <peter.mayd...@linaro.org>,
> mich...@walle.cc, a...@alex.org.uk, stefa...@redhat.com, kw...@redhat.com,
> quint...@redhat.com, ebl...@redhat.com,
> m...@tls.msk.ru, qemu-devel@nongnu.org
> Envoyé: Mardi 27 Mai 2014 17:54:33
> Objet: Re: [RFC PATCH 3/4] cpu_exec: Add sleeping algorithm
>
> Il 27/05/2014 17:49, Sebastian Tanase ha scritto:
> >> Why doesn't it have to update original_low and original_extra, and
> >> why doesn't it have to take into account original_extra (the new
> >> cpu->icount_extra is zero, but what about the old one)?
> >
> > The reason I don't update original_low and original_extra is
> > because
> > in this case the function will exit (from what I understood):
>
> You're right. Of course this becomes moot if you move the updating
> code
> to a separate function; otherwise, please add a comment.
>
> You didn't answer the rest of the question---is it right to ignore
> original_extra, or was it a bug?
>
> Paolo
>
>From my understanding, cpu->icount_extra is only updated if we end up in the
TB_EXIT_ICOUNT_EXPIRED case and if the interrupt flag is 0
(cpu->icount_decr.u32 is
positive when treated as a signed integer). In this situation, we also update
original_extra.
case TB_EXIT_ICOUNT_EXPIRED:
{
/* Instruction counter expired. */
int insns_left;
tb = (TranslationBlock *)(next_tb & ~TB_EXIT_MASK);
insns_left = cpu->icount_decr.u32;
if (cpu->icount_extra && insns_left >= 0) {
/* Refill decrementer and continue execution. */
cpu->icount_extra += insns_left;
if (cpu->icount_extra > 0xffff) {
insns_left = 0xffff;
} else {
insns_left = cpu->icount_extra;
}
cpu->icount_extra -= insns_left;
cpu->icount_decr.u16.low = insns_left;
if (icount_align_option) {
/* Instruction counters have been
updated so we update ours */
original_extra = cpu->icount_extra;
original_low = cpu->icount_decr.u16.low;
}
...
When cpu->icount_extra becomes 0, original_extra also becomes 0, so next time
we end up in the TB_EXIT_ICOUNT_EXPIRED case, we will go into the else branch
and given that cpu->icount_extra is 0, original_extra has to be 0 also.
} else {
if (insns_left > 0) {
/* Execute remaining instructions. */
cpu_exec_nocache(env, insns_left, tb);
if (icount_align_option) {
instr_exec_time = original_low -
cpu->icount_decr.u16.low;
instr_exec_time = instr_exec_time <<
icount_time_shift;
diff_clk += instr_exec_time;
if (diff_clk > VM_CLOCK_ADVANCE) {
delay_host();
}
}
}
That being said, I think that updating original_extra in other cases is useless.
case 1:
case 0:
if (icount_align_option) {
instr_exec_time = original_extra -
cpu->icount_extra +
original_low -
cpu->icount_decr.u16.low;
instr_exec_time = instr_exec_time <<
icount_time_shift;
diff_clk += instr_exec_time;
original_extra = cpu->icount_extra;
original_low = cpu->icount_decr.u16.low;
}
Overall, I think that using one counter to track both icount_extra and
icount_decr.u16.low, as you suggested, is better and makes the code easier
to understand.
Regards,
Sebastian Tanase
Open Wide Ingenierie
23, rue Daviel
75013 Paris - France
www.openwide.fr
