Re: [PATCH v3 7/8] kvm: x86: mmu: Lockless access tracking for Intel CPUs without EPT A bits.

[Paolo Bonzini <pbonzini@redhat.com>]

On 21/12/2016 10:49, Xiao Guangrong wrote:
> 
> 
> On 12/16/2016 11:23 PM, Paolo Bonzini wrote:
>>
>>
>> On 16/12/2016 14:04, Xiao Guangrong wrote:
>>>> +    /*
>>>> +     * #PF can be fast if:
>>>> +     * 1. The shadow page table entry is not present, which could
>>>> mean that
>>>> +     *    the fault is potentially caused by access tracking (if
>>>> enabled).
>>>> +     * 2. The shadow page table entry is present and the fault
>>>> +     *    is caused by write-protect, that means we just need
>>>> change the W
>>>> +     *    bit of the spte which can be done out of mmu-lock.
>>>> +     *
>>>> +     * However, if access tracking is disabled we know that a
>>>> non-present
>>>> +     * page must be a genuine page fault where we have to create a
>>>> new SPTE.
>>>> +     * So, if access tracking is disabled, we return true only for
>>>> write
>>>> +     * accesses to a present page.
>>>> +     */
>>>> +
>>>> +    return shadow_acc_track_mask != 0 ||
>>>> +           ((error_code & (PFERR_WRITE_MASK | PFERR_PRESENT_MASK))
>>>> +        == (PFERR_WRITE_MASK | PFERR_PRESENT_MASK));
>>>
>>> acc-track can not fix a WRITE-access, this should be:
>>>
>>> !(error_code & (PFERR_WRITE_MASK)) && shadow_acc_track_mask != 0 || ...
>>
>> Access tracking makes pages non-present, so a !W !P fault can sometimes
>> be fixed.
>>
>> One possibility is to test is_access_track_pte, but it is handled a
>> little below the call to page_fault_can_be_fast:
>>
>>             remove_acc_track = is_access_track_spte(spte);
>>
>>             /* Verify that the fault can be handled in the fast path */
>>             if (!remove_acc_track && !remove_write_prot)
>>                     break;
>>
>> It's not different from the way page_fault_can_be_fast return true for
>> writes, even if spte_can_locklessly_be_made_writable will return false
>> later.
>>
>> So I think Junaid's patch is okay.
> 
> Yes, it is workable.
> 
> My suggestion is just a optimization. Figure out Write access which can
> not be fixed by acc-track earlier in page_fault_can_be_fast() can stop
> useless  lockless-ly page-table walking.

That optimization can be done as a follow-up, but your suggestion was
not complete.  page_fault_can_be_fast must be conservative and return
true if unsure.

Paolo