[Qemu-devel] [PATCH] exec: Handle multipage ranges in invalidate_and_set_dirty()

[Qemu-devel] [PATCH] exec: Handle multipage ranges in invalidate_and_set_dirty()

[Peter Maydell]

The code in invalidate_and_set_dirty() needs to handle addr/length
combinations which cross guest physical page boundaries. This can happen,
for example, when disk I/O reads large blocks into guest RAM which previously
held code that we have cached translations for. Unfortunately we were only
checking the clean/dirty status of the first page in the range, and then
were calling a tb_invalidate function which only handles ranges that don't
cross page boundaries. Fix the function to deal with multipage ranges.

The symptoms of this bug were that guest code would misbehave (eg segfault),
in particular after a guest reboot but potentially any time the guest
reused a page of its physical RAM for new code.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This seems pretty nasty, and I have no idea why it hasn't been wreaking
more havoc than this before. I'm not entirely sure why we invalidate TBs
if any of the dirty bits is set rather than only if the code bit is set,
but I left that logic as it is.

Review appreciated -- it would be nice to get this into rc2 if
we can, I think.

 exec.c                  |  6 ++----
 include/exec/ram_addr.h | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/exec.c b/exec.c
index 759055d..f0e2bd3 100644
--- a/exec.c
+++ b/exec.c
@@ -2066,10 +2066,8 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
 static void invalidate_and_set_dirty(hwaddr addr,
                                      hwaddr length)
 {
-    if (cpu_physical_memory_is_clean(addr)) {
-        /* invalidate code */
-        tb_invalidate_phys_page_range(addr, addr + length, 0);
-        /* set dirty bit */
+    if (cpu_physical_memory_range_includes_clean(addr, length)) {
+        tb_invalidate_phys_range(addr, addr + length, 0);
         cpu_physical_memory_set_dirty_range_nocode(addr, length);
     }
     xen_modified_memory(addr, length);
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index cf1d4c7..8fc75cd 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -49,6 +49,21 @@ static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
     return next < end;
 }
 
+static inline bool cpu_physical_memory_get_clean(ram_addr_t start,
+                                                 ram_addr_t length,
+                                                 unsigned client)
+{
+    unsigned long end, page, next;
+
+    assert(client < DIRTY_MEMORY_NUM);
+
+    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
+    page = start >> TARGET_PAGE_BITS;
+    next = find_next_zero_bit(ram_list.dirty_memory[client], end, page);
+
+    return next < end;
+}
+
 static inline bool cpu_physical_memory_get_dirty_flag(ram_addr_t addr,
                                                       unsigned client)
 {
@@ -64,6 +79,16 @@ static inline bool cpu_physical_memory_is_clean(ram_addr_t addr)
     return !(vga && code && migration);
 }
 
+static inline bool cpu_physical_memory_range_includes_clean(ram_addr_t start,
+                                                            ram_addr_t length)
+{
+    bool vga = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_VGA);
+    bool code = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_CODE);
+    bool migration =
+        cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_MIGRATION);
+    return vga || code || migration;
+}
+
 static inline void cpu_physical_memory_set_dirty_flag(ram_addr_t addr,
                                                       unsigned client)
 {
-- 
1.9.1

Re: [Qemu-devel] [PATCH] exec: Handle multipage ranges in invalidate_and_set_dirty()

[Paolo Bonzini]

On 16/11/2014 20:44, Peter Maydell wrote:
> The code in invalidate_and_set_dirty() needs to handle addr/length
> combinations which cross guest physical page boundaries. This can happen,
> for example, when disk I/O reads large blocks into guest RAM which previously
> held code that we have cached translations for. Unfortunately we were only
> checking the clean/dirty status of the first page in the range, and then
> were calling a tb_invalidate function which only handles ranges that don't
> cross page boundaries. Fix the function to deal with multipage ranges.
> 
> The symptoms of this bug were that guest code would misbehave (eg segfault),
> in particular after a guest reboot but potentially any time the guest
> reused a page of its physical RAM for new code.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This seems pretty nasty, and I have no idea why it hasn't been wreaking
> more havoc than this before. I'm not entirely sure why we invalidate TBs
> if any of the dirty bits is set rather than only if the code bit is set,
> but I left that logic as it is.

I think it's a remain of when we had a single bitmap with three bits in
it.  We can clean up in 2.3.

> Review appreciated -- it would be nice to get this into rc2 if
> we can, I think.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

>  exec.c                  |  6 ++----
>  include/exec/ram_addr.h | 25 +++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 759055d..f0e2bd3 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2066,10 +2066,8 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>  static void invalidate_and_set_dirty(hwaddr addr,
>                                       hwaddr length)
>  {
> -    if (cpu_physical_memory_is_clean(addr)) {
> -        /* invalidate code */
> -        tb_invalidate_phys_page_range(addr, addr + length, 0);
> -        /* set dirty bit */
> +    if (cpu_physical_memory_range_includes_clean(addr, length)) {
> +        tb_invalidate_phys_range(addr, addr + length, 0);
>          cpu_physical_memory_set_dirty_range_nocode(addr, length);
>      }
>      xen_modified_memory(addr, length);
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index cf1d4c7..8fc75cd 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -49,6 +49,21 @@ static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
>      return next < end;
>  }
>  
> +static inline bool cpu_physical_memory_get_clean(ram_addr_t start,
> +                                                 ram_addr_t length,
> +                                                 unsigned client)
> +{
> +    unsigned long end, page, next;
> +
> +    assert(client < DIRTY_MEMORY_NUM);
> +
> +    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> +    page = start >> TARGET_PAGE_BITS;
> +    next = find_next_zero_bit(ram_list.dirty_memory[client], end, page);
> +
> +    return next < end;
> +}
> +
>  static inline bool cpu_physical_memory_get_dirty_flag(ram_addr_t addr,
>                                                        unsigned client)
>  {
> @@ -64,6 +79,16 @@ static inline bool cpu_physical_memory_is_clean(ram_addr_t addr)
>      return !(vga && code && migration);
>  }
>  
> +static inline bool cpu_physical_memory_range_includes_clean(ram_addr_t start,
> +                                                            ram_addr_t length)
> +{
> +    bool vga = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_VGA);
> +    bool code = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_CODE);
> +    bool migration =
> +        cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_MIGRATION);
> +    return vga || code || migration;
> +}
> +
>  static inline void cpu_physical_memory_set_dirty_flag(ram_addr_t addr,
>                                                        unsigned client)
>  {
> 

Re: [Qemu-devel] [PATCH] exec: Handle multipage ranges in invalidate_and_set_dirty()

[Peter Maydell]

On 17 November 2014 11:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 16/11/2014 20:44, Peter Maydell wrote:
>> The code in invalidate_and_set_dirty() needs to handle addr/length
>> combinations which cross guest physical page boundaries. This can happen,
>> for example, when disk I/O reads large blocks into guest RAM which previously
>> held code that we have cached translations for. Unfortunately we were only
>> checking the clean/dirty status of the first page in the range, and then
>> were calling a tb_invalidate function which only handles ranges that don't
>> cross page boundaries. Fix the function to deal with multipage ranges.
>>
>> The symptoms of this bug were that guest code would misbehave (eg segfault),
>> in particular after a guest reboot but potentially any time the guest
>> reused a page of its physical RAM for new code.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> This seems pretty nasty, and I have no idea why it hasn't been wreaking
>> more havoc than this before. I'm not entirely sure why we invalidate TBs
>> if any of the dirty bits is set rather than only if the code bit is set,
>> but I left that logic as it is.
>
> I think it's a remain of when we had a single bitmap with three bits in
> it.  We can clean up in 2.3.
>
>> Review appreciated -- it would be nice to get this into rc2 if
>> we can, I think.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Applied to master; thanks.

-- PMM