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: address@hidden
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> 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 <address@hidden>

>  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)
>  {
>