lkml.org 
[lkml]   [2012]   [Apr]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 09/13] KVM: MMU: get expected spte out of mmu-lock
    On 04/05/2012 09:25 PM, Xiao Guangrong wrote:
    > On 04/01/2012 11:53 PM, Avi Kivity wrote:
    >
    > > On 03/29/2012 11:25 AM, Xiao Guangrong wrote:
    > >> It depends on PTE_LIST_WRITE_PROTECT bit in rmap which let us quickly know
    > >> whether the page is writable out of mmu-lock
    > >>
    > >> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
    > >> ---
    > >> arch/x86/kvm/mmu.c | 17 +++++++++++++----
    > >> arch/x86/kvm/paging_tmpl.h | 2 +-
    > >> 2 files changed, 14 insertions(+), 5 deletions(-)
    > >>
    > >> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
    > >> index 3887a07..c029185 100644
    > >> --- a/arch/x86/kvm/mmu.c
    > >> +++ b/arch/x86/kvm/mmu.c
    > >> @@ -1148,6 +1148,12 @@ static int rmap_write_protect(struct kvm *kvm, u64 gfn)
    > >>
    > >> *rmapp |= PTE_LIST_WRITE_PROTECT;
    > >>
    > >> + /*
    > >> + * Setting PTE_LIST_WRITE_PROTECT bit before doing page
    > >> + * write-protect.
    > >> + */
    > >> + smp_mb();
    > >> +
    > >
    > > wmb only needed.
    > >
    >
    >
    > We should ensure setting this bit before reading spte, it cooperates with
    > fast page fault path to avoid this case:
    >
    > On fast page fault path: On rmap_write_protect path:
    > read spte: old_spte = *spte
    > (reading spte is reordered to the front of
    > setting PTE_LIST_WRITE_PROTECT bit)
    > set spte.identification
    > smp_mb
    > if (!rmap.PTE_LIST_WRITE_PROTECT)
    > set rmap.PTE_LIST_WRITE_PROTECT
    > cmpxchg(sptep, spte, spte | WRITABLE)
    > see old_spte.identification is not set,
    > so it does not write-protect this page
    > OOPS!!!

    Ah, so it's protecting two paths at the same time: rmap.write_protect ->
    fast page fault, and lock(sptep) -> write protect.

    The whole thing needs to be documented very carefully in locking.txt,
    otherwise mmu.c will be write-protected to everyone except you.

    > > Would it be better to store this bit in all the sptes instead? We're
    > > touching them in any case. More work to clear them, but
    > > un-write-protecting a page is beneficial anyway as it can save a fault.
    > >
    >
    > There are two reasons:
    > - if we set this bit in rmap, we can do the quickly check to see the page is
    > writble before doing shadow page walking.
    >
    > - since a full barrier is needed, we should use smp_mb for every spte like this:
    >
    > while ((spte = rmap_next(rmapp, spte))) {
    > read spte
    > smp_mb
    > write-protect spte
    > }
    >
    > smp_mb is called in the loop, i think it is not good, yes?

    Yes, agree.

    > If you just want to save the fault, we can let all spte to be writeable in
    > mmu_need_write_protect, but we should cache gpte access bits into spte firstly.
    > It should be another patchset i think. :)

    Yes.

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



    \
     
     \ /
      Last update: 2012-04-09 14:31    [W:0.025 / U:2.724 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site