Received-SPF: pass (google.com: domain of linux-kernel+bounces-153061-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: Mon, 22 Apr 2024 17:08:14 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: <isaku.yamahata@intel.com>
CC: <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<isaku.yamahata@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>,
	<erdemaktas@google.com>, Sean Christopherson <seanjc@google.com>, Sagi Shahar
	<sagis@google.com>, Kai Huang <kai.huang@intel.com>, <chen.bo@intel.com>,
	<hang.yuan@intel.com>, <tina.zhang@intel.com>
Subject: Re: [PATCH v19 062/130] KVM: x86/tdp_mmu: Support TDX private
 mapping for TDP MMU
Message-ID: <ZiYo/giv0/sNhjX1@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <fc97847d04f2b469d8f4cfceee84c7ef055ab1ac.1708933498.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <fc97847d04f2b469d8f4cfceee84c7ef055ab1ac.1708933498.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?WWnlkxAcEDzl0kWtsP2mgGxj5oUoaZ+3daqPsai8wN3CNf/7Ii/EUTwOL/2/?=
 =?us-ascii?Q?tqCKpj8MUwSpoY7zwCdWWqVjv859AMYaMyGRM07rE4SNMP+/iHimlMBxviy9?=
 =?us-ascii?Q?Jw9dLcrA9cLmkynYe383g9+df15BsRXl0EaJrVRHqffUtTaqEOQwlTwi+YR+?=
 =?us-ascii?Q?83y57kwe5cwV9A2tTIQovavqRumByp+1mmQviEn7AsNSSsyu6gJjldn5zWHM?=
 =?us-ascii?Q?CeT0cUlhK1igxzct4YPX9B3VgJDZm/PV0SdkvgMnLznF1QxPEMqgTC9cw6TB?=
 =?us-ascii?Q?hZAHqgy7ybFif0mZ8419i/UHnaDFTLl1tRzvVIN7HXnopZfa7m0A1d85WVTj?=
 =?us-ascii?Q?PveUeGgTbLxEv9zJFd+5+wedM1RZK2QDzXm3UyAvCzBh81Q8grZVMK6IyF40?=
 =?us-ascii?Q?XNHm/Ij1efayciUYkJO0W/AhUjbTkg+LfNt7purnDxWXlI4xHuT2iLgttfo9?=
 =?us-ascii?Q?Z9y3/JZrRaNVUUV6WZaqKJAtkr8gZh1LSK7BwL7FD75YKAVXoBBfF0E07+LX?=
 =?us-ascii?Q?Tg/3ts34KSxaRaSAMjSFeuMn7k1lCHk10smD8vB+eA7QAeLD4lvPjq7qLEU0?=
 =?us-ascii?Q?qetHDdXjRbnDRvNfEYUyaOB1dVG34K77vhbPNPFRQrnjwvrsR63cpa0Ckc4i?=
 =?us-ascii?Q?GGOFGuxv+LmzTaR0rZYdEdL23ySuqDiWiMmLQZq2Yh4+jcxZgO336ykrnTKu?=
 =?us-ascii?Q?9Syx1fpUCgXVTkDeBzF5QrEyCjNJNp9Q1mfMjJc0nuTUW/xKilLp8+MoEwdP?=
 =?us-ascii?Q?6d7z192Jo+sJIPGkEiaqzU1H8HQcOwb8tHMZ1VyQ+im7HuqcdTpgk5P734wl?=
 =?us-ascii?Q?cv9vXxYylmAyxQmzrdB5L0Te+QauX8Slpc+bMDV7PYGRQAPchiulSwssfiSK?=
 =?us-ascii?Q?MfyZPu1ZZeJUvUmNomNI800eK4hgyxCjtH+5NUcJq/RzI/ZcPLjC3gmwh1L9?=
 =?us-ascii?Q?13fcIqg7W5BmJ3Jv26R6wz044+KI5XAD/JCVtXT47p1FpE6WJPyyzy/CXvEG?=
 =?us-ascii?Q?7j8YCKYBDfNVJtep6RydnY3qP2EMzKcMBkSeFpKfqD6E2jVHt2iol4Ada3vP?=
 =?us-ascii?Q?qx8UgawhCv1mcVvSP7hjAO++/ht7U17K+4K9ZNAt58kbFBKTNq/McNI8q6GQ?=
 =?us-ascii?Q?8CB7ZVLPKTAGmRkaeKlMWAYVTMkB4B3urTba07dotAqwyfhaUcsiezA1WIT5?=
 =?us-ascii?Q?lALnzGGuH/fZ2498FLWCvs+k++gjPj6CcwjKhN+BAz9wDUAm2rn1S6TI+t6Q?=
 =?us-ascii?Q?xxkb+Z0fmsiAcnVPXASQUv/dMpPR6Gz65pQmkXwYTu2vhAm5a1jpWe095zdB?=
 =?us-ascii?Q?PQqSBf6hWLeneN8X930urRcUbADUQwo36O85UuS7b6vzxf3fs7dF7/O4dP+l?=
 =?us-ascii?Q?lAsZjaMM5PepsqfK6xI+g1ao3I4aaWN/A2ZfziYmUQcdoLmND6mefUvB4nKO?=
 =?us-ascii?Q?y+mzxeD9zdBBV3274IfNFPyYdvXqUNa8sRz9p/Tvw/7k73A4pT6elEb8FKaR?=
 =?us-ascii?Q?z+q7r09ISXYoTWYjytnQ0k7/wleEzD6RyKLoZL4FDLOc2cAO/07er/hRXr7H?=
 =?us-ascii?Q?vMq34H4YrYic/8eohcT/m3eN03l5YK0YarF7XWDX?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4a5ea0c6-0fc8-4bce-2e23-08dc62abd0f7
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Apr 2024 09:08:46.6118
 (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: A27/+k1QrBfHe85gzZWdRPEXyWBudYffW8sZkEm8Ownno0/q+wrEEGVW7gMyxXgLJ7ybMX403Fc9S9nwQbHBVA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7214
X-OriginatorOrg: intel.com

On Mon, Feb 26, 2024 at 12:26:04AM -0800, isaku.yamahata@intel.com wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> @@ -1041,6 +1255,8 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
>  	struct kvm *kvm = vcpu->kvm;
>  	struct tdp_iter iter;
>  	struct kvm_mmu_page *sp;
> +	gfn_t raw_gfn;
> +	bool is_private = fault->is_private && kvm_gfn_shared_mask(kvm);
Why not put the checking of kvm_gfn_shared_mask() earlier before determining
fault->is_private?
e.g. in kvm_mmu_page_fault().

>  	int ret = RET_PF_RETRY;
>  
>  	kvm_mmu_hugepage_adjust(vcpu, fault);
> @@ -1049,7 +1265,17 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
>  
>  	rcu_read_lock();
>  
> -	tdp_mmu_for_each_pte(iter, mmu, fault->gfn, fault->gfn + 1) {
> +	raw_gfn = gpa_to_gfn(fault->addr);
> +
> +	if (is_error_noslot_pfn(fault->pfn) ||
The checking for noslot pfn is not required after this cleanup series [1] from
Sean, right?

> +	    !kvm_pfn_to_refcounted_page(fault->pfn)) {
What's the purpose of rejecting non-refcounted page (which is useful for trusted
IO)?

Besides, looks pages are allocated via kvm_faultin_pfn_private() if
fault->is_private, so where is the non-refcounted page from?


> +		if (is_private) {
> +			rcu_read_unlock();
> +			return -EFAULT;
> +		}
> +	}

[1] https://lore.kernel.org/all/20240228024147.41573-1-seanjc@google.com/

> +	tdp_mmu_for_each_pte(iter, mmu, is_private, raw_gfn, raw_gfn + 1) {
>  		int r;
>  
>  		if (fault->nx_huge_page_workaround_enabled)
..

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d399009ef1d7..e27c22449d85 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -201,6 +201,7 @@ struct page *kvm_pfn_to_refcounted_page(kvm_pfn_t pfn)
>  
>  	return NULL;
>  }
> +EXPORT_SYMBOL_GPL(kvm_pfn_to_refcounted_page);
>  
>  /*
>   * Switches to specified vcpu, until a matching vcpu_put()
> -- 
> 2.25.1
> 
>