[Qemu-devel] [PATCH] cpu: flush TB cache when loading VMState

[Qemu-devel] [PATCH] cpu: flush TB cache when loading VMState

[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 <pavel.dovgaluk@ispras.ru>
Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru>
---
 exec.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/exec.c b/exec.c
index 4722e52..ff31e71 100644
--- a/exec.c
+++ b/exec.c
@@ -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;
 }

Re: [Qemu-devel] [PATCH] cpu: flush TB cache when loading VMState

[Paolo Bonzini]

On 10/01/2018 14:48, Pavel Dovgalyuk wrote:
> 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 <pavel.dovgaluk@ispras.ru>
> Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru>
> ---
>  exec.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/exec.c b/exec.c
> index 4722e52..ff31e71 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -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;
>  }
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH] cpu: flush TB cache when loading VMState

[Richard Henderson]

On 01/10/2018 05:48 AM, Pavel Dovgalyuk wrote:
> 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 <pavel.dovgaluk@ispras.ru>
> Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru>
> ---
>  exec.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/exec.c b/exec.c
> index 4722e52..ff31e71 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -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?


r~

Re: [Qemu-devel] [PATCH] cpu: flush TB cache when loading VMState

[Dr. David Alan Gilbert]

* Richard Henderson (richard.henderson@linaro.org) wrote:
> On 01/10/2018 05:48 AM, Pavel Dovgalyuk wrote:
> > 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 <pavel.dovgaluk@ispras.ru>
> > Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru>
> > ---
> >  exec.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/exec.c b/exec.c
> > index 4722e52..ff31e71 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -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?

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

> 
> r~
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] cpu: flush TB cache when loading VMState

[Peter Maydell]

On 10 January 2018 at 17:49, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 01/10/2018 05:48 AM, Pavel Dovgalyuk wrote:
>> 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 <pavel.dovgaluk@ispras.ru>
>> Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru>
>> ---
>>  exec.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/exec.c b/exec.c
>> index 4722e52..ff31e71 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -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.

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

Re: [Qemu-devel] [PATCH] cpu: flush TB cache when loading VMState

[Paolo Bonzini]

On 10/01/2018 19:32, Peter Maydell wrote:
> On 10 January 2018 at 17:49, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> On 01/10/2018 05:48 AM, Pavel Dovgalyuk wrote:
>>> 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 <pavel.dovgaluk@ispras.ru>
>>> Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru>
>>> ---
>>>  exec.c |    1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/exec.c b/exec.c
>>> index 4722e52..ff31e71 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -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.
> 
> 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

Re: [Qemu-devel] [PATCH] cpu: flush TB cache when loading VMState

[Peter Maydell]

On 11 January 2018 at 10:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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

Re: [Qemu-devel] [PATCH] cpu: flush TB cache when loading VMState

[Paolo Bonzini]

On 11/01/2018 11:20, Peter Maydell wrote:
> On 11 January 2018 at 10:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> 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
> 

Sure, will add it.

Paolo