cpu: flush TB cache when loading VMState

PATCH Applied Pavel Dovgalyuk 5 participants 8 messages Jan 10, 2018 – Jan 11, 2018

Share & tools

Pavel Dovgalyuk

author

2018-01-10 13:48 UTC #0 lore raw

PATCH 1/1 cpu: flush TB cache when loading VMState

Flushing TB cache is required because TBs key in the cache may match different code which existed in the previous state.

Signed-off-by Pavel Dovgalyuk <[email protected]>

Signed-off-by Maria Klimushenkova <[email protected]>

---
 exec.c |    1 +
 1 file changed, 1 insertion(+)

exec.c +1 -0

index 4722e52..ff31e71 100644
@@ -622,6 +622,7 @@ static int cpu_common_post_load(void *opaque, int version_id)
        version_id is increased. */
     cpu->interrupt_request &= ~0x01;
     tlb_flush(cpu);
+    tb_flush(cpu);
 
     return 0;
 }

Richard Henderson

2018-01-10 17:50 UTC #2 lore raw

In reply to Pavel Dovgalyuk

Quoted 14 lines · Pavel Dovgalyuk

Flushing TB cache is required because TBs key in the cache may match different code which existed in the previous state.
Signed-off-by Pavel Dovgalyuk <[email protected]>
Signed-off-by Maria Klimushenkova <[email protected]>
---
 exec.c |    1 +
 1 file changed, 1 insertion(+)
exec.c +0 -0
index 4722e52..ff31e71 100644

exec.c static int cpu_common_post_load(void *opaque, int version_id) · line 625

Quoted 4 lines

exec.c +1 -0

@@ -622,6 +622,7 @@ static int cpu_common_post_load(void *opaque, int version_id)
        version_id is increased. */
     cpu->interrupt_request &= ~0x01;
     tlb_flush(cpu);
+    tb_flush(cpu);

I'm not necessarily objecting, but what do you mean by "may match different code"?

What this patch suggests is that the inputs to the computation of TB->FLAGS are different for some unspecified reason. Without further explanation, to me this suggests a bug in vmstate save/restore.

What is the observed problem here?

Dr. David Alan Gilbert

2018-01-10 17:55 UTC #3 lore raw

Quoted 22 lines · * Richard Henderson ([email protected]) wrote:

Quoted 14 lines · Pavel Dovgalyuk
Flushing TB cache is required because TBs key in the cache may match different code which existed in the previous state.
Signed-off-by Pavel Dovgalyuk <[email protected]>
Signed-off-by Maria Klimushenkova <[email protected]>
---
 exec.c |    1 +
 1 file changed, 1 insertion(+)
exec.c +0 -0
index 4722e52..ff31e71 100644
exec.c static int cpu_common_post_load(void *opaque, int version_id) · line 625
Quoted 4 lines
exec.c +1 -0
@@ -622,6 +622,7 @@ static int cpu_common_post_load(void *opaque, int version_id)
        version_id is increased. */
     cpu->interrupt_request &= ~0x01;
     tlb_flush(cpu);
+    tb_flush(cpu);
I'm not necessarily objecting, but what do you mean by "may match different code"?

Quoted 5 lines

What this patch suggests is that the inputs to the computation of TB->FLAGS are different for some unspecified reason. Without further explanation, to me this suggests a bug in vmstate save/restore.
What is the observed problem here?

Is this a case where you're repeatedly running 'loadvm' to revert to a previous snapshot (or I guess the debug stuff); so you've been running and translating code and then reload state ?

Dave

Quoted 2 lines

r~

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Peter Maydell

2018-01-10 18:32 UTC #4 lore raw

In reply to Richard Henderson

Quoted 22 lines · Richard Henderson

Quoted 14 lines · Pavel Dovgalyuk
Flushing TB cache is required because TBs key in the cache may match different code which existed in the previous state.
Signed-off-by Pavel Dovgalyuk <[email protected]>
Signed-off-by Maria Klimushenkova <[email protected]>
---
 exec.c |    1 +
 1 file changed, 1 insertion(+)
