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

Avi Kivity
2010-07-03T12:08:29Z

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?

> @@ -322,6 +334,12 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
> level = iterator.level;
> sptep = iterator.sptep;
> if (iterator.level == hlevel) {
> + if (check&& level == gw->level&&
> + !FNAME(check_level_mapping)(vcpu, gw, hlevel)) {
> + kvm_release_pfn_clean(pfn);
> + break;
> + }
> +
>

Now we check here...

> mmu_set_spte(vcpu, sptep, access,
> gw->pte_access& access,
> user_fault, write_fault,
> @@ -376,10 +394,10 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
> sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
> direct, access, sptep);
> 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]) {
> + if (hlevel == level - 1)
> + check = false;
> +
> + if (!FNAME(check_level_mapping)(vcpu, gw, level - 1)) {
>

... and here? Why?

(looking at the code, we have a call to kvm_host_page_size() on every
page fault, that takes mmap_sem... that's got to impact scaling)

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