Received-SPF: pass (google.com: domain of linux-kernel+bounces-148022-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 17 Apr 2024 15:04:08 +0800
From: Chao Gao <chao.gao@intel.com>
To: <isaku.yamahata@intel.com>
CC: <kvm@vger.kernel.org>, <isaku.yamahata@gmail.com>,
	<linux-kernel@vger.kernel.org>, Sean Christopherson <seanjc@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>, Michael Roth <michael.roth@amd.com>,
	David Matlack <dmatlack@google.com>, Federico Parola
	<federico.parola@polito.it>, Kai Huang <kai.huang@intel.com>
Subject: Re: [PATCH v2 05/10] KVM: x86/mmu: Introduce kvm_tdp_map_page() to
 populate guest memory
Message-ID: <Zh90aFh2xr+nEcCQ@chao-email>
References: <cover.1712785629.git.isaku.yamahata@intel.com>
 <9b866a0ae7147f96571c439e75429a03dcb659b6.1712785629.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <9b866a0ae7147f96571c439e75429a03dcb659b6.1712785629.git.isaku.yamahata@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?QCrlPFD27o0uiS7ee+phpz5p2GljC8u5JmqlYCu/IYR31RfC0HpyF3zNnXqE?=
 =?us-ascii?Q?qJMbt2aY2EHlCQeLZJVKB2Zu+SVc4QbsUEhvzBKXYymx+C+Bk9Q/f5KRDYna?=
 =?us-ascii?Q?svjMnSUGg6NaeNq05lb80uzrTNsPJXg3InnjR20h/47/6aLscY7F83WtqDH9?=
 =?us-ascii?Q?wPolT9vQcxBYaE8benrKo5rLhm9SJ+ykDnj8W0pmX73Qv2LEs5yZ/dIVrXJw?=
 =?us-ascii?Q?4yx3lZgQ1YA71yvKnwiiHxqqPZ/WTn9KaU18k/Fi8UhfbUC07O6qOJKDvv2k?=
 =?us-ascii?Q?T86cEbZZGM6XwvmQbA0m/1mjKIQBrC0iP0BVQi4uxYRy0k+G2Uv7kxp0arSs?=
 =?us-ascii?Q?O2qN4ETtRcmNg6yPaW0PM+oz0OHB1lsLLcb72+H/17hxBeQivdcEUx3mKZ9K?=
 =?us-ascii?Q?qXt+Y/3Y6VHvKgjFK4rDxhmPlcAcnMF4QgSNEBs+oJ/x/1oNrSwCfQmb3n7l?=
 =?us-ascii?Q?uGYTJ6G5IfSCJ/HhFNraPfTfgTIYCbre7TjWNvxjcFZAAPhkrRxz3I007Sd0?=
 =?us-ascii?Q?UP15kWXFGvdNh4AFxepkNlGiQfUh50TQB19d+NLZ9Xxk89B9MZQ2ngsR/mI7?=
 =?us-ascii?Q?FZuCoBG8FY7CHTGwS8jEibY00T0m0nrlxTiTHkbjLkGMIU+Eua+wN8ft9aXK?=
 =?us-ascii?Q?LHFL7vHjOMCJ3qTaKfzztHEUfOsZkQ/OzaeARjastLNIkyr7XYtkuX9R3d+b?=
 =?us-ascii?Q?v2c8vrMFXmeo/37LVJ6pC8V/eGg4a9Ns8hgD+McQoQMkDLgfXkInNEOZ7PDy?=
 =?us-ascii?Q?WlWS6AllPLywLDfwGtgsfWABnG5zjiLNtibC4WfYh/dfw+pa2htOFJuNRMmn?=
 =?us-ascii?Q?d3uO+4hhdEovtfe+ccnv9BcuJmmmt2AXNuZtsGIk6f/djhDg9t5RajMRDRuz?=
 =?us-ascii?Q?jq4ZWYZPPzR4Q9o8mKxFjosvAk9HlMIL16QsYsiT70owbQHpRbU4RTG8P0yP?=
 =?us-ascii?Q?vkO3877+n+j+CJgca7HVyM7sXVuSG793pDdvoYJLLrMUyJ/4IZratpdhVh6d?=
 =?us-ascii?Q?FsQrMq4fFPa67x4gk2yraqNRTWPFyjgtKIaQpML0ZtfyUNe+K9WYgCnpFEVx?=
 =?us-ascii?Q?AvI8ra/922Vcy5nUO9jIByXeKZhztbejZ0Vj27UdTMdRGbskBGMG7z6FZXZS?=
 =?us-ascii?Q?99GOT683gFDyAO0um0QiOSS2J6xqbg+rlcjEVD6ED2zGyNjmNblFcz1Hy9F7?=
 =?us-ascii?Q?CShCY6CKEkom6hthaZIsT3KoYaV7uwwjsJCr/yth1FlfV1rkNnaIgTd3qVod?=
 =?us-ascii?Q?NJ4BwXfQopEH/CBwQiEXy/JGbYS/CZ66XY0IQ+URUCyk/geVBG7VUqPl0yb4?=
 =?us-ascii?Q?7kuANjgM1uUFcXX4prep0olRY/VGEy3azKpTyXAYxbM6GbGMjIFwVRYmt36m?=
 =?us-ascii?Q?jo97KpFVihVCHUD5WCBoA1gC4i0q4kIZ7Brk/TQketJmEU5tHj8xfi1IoN9L?=
 =?us-ascii?Q?4DT//ALst5gYViMTTSRjurlhl10pQHVfyRAgNn4u4UAISRB353J1DbiFXcfG?=
 =?us-ascii?Q?5b92gOIHixPHuF2GgsiEiLWtLbtqJBYTT9EycHqH3b7Q+gn0XBmIB5jGFZRt?=
 =?us-ascii?Q?2/sgquEXoP8X1k0+EINn9C3+f1fhEQ/mjCFkfD4M?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 438b110a-fd77-49d2-f965-08dc5eac9936
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Apr 2024 07:04:17.8997
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: eQuoygMToiYShLSSHtoNoeizZOVLDDG6V77qJfGP2JRbRoszCvlRK+3quqmS+wwd9KNisK9I7WKAMU+wFn/Beg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5078
X-OriginatorOrg: intel.com

On Wed, Apr 10, 2024 at 03:07:31PM -0700, isaku.yamahata@intel.com wrote:
>From: Isaku Yamahata <isaku.yamahata@intel.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@intel.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?