Re: [Qemu-devel] tb_invalidate_phys_range and tb_invalidate_phys_page_range
On Sat, Jul 29, 2017 at 10:23:42 +0100, Peter Maydell wrote: > On 29 July 2017 at 01:33, Emilio G. Cotawrote: (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
Re: [Qemu-devel] tb_invalidate_phys_range and tb_invalidate_phys_page_range
On 29 July 2017 at 01:33, Emilio G. Cotawrote: > 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
[Qemu-devel] tb_invalidate_phys_range and tb_invalidate_phys_page_range
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