[Qemu-devel] tb_invalidate_phys_range and tb_invalidate_phys_page_range

[Qemu-devel] tb_invalidate_phys_range and tb_invalidate_phys_page_range

[Emilio G. Cota]

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?

Thanks,

		Emilio

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 <cota@braap.org> 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

Re: [Qemu-devel] tb_invalidate_phys_range and tb_invalidate_phys_page_range

[Emilio G. Cota]

On Sat, Jul 29, 2017 at 10:23:42 +0100, Peter Maydell wrote:
> On 29 July 2017 at 01:33, Emilio G. Cota <cota@braap.org> wrote:
(snip)
> > +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 Peter.

I just sent a patch to improve this as part of the "tb lock removal"
series. The patch is here:
  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01270.html

Cheers,

		Emilio