[Qemu-devel] [PATCH] translate-all: fix locking of TBs whose two pages share the same physical page

[Qemu-devel] [PATCH] translate-all: fix locking of TBs whose two pages share the same physical page

[Emilio G. Cota]

Commit 0b5c91f ("translate-all: use per-page locking in !user-mode",
2018-06-15) introduced per-page locking. It assumed that the physical
pages corresponding to a TB (at most two pages) are always distinct,
which is wrong. For instance, an xtensa test provided by Max Filippov
is broken by the commit, since the test maps two virtual pages
to the same physical page:

	virt1: 7fff, virt2: 8000
	phys1 6000fff, phys2 6000000

Fix it by removing the assumption from page_lock_pair.
If the two physical page addresses are equal, we only lock
the PageDesc once. Note that the two callers of page_lock_pair,
namely page_unlock_tb and tb_link_page, are also updated so that
we do not try to unlock the same PageDesc twice.

Fixes: 0b5c91f74f3c83a36f37740969df8c775c997e69
Reported-by: Max Filippov <jcmvbkbc@gmail.com>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 accel/tcg/translate-all.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index f0c3fd4..8e0203e 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -669,9 +669,15 @@ static inline void page_lock_tb(const TranslationBlock *tb)
 
 static inline void page_unlock_tb(const TranslationBlock *tb)
 {
-    page_unlock(page_find(tb->page_addr[0] >> TARGET_PAGE_BITS));
+    PageDesc *p1 = page_find(tb->page_addr[0] >> TARGET_PAGE_BITS);
+
+    page_unlock(p1);
     if (unlikely(tb->page_addr[1] != -1)) {
-        page_unlock(page_find(tb->page_addr[1] >> TARGET_PAGE_BITS));
+        PageDesc *p2 = page_find(tb->page_addr[1] >> TARGET_PAGE_BITS);
+
+        if (p2 != p1) {
+            page_unlock(p2);
+        }
     }
 }
 
