Re: [Qemu-devel] "tcg: Clean up direct block chaining safety checks" breaks target-xtensa mmu test

[Sergey Fedorov]

On 15/05/16 22:56, Sergey Fedorov wrote:
> On 15/05/16 22:53, Max Filippov wrote:
>> On Sun, May 15, 2016 at 10:38:46PM +0300, Sergey Fedorov wrote:
>>> On 15/05/16 21:58, Max Filippov wrote:
>>>> I've noticed that the commit 5b053a4a28278 (tcg: Clean up direct block
>>>> chaining safety checks) has broken tearget-xtensa test cross_page_tb
>>>> from the tests/tcg/xtensa/test_mmu.S. The test runs a TB that spans two
>>>> adjacent pages, then unmaps the second page and runs it again. It
>>>> expects an instruction fetch exception on the second run, but with the
>>>> said commit doesn't get it. Reverting that commit fixes the test.
>>>> Any suggestions?
>>> That's too strange. How do I run the test?
>> I've minimized the test case, the source and the binary are available
>> here:
>>   http://jcmvbkbc.spb.ru/~jcmvbkbc/tmp/201605152245/
>>
>> You can run it as
>>   qemu-system-xtensa -M sim -cpu dc232b -nographic -semihosting -kernel 
>> ./test_mmu.tst
>>
> Thank you for this. I'll try it tomorrow and figure out what's going wrong.

I couldn't sleep without first trying the test :) Now I really
understand why things went wrong. I mixed up 'next_tb' and 'tb' in this
piece of code:

    /* see if we can patch the calling TB. When the TB           
       spans two pages, we cannot safely do a direct             
       jump. */                                                  
    if (next_tb != 0 && tb->page_addr[1] == -1                   
        && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {            
        tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK),
                    next_tb & TB_EXIT_MASK, tb);                 
    }                                                            

So I removed 'tb->page_addr[1] == -1' check thinking it's for the last
executed TB. But actually, it checks the next TB despite there's also
the variable called 'next_tb'. Indeed, we cannot safely direct jump *to*
the TB spanning pages in system emulation because we don't take care of
direct jumps when address mapping changes. However we can do this in
user emulation because there's only static address translation and TBs
get always invalidated properly.

I'll prepare a patch and fix this tomorrow then.

Nice test and nice catch!

Thanks,
Sergey