[PATCH 0/2] accel/tcg: fix page invalidation in tb_invalidate_phys_range()

[PATCH 0/2] accel/tcg: fix page invalidation in tb_invalidate_phys_range()

[Mark Cave-Ayland]

This series contains 2 patches: the first is a fix for page invalidation in
tb_invalidate_phys_range() which resolves the crash reported by Howard and
Cédric when booting MacOS 9 under qemu-system-ppc -M mac99,via=pmu.

The second patch adds an assert() to tb_invalidate_phys_page_range__locked()
which is enabled by --enable-debug-tcg to ensure that both the start and last
addresses are within the same target page.

I've confirmed that this assert() is first triggered by the commit that
initially introduced the bug e506ad6a05 ("accel/tcg: Pass last not end to
tb_invalidate_phys_range") when building QEMU with --enable-debug and
doesn't trigger after the series is applied.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (2):
  accel/tcg: fix start page passed to
    tb_invalidate_phys_page_range__locked()
  accel/tcg: add assert() check in
    tb_invalidate_phys_page_range__locked()

 accel/tcg/tb-maint.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

-- 
2.30.2

[PATCH 1/2] accel/tcg: fix start page passed to tb_invalidate_phys_page_range__locked()

[Mark Cave-Ayland]

Due to a copy-paste error in tb_invalidate_phys_range() the start address of the
invalidation range was being passed to tb_invalidate_phys_page_range__locked()
instead of the start address of the current page.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Fixes: e506ad6a05 ("accel/tcg: Pass last not end to tb_invalidate_phys_range")
---
 accel/tcg/tb-maint.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index 3541419845..33ea1aadd1 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -1182,15 +1182,17 @@ void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t last)
     index_last = last >> TARGET_PAGE_BITS;
     for (index = start >> TARGET_PAGE_BITS; index <= index_last; index++) {
         PageDesc *pd = page_find(index);
-        tb_page_addr_t bound;
+        tb_page_addr_t page_start, page_last;
 
         if (pd == NULL) {
             continue;
         }
         assert_page_locked(pd);
-        bound = (index << TARGET_PAGE_BITS) | ~TARGET_PAGE_MASK;
-        bound = MIN(bound, last);
-        tb_invalidate_phys_page_range__locked(pages, pd, start, bound, 0);
+        page_start = index << TARGET_PAGE_BITS;
+        page_last = page_start | ~TARGET_PAGE_MASK;
+        page_last = MIN(page_last, last);
+        tb_invalidate_phys_page_range__locked(pages, pd,
+                                              page_start, page_last, 0);
     }
     page_collection_unlock(pages);
 }
-- 
2.30.2

[PATCH 2/2] accel/tcg: add assert() check in tb_invalidate_phys_page_range__locked()

[Mark Cave-Ayland]

Add an assert() check in tb_invalidate_phys_page_range__locked() to ensure that
both the start and last addresses are within the same target page. Note that
due to performance concerns the check is only enabled when QEMU is configured
with --enable-debug-tcg.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 accel/tcg/tb-maint.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index 33ea1aadd1..8cd730dcb0 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -1092,6 +1092,10 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
     TranslationBlock *current_tb = retaddr ? tcg_tb_lookup(retaddr) : NULL;
 #endif /* TARGET_HAS_PRECISE_SMC */
 
