Re: [PATCH v4 5/6] KVM: MMU: combine guest pte read between walkand pte prefetch

[Avi Kivity]

On 07/03/2010 03:16 PM, Xiao Guangrong wrote:
> Avi Kivity wrote:
>    
> > On 07/03/2010 01:31 PM, Xiao Guangrong wrote:
> >      
> > >        
> > > > See how the pte is reread inside fetch with mmu_lock held.
> > > >
> > > >
> > > >          
> > > It looks like something is broken in 'fetch' functions, this patch will
> > > fix it.
> > >
> > > Subject: [PATCH] KVM: MMU: fix last level broken in FNAME(fetch)
> > >
> > > We read the guest level out of 'mmu_lock', sometimes, the host mapping is
> > > confusion. Consider this case:
> > >
> > > VCPU0:                                              VCPU1
> > >
> > > Read guest mapping, assume the mapping is:
> > > GLV3 ->   GLV2 ->   GLV1 ->   GFNA,
> > > And in the host, the corresponding mapping is
> > > HLV3 ->   HLV2 ->   HLV1(P=0)
> > >
> > >                                                      Write GLV1 and
> > > cause the
> > >                                                      mapping point to GFNB
> > >                                                      (May occur in
> > > pte_write or
> > >                                                         invlpg path)
> > >
> > > Mapping GLV1 to GFNA
> > >
> > > This issue only occurs in the last indirect mapping, since if the middle
> > > mapping is changed, the mapping will be zapped, then it will be detected
> > > in the FNAME(fetch) path, but when it map the last level, it not checked.
> > >
> > > Fixed by also check the last level.
> > >
> > >
> > >        
> > I don't really see what is fixed.  We already check the gpte.  What's
> > special about the new scenario?
> >
> >      
> I mean is: while we map the last level, we will directly set to the pfn but
> the pfn is got by walk_addr, at this time, the guest mapping may be changed.
>
> What is the 'We already check the gpte' mean? i think i miss something :-(
>    

        if (!direct) {
            r = kvm_read_guest_atomic(vcpu->kvm,
                          gw->pte_gpa[level - 2],
&curr_pte, sizeof(curr_pte));
            if (r || curr_pte != gw->ptes[level - 2]) {
                kvm_mmu_put_page(shadow_page, sptep);
                kvm_release_pfn_clean(pfn);
                sptep = NULL;
                break;
            }
        }

the code you moved... under what scenario is it not sufficient?

--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/