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-sta...@nongnu.org
> Signed-off-by: Peter Maydell <peter.mayd...@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 <pbonz...@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)
> {
>
