Re: [PULL v2 07/14] accel/tcg: Use interval tree for user-only page tracking

[Richard Henderson]

On 12/23/22 06:32, Ilya Leoshkevich wrote:
> > +    mmap_lock();
> > +    p = pageflags_find(address, address);
> > +    mmap_unlock();
>
> How does the code ensure that p is not freed here?
>
> > +    return p ? p->flags : 0;

Yep, need to use g_free_rcu.

> > +    while (true) {
> > +        PageFlagsNode *p = pageflags_find(start, last);
>
> We can end up here without mmap_lock if we come from the syscall code.
> Do we need a retry like in page_get_flags()?

Yep, need to retry.


> Speaking of which: does lock_user() actually guarantee that it's safe
> to access the respective pages until unlock_user()? If yes, doesn't
> this mean that mmap_lock must be held between the two? And if no, and
> the SEGV handler is already supposed to gracefully handle SEGVs in
> syscall.c, do we need to call access_ok_untagged() there at all?

No, it doesn't, really guarantee anything.  We can't hold a lock across a blocking 
syscall, so lock_user() can't really lock.  We do need to page_unprotect writable pages, 
which is the main thing accomplished by the access_ok check.

> > +        if (qemu_host_page_size <= TARGET_PAGE_SIZE) {
> > +            start = address & TARGET_PAGE_MASK;
> > +            len = TARGET_PAGE_SIZE;
> > +            prot = p->flags | PAGE_WRITE;
> > +            pageflags_set_clear(start, start + len - 1, PAGE_WRITE, 0);
> > +            current_tb_invalidated = tb_invalidate_phys_page_unwind(start, pc);
>
> When we come from page_check_range(), pc == 0 and the assertion in
> tb_invalidate_phys_page_unwind() fires.

Yep, the assertion is wrong.


> Should we pass
> current_cpu->cc->get_pc() to page_unprotect() instead of 0, so that
> current_tb is resolved to the TB that invoked the syscall?

It's not a guest pc, but the host unwind pc, so, no.


r~