Re: [PATCH 09/11] KVM: MMU: simplify mmu_need_write_protect

[Kai Huang <kai.huang@linux.intel.com>]

On 12/16/2015 04:48 PM, Xiao Guangrong wrote:
>
>
> On 12/16/2015 04:05 PM, Kai Huang wrote:
>>
>>
>> On 12/15/2015 05:25 PM, Xiao Guangrong wrote:
>>>
>>>
>>> On 12/15/2015 04:43 PM, Kai Huang wrote:
>>>>
>>>>
>>>> On 12/01/2015 02:26 AM, Xiao Guangrong wrote:
>>>>> Now, all non-leaf shadow page are page tracked, if gfn is not tracked
>>>>> there is no non-leaf shadow page of gfn is existed, we can directly
>>>>> make the shadow page of gfn to unsync
>>>>>
>>>>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>>>>> ---
>>>>>   arch/x86/kvm/mmu.c | 26 ++++++++------------------
>>>>>   1 file changed, 8 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>>>>> index 5a2ca73..f89e77f 100644
>>>>> --- a/arch/x86/kvm/mmu.c
>>>>> +++ b/arch/x86/kvm/mmu.c
>>>>> @@ -2461,41 +2461,31 @@ static void __kvm_unsync_page(struct 
>>>>> kvm_vcpu *vcpu, struct kvm_mmu_page
>>>>> *sp)
>>>>>       kvm_mmu_mark_parents_unsync(sp);
>>>>>   }
>>>>> -static void kvm_unsync_pages(struct kvm_vcpu *vcpu, gfn_t gfn)
>>>>> +static bool kvm_unsync_pages(struct kvm_vcpu *vcpu, gfn_t gfn,
>>>>> +                 bool can_unsync)
>>>>>   {
>>>>>       struct kvm_mmu_page *s;
>>>>>       for_each_gfn_indirect_valid_sp(vcpu->kvm, s, gfn) {
>>>>> +        if (!can_unsync)
>>>>> +            return true;
>>>> How about moving this right before for_each_gfn_indirect_valid_sp? 
>>>> As can_unsync is passed as
>>>> parameter, so there's no point checking it several times.
>>>>
>>>
>>> We can not do this. What we are doing here is checking if we have 
>>> shadow page mapping
>>> for 'gfn':
>>> a) if no, it can be writable.
>> I think in this case you should also check whether the GFN is being 
>> write protection tracked. Ex, if
>> the spte never exists when you add the GFN to write protection 
>> tracking, and in this case I think
>> mmu_need_write_protect should also report true. Right?
>
> We have already checked it:
>
> static bool mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
>                                    bool can_unsync)
> {
>         if (kvm_page_track_check_mode(vcpu, gfn, KVM_PAGE_TRACK_WRITE))
>                 return true;
>
>         return kvm_unsync_pages(vcpu, gfn, can_unsync);
> }
Oh sorry I missed this :)

>
>
>>
>>> b) if yes, check 'can_unsync' to see if these shadow pages can make 
>>> to be 'unsync'.
>>>
>>> Your suggestion can break the point a).
>>>
>>>> A further thinking is can we move it to mmu_need_write_protect? 
>>>> Passing can_unsync as parameter to
>>>> kvm_unsync_pages sounds a little bit odd.
>>>>
>>>>> +
>>>>>           if (s->unsync)
>>>>>               continue;
>>>>>           WARN_ON(s->role.level != PT_PAGE_TABLE_LEVEL);
>>>> How about large page mapping? Such as if guest uses 2M mapping and 
>>>> its shadow is indirect, does
>>>> above WARN_ON still meet? As you removed the PT level check in 
>>>> mmu_need_write_protect.
>>>
>>> The lager mapping are on the non-leaf shadow pages which can be 
>>> figured out by
>>> kvm_page_track_check_mode() before we call this function.
>> Actually I am not quite understanding how large page mapping is 
>> implemented. I see in
>> kvm_mmu_get_page, when sp is allocated, it is large page mapping 
>> disabled, but I think we do support
>> large shadow mapping, right? I mean theoretically if guest uses 2M 
>> mapping and shadow mapping can
>> certainly use 2M mapping as well, and the 2M shadow mapping can also 
>> be 'unsynced' (as a leaf
>> mapping table). But in your series I see if we write protect some  
>> GFN, the shadow large page
>> mapping is always disabled.
>>
>> Am I wrong?
>
> If the large page contains the page which is used as page table, kvm 
> does not map large page for
> it, the reason is we track the 4k page instead of the whole large page 
> to reduce write emulation.
I don't know why breaking large page to 4K mapping can reduce write 
emulation, but this explanation works for me. I guess KVM-GT doesn't 
care about it neither. :)

Thanks,
-Kai
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>