From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@qumranet.com>
Subject: Re: KVM: MMU: nuke shadowed pgtable pages and pte's on memslot	destruction
Date: Sun, 06 Jul 2008 00:15:56 +0300
Message-ID: <486FE48C.7030002@qumranet.com>
References: <20080704010618.GA25834@dmt.cnet> <486FAE8A.2050308@qumranet.com> <20080705192344.GA16596@dmt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: "Richard W.M. Jones" <rjones@redhat.com>,
	kvm-devel <kvm@vger.kernel.org>
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from il.qumranet.com ([212.179.150.194]:20553 "EHLO il.qumranet.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752193AbYGEVP7 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sat, 5 Jul 2008 17:15:59 -0400
In-Reply-To: <20080705192344.GA16596@dmt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.