Re: [PATCH 15/24] accel/tcg: Use interval tree for TBs in user-only mode

[Richard Henderson <richard.henderson@linaro.org>]

On 10/26/22 01:58, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> Begin weaning user-only away from PageDesc.
>>
>> Since, for user-only, all TB (and page) manipulation is done with
>> a single mutex, and there is no virtual/physical discontinuity to
>> split a TB across discontinuous pages, place all of the TBs into
>> a single IntervalTree. This makes it trivial to find all of the
>> TBs intersecting a range.
>>
>> Retain the existing PageDesc + linked list implementation for
>> system mode.  Move the portion of the implementation that overlaps
>> the new user-only code behind the common ifdef.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   accel/tcg/internal.h      |  16 +-
>>   include/exec/exec-all.h   |  43 ++++-
>>   accel/tcg/tb-maint.c      | 388 ++++++++++++++++++++++----------------
>>   accel/tcg/translate-all.c |   4 +-
>>   4 files changed, 280 insertions(+), 171 deletions(-)
>>
> <snip>
>> diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
>> index c8e921089d..14e8e47a6a 100644
>> --- a/accel/tcg/tb-maint.c
>> +++ b/accel/tcg/tb-maint.c
>> @@ -18,6 +18,7 @@
>>    */
>>   
>>   #include "qemu/osdep.h"
>> +#include "qemu/interval-tree.h"
>>   #include "exec/cputlb.h"
>>   #include "exec/log.h"
>>   #include "exec/exec-all.h"
>> @@ -50,6 +51,75 @@ void tb_htable_init(void)
>>       qht_init(&tb_ctx.htable, tb_cmp, CODE_GEN_HTABLE_SIZE, mode);
>>   }
> 
> I wonder for the sake of avoiding recompilation of units later on and
> having a clean separation between user and system mode it would be worth
> putting this stuff in a tb-maint-user.c?

Something like that might be possible toward the end of the series, but there's too much 
tangle in the middle.

>> +bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc)
>> +{
>> +    assert(pc != 0);
>> +#ifdef TARGET_HAS_PRECISE_SMC
>> +    assert_memory_lock();
> 
> Out of interest is this just because x86 has such a strong memory model
> you can get away with this sort of patching without explicit flushes?

Yes.

> I'm curious why this is the only arch we jump through these hoops for?

It's probably the only one for which we have extensive test cases for self-modifying code 
(aka msdos).  I can imagine that other old targets might technically require it, e.g. 
m68k, or some of the micro-controllers.

I'm actually not sure why we don't enable it everywhere -- it doesn't seem like it's 
adding lots of overhead.  But until someone reports a bug, or it gets in the way of 
multi-target, I'm happy to not enable it anywhere else.

>>       /*
>>        * We remove all the TBs in the range [start, end[.
>>        * XXX: see if in some cases it could be faster to invalidate all the code
>>        */
> 
> I'm guessing this comment is quite stale now given we try quite hard to
> avoid doing lots of code gen over and over again. The only case I can
> think of is memory clear routines after we've had code which there might
> be some heuristics we could use to detect but don't currently.

I think there's still room for exploration here, especially detecting page clearing as you 
say.


r~