Received-SPF: pass (google.com: domain of linux-kernel+bounces-79455-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <b8054f76-e427-ac8e-5326-863653057f83@oracle.com>
Date: Fri, 23 Feb 2024 19:18:43 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v2 3/3] KVM: VMX: Combine "check" and "get" APIs for
 passthrough MSR lookups
Content-Language: en-US
To: Sean Christopherson <seanjc@google.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>
References: <20240223202104.3330974-1-seanjc@google.com>
 <20240223202104.3330974-4-seanjc@google.com>
From: Dongli Zhang <dongli.zhang@oracle.com>
In-Reply-To: <20240223202104.3330974-4-seanjc@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?NkhJUnVvMTZzaGRqd0svOXpnakdMcTlOU1JOMVRQYXZaVFVQd1dWV3prTEQr?=
 =?utf-8?B?dkQrNE53Z0VqWk9nNThjc3dib3ZOTFNxTkxEZGVKSjVBNlFNT01tbzhabEtD?=
 =?utf-8?B?TjBzeFlzc1ZEUWNET3hYbkhSSGdCM2loZEhob2hlM1EzTVN6UWQ0Z2JtWkVn?=
 =?utf-8?B?TTNtNURTYnVWcTdBcEozeHNoRzExeWNUWWhMYWYzYjJMVld2TXp6WlQyYXZl?=
 =?utf-8?B?VDVNV3hoaHR0ZDMydnNOWFd3SzFrVjBJL3d4YmNjZURlZGRtdk1SVGlocHAz?=
 =?utf-8?B?K2RoUjQwNEZNaXIwQ0Y1NXZMVWw2M01vSVg0VlNQSWxMZ2N3NTZna3pqbldL?=
 =?utf-8?B?Qm9oTUtrb0lCSHAxWWE3K3NwQWNtK0tENnNMd3IrUjNyR203dW1LQVNDVlJX?=
 =?utf-8?B?TklmeExHSWN2a1RGNWEwREM2RFZpQnVrV3lXQTcvU1N4cUFSaW11T3Q1QStt?=
 =?utf-8?B?Nnkyd2V3MzlCNE0zU2NhdVhBUWswVlpYbDR4N1Y2Q2I3NzZNTDdNV2xZRzRu?=
 =?utf-8?B?bit4SFlSUWYvUUZLOFNrSk5LRThKclFzWWs4bmZ0ek1OK1o1VWUvalJxTThi?=
 =?utf-8?B?SXhjUU11NG01YUtPTFJSbzRxVml3bGhpU0xKQm9HSGNYVSsrL29XNkcvQ3VU?=
 =?utf-8?B?ZWpjYm1ZQkVLNFhBdSt4OTVyZnJIRmRXdzlKSlhGL2o3Q1UyeHcvR2pLNitE?=
 =?utf-8?B?Umd3dDJMd1hZdmZPNmhPZ3czZHNCeGoxR2NEdTAwY3JjUXlOcFhrZ3BJN01Q?=
 =?utf-8?B?WVRWeWUzdmRVcU8zbmJ4azNuOXJXajhHVzNlVjloeEpGRDNsRjB2QWF1SjhE?=
 =?utf-8?B?WHNGWE1ZSkxnK0dNVVl5NzA2eEJwNTY5dDZnM2J6eXNlYlV5cjlWd1pjaThj?=
 =?utf-8?B?eCtJdHdmNHZabUhJOExHdHJFMlVMcmZkYmpxQ3hVNE9VZjN5ZHJoaWdvUGdW?=
 =?utf-8?B?cXBSalovUXg0K2VlSTQ3VHY4K2RvU3BXd1Rrdng2anVJclI4WUZsZzNtMTV5?=
 =?utf-8?B?czFUUmJSaU95WURYaWd4Tkp2b00yNXhTWHN1TG1zbCtZbUs2Z1ZQQnJicE1M?=
 =?utf-8?B?SGhETjBCYW1GMm9XL0JPZVZkUnVvYmg1QUYvWm9wcTdXRU5STEpqWHRmZlU0?=
 =?utf-8?B?dXNmMWNDQmh0UlJUZ3dEMnhRR0Y3RVo4bDRkL013ejFZSDB0c09OUE5WdEFZ?=
 =?utf-8?B?SXdOTnh6cytwSVpnMWRyTEZ2eExkZzFpSGF0WTFMUjhmb00rNGI2bVlUVFg4?=
 =?utf-8?B?cjBGUTh2Mk1SdXpQaCtuazUrTDRtSkEzVUlGdktibUJyWnlJMVp4WWsyQ3lo?=
 =?utf-8?B?WW1HdlBFVExiQ2dST01ERGpuVFZtK3VVN0R4OU56MmNZQjVJcmRnN1dQM00r?=
 =?utf-8?B?bkV5dmd2aExLbU94T2xVNnB4TTB0RkJlRlo3Mk41MTRrSytkajd1WDZ4Uk1s?=
 =?utf-8?B?OThOdWcwS3JGY3Q3eWticnJHY2NOWC8raU9iYmROWElJdEcvUVNFVXJPemt3?=
 =?utf-8?B?a01YMitKZ0dha1JPT09SVnlGbUtFdnd0TVJUeWp0eUlkNXlUMTZDVGw5aEtV?=
 =?utf-8?B?V1pjNDVmK1FQWnMycTdkcFYrTGdRNUlDenFVUTZXYkkyMnFCRTJ2NlZucW8w?=
 =?utf-8?B?TFdvM3h6QVRqRGRIQWpNTUEwbFZHeTNnMGxKSkkxVHNlYjNHQ1NQY3FzUjFU?=
 =?utf-8?B?NzFyKzlqR0VlVlB0bTRudFVJRFlFdTJ1WDdsbmJzMW5iUGZNSWlmVE1UMnRq?=
 =?utf-8?B?ZmIreUV1N2xBY016RXIyWXNZemxvL05nWFErUFVVNnpUU2MxNjlHQjBXRllJ?=
 =?utf-8?B?bzZmNGI4K2dVUXFlQkttay94aTVSS2NOdEZTRllqcTRNT3pOQ1JwdytRUUQ0?=
 =?utf-8?B?czBLTzNZSGZrY1hkQ04zbnhnMDBWU3VudCtQdUtjSjdMQW5UNU9VZVBrV2pJ?=
 =?utf-8?B?T2ZkRXhtM2ZhUVhjQnRPTVNYbUFiV2ZWZC9LbHpLakRkbVgwVUE1a2Y5Q1ZI?=
 =?utf-8?B?Mzg0YzF4dVJlNWRYVFJpcUxta0pFblIwTW5NWVlXUTE0TVgrZjlsN21oeUZk?=
 =?utf-8?B?blp0U0t5dlB1NEtKWVlnOGs3bTE1aEQyS201NENRaEdEMVYzOEZWRnVYeS82?=
 =?utf-8?Q?OVLCVjnsnCftP4JkgfoC1abhv?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	K4jV5F7n0DBKUqpYOgWq1z18tvvarsnfOHx2rDkkkt+JuvC3WNQRylRDLLBaYgwpMIuh0EhQlZTDsHRQJ6Forcvrw9KMIERYIAZ9mAWydGhOxtvJX0WAAK9uBlTdAQDkIKOb7oKdKJZBNUeD/q763pi1hcKejI9KAD5KdQM5DiqCAl2+aj4OtFxWfaprUMZuYkNCmRFVwyvTTXZM2Y6nmwp3p0/L2944c1ZtwVbtHDR/t4yNWcCuPp6dSlS5HpiAiiu6KBP9s76JRehHzLWOM5+nntZIy5VyB8qrb885QAuiLdnw8nScxw1loOF5N4b28uYPK7/TaWxErPtMzUzd0bk86C8zg9evBEbzM9ZFAJWWkbBoidmp+PubFTaf7sW97EAQBBIViMMGY/tS0h47M390BhxQeQH0HgK9gYWDYY2g9bspvCpmFOEjE4MuUCpkTcgKjya+69rfOE4EHAuBJdRUSpmdoT6VvYpTU2TJ/MZsfgIDTK8UC9B2B+Hqb8LNmt9HJ9la4xcySA+BAkRHZzXXupt4SY51+KRZIsla06vila6PsbZZd+a1KVfNIu01M/+yBJI4SV3t6DTLOgqp73K/V23KVTS7aukhlgp6YOE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ca22a0b8-9477-4681-fa68-08dc34e74f7d
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2663.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Feb 2024 03:18:45.6310
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fhfIpnxRZZD/5pXngntQzp847tbblqcJpeZnRmL3KPElloTG+Al+h18+cW6Q3jiNnmtzQrs+I5wM8UdxvH1ivg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB6981
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-23_08,2024-02-23_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 phishscore=0
 adultscore=0 mlxlogscore=999 malwarescore=0 spamscore=0 bulkscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402240025
