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

[Avi Kivity <avi@redhat.com>]

On 07/05/2010 05:52 AM, Xiao Guangrong wrote:
>
> Avi Kivity wrote:
>
>    
>>> Umm, if we move the check before the judgment, it'll check every level,
>>> actually, only the opened mapping and the laster level need checked, so
>>> for the performance reason, maybe it's better to keep two check-point.
>>>
>>>        
>> What exactly are the conditions when you want to check?
>>
>> Perhaps we do need to check every level.  A write to a PDE (or higher
>> level) will clear the corresponding spte, but a fault immediately
>> afterwards can re-establish the spte to point to the new page.
>>
>>      
> Looks into the code more carefully, maybe this code is wrong:
>
>
>               if (!direct) {
>                       r = kvm_read_guest_atomic(vcpu->kvm,
> -                                          gw->pte_gpa[level - 2],
> +                                          gw->pte_gpa[level - 1],
>                                        &curr_pte, sizeof(curr_pte));
> -                    if (r || curr_pte != gw->ptes[level - 2]) {
> +                    if (r || curr_pte != gw->ptes[level - 1]) {
>                                  kvm_mmu_put_page(sp, sptep);
>                                  kvm_release_pfn_clean(pfn);
>                                  sptep = NULL;
>
> It should check the 'level' mapping not 'level - 1', in the later description
> i'll explain it.
>    

Right, this fixes the check for the top level, but it removes a check 
from the bottom level.

We need to move this to the top of the loop so we check all levels.  I 
guess this is why you needed to add a new check point.  But since we 
loop at least glevels times, we don't need two check points.

> Since the 'walk_addr' functions is out of mmu_lock protection, we should
> handle the guest modify its mapping between 'walk_addr' and 'fetch'.
> Now, let's consider every case may be happened while handle guest #PF.
>
> One case is host handle guest written after 'fetch', just like this order:
>
> VCPU 0		VCPU 1
> walk_addr
>                  guest modify its mapping
> fetch
>                  host handle this written(pte_write or invlpg)
>
> This case is not broken anything, even if 'fetch' setup the wrong mapping, the
> later written handler will fix it.
>    

Ok.

> Another case is host handle guest written before 'fetch', like this:
>
> CPU 0		VCPU 1
> walk_addr
>                  guest modify its mapping
>                  host handle this written(pte_write or invlpg)
> fetch
>
> We should notice this case, since the later 'fetch' will setup the wrong mapping.
>
> For example, the guest mapping which 'walk_addr' got is:
>
> GPML4E ->  GPDPE ->  GPDE ->  GPTE ->  GFNA
> (Just take small page for example, other mapping way is also applied)
>
> And, for good to describe, we abstract 'fetch''s work:
>
> for_each_shadow_entry(vcpu, addr, iterator) {
> 	if (iterator.level == hlevel)
> 		Mapping the later level
>
> 	if (is_shadow_present_pte(*sptep)&&
> 		!is_large_pte(*sptep))		<------ Point A
> 		continue;
>
> 	/* handle the non-present/wrong  middle level */
>
> 	find/create the corresponding sp<----- Point B
>
> 	if (the guest mapping is modified)<----- Point C
> 		break;
> 	setup this level's mapping<----- Point D
>
> }
>
> [
>   Note: the later level means PTE, PDE if 2M page size, PDPE if 1G page size
>   the middle level means PDE, PDPE if not using large page size / PML4E
> ]
>
> There are two cases:
>
> 1: Guest modify the middle level, for example, guest modify the GPDE.
>     a: the GPDE has corresponding sp entry, after VCPU1 handle this written,
>        the corresponding host mapping is like this:
>        HPML4E ->  HPDPE ->  HPDE
>        [ HPDE.P = 0 since VCPU1 written handler zapped it in pte_wirte ]
>
>        Under this case, it can broke Point A's judgment and Point C can detect
>        the guest mapping is modified, so it exits this function without setup the
>        mapping.
>
>        And, we should check the guest mapping at GPDE not GPTE since it's GPDE
>        modified not GPTE, it's the explanation for the front fix.
>    

Agree.

>    b: the GPDE not has corresponding sp entry(the area is firstly accessed),
>       corresponding host mapping is like this:
>       HPML4E ->  HPDPE
>       [ HPDPE.P = 0]
>
>       under this case, VCPU1 happily write GPDE without #PF since the GPDE not has shadow
>       page, it's not write-protected.
>
>       while we handle HPDPE, we will create the shadow page for GPDE
>       at Point B, then the host mapping is like this:
>       HPML4E ->  HPDPE ->  HPDE
>       [ HPDE.P = 0 ]
>       it's the same as 1.a, so do the same work as 1.a
>
>       Note: there is a trick: we should put 'Point C' behind of 'Point B', since after we
>             create sp for GPDE, the later modify GPDE will cause #PF, it can ensure later
>             check is valid
>
> 2: Guest modify the later level.
>     Form 'fetch''s abstract, we can see the late level is not checked by 'Point C', if
>     guest modified the GPTE's mapping, the wrong-mapping will be setup by 'fetch', this
>     is just this path does
>
>    

Ok.   So moving the check to before point A, and s/level - 2/level - 1/ 
should work, yes?

Should be slightly simpler since we don't need to kvm_mmu_put_page(sp, 
sptep) any more.

Thanks for the detailed explanation.

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