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

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

[Max Filippov]

Hi Sergey,

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?

-- 
Thanks.
-- Max

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

[Sergey Fedorov]

On 15/05/16 21:58, Max Filippov wrote:
> Hi Sergey,
>
> 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?
>

Hi Max,

That's too strange. How do I run the test?

Kind regards,
Sergey

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

[Max Filippov]

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

-- 
Thanks.
-- Max

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

[Sergey Fedorov]

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.

Kind regards,
Sergey

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

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