Re: [patch 09/10] KVM: MMU: out of sync shadow core v2

Avi Kivity
2008-09-22T20:41:40Z

Marcelo Tosatti wrote:
>>> + while (parent->unsync_children) {
>>> + for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
>>> + u64 ent = sp->spt[i];
>>> +
>>> + if (is_shadow_present_pte(ent)) {
>>> + struct kvm_mmu_page *child;
>>> + child = page_header(ent & PT64_BASE_ADDR_MASK);
>>>
>> What does this do?
>>
>
> Walks all children of given page with no efficiency. Its replaced later
> by the bitmap version.
>
>

I don't understand how the variables sp, child, and parent interact. You
either need recursion or an explicit stack?

>>> +static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
>>> +{
>>> + if (sp->role.glevels != vcpu->arch.mmu.root_level) {
>>> + kvm_mmu_zap_page(vcpu->kvm, sp);
>>> + return 1;
>>> + }
>>>
>>>
>> Suppose we switch to real mode, touch a pte, switch back. Is this handled?
>>
>
> The shadow page will go unsync on pte touch and resynced as soon as its
> visible (after return to paging).
>
> Or, while still in real mode, it might be zapped by
> kvm_mmu_get_page->kvm_sync_page.
>
> Am I missing something?
>
>

I guess I was. Ok.

>
>>> static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
>>> - return 0;
>>> + return ret;
>>> }
>>>
>>>
>> Why does the caller care if zap also zapped some other random pages? To
>> restart walking the list?
>>
>
> Yes. The next element for_each_entry_safe saved could have been zapped.
>
>

Ouch. Ouch.

I hate doing this. Can see no alternative though.

>>> + /* don't unsync if pagetable is shadowed with multiple roles */
>>> + hlist_for_each_entry_safe(s, node, n, bucket, hash_link) {
>>> + if (s->gfn != sp->gfn || s->role.metaphysical)
>>> + continue;
>>> + if (s->role.word != sp->role.word)
>>> + return 1;
>>> + }
>>>
>>>
>> This will happen for nonpae paging. But why not allow it? Zap all
>> unsynced pages on mode switch.
>>
>> Oh, if a page is both a page directory and page table, yes.
>>
>
> Yes.
>
>
>> So to allow nonpae oos, check the level instead.
>>
>
> Windows 2008 64-bit has all sorts of sharing a pagetable at multiple
> levels too.
>
>

We still want to allow oos for the two quadrants of a nonpae shadow page.

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