@@ -850,22 +856,34 @@ static void page_lock_pair(PageDesc **ret_p1, tb_page_addr_t phys1,
                            PageDesc **ret_p2, tb_page_addr_t phys2, int alloc)
 {
     PageDesc *p1, *p2;
+    tb_page_addr_t page1;
+    tb_page_addr_t page2;
 
     assert_memory_lock();
-    g_assert(phys1 != -1 && phys1 != phys2);
-    p1 = page_find_alloc(phys1 >> TARGET_PAGE_BITS, alloc);
+    g_assert(phys1 != -1);
+
+    page1 = phys1 >> TARGET_PAGE_BITS;
+    page2 = phys2 >> TARGET_PAGE_BITS;
+
+    p1 = page_find_alloc(page1, alloc);
     if (ret_p1) {
         *ret_p1 = p1;
     }
     if (likely(phys2 == -1)) {
         page_lock(p1);
         return;
+    } else if (page1 == page2) {
+        page_lock(p1);
+        if (ret_p2) {
+            *ret_p2 = p1;
+        }
+        return;
     }
-    p2 = page_find_alloc(phys2 >> TARGET_PAGE_BITS, alloc);
+    p2 = page_find_alloc(page2, alloc);
     if (ret_p2) {
         *ret_p2 = p2;
     }
-    if (phys1 < phys2) {
+    if (page1 < page2) {
         page_lock(p1);
         page_lock(p2);
     } else {
@@ -1623,7 +1641,7 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
         tb = existing_tb;
     }
 
-    if (p2) {
+    if (p2 && p2 != p) {
         page_unlock(p2);
     }
     page_unlock(p);
-- 
2.7.4

Re: [Qemu-devel] [PATCH] translate-all: fix locking of TBs whose two pages share the same physical page

[Max Filippov]

On Mon, Jun 25, 2018 at 9:31 AM, Emilio G. Cota <cota@braap.org> wrote:
> Commit 0b5c91f ("translate-all: use per-page locking in !user-mode",
> 2018-06-15) introduced per-page locking. It assumed that the physical
> pages corresponding to a TB (at most two pages) are always distinct,
> which is wrong. For instance, an xtensa test provided by Max Filippov
> is broken by the commit, since the test maps two virtual pages
> to the same physical page:
>
>         virt1: 7fff, virt2: 8000
>         phys1 6000fff, phys2 6000000
>
> Fix it by removing the assumption from page_lock_pair.
> If the two physical page addresses are equal, we only lock
> the PageDesc once. Note that the two callers of page_lock_pair,
> namely page_unlock_tb and tb_link_page, are also updated so that
> we do not try to unlock the same PageDesc twice.
>
> Fixes: 0b5c91f74f3c83a36f37740969df8c775c997e69
> Reported-by: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  accel/tcg/translate-all.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)

Tested-by: Max Filippov <jcmvbkbc@gmail.com>
Thank you!

-- Max

Re: [Qemu-devel] [PATCH] translate-all: fix locking of TBs whose two pages share the same physical page

[Philippe Mathieu-Daudé]

On 06/25/2018 01:31 PM, Emilio G. Cota wrote:
> Commit 0b5c91f ("translate-all: use per-page locking in !user-mode",
> 2018-06-15) introduced per-page locking. It assumed that the physical
> pages corresponding to a TB (at most two pages) are always distinct,
> which is wrong. For instance, an xtensa test provided by Max Filippov
> is broken by the commit, since the test maps two virtual pages
> to the same physical page:
> 
> 	virt1: 7fff, virt2: 8000
> 	phys1 6000fff, phys2 6000000
> 
> Fix it by removing the assumption from page_lock_pair.
> If the two physical page addresses are equal, we only lock
> the PageDesc once. Note that the two callers of page_lock_pair,
> namely page_unlock_tb and tb_link_page, are also updated so that
> we do not try to unlock the same PageDesc twice.
> 
> Fixes: 0b5c91f74f3c83a36f37740969df8c775c997e69
> Reported-by: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>

$ make check-tcg (qemu:debian-xtensa-cross)

Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  accel/tcg/translate-all.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index f0c3fd4..8e0203e 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -669,9 +669,15 @@ static inline void page_lock_tb(const TranslationBlock *tb)
>  
>  static inline void page_unlock_tb(const TranslationBlock *tb)
>  {
> -    page_unlock(page_find(tb->page_addr[0] >> TARGET_PAGE_BITS));
> +    PageDesc *p1 = page_find(tb->page_addr[0] >> TARGET_PAGE_BITS);
> +
> +    page_unlock(p1);
>      if (unlikely(tb->page_addr[1] != -1)) {
> -        page_unlock(page_find(tb->page_addr[1] >> TARGET_PAGE_BITS));
> +        PageDesc *p2 = page_find(tb->page_addr[1] >> TARGET_PAGE_BITS);
> +
> +        if (p2 != p1) {
> +            page_unlock(p2);
> +        }
>      }
>  }
>  
> @@ -850,22 +856,34 @@ static void page_lock_pair(PageDesc **ret_p1, tb_page_addr_t phys1,
>                             PageDesc **ret_p2, tb_page_addr_t phys2, int alloc)
>  {
>      PageDesc *p1, *p2;
> +    tb_page_addr_t page1;
> +    tb_page_addr_t page2;
>  
>      assert_memory_lock();
> -    g_assert(phys1 != -1 && phys1 != phys2);
> -    p1 = page_find_alloc(phys1 >> TARGET_PAGE_BITS, alloc);
> +    g_assert(phys1 != -1);
> +
> +    page1 = phys1 >> TARGET_PAGE_BITS;
> +    page2 = phys2 >> TARGET_PAGE_BITS;
> +
> +    p1 = page_find_alloc(page1, alloc);
>      if (ret_p1) {
>          *ret_p1 = p1;
>      }
>      if (likely(phys2 == -1)) {
>          page_lock(p1);
>          return;
> +    } else if (page1 == page2) {
> +        page_lock(p1);
> +        if (ret_p2) {
> +            *ret_p2 = p1;
> +        }
> +        return;
>      }
> -    p2 = page_find_alloc(phys2 >> TARGET_PAGE_BITS, alloc);
> +    p2 = page_find_alloc(page2, alloc);
>      if (ret_p2) {
>          *ret_p2 = p2;
>      }
> -    if (phys1 < phys2) {
> +    if (page1 < page2) {
>          page_lock(p1);
>          page_lock(p2);
>      } else {
> @@ -1623,7 +1641,7 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>          tb = existing_tb;
>      }
>  
> -    if (p2) {
> +    if (p2 && p2 != p) {
>          page_unlock(p2);
>      }
>      page_unlock(p);
> 

Re: [Qemu-devel] [PATCH] translate-all: fix locking of TBs whose two pages share the same physical page

[Richard Henderson]

On 06/25/2018 09:31 AM, Emilio G. Cota wrote:
> +    } else if (page1 == page2) {
> +        page_lock(p1);
> +        if (ret_p2) {
> +            *ret_p2 = p1;

I think you should set NULL here...

> @@ -1623,7 +1641,7 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>          tb = existing_tb;
>      }
>  
> -    if (p2) {
> +    if (p2 && p2 != p) {
>          page_unlock(p2);

... so that you need no change here.
Otherwise it looks good.


r~

Re: [Qemu-devel] [PATCH] translate-all: fix locking of TBs whose two pages share the same physical page

[Emilio G. Cota]

On Tue, Jun 26, 2018 at 19:28:19 -0700, Richard Henderson wrote:
> On 06/25/2018 09:31 AM, Emilio G. Cota wrote:
> > +    } else if (page1 == page2) {
> > +        page_lock(p1);
> > +        if (ret_p2) {
> > +            *ret_p2 = p1;
> 
> I think you should set NULL here...
> 
> > @@ -1623,7 +1641,7 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
> >          tb = existing_tb;
> >      }
> >  
> > -    if (p2) {
> > +    if (p2 && p2 != p) {
> >          page_unlock(p2);
> 
> ... so that you need no change here.
> Otherwise it looks good.

I did that initially. However, note that if we do that then
the second page is not added to the list of pages for this
TB (via tb_page_add), which breaks the provided test case.

    page_lock_pair(&p, phys_pc, &p2, phys_page2, 1);
    tb_page_add(p, tb, 0, phys_pc & TARGET_PAGE_MASK);
    if (p2) {
        tb_page_add(p2, tb, 1, phys_page2);
    } else {
        tb->page_addr[1] = -1;
    }

Regardless of whether p1 and p2 point to the same physical page,
the fact that the TB goes across two virtual pages should be
preserved, and in this case tb_page_add must be called twice.

Thanks,

		Emilio

Re: [Qemu-devel] [PATCH] translate-all: fix locking of TBs whose two pages share the same physical page

[Richard Henderson]

On 06/27/2018 09:47 AM, Emilio G. Cota wrote:
>>> -    if (p2) {
>>> +    if (p2 && p2 != p) {
>>>          page_unlock(p2);
>>
>> ... so that you need no change here.
>> Otherwise it looks good.
> 
> I did that initially. However, note that if we do that then
> the second page is not added to the list of pages for this
> TB (via tb_page_add), which breaks the provided test case.
> 
>     page_lock_pair(&p, phys_pc, &p2, phys_page2, 1);
>     tb_page_add(p, tb, 0, phys_pc & TARGET_PAGE_MASK);
>     if (p2) {
>         tb_page_add(p2, tb, 1, phys_page2);
>     } else {
>         tb->page_addr[1] = -1;
>     }
> 
> Regardless of whether p1 and p2 point to the same physical page,
> the fact that the TB goes across two virtual pages should be
> preserved, and in this case tb_page_add must be called twice.

Hmm.  Ok.  Queued.


r~