X-Proofpoint-GUID: LfwTBIomQiJTuEbiKdMldPjy9-p24W0y
X-Proofpoint-ORIG-GUID: LfwTBIomQiJTuEbiKdMldPjy9-p24W0y


On 2/23/24 12:21, Sean Christopherson wrote:
> Combine possible_passthrough_msr_slot() and is_valid_passthrough_msr()
> into a single function, vmx_get_passthrough_msr_slot(), and have the
> combined helper return the slot on success, using a negative value to
> indicate "failure".
> 
> Combining the operations avoids iterating over the array of passthrough
> MSRs twice for relevant MSRs.
> 
> Suggested-by: Dongli Zhang <dongli.zhang@oracle.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 63 +++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 014cf47dc66b..969fd3aa0da3 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -658,25 +658,14 @@ static inline bool cpu_need_virtualize_apic_accesses(struct kvm_vcpu *vcpu)
>  	return flexpriority_enabled && lapic_in_kernel(vcpu);
>  }
>  
> -static int possible_passthrough_msr_slot(u32 msr)
> +static int vmx_get_passthrough_msr_slot(u32 msr)
>  {
> -	u32 i;
> -
> -	for (i = 0; i < ARRAY_SIZE(vmx_possible_passthrough_msrs); i++)
> -		if (vmx_possible_passthrough_msrs[i] == msr)
> -			return i;
> -
> -	return -ENOENT;
> -}
> -
> -static bool is_valid_passthrough_msr(u32 msr)
> -{
> -	bool r;
> +	int i;
>  
>  	switch (msr) {
>  	case 0x800 ... 0x8ff:
>  		/* x2APIC MSRs. These are handled in vmx_update_msr_bitmap_x2apic() */
> -		return true;
> +		return -ENOENT;
>  	case MSR_IA32_RTIT_STATUS:
>  	case MSR_IA32_RTIT_OUTPUT_BASE:
>  	case MSR_IA32_RTIT_OUTPUT_MASK:
> @@ -691,14 +680,16 @@ static bool is_valid_passthrough_msr(u32 msr)
>  	case MSR_LBR_CORE_FROM ... MSR_LBR_CORE_FROM + 8:
>  	case MSR_LBR_CORE_TO ... MSR_LBR_CORE_TO + 8:
>  		/* LBR MSRs. These are handled in vmx_update_intercept_for_lbr_msrs() */
> -		return true;
> +		return -ENOENT;
>  	}
>  
> -	r = possible_passthrough_msr_slot(msr) != -ENOENT;
> -
> -	WARN(!r, "Invalid MSR %x, please adapt vmx_possible_passthrough_msrs[]", msr);
> +	for (i = 0; i < ARRAY_SIZE(vmx_possible_passthrough_msrs); i++) {
> +		if (vmx_possible_passthrough_msrs[i] == msr)
> +			return i;
> +	}
>  
> -	return r;
> +	WARN(1, "Invalid MSR %x, please adapt vmx_possible_passthrough_msrs[]", msr);
> +	return -ENOENT;
>  }
>  
>  struct vmx_uret_msr *vmx_find_uret_msr(struct vcpu_vmx *vmx, u32 msr)
> @@ -3954,6 +3945,7 @@ void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap;
> +	int idx;
>  
>  	if (!cpu_has_vmx_msr_bitmap())
>  		return;
> @@ -3963,16 +3955,13 @@ void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
>  	/*
>  	 * Mark the desired intercept state in shadow bitmap, this is needed
>  	 * for resync when the MSR filters change.
> -	*/
> -	if (is_valid_passthrough_msr(msr)) {
> -		int idx = possible_passthrough_msr_slot(msr);
> -
> -		if (idx != -ENOENT) {
> -			if (type & MSR_TYPE_R)
> -				clear_bit(idx, vmx->shadow_msr_intercept.read);
> -			if (type & MSR_TYPE_W)
> -				clear_bit(idx, vmx->shadow_msr_intercept.write);
> -		}
> +	 */
> +	idx = vmx_get_passthrough_msr_slot(msr);
> +	if (idx >= 0) {
> +		if (type & MSR_TYPE_R)
> +			clear_bit(idx, vmx->shadow_msr_intercept.read);
> +		if (type & MSR_TYPE_W)
> +			clear_bit(idx, vmx->shadow_msr_intercept.write);
>  	}
>  
>  	if ((type & MSR_TYPE_R) &&
> @@ -3998,6 +3987,7 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap;
> +	int idx;
>  
>  	if (!cpu_has_vmx_msr_bitmap())
>  		return;
> @@ -4008,15 +3998,12 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
>  	 * Mark the desired intercept state in shadow bitmap, this is needed
>  	 * for resync when the MSR filter changes.
>  	*/

BTW, perhaps fix above the above indentation issue too? I did not notice that
when working on the initial patch.

Thank you very much!

Dongli Zhang