Re: [Qemu-devel] tb_invalidate_phys_range and tb_invalidate_phys_page_range

[Peter Maydell]

On 29 July 2017 at 01:33, Emilio G. Cota <address@hidden> wrote:
> While working on the removal of tb_lock, I stumbled upon the following.
>
> Commit 77a8f1a51 ("linux-user: Fix stale tbs after mmap") introduced
> tb_invalidate_phys_range(), which we now have in accel/tcg/translate-all.c
> [ patchwork thread here: https://patchwork.ozlabs.org/patch/158556/ ]
>
> This function calls
> tb_invalidate_phys_page_range(). As the name suggests, the latter
> function expects the range to be within the same page.
> The former does not have this requirement, as stated in the comment
> above the function (which I confirmed running some linux-user code):
>
> +/*
> + * invalidate all TBs which intersect with the target physical pages
> + * starting in range [start;end[. NOTE: start and end may refer to
> + * different physical pages. 'is_cpu_write_access' should be true if called
> + * from a real cpu write access: the virtual CPU will exit the current
> + * TB if code is modified inside this TB.
> + */
> +void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end,
> +                              int is_cpu_write_access)
> +{
> +    while (start < end) {
> +        tb_invalidate_phys_page_range(start, end, is_cpu_write_access);
> +        start &= TARGET_PAGE_MASK;
> +        start += TARGET_PAGE_SIZE;
> +    }
> +}
>
> What I find puzzling is that here we pass 'end' unmodified, instead of
> making sure the range passed is within a page.
>
> Is this a bug, or am I missing something?

It's a bit odd, but looking at the code I think it doesn't matter.
The only thing that tb_invalidate_phys_page_range() uses 'end'
for is when it does the "does this TB I've found overlap the
range we're flushing" check with
 if (!(tb_end <= start || tb_start >= end))

If we pass an 'end' value that's larger than it would be if
we confined it to the page, this is safe because at worst
we'll invalidate more TBs than we needed to (and in practice
if the TB we've found is within the full range that
tb_invalidate_phys_range() is checking we need to invalidate
it anyway). I haven't quite worked it through but I rather
suspect that you can't have a TB that's in the list for
the PageDesc for 'start' and which overlaps in the
"full" [start-end) range but which wouldn't overlap
in a truncated [start-(end rounded down to end of page))
range.

Basically the reason for the "same page" restriction
is that tb_invalidate_phys_page_range() only does
a single page_find() for the 'start' address. So
as long as we call it once per page in the range
it doesn't matter that we pass an overly pessimistic
end. This is kind of bordering on "happens to work"
territory though, so maybe we could improve it...

thanks
-- PMM