diff mbox

[PATCH/RFC] exec: add cpu_synchronize_state to cpu_memory_rw_debug

Message ID 1488896348-13560-1-git-send-email-borntraeger@de.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Christian Borntraeger March 7, 2017, 2:19 p.m. UTC
I sometimes got "Cannot access memory" when using the x command
on the monitor. Turns out that the cpu env did contain stale data
(e.g. wrong control register content for page table origin).
We must synchronize the state of the CPU before walking the page
tables. A similar issues happens for a remote gdb, so lets
do the cpu_synchronize_state in cpu_memory_rw_debug.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 exec.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Alex Bennée March 7, 2017, 3:35 p.m. UTC | #1
Christian Borntraeger <borntraeger@de.ibm.com> writes:

> I sometimes got "Cannot access memory" when using the x command
> on the monitor. Turns out that the cpu env did contain stale data
> (e.g. wrong control register content for page table origin).
> We must synchronize the state of the CPU before walking the page
> tables. A similar issues happens for a remote gdb, so lets
> do the cpu_synchronize_state in cpu_memory_rw_debug.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  exec.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/exec.c b/exec.c
> index aabb035..e754a03 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -43,6 +43,7 @@
>  #include "exec/ioport.h"
>  #include "sysemu/dma.h"
>  #include "sysemu/numa.h"
> +#include "sysemu/hw_accel.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/xen-mapcache.h"
>  #include "trace-root.h"
> @@ -3309,6 +3310,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>      hwaddr phys_addr;
>      target_ulong page;
>
> +    cpu_synchronize_state(cpu);
>      while (len > 0) {
>          int asidx;
>          MemTxAttrs attrs;

This seems like the wrong place to put it. Would we end up doing a
potentially expensive sync operations for every byte/word we dump out?

Certainly when I was messing around with ARM KVM debug I did the
synchronise state as we entered the debug handling (e.g.
gdb_handle_packet/memory_dump)?

--
Alex Bennée
Christian Borntraeger March 7, 2017, 5:36 p.m. UTC | #2
On 03/07/2017 04:35 PM, Alex Bennée wrote:
> 
> Christian Borntraeger <borntraeger@de.ibm.com> writes:
> 
>> I sometimes got "Cannot access memory" when using the x command
>> on the monitor. Turns out that the cpu env did contain stale data
>> (e.g. wrong control register content for page table origin).
>> We must synchronize the state of the CPU before walking the page
>> tables. A similar issues happens for a remote gdb, so lets
>> do the cpu_synchronize_state in cpu_memory_rw_debug.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  exec.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/exec.c b/exec.c
>> index aabb035..e754a03 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -43,6 +43,7 @@
>>  #include "exec/ioport.h"
>>  #include "sysemu/dma.h"
>>  #include "sysemu/numa.h"
>> +#include "sysemu/hw_accel.h"
>>  #include "exec/address-spaces.h"
>>  #include "sysemu/xen-mapcache.h"
>>  #include "trace-root.h"
>> @@ -3309,6 +3310,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>>      hwaddr phys_addr;
>>      target_ulong page;
>>
>> +    cpu_synchronize_state(cpu);
>>      while (len > 0) {
>>          int asidx;
>>          MemTxAttrs attrs;
> 
> This seems like the wrong place to put it. Would we end up doing a
> potentially expensive sync operations for every byte/word we dump out?

kvm_synchronize_state is clever enough to only sync once. This function mostly
seems to be for debugging stuff (gdb, monitor). Instead of adding the sync
in several places putting it here is a big hammer but it probably fixes all places.
I fear that we will forget this in other places. No idea whats right.
 
> Certainly when I was messing around with ARM KVM debug I did the
> synchronise state as we entered the debug handling (e.g.
> gdb_handle_packet/memory_dump)?
> 
> --
> Alex Bennée
>
Paolo Bonzini March 9, 2017, 4:42 p.m. UTC | #3
On 07/03/2017 15:19, Christian Borntraeger wrote:
> I sometimes got "Cannot access memory" when using the x command
> on the monitor. Turns out that the cpu env did contain stale data
> (e.g. wrong control register content for page table origin).
> We must synchronize the state of the CPU before walking the page
> tables. A similar issues happens for a remote gdb, so lets
> do the cpu_synchronize_state in cpu_memory_rw_debug.

Makes sense (the bit missing from the commit message, at least the one
that I had to look up, is that cpu_memory_rw_debug takes virtual addresses).

Paolo

> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  exec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index aabb035..e754a03 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -43,6 +43,7 @@
>  #include "exec/ioport.h"
>  #include "sysemu/dma.h"
>  #include "sysemu/numa.h"
> +#include "sysemu/hw_accel.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/xen-mapcache.h"
>  #include "trace-root.h"
> @@ -3309,6 +3310,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>      hwaddr phys_addr;
>      target_ulong page;
>  
> +    cpu_synchronize_state(cpu);
>      while (len > 0) {
>          int asidx;
>          MemTxAttrs attrs;
>
diff mbox

Patch

diff --git a/exec.c b/exec.c
index aabb035..e754a03 100644
--- a/exec.c
+++ b/exec.c
@@ -43,6 +43,7 @@ 
 #include "exec/ioport.h"
 #include "sysemu/dma.h"
 #include "sysemu/numa.h"
+#include "sysemu/hw_accel.h"
 #include "exec/address-spaces.h"
 #include "sysemu/xen-mapcache.h"
 #include "trace-root.h"
@@ -3309,6 +3310,7 @@  int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
     hwaddr phys_addr;
     target_ulong page;
 
+    cpu_synchronize_state(cpu);
     while (len > 0) {
         int asidx;
         MemTxAttrs attrs;