Re: [PATCH 0/4] Unlocked TLB flush

[Avi Kivity <avi@redhat.com>]

On 05/08/2012 04:27 AM, Marcelo Tosatti wrote:
> On Mon, May 07, 2012 at 10:25:34PM -0300, Marcelo Tosatti wrote:
> > On Thu, May 03, 2012 at 02:22:58PM +0300, Avi Kivity wrote:
> > > This patchset implements unlocked TLB flushing for KVM.  An operation that
> > > generates stale TLB entries can mark the TLB as dirty instead of flushing
> > > immediately, and then flush after releasing mmu_lock but before returning
> > > to the guest or the caller.  A few call sites are converted too.
> > > 
> > > Note not all call sites are easily convertible; as an example, sync_page()
> > > must flush before reading the guest page table.
> > 
> > Huh? Are you referring to:
> > 
> >  * Note:
> >  *   We should flush all tlbs if spte is dropped even though guest is
> >  *   responsible for it. Since if we don't,
> >  *   kvm_mmu_notifier_invalidate_page
> >  *   and kvm_mmu_notifier_invalidate_range_start detect the mapping page
> >  *   isn't
> >  *   used by guest then tlbs are not flushed, so guest is allowed to
> >  *   access the
> >  *   freed pages.
> >  *   And we increase kvm->tlbs_dirty to delay tlbs flush in this case.
> > 
> > With an increased dirtied_count the flush can be performed
> > by kvm_mmu_notifier_invalidate_page.
>
> Which is what patch 1 does. Your comment regarding sync_page()
> above is what is outdated, unless i am missing something.

I wasn't referring to that.  sync_page() (and page_fault()) must be sure
the guest page table is write-protected before reading gptes from it. 
(page_fault() reads it before write protection, but verifies it afterwards:

        /*
         * Verify that the gpte in the page we've just write
         * protected is still there.
         */
        if (FNAME(gpte_changed)(vcpu, gw, it.level - 1))
            goto out_gpte_changed;

so we must kvm_cond_flush_remote_tlbs() before calling gpte_changed().

-- 
error compiling committee.c: too many arguments to function