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

[Sergey Fedorov]

On 15/05/16 23:54, Sergey Fedorov wrote:
> 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.

http://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg02506.html

Thanks,
Sergey