Received-SPF: pass (google.com: domain of linux-kernel+bounces-145244-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, 15 Apr 2024 21:21:44 +0800
From: Chao Gao <chao.gao@intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>
CC: <linux-kernel@vger.kernel.org>, <kvm@vger.kernel.org>, Isaku Yamahata
	<isaku.yamahata@intel.com>
Subject: Re: [PATCH 07/10] KVM: VMX: Introduce test mode related to EPT
 violation VE
Message-ID: <Zh0p6Jz5eKBBmWci@chao-email>
References: <20240412173532.3481264-1-pbonzini@redhat.com>
 <20240412173532.3481264-8-pbonzini@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240412173532.3481264-8-pbonzini@redhat.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?SwoGRCEXujtNCAVfUXllDy1puYRVo+jBQTv8P8E1QDZ1L+/atZwBUBmd4HE0?=
 =?us-ascii?Q?0CM/fserM/NPEs8RHjqvbXtiP6kNO++C2xu7QvGwCNmFk4SDlxvDIG4MLnxH?=
 =?us-ascii?Q?CESsdqA9smNSjfpANDioYSzWFwKP4GOKNc2KS4AgJ6LoWnfspGeAJRGd/ZWt?=
 =?us-ascii?Q?ahnCO3V7hr5+F4S+E7OoEJCa7mq8Ap52hF1/A+4mwx687A9uIvyA0Fiow1e/?=
 =?us-ascii?Q?eyMCj1/0mu35fu2RHkYDPVd1FZVMu+JuUEN+cVh2hi76ncY/ImEnoJHDcZy2?=
 =?us-ascii?Q?SQBjug5Q2Sp8HmDD/6MA9I/qADUuFNusKqeWp8fgyA48SMRKKkGmpXJuvDXl?=
 =?us-ascii?Q?XQQ2H4VnDzfk6jTbuZ0q6CbPFEg8hk4J+cUlKEwW2qz1kt6wJlDzGz/q/XtN?=
 =?us-ascii?Q?CQlZJ0TBxda/SKkR7sidJeYwQwzc4unf9m4p+4u/8fM/LSdt9smeG2m2npJ6?=
 =?us-ascii?Q?YWj8F1klxRsmNWfFHQd3AvzxM3idcRZ02ErRAcE8HbX1WMIplwK1hxTFGnSm?=
 =?us-ascii?Q?s2FfEq6mxhfOSwSedeFaO7zgday4mXOyQ9nsgxs0oRe02H1utqiSZWjwiWzd?=
 =?us-ascii?Q?HcGxGT9i6C208UGJU93g1W2BVKRBiHAnDBao3/scNCyXLorR4Rohsw0/W9DA?=
 =?us-ascii?Q?qNFL+sQtWi7llBTd2/pLK+GPaZlJuZQ0IJ45nThGYNlUpgWrAwndDtjm1pwE?=
 =?us-ascii?Q?FzGT8OGZxM9guR0p7916XW3+M5wMcWwr4Oe4C+R62fuc26VNYcdcWot4zyvA?=
 =?us-ascii?Q?phcFZj/rBM0qxj//iBXkt8++Oj2+/Kak8zwGWMEdDoEJsvxK8dPJ9Ja7hMJY?=
 =?us-ascii?Q?/mtr2pZaSEsM/cp2CsegemG3pljDoP8WMzIPZvVziKyK/6Of8MPYG9Y1m2sU?=
 =?us-ascii?Q?ZDTrHgzt/4GOn7rwkLkSLeepoplzLdyR16M+azZIU7xSliYWNrCgsUOVNgWJ?=
 =?us-ascii?Q?/EbDD4HulObVh8ahoAihreyksnXsb+O+C4BpGIflvf0Sw53b/uBDmH6ynBRm?=
 =?us-ascii?Q?97OkqXBv2I4WiAPHVLUZ48GMuP7Ha6q3Yzx7q4rN0/FSEcHDFqcnRUQKHTPS?=
 =?us-ascii?Q?QA2YsCC4Thk3iJXLK0+Gk9Cp9m33asGB+C50r91xqmPQruFmuQwHJCtinBma?=
 =?us-ascii?Q?XTfLGJ0nn11zDzoH1qA4rwbr5xCqCrWAgbRSQ1fEg9NZbHorAvdBV0rUuQ0o?=
 =?us-ascii?Q?VyMDEDWmZO6TJPa0jzgJ5iDbNsj8UEBkuSUgnQvzgAf4R8gMrnkptuJSo6ZE?=
 =?us-ascii?Q?n9Yv9p+bbh9A03miUiOWPzFGXxvuKjVSoVKyypsVf564uEOgDqSUL/YJuTeQ?=
 =?us-ascii?Q?oicN2pH1QuWBGRgGbh9G7hx/e+SxLGB4QrcsgC+na6cUlpvrHGC01KEb8Nzv?=
 =?us-ascii?Q?wtaFlNC4A4qwHvf1VGqGwyqmjOkkViIpKzrH7JY8k0LhaIbUiXdfWC6zCKlS?=
 =?us-ascii?Q?ea/+7RyY6FkI07WidnKi+FhkIvBv6dJAMIzqFJa3GzxvSrMGHiHUB6APkPC2?=
 =?us-ascii?Q?9F/bzDip3dgOL/6PXCyHQkeRn+VjOMWVQN/W+FUGoaPEHURcoHG5VWd1rBz7?=
 =?us-ascii?Q?S98kmkorgs53wVPtd/7l28WPGoZZg4d7D8srS5n3?=
X-MS-Exchange-CrossTenant-Network-Message-Id: dadbd176-7879-4a9b-6492-08dc5d4f02dc
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Apr 2024 13:21:51.5032
 (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: Zdwlj0kOL+D84O6OA5YiG/MrOJpm7Vq/3LJiBGhedy5UYNP3MMjqeoaKeVK1lCHhmhi5anX+uNYRhqyav+ZmFQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6017
X-OriginatorOrg: intel.com

> 
>-	if (cpu_has_secondary_exec_ctrls())
>+	if (cpu_has_secondary_exec_ctrls()) {
> 		secondary_exec_controls_set(vmx, vmx_secondary_exec_control(vmx));
>+		if (secondary_exec_controls_get(vmx) &
>+		    SECONDARY_EXEC_EPT_VIOLATION_VE) {
>+			if (!vmx->ve_info) {

how about allocating ve_info in vmx_vcpu_create()? It is better to me because:

a. symmetry. ve_info is free'd in vmx_vcpu_free().
b. no need to check if this is the first call of init_vmcs(). and ENOMEM can
be returned on allocation failure.

>+				/* ve_info must be page aligned. */
>+				struct page *page;
>+
>+				BUILD_BUG_ON(sizeof(*vmx->ve_info) > PAGE_SIZE);
>+				page = alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
>+				if (page)
>+					vmx->ve_info = page_to_virt(page);
>+			}
>+			if (vmx->ve_info) {
>+				/*
>+				 * Allow #VE delivery. CPU sets this field to
>+				 * 0xFFFFFFFF on #VE delivery.  Another #VE can
>+				 * occur only if software clears the field.
>+				 */
>+				vmx->ve_info->delivery = 0;

Is it necessary to reset ve_info->delivery to 0 given __GFP_ZERO?

>+				vmcs_write64(VE_INFORMATION_ADDRESS,
>+					     __pa(vmx->ve_info));

I think the logic here should just be:

		if (secondary_exec_controls_get(vmx) & SECONDARY_EXEC_EPT_VIOLATION_VE)
			vmcs_write64(VE_INFORMATION_ADDRESS, __pa(vmx->ve_info));

>+			} else {
>+				/*
>+				 * Because SECONDARY_EXEC_EPT_VIOLATION_VE is
>+				 * used only for debugging, it's okay to leave
>+				 * it disabled.
>+				 */
>+				pr_err("Failed to allocate ve_info. disabling EPT_VIOLATION_VE.\n");
>+				secondary_exec_controls_clearbit(vmx,
>+								 SECONDARY_EXEC_EPT_VIOLATION_VE);
>+			}
>+		}
>+	}
> 
> 	if (cpu_has_tertiary_exec_ctrls())
> 		tertiary_exec_controls_set(vmx, vmx_tertiary_exec_control(vmx));
>@@ -5200,6 +5243,12 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
> 	if (is_invalid_opcode(intr_info))
> 		return handle_ud(vcpu);
> 
>+	/*
>+	 * #VE isn't supposed to happen.  Block the VM if it does.
>+	 */
>+	if (KVM_BUG_ON(is_ve_fault(intr_info), vcpu->kvm))
>+		return -EIO;
>+
> 	error_code = 0;
> 	if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
> 		error_code = vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
>@@ -7474,6 +7523,8 @@ void vmx_vcpu_free(struct kvm_vcpu *vcpu)
> 	free_vpid(vmx->vpid);
> 	nested_vmx_free_vcpu(vcpu);
> 	free_loaded_vmcs(vmx->loaded_vmcs);
>+	if (vmx->ve_info)
>+		free_page((unsigned long)vmx->ve_info);
> }
> 
> int vmx_vcpu_create(struct kvm_vcpu *vcpu)
>diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>index 65786dbe7d60..0da79a386825 100644
>--- a/arch/x86/kvm/vmx/vmx.h
>+++ b/arch/x86/kvm/vmx/vmx.h
>@@ -362,6 +362,9 @@ struct vcpu_vmx {
> 		DECLARE_BITMAP(read, MAX_POSSIBLE_PASSTHROUGH_MSRS);
> 		DECLARE_BITMAP(write, MAX_POSSIBLE_PASSTHROUGH_MSRS);
> 	} shadow_msr_intercept;
>+
>+	/* ve_info must be page aligned. */

this comment is not so useful. I think this should be placed above the call
of alloc_page().

>+	struct vmx_ve_information *ve_info;
> };
> 
> struct kvm_vmx {
>@@ -574,7 +577,8 @@ static inline u8 vmx_get_rvi(void)
> 	 SECONDARY_EXEC_ENABLE_VMFUNC |					\
> 	 SECONDARY_EXEC_BUS_LOCK_DETECTION |				\
> 	 SECONDARY_EXEC_NOTIFY_VM_EXITING |				\
>-	 SECONDARY_EXEC_ENCLS_EXITING)
>+	 SECONDARY_EXEC_ENCLS_EXITING |					\
>+	 SECONDARY_EXEC_EPT_VIOLATION_VE)
> 
> #define KVM_REQUIRED_VMX_TERTIARY_VM_EXEC_CONTROL 0
> #define KVM_OPTIONAL_VMX_TERTIARY_VM_EXEC_CONTROL			\
>-- 
>2.43.0
>
>
>