Re: [Qemu-devel] tb_invalidate_phys_range and tb_invalidate_phys_page_range

["Emilio G. Cota" <cota@braap.org>]

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