exec.c +0 -0
index 4722e52..ff31e71 100644
exec.c static int cpu_common_post_load(void *opaque, int version_id) · line 625
Quoted 4 lines
exec.c +1 -0
@@ -622,6 +622,7 @@ static int cpu_common_post_load(void *opaque, int version_id)
        version_id is increased. */
     cpu->interrupt_request &= ~0x01;
     tlb_flush(cpu);
+    tb_flush(cpu);
I'm not necessarily objecting, but what do you mean by "may match different code"?

Quoted 3 lines

What this patch suggests is that the inputs to the computation of TB->FLAGS are different for some unspecified reason. Without further explanation, to me this suggests a bug in vmstate save/restore.

Yeah, this looks a little fishy. If there's code in the TB cache which would be wrong for the freshly-reset (or whatever) CPU after a VM state load, then it could just as easily be wrong for a 2nd CPU in an SMP config.

I used to think it was OK to have the generated code bake in some information that wasn't in tb_flags as long as you then did a tb_flush when that information changed, but I realized I was wrong about that (because of the SMP issue).

git grep suggests we do still have a few places in targets that are calling tb_flush(), but I think we should try to fix those.

thanks
-- PMM

Paolo Bonzini

2018-01-11 10:15 UTC #5 lore raw

Quoted 24 lines · Peter Maydell

Quoted 22 lines · Richard Henderson
Quoted 14 lines · Pavel Dovgalyuk
Flushing TB cache is required because TBs key in the cache may match different code which existed in the previous state.
Signed-off-by Pavel Dovgalyuk <[email protected]>
Signed-off-by Maria Klimushenkova <[email protected]>
---
 exec.c |    1 +
 1 file changed, 1 insertion(+)
exec.c +0 -0
index 4722e52..ff31e71 100644
exec.c static int cpu_common_post_load(void *opaque, int version_id) · line 625
Quoted 4 lines
exec.c +1 -0
@@ -622,6 +622,7 @@ static int cpu_common_post_load(void *opaque, int version_id)
        version_id is increased. */
     cpu->interrupt_request &= ~0x01;
     tlb_flush(cpu);
+    tb_flush(cpu);
I'm not necessarily objecting, but what do you mean by "may match different code"?

Quoted 8 lines

Quoted 3 lines
What this patch suggests is that the inputs to the computation of TB->FLAGS are different for some unspecified reason. Without further explanation, to me this suggests a bug in vmstate save/restore.
Yeah, this looks a little fishy. If there's code in the TB cache which would be wrong for the freshly-reset (or whatever) CPU after a VM state load, then it could just as easily be wrong for a 2nd CPU in an SMP config.

RAM contents are memcpy'd blindly during loadvm. I think that's what requires a tb_flush.

Paolo

Peter Maydell

2018-01-11 10:20 UTC #6 lore raw

Quoted 2 lines · Paolo Bonzini

RAM contents are memcpy'd blindly during loadvm. I think that's what requires a tb_flush.

Ah, that makes sense. Could we have a comment documenting the rationale, please? Something like:

/* vmload has just updated the content of RAM, bypassing the
* usual mechanisms that ensure we flush TBs for writes to
* memory we've translated code from. So we must flush all TBs,
* which will now be stale.
*/

thanks
-- PMM

Paolo Bonzini

2018-01-11 13:24 UTC #7 lore raw

Quoted 7 lines · Peter Maydell

Quoted 2 lines · Paolo Bonzini
RAM contents are memcpy'd blindly during loadvm. I think that's what requires a tb_flush.
Ah, that makes sense. Could we have a comment documenting the rationale, please? Something like:

Quoted 9 lines

/* vmload has just updated the content of RAM, bypassing the
* usual mechanisms that ensure we flush TBs for writes to
* memory we've translated code from. So we must flush all TBs,
* which will now be stale.
*/
thanks
-- PMM

Sure, will add it.

Paolo