Re: [PATCH v4 06/10] KVM: MMU: fast path of handling guest page fault

[Marcelo Tosatti <mtosatti@redhat.com>]

On Sat, Apr 28, 2012 at 02:10:33PM +0800, Xiao Guangrong wrote:
> On 04/27/2012 10:52 PM, Marcelo Tosatti wrote:
> 
> 
> >> Actually, in this patch, all the spte update is under mmu-lock, and we
> >> lockless-ly read spte , but the spte will be verified again after holding
> >> mmu-lock.
> > 
> > Yes but the objective you are aiming for is to read and write sptes
> > without mmu_lock. That is, i am not talking about this patch. 
> > Please read carefully the two examples i gave (separated by "example)").
> > 
> 
> 
> Thanks for your patience, Marcelo!
> 
> >> +	spin_lock(&vcpu->kvm->mmu_lock);
> >> +
> >> +	/* The spte has been changed. */
> >> +	if (*sptep != spte)
> >> +		goto exit;
> >> +
> >> +	gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
> >> +
> >> +	*sptep = spte | PT_WRITABLE_MASK;
> >> +	mark_page_dirty(vcpu->kvm, gfn);
> >> +
> >> +exit:
> >> +	spin_unlock(&vcpu->kvm->mmu_lock);
> >>
> >> Is not the same as both read/update spte under mmu-lock?
> >>
> >> Hmm, this is what you want?
> > 
> > The rules for code under mmu_lock should be:
> > 
> > 1) Spte updates under mmu lock must always be atomic and 
> > with locked instructions.
> 
> 
> How about treating the spte is 'volatile' if the spte can be
> updated out of mmu-lock? In this case, the update is always
> atomic.
> 
> The piece of code:
> 
> +static bool spte_can_be_writable(u64 spte)
> +{
> +	u64 mask = SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE;
> +
> +	return (spte & mask) == mask;
> +}
> +
> +static bool spte_can_lockless_update(u64 spte)
> +{
> +	return  !is_writable_pte(spte) && spte_can_be_writable(spte);
> +}
> +
>  static bool spte_has_volatile_bits(u64 spte)
>  {
> +	/*
> +	 * Always atomicly update spte if it can be updated
> +	 * out of mmu-lock.
> +	 */
> +	if (spte_can_lockless_update(spte))
> +		return true;
> +
> 
> > 2) Spte values must be read once, and appropriate action
> > must be taken when writing them back in case their value 
> > has changed (remote TLB flush might be required).
> > 
> 
> 
> Okay, may be i get your idea now. :)
> 
> I will fix mmu_spte_update, let it to return the latest old value which
> will be checked in the caller before it is updated.
> 
> > The maintenance of:
> > 
> > - gpte writable bit 
> > - protected by dirty log
> > 
> > Bits is tricky. We should think of a way to simplify things 
> > and get rid of them (or at least one of them), if possible.
> > 
> 
> Maybe SPTE_MMU_WRITEABLE is sufficient, the second bit will be dropped.
> 
> Marcelo, do you satisfied with this patch?

It is getting better, but not yet, there are still reads of sptep
scattered all over (as mentioned before, i think a pattern of read spte
once, work on top of that, atomically write and then deal with results
_everywhere_ (where mmu lock is held) is more consistent.

        /*
         * If we overwrite a writable spte with a read-only one we
         * should flush remote TLBs. Otherwise rmap_write_protect
         * will find a read-only spte, even though the writable spte
         * might be cached on a CPU's TLB.
         */
        if (is_writable_pte(entry) && !is_writable_pte(*sptep))
                kvm_flush_remote_tlbs(vcpu->kvm);

This is inconsistent with the above obviously.