+#ifdef CONFIG_DEBUG_TCG
+    assert((last & TARGET_PAGE_MASK) == (start & TARGET_PAGE_MASK));
+#endif
+
     /*
      * We remove all the TBs in the range [start, last].
      * XXX: see if in some cases it could be faster to invalidate all the code
-- 
2.30.2

Re: [PATCH 2/2] accel/tcg: add assert() check in tb_invalidate_phys_page_range__locked()

[Philippe Mathieu-Daudé]

On 29/6/23 10:25, Mark Cave-Ayland wrote:
> Add an assert() check in tb_invalidate_phys_page_range__locked() to ensure that
> both the start and last addresses are within the same target page. Note that
> due to performance concerns the check is only enabled when QEMU is configured
> with --enable-debug-tcg.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   accel/tcg/tb-maint.c | 4 ++++
>   1 file changed, 4 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/2] accel/tcg: fix start page passed to tb_invalidate_phys_page_range__locked()

[Cédric Le Goater]

On 6/29/23 10:25, Mark Cave-Ayland wrote:
> Due to a copy-paste error in tb_invalidate_phys_range() the start address of the
> invalidation range was being passed to tb_invalidate_phys_page_range__locked()
> instead of the start address of the current page.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: e506ad6a05 ("accel/tcg: Pass last not end to tb_invalidate_phys_range")


Cc: qemu-stable@nongnu.org

> ---
>   accel/tcg/tb-maint.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
> index 3541419845..33ea1aadd1 100644
> --- a/accel/tcg/tb-maint.c
> +++ b/accel/tcg/tb-maint.c
> @@ -1182,15 +1182,17 @@ void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t last)
>       index_last = last >> TARGET_PAGE_BITS;
>       for (index = start >> TARGET_PAGE_BITS; index <= index_last; index++) {
>           PageDesc *pd = page_find(index);
> -        tb_page_addr_t bound;
> +        tb_page_addr_t page_start, page_last;
>   
>           if (pd == NULL) {
>               continue;
>           }
>           assert_page_locked(pd);
> -        bound = (index << TARGET_PAGE_BITS) | ~TARGET_PAGE_MASK;
> -        bound = MIN(bound, last);
> -        tb_invalidate_phys_page_range__locked(pages, pd, start, bound, 0);
> +        page_start = index << TARGET_PAGE_BITS;
> +        page_last = page_start | ~TARGET_PAGE_MASK;
> +        page_last = MIN(page_last, last);
> +        tb_invalidate_phys_page_range__locked(pages, pd,
> +                                              page_start, page_last, 0);
>       }
>       page_collection_unlock(pages);
>   }

Re: [PATCH 2/2] accel/tcg: add assert() check in tb_invalidate_phys_page_range__locked()

[Michael Tokarev]

29.06.2023 11:25, Mark Cave-Ayland wrote:
> Add an assert() check in tb_invalidate_phys_page_range__locked() to ensure that
> both the start and last addresses are within the same target page. Note that
> due to performance concerns the check is only enabled when QEMU is configured
> with --enable-debug-tcg.

Performance concerns? That's two ANDs and on compare, - is it really that performance
critical?

I'm just asking, I dunno.

Thanks,

/mjt

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   accel/tcg/tb-maint.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
> index 33ea1aadd1..8cd730dcb0 100644
> --- a/accel/tcg/tb-maint.c
> +++ b/accel/tcg/tb-maint.c
> @@ -1092,6 +1092,10 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
>       TranslationBlock *current_tb = retaddr ? tcg_tb_lookup(retaddr) : NULL;
>   #endif /* TARGET_HAS_PRECISE_SMC */
>   
> +#ifdef CONFIG_DEBUG_TCG
> +    assert((last & TARGET_PAGE_MASK) == (start & TARGET_PAGE_MASK));
> +#endif
> +
>       /*
>        * We remove all the TBs in the range [start, last].
>        * XXX: see if in some cases it could be faster to invalidate all the code

Re: [PATCH 2/2] accel/tcg: add assert() check in tb_invalidate_phys_page_range__locked()

[BALATON Zoltan]

On Fri, 30 Jun 2023, Michael Tokarev wrote:
> 29.06.2023 11:25, Mark Cave-Ayland wrote:
>> Add an assert() check in tb_invalidate_phys_page_range__locked() to ensure 
>> that
>> both the start and last addresses are within the same target page. Note 
>> that
>> due to performance concerns the check is only enabled when QEMU is 
>> configured
>> with --enable-debug-tcg.
>
> Performance concerns? That's two ANDs and on compare, - is it really that 
> performance
> critical?
>
> I'm just asking, I dunno.

If something is called frequently enough any small computaion can add up. 
In this case invalidating pages is probably a performance hit already and 
hopefully does not happen too often but then it's a good idea not to make 
it worse. As this is not something that should or could normally happen 
and only checks for programming errors I think it's good idea to only do 
it when debugging.

Regards,
BALATON Zoltan

> Thanks,
>
> /mjt
>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   accel/tcg/tb-maint.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>> 
>> diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
>> index 33ea1aadd1..8cd730dcb0 100644
>> --- a/accel/tcg/tb-maint.c
>> +++ b/accel/tcg/tb-maint.c
>> @@ -1092,6 +1092,10 @@ tb_invalidate_phys_page_range__locked(struct 
>> page_collection *pages,
>>       TranslationBlock *current_tb = retaddr ? tcg_tb_lookup(retaddr) : 
>> NULL;
>>   #endif /* TARGET_HAS_PRECISE_SMC */
>>   +#ifdef CONFIG_DEBUG_TCG
>> +    assert((last & TARGET_PAGE_MASK) == (start & TARGET_PAGE_MASK));
>> +#endif
>> +
>>       /*
>>        * We remove all the TBs in the range [start, last].
>>        * XXX: see if in some cases it could be faster to invalidate all the 
>> code
>
>
>

Re: [PATCH 0/2] accel/tcg: fix page invalidation in tb_invalidate_phys_range()

[Richard Henderson]

On 6/29/23 10:25, Mark Cave-Ayland wrote:
> This series contains 2 patches: the first is a fix for page invalidation in
> tb_invalidate_phys_range() which resolves the crash reported by Howard and
> Cédric when booting MacOS 9 under qemu-system-ppc -M mac99,via=pmu.
> 
> The second patch adds an assert() to tb_invalidate_phys_page_range__locked()
> which is enabled by --enable-debug-tcg to ensure that both the start and last
> addresses are within the same target page.
> 
> I've confirmed that this assert() is first triggered by the commit that
> initially introduced the bug e506ad6a05 ("accel/tcg: Pass last not end to
> tb_invalidate_phys_range") when building QEMU with --enable-debug and
> doesn't trigger after the series is applied.
> 
> Signed-off-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (2):
>    accel/tcg: fix start page passed to
>      tb_invalidate_phys_page_range__locked()
>    accel/tcg: add assert() check in
>      tb_invalidate_phys_page_range__locked()

Queued to tcg-next, with some wording changes.
And to use tcg_debug_assert instead of the ifdef.


r~