Re: [Qemu-devel] [PULL 23/32] tcg: Support MMU protection regions smaller than TARGET_PAGE_SIZE

[Alex Bennée]

Peter Maydell <address@hidden> writes:

> On 28 June 2018 at 23:26, Laurent Vivier <address@hidden> wrote:
>> ./m68k-softmmu/qemu-system-m68k -M q800 \
>>     -serial none -serial mon:stdio \
>>     -kernel vmlinux-4.15.0-2-m68k \
>>     -nographic
>
> Thanks for the test case. I'm still investigating, but there
> are a couple of things happening here.
>
> First, there's a bug in get_page_addr_code()'s "is this a
> TLB miss?" condition which was introduced in commit 71b9a45330fe22:
>
>     if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
>                  (addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK)))) {
>
> takes a (not necessarily page aligned) address, and masks out
> everything but the page-aligned top half (good) and the
> TLB_INVALID bit (not good, because that could be either 0 or 1
> depending on the address). This means sometimes we'll incorrectly
> decide we got a miss in the TLB and do an unnecessary refill.
>
> The second thing that's going on here is that the m68k target
> code writes TLB entries for the same address with different
> prot bits without doing a flush in between:
>
> tlb_set_page_with_attrs: vaddr=0029b000 paddr=0x000000000029b000 prot=3 idx=0
> tlb_set_page_with_attrs: vaddr=0029b000 paddr=0x000000000029b000 prot=7 idx=0
>
> The tlb_set_page_with_attrs() code isn't expecting this, so
> we end up with two TLB entries for the same address, one in
> the main TLB and one in the victim cache TLB. The bug above
> means that we get this sequence of events:
>  * fill main TLB entry with prot=3 entry
>  * later, fill main TLB with prot=7 entry, and evict prot=3
>    entry to victim cache
>  * hit on the prot=7 entry in the main TLB
>  * refill condition incorrectly fails, but we hit in the victim cache
>  * so we pull the prot=3 entry from victim to main TLB
>  * prot=3 means "addr_code == -1", so the check of the TLB_RECHECK
>    bit succeeds
>  * in the TLB_RECHECK code we do a tlb_fill()
>  * that fills in the main TLB with a prot=7 entry again, bouncing
>    the prot=3 entry back out to the victim cache
>  * prot=7 means the addr_code is correct, so we find ourselves in
>    the "TLB_RECHECK but this is RAM" abort code path
>
> I'm not sure whether it's supposed to be the responsibility
> of the target code or the common accel/tcg code to ensure
> that we don't have multiple TLB entries for the same address.

My gut feeling is we should fail safely in the case of the guest writing
two mostly identical page entries in a row. We can check for aliasing
when we update and either evict to the victim cache or reset the vtlb
entry.

>
> thanks
> -- PMM


--
Alex Bennée