Re: KVM: MMU: nuke shadowed pgtable pages and pte's on memslot destruction

Avi Kivity
2008-07-05T21:15:59Z

Marcelo Tosatti wrote:
> On Sat, Jul 05, 2008 at 08:25:30PM +0300, Avi Kivity wrote:
>
>>> @@ -1955,6 +1955,22 @@ void kvm_mmu_slot_remove_write_access(st
>>> }
>>> }
>>> +int kvm_mmu_slot_has_shadowed_page(struct kvm *kvm, int slot)
>>> +{
>>> + struct kvm_mmu_page *sp;
>>> + int ret = 0;
>>> +
>>> + spin_lock(&kvm->mmu_lock);
>>> + list_for_each_entry(sp, &kvm->arch.active_mmu_pages, link) {
>>> + if (test_bit(slot, &sp->slot_bitmap)) {
>>> + ret = -EINVAL;
>>> + break;
>>> + }
>>> + }
>>> + spin_unlock(&kvm->mmu_lock);
>>> + return ret;
>>> +}
>>> +
>>>
>>>
>> I don't like the guest influencing host actions in this way. It's just
>> a guest.
>>
>> But I think it's unneeded. kvm_mmu_zap_page() will mark a root shadow
>> page invalid and force all vcpus to reload it, so all that's needed is
>> to keep the mmu spinlock held while removing the slot.
>>
>
> You're still keeping a shadowed page around with sp->gfn pointing to
> non-existant memslot. The code generally makes the assumption that
> gfn_to_memslot(gfn) on shadowed info will not fail.
>
> kvm_mmu_zap_page -> unaccount_shadowed, for example.
>
>

The page has already been zapped, so we might as well
unaccount_shadowed() on the first run. It needs to be moved until after
the reload_remote_mmus() call, though.

> The other option is to harden gfn_to_memslot() callers to handle
> failure, is that saner?
>

I don't think so.

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