linux-kernel - Re: [PATCH v2 05/10] KVM: x86/mmu: Introduce kvm_tdp_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zh90aFh2xr+nEcCQ@chao-email>
Date: Wed, 17 Apr 2024 15:04:08 +0800
From: Chao Gao <chao.gao@...el.com>
To: <isaku.yamahata@...el.com>
CC: <kvm@...r.kernel.org>, <isaku.yamahata@...il.com>,
	<linux-kernel@...r.kernel.org>, Sean Christopherson <seanjc@...gle.com>,
	Paolo Bonzini <pbonzini@...hat.com>, Michael Roth <michael.roth@....com>,
	David Matlack <dmatlack@...gle.com>, Federico Parola
	<federico.parola@...ito.it>, Kai Huang <kai.huang@...el.com>
Subject: Re: [PATCH v2 05/10] KVM: x86/mmu: Introduce kvm_tdp_map_page() to
 populate guest memory

On Wed, Apr 10, 2024 at 03:07:31PM -0700, isaku.yamahata@...el.com wrote:
>From: Isaku Yamahata <isaku.yamahata@...el.com>
>
>Introduce a helper function to call the KVM fault handler.  It allows a new
>ioctl to invoke the KVM fault handler to populate without seeing RET_PF_*
>enums or other KVM MMU internal definitions because RET_PF_* are internal
>to x86 KVM MMU.  The implementation is restricted to two-dimensional paging
>for simplicity.  The shadow paging uses GVA for faulting instead of L1 GPA.
>It makes the API difficult to use.
>
>Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
>---
>v2:
>- Make the helper function two-dimensional paging specific. (David)
>- Return error when vcpu is in guest mode. (David)
>- Rename goal_level to level in kvm_tdp_mmu_map_page(). (Sean)
>- Update return code conversion. Don't check pfn.
>  RET_PF_EMULATE => EINVAL, RET_PF_CONTINUE => EIO (Sean)
>- Add WARN_ON_ONCE on RET_PF_CONTINUE and RET_PF_INVALID. (Sean)
>- Drop unnecessary EXPORT_SYMBOL_GPL(). (Sean)
>---
> arch/x86/kvm/mmu.h     |  3 +++
> arch/x86/kvm/mmu/mmu.c | 32 ++++++++++++++++++++++++++++++++
> 2 files changed, 35 insertions(+)
>
>diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
>index e8b620a85627..51ff4f67e115 100644
>--- a/arch/x86/kvm/mmu.h
>+++ b/arch/x86/kvm/mmu.h
>@@ -183,6 +183,9 @@ static inline void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu,
> 	__kvm_mmu_refresh_passthrough_bits(vcpu, mmu);
> }
> 
>+int kvm_tdp_map_page(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code,
>+		     u8 *level);
>+
> /*
>  * Check if a given access (described through the I/D, W/R and U/S bits of a
>  * page fault error code pfec) causes a permission fault with the given PTE
>diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
>index 91dd4c44b7d8..a34f4af44cbd 100644
>--- a/arch/x86/kvm/mmu/mmu.c
>+++ b/arch/x86/kvm/mmu/mmu.c
>@@ -4687,6 +4687,38 @@ int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> 	return direct_page_fault(vcpu, fault);
> }
> 
>+int kvm_tdp_map_page(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code,
>+		     u8 *level)
>+{
>+	int r;
>+
>+	/* Restrict to TDP page fault. */

need to explain why. (just as you do in the changelog)

>+	if (vcpu->arch.mmu->page_fault != kvm_tdp_page_fault)

page fault handlers (i.e., vcpu->arch.mmu->page_fault()) will be called
finally. why not let page fault handlers reject the request to get rid of
this ad-hoc check? We just need to plumb a flag indicating this is a
pre-population request into the handlers. I think this way is clearer.

What do you think?