Cc: brijesh.singh@amd.com, kvm@vger.kernel.org,
        thomas lendacky <thomas.lendacky@amd.com>, rkrcmar@redhat.com,
        joro@8bytes.org, x86@kernel.org, linux-kernel@vger.kernel.org,
        mingo@redhat.com, hpa@zytor.com, tglx@linutronix.de, bp@suse.de
Subject: Re: [PATCH v2 1/3] kvm: svm: Add support for additional SVM NPF error
 codes
To: Paolo Bonzini <pbonzini@redhat.com>
References: <147992048887.27638.17559991037474542240.stgit@brijesh-build-machine>
 <147992049856.27638.17076562184960611399.stgit@brijesh-build-machine>
 <da3f8d2d-04c2-d2f8-0752-7d484c9bc25b@redhat.com>
 <e8911a18-ce93-ca01-2876-3f7e90091f92@amd.com>
 <21b9f4db-f929-80f6-6ad2-6fa3b77f82c0@redhat.com>
 <d7266e60-3bcc-946d-4325-dfd1126c9fc4@amd.com>
 <98086274.371452.1501531542630.JavaMail.zimbra@redhat.com>
From: Brijesh Singh <brijesh.singh@amd.com>
Message-ID: <661faa8a-87af-743f-d3ea-b95ada0d7677@amd.com>
Date: Tue, 1 Aug 2017 08:36:09 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <98086274.371452.1501531542630.JavaMail.zimbra@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Aug 2017 13:36:13.9113 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB0158
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2590
Lines: 67


On 07/31/2017 03:05 PM, Paolo Bonzini wrote:
> 
>>> There can be different cases where an L0->L2 shadow nested page table is
>>> marked read only, in particular when a page is read only in L1's nested
>>> page tables.  If such a page is accessed by L2 while walking page tables
>>> it will cause a nested page fault (page table walks are write accesses).
>>>    However, after kvm_mmu_unprotect_page you will get another page fault,
>>> and again in an endless stream.
>>>
>>> Instead, emulation would have caused a nested page fault vmexit, I think.
>>
>> If possible could you please give me some pointer on how to create this use
>> case so that we can get definitive answer.
>>
>> Looking at the code path is giving me indication that the new code
>> (the kvm_mmu_unprotect_page call) only happens if vcpu->arch.mmu_page_fault()
>> returns an indication that the instruction should be emulated. I would not
>> expect that to be the case scenario you described above since L1 making a page
>> read-only (this is a page table for L2) is an error and should result in #NPF
>> being injected into L1.
> 
> The flow is:
> 
>    hardware walks page table; L2 page table points to read only memory
>    -> pf_interception (code =
>    -> kvm_handle_page_fault (need_unprotect = false)
>    -> kvm_mmu_page_fault
>    -> paging64_page_fault (for example)
>       -> try_async_pf
>          map_writable set to false
>       -> paging64_fetch(write_fault = true, map_writable = false, prefault = false)
>          -> mmu_set_spte(speculative = false, host_writable = false, write_fault = true)
>             -> set_spte
>                mmu_need_write_protect returns true
>                return true
>             write_fault == true -> set emulate = true
>             return true
>          return true
>       return true
>    emulate
> 
> Without this patch, emulation would have called
> 
>    ..._gva_to_gpa_nested
>    -> translate_nested_gpa
>    -> paging64_gva_to_gpa
>    -> paging64_walk_addr
>    -> paging64_walk_addr_generic
>       set fault (nested_page_fault=true)
> 
> and then:
> 
>     kvm_propagate_fault
>     -> nested_svm_inject_npf_exit
> 

maybe then safer thing would be to qualify the new error_code check with
!mmu_is_nested(vcpu) or something like that. So that way it would run on
L1 guest, and not the L2 guest. I believe that would restrict it avoid
hitting this case. Are you okay with this change ?

IIRC, the main place where this check was valuable was when L1 guest had
a fault (when coming out of the L2 guest) and emulation was not needed.

-Brijesh