From mboxrd@z Thu Jan  1 00:00:00 1970
From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Subject: Re: [PATCH v3 7/8] kvm: x86: mmu: Lockless access tracking for Intel
 CPUs without EPT A bits.
Date: Wed, 21 Dec 2016 17:49:04 +0800
Message-ID: <ec8fc987-d4dc-0b52-7f07-45753a4928e8@linux.intel.com>
References: <cover.1476839873.git.junaids@google.com>
 <1481071577-40250-1-git-send-email-junaids@google.com>
 <1481071577-40250-8-git-send-email-junaids@google.com>
 <93b5692a-0f76-a31d-46f3-b85d19298d92@linux.intel.com>
 <736ae8e7-271a-d4d0-5535-5899f64d2b93@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: andreslc@google.com, pfeiner@google.com
To: Paolo Bonzini <pbonzini@redhat.com>,
        Junaid Shahid <junaids@google.com>, kvm@vger.kernel.org
Return-path: <kvm-owner@vger.kernel.org>
Received: from mga11.intel.com ([192.55.52.93]:14277 "EHLO mga11.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1756908AbcLUJ5r (ORCPT <rfc822;kvm@vger.kernel.org>);
        Wed, 21 Dec 2016 04:57:47 -0500
In-Reply-To: <736ae8e7-271a-d4d0-5535-5899f64d2b93@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>



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.