[Qemu-devel] throwing away translated code on CPU reset

[Qemu-devel] throwing away translated code on CPU reset

[Peter Maydell]

When doing TCG code translation, the target-foo translate.c
code is allowed to bake assumptions into the generated code from
the current values of various fields in the CPUState. This then
imposes the requirement that if the field is changed then tb_flush
must be called to throw away the now-incorrect generated code.

However, cpu_reset() changes (unsurprisingly) lots of fields in
the CPUState, but it doesn't call tb_flush()...

So should cpu_reset() implementations be changed to call tb_flush()
as well as tlb_flush(), or is this supposed to work in some other
way?

thanks
-- PMM

Re: [Qemu-devel] throwing away translated code on CPU reset

[Andreas Färber]

Am 12.01.2012 15:00, schrieb Peter Maydell:
> When doing TCG code translation, the target-foo translate.c
> code is allowed to bake assumptions into the generated code from
> the current values of various fields in the CPUState. This then
> imposes the requirement that if the field is changed then tb_flush
> must be called to throw away the now-incorrect generated code.
> 
> However, cpu_reset() changes (unsurprisingly) lots of fields in
> the CPUState, but it doesn't call tb_flush()...
> 
> So should cpu_reset() implementations be changed to call tb_flush()
> as well as tlb_flush(), or is this supposed to work in some other
> way?

I would rather suggest to introduce a new cpu_common_reset() that hides
these details - memset() for common parts and whatever necessary here.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] throwing away translated code on CPU reset

[陳韋任]

On Thu, Jan 12, 2012 at 02:00:38PM +0000, Peter Maydell wrote:
> When doing TCG code translation, the target-foo translate.c
> code is allowed to bake assumptions into the generated code from
> the current values of various fields in the CPUState. This then
> imposes the requirement that if the field is changed then tb_flush
> must be called to throw away the now-incorrect generated code.
> 
> However, cpu_reset() changes (unsurprisingly) lots of fields in
> the CPUState, but it doesn't call tb_flush()...

  I dig what tlb_flush does further and think maybe we don't need to call tb_flush
when tlb_flush is called.

  First, look at tlb_flush (exec.c). It clears env's tb_jmp_cache which use GHA
as an index to search if there is a translated code. Since tb_jmp_cache is reset
now, QEMU is forced to call tb_find_slow which uses GPA as the index.

  In tb_find_slow's for loop, it compares hit TranslationBlock's various fields
with current values. To be more specific,

static TranslationBlock *tb_find_slow(...) {

    for(;;) {
        tb = *ptb1;
        if (!tb)
            goto not_found;
        if (tb->pc == pc &&                    <--- Here
            tb->page_addr[0] == phys_page1 &&
            tb->cs_base == cs_base &&
            tb->flags == flags) {
        }
   }
}

  What do you think?

Regards,
chenwj

-- 
Wei-Ren Chen (陳韋任)
Computer Systems Lab, Institute of Information Science,
Academia Sinica, Taiwan (R.O.C.)
Tel:886-2-2788-3799 #1667
Homepage: http://people.cs.nctu.edu.tw/~chenwj

Re: [Qemu-devel] throwing away translated code on CPU reset

[Peter Maydell]

On 13 January 2012 07:55, 陳韋任 <chenwj@iis.sinica.edu.tw> wrote:
> On Thu, Jan 12, 2012 at 02:00:38PM +0000, Peter Maydell wrote:
>> When doing TCG code translation, the target-foo translate.c
>> code is allowed to bake assumptions into the generated code from
>> the current values of various fields in the CPUState. This then
>> imposes the requirement that if the field is changed then tb_flush
>> must be called to throw away the now-incorrect generated code.
>>
>> However, cpu_reset() changes (unsurprisingly) lots of fields in
>> the CPUState, but it doesn't call tb_flush()...
>
>  I dig what tlb_flush does further and think maybe we don't need to call tb_flush
> when tlb_flush is called.
>
>  First, look at tlb_flush (exec.c). It clears env's tb_jmp_cache which use GHA
> as an index to search if there is a translated code. Since tb_jmp_cache is reset
> now, QEMU is forced to call tb_find_slow which uses GPA as the index.
>
>  In tb_find_slow's for loop, it compares hit TranslationBlock's various fields
> with current values. To be more specific,
>
> static TranslationBlock *tb_find_slow(...) {
>
>    for(;;) {
>        tb = *ptb1;
>        if (!tb)
>            goto not_found;
>        if (tb->pc == pc &&                    <--- Here
>            tb->page_addr[0] == phys_page1 &&
>            tb->cs_base == cs_base &&
>            tb->flags == flags) {
>        }
>   }
> }
>
>  What do you think?

This is true, but the translated code may have assumptions about
fields which are not encoded in tb_flags, if it is handling those
fields with the "tb_flush if field changes" strategy (eg on ARM
env->teecr and others).

-- PMM

Re: [Qemu-devel] throwing away translated code on CPU reset

[Aurelien Jarno]

On Thu, Jan 12, 2012 at 02:00:38PM +0000, Peter Maydell wrote:
> When doing TCG code translation, the target-foo translate.c
> code is allowed to bake assumptions into the generated code from
> the current values of various fields in the CPUState. This then
> imposes the requirement that if the field is changed then tb_flush
> must be called to throw away the now-incorrect generated code.
> 
> However, cpu_reset() changes (unsurprisingly) lots of fields in
> the CPUState, but it doesn't call tb_flush()...
> 
> So should cpu_reset() implementations be changed to call tb_flush()
> as well as tlb_flush(), or is this supposed to work in some other
> way?

We use the hflags to determine in which conditions the cached code has
been generated, so that we only used the cache code if the CPU is in the
same mode. I therefore don't think there is a real need to flush the
cached code.

What should be ensured on the other hand, is that hflag is correctly
updated during or after the reset. This is the case for at least PowerPC
and MIPS.


-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] throwing away translated code on CPU reset

[Peter Maydell]

On 14 January 2012 14:48, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Thu, Jan 12, 2012 at 02:00:38PM +0000, Peter Maydell wrote:
>> When doing TCG code translation, the target-foo translate.c
>> code is allowed to bake assumptions into the generated code from
>> the current values of various fields in the CPUState. This then
>> imposes the requirement that if the field is changed then tb_flush
>> must be called to throw away the now-incorrect generated code.
>>
>> However, cpu_reset() changes (unsurprisingly) lots of fields in
>> the CPUState, but it doesn't call tb_flush()...
>>
>> So should cpu_reset() implementations be changed to call tb_flush()
>> as well as tlb_flush(), or is this supposed to work in some other
>> way?
>
> We use the hflags to determine in which conditions the cached code has
> been generated, so that we only used the cache code if the CPU is in the
> same mode. I therefore don't think there is a real need to flush the
> cached code.

This only applies for things which are encoded in tb->flags.
Look at handling of eg env->cp15.c1_coproc or env->teecr for
env changes that do need a flush.

Perhaps this just indicates that CPUs using this approach for
some env fields should be calling tb_flush() but not those
that do not. It looks as if that's just ARM at the moment. Hmm.

-- PMM