* Re: [Qemu-devel] throwing away translated code on CPU reset
2012-01-12 14:00 [Qemu-devel] throwing away translated code on CPU reset Peter Maydell
@ 2012-01-12 14:18 ` Andreas Färber
2012-01-13 7:55 ` 陳韋任
2012-01-14 14:48 ` Aurelien Jarno
2 siblings, 0 replies; 6+ messages in thread
From: Andreas Färber @ 2012-01-12 14:18 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, Alexander Graf
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] throwing away translated code on CPU reset
2012-01-12 14:00 [Qemu-devel] throwing away translated code on CPU reset Peter Maydell
2012-01-12 14:18 ` Andreas Färber
@ 2012-01-13 7:55 ` 陳韋任
2012-01-13 9:08 ` Peter Maydell
2012-01-14 14:48 ` Aurelien Jarno
2 siblings, 1 reply; 6+ messages in thread
From: 陳韋任 @ 2012-01-13 7:55 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, Alexander Graf
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] throwing away translated code on CPU reset
2012-01-13 7:55 ` 陳韋任
@ 2012-01-13 9:08 ` Peter Maydell
0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2012-01-13 9:08 UTC (permalink / raw)
To: 陳韋任; +Cc: QEMU Developers, Alexander Graf
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] throwing away translated code on CPU reset
2012-01-12 14:00 [Qemu-devel] throwing away translated code on CPU reset Peter Maydell
2012-01-12 14:18 ` Andreas Färber
2012-01-13 7:55 ` 陳韋任
@ 2012-01-14 14:48 ` Aurelien Jarno
2012-01-15 16:43 ` Peter Maydell
2 siblings, 1 reply; 6+ messages in thread
From: Aurelien Jarno @ 2012-01-14 14:48 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, Alexander Graf
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] throwing away translated code on CPU reset
2012-01-14 14:48 ` Aurelien Jarno
@ 2012-01-15 16:43 ` Peter Maydell
0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2012-01-15 16:43 UTC (permalink / raw)
To: Aurelien Jarno; +Cc: QEMU Developers, Alexander Graf
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
^ permalink raw reply [flat|nested] 6+ messages in thread