Received-SPF: pass (google.com: domain of linux-kernel+bounces-75139-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <0c8f3f2d-11af-57b9-c922-94755001b973@oracle.com>
Date: Wed, 21 Feb 2024 08:50:04 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 3/3] KVM: VMX: simplify MSR interception enable/disable
To: Sean Christopherson <seanjc@google.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, linux-kernel@vger.kernel.org
References: <20240217014513.7853-1-dongli.zhang@oracle.com>
 <20240217014513.7853-4-dongli.zhang@oracle.com> <ZdPXTfHj4uxfe0Ay@google.com>
 <05571373-5072-b63b-4a79-f21037556cfa@oracle.com>
 <ZdYaIn7xwqTPdofq@google.com>
Content-Language: en-US
From: Dongli Zhang <dongli.zhang@oracle.com>
In-Reply-To: <ZdYaIn7xwqTPdofq@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?NUI5V0hXbCt5MC9TQ0ZRNkMzeGxkV2hwcnNwS2xCRm5HbSs4MWx6enRMZW4y?=
 =?utf-8?B?UG1ySEpqME9oZ0lIN1RxaEpyVEV6NjI3TXJZbS9IdTVCdiswRW4vN0hDWWtQ?=
 =?utf-8?B?d0dVOFVRTGJXM1pxM1kvRzlOREI1S3dCTk5oOG5hZi9DRi9NWFkvbHFtSTRm?=
 =?utf-8?B?c3NMdmJpWFUvUTYxZDk2RzhnUnBsSHhYYzJjK2kyUjdsMGxqNTVOWE9mOE4w?=
 =?utf-8?B?OVQzaTFDZFlMME02a2NadDV1ZFg2ZzlhbzM2N0l0THNkamJRN2t0MkY1RS94?=
 =?utf-8?B?U2FNenpEWGt0UjM5S2dZRGQrS2ZSblV0dlZNTnNaOFAydEh5MkloWHZCRXho?=
 =?utf-8?B?a0JZTXdSUjA4eTA0dVNmNEFaenplNGx1R1NHaDV3MWlBQitFaWtYSTBXTk9W?=
 =?utf-8?B?M0pON05ZTTZPREE1a1A4RG03QlNCckQ0a2J4R29wRXVSemp2UDJUaFduZldX?=
 =?utf-8?B?cm1id0tCSVpMcDdjeUlCWXlJNHB1SEJNMXk4ckovbWZNRXVxbzFmajlsdEhU?=
 =?utf-8?B?Vjd5Z2h5b2lCSmZ4cWhIN3Nrek8wYmxiMjQzdnlSZWdqTGsyUFdkcVNEWmhN?=
 =?utf-8?B?QlRoaHFoT2RYTHhCL3VFak1OdGErNGRuMmRudHZCM3V1U3R5SGxhZEVITlZ5?=
 =?utf-8?B?Ym5HOFJBSjhUS3ZzYitCYzVoOFFGQnU5NFErZWlVNVdEOHlpV0kvVlBCYzUv?=
 =?utf-8?B?MEJSOVJkQzhZTTZ4NDRoVUpuSzRCQ3pJc2lxdzIraXYvNlh2c3A3UzQva01l?=
 =?utf-8?B?SGVzQnpCdnRKbEFaMWhVaEdXd2c0L3dhbnAyZWRFYi9aTXBNQm9Xc2EvSFI5?=
 =?utf-8?B?VVBsQ25qSFg4a0lzdGtYa21YV0ZNbGNZZ0RERlVoUmM3eS85NTFqQVE5MHAv?=
 =?utf-8?B?ZkhNb2JqQU1reTR6WGJRcmphdFM4MDRFeXRRczJ2MndKZlBTUXlsc1ZybUNZ?=
 =?utf-8?B?RUdQUG1XeUp0QzMrTUxGL1BTckhoZ2ppSG5pb3A3bng5aDRiQm9xZUNyd2My?=
 =?utf-8?B?bU9oSnRMckI5c0taUThsTVVVMVhPSGpxYktLOERQcjVBdzNYVlFHTEp6NXdk?=
 =?utf-8?B?NnJVd3FKalR2UzhGZEFTSE9DWXNqeVZrZ3QzU2RROVZJbDJ5U09IeW94aitB?=
 =?utf-8?B?VDVTdmxXU2pTWVVPTDRlM0FKN1dVSVlmbXd1TDZlREY0RGNsUnNqK21OS0t2?=
 =?utf-8?B?eTBGUVNKbi83eUNOeHNKd09BL0ZnbVRmNkVnZFkvWXhhREJ6VmhyMGNNZkdE?=
 =?utf-8?B?VGlTUFZ0TForbE42UHViUCtjZCt2anNQNXNWSC80S3BZRWQ5ak8wMXAzY2lN?=
 =?utf-8?B?ZnNLT0pUTk5QdHUrVDNQbkdJMHcwYVZZOWg5SVc2OFo1VFJ3d2s5aHBPd3Bi?=
 =?utf-8?B?emtXYk5wZ2tvZHRSKzVHT0wxNnJGblBLVUU1blZ1c2tyclZlWjg3bEZRY3pF?=
 =?utf-8?B?c3A4aUx4SHUyQkw2QjJjWU9oNC94QVFzVGJFOVkyWU13N2dFb0NFWnhBakFR?=
 =?utf-8?B?WGNZTUgxSWk1YVl0T3JxNUY3RnBuM0xPcjNYT2VkMG5EeEFnQUNYOU52V0sz?=
 =?utf-8?B?V1AxQWxnVWZTRStNM25IUGRoQk9QeG8rTWR4S2IyTTRsZjlyWmkwV0E0QzVV?=
 =?utf-8?B?QXB4NlRhZno4aFg0RUw4Z3RRVEQ0Si9Sc0QyemFXeUI2QUtjYU9UNjlGaVJT?=
 =?utf-8?B?NlpZRWl2SkwrbVpJNlNXVHlpbWZtazVrd080RDBDTFRYdTcrSTdqTzM4bmU5?=
 =?utf-8?B?aTc4eVF1SGg3NXBjaTJoc2hXYkxnWWYvdWduRHhydTVUUnJ4WE0xRWcrSnli?=
 =?utf-8?B?TXJkQXFXbkVQVjhYY3RUNFZBQ21BdHBtUkxQb2R2cnRvZTh3TVVvM2JtTDNo?=
 =?utf-8?B?ZjRwbE9DSFgrZXQ0cGNZUzhwQzRSeFZMUmd5VWdUTTduZkNPQldCa2ZoTzdL?=
 =?utf-8?B?UU9CRTVLWDhyWEErdWRTejM0RUc4OExMTWF2bWxITGl4Q041RkRFYmI0UjV3?=
 =?utf-8?B?Rm5ISzZRY1FHZWduOTJ0WW5sbGhyWkh2OWJPZlkxdCtib21kYjFBdFhrUmcr?=
 =?utf-8?B?TlRoemo4SnFkMW10ZTgrZ0RkTTdjVk9oMFBUVjJiR3pHN2Vnc1FPcWR0ZDJt?=
 =?utf-8?Q?uz3pYqris5YsgoizW5DGHKzfo?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	6+urZHnRUePSppORM/IFrp01iD9mGIlOmpDiZ+CajFAzozdBWzCWP2naFxXsIE5T1gi6eddNlXdYnG3yzCvSNoDyNYTWkA0NdGpg2JKYRRfYSfpTQ/f2kKZGMee9aGBOAk9tPaK/ldPXGAzb4wzkOcvVJEIbZ3wI732ALo5lA7+ezBlHtne3F4kA7N7fcZGlOXZM66YMy8tzB50cIAkDnguOjLDP7kUJZUgf8a8Uj8QnG6ugvLR2C07IGPVDw3rZqot6pvjd++EZp8tt76fYb0EDzESzJTzpr0RPaf6cA3JBFsbNBlF9mGVuAu4S3JZ71jRCNsuXu2+HVttp4WnW9gVPJl8nvi5cAQ5Vc2WndxCqQxPX/P8DtfQr0jTpfPdigmwvbTwXz0ZbstrnNMv4nK2yY5qqdcuiavJQVkyQkO5VBud0yoTwmgds4jTI3wwzEFCSyjngdXEz2pEX7AahSLvKJm9FUCF48ymw+D4hTfAyv+oMYY/wkJy4WJut/Zmqx75+Hk+1m1nSIk+u7BtiZ7RbId6r2ujPakhpwy/1BFa1p6/auuhevpQUy1h1/vYccZqrIWODJFia49vLzRNHwAmvRqeBG3x7l/cjzeA73ok=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 22b0b20e-569d-41c0-da07-08dc32fd2968
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2663.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Feb 2024 16:50:08.4129
 (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: zE23aJzHPX+DKKCmm0fMRZe1nkIi/C77qC3+HpRrv+NOkA2ZmbeYvwbyliilyge7pr6izDL2lNn7G9HNsUnXEw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR10MB6412
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-21_04,2024-02-21_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0
 phishscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 bulkscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402210130
X-Proofpoint-GUID: ApMURgN-KF0YXfom1dCpG3bCRosi23KX
X-Proofpoint-ORIG-GUID: ApMURgN-KF0YXfom1dCpG3bCRosi23KX


On 2/21/24 07:43, Sean Christopherson wrote:
> On Tue, Feb 20, 2024, Dongli Zhang wrote:
>> Hi Sean,
>>
>> On 2/19/24 14:33, Sean Christopherson wrote:
>>> On Fri, Feb 16, 2024, Dongli Zhang wrote:
>>>> ---
>>>>  arch/x86/kvm/vmx/vmx.c | 55 +++++++++++++++++++++---------------------
>>>>  1 file changed, 28 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>>> index 5a866d3c2bc8..76dff0e7d8bd 100644
>>>> --- a/arch/x86/kvm/vmx/vmx.c
>>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>>> @@ -669,14 +669,18 @@ static int possible_passthrough_msr_slot(u32 msr)
>>>>  	return -ENOENT;
>>>>  }
>>>>  
>>>> -static bool is_valid_passthrough_msr(u32 msr)
>>>> +#define VMX_POSSIBLE_PASSTHROUGH	1
>>>> +#define VMX_OTHER_PASSTHROUGH		2
>>>> +/*
>>>> + * Vefify if the msr is the passthrough MSRs.
>>>> + * Return the index in *possible_idx if it is a possible passthrough MSR.
>>>> + */
>>>> +static int validate_passthrough_msr(u32 msr, int *possible_idx)
>>>
>>> There's no need for a custom tri-state return value or an out-param, just return
>>> the slot/-ENOENT.  Not fully tested yet, but this should do the trick.
>>
>> The new patch looks good to me, from functionality's perspective.
>>
>> Just that the new patched function looks confusing. That's why I was adding the
>> out-param initially to differentiate from different cases.
>>
>> The new vmx_get_passthrough_msr_slot() is just doing the trick by combining many
>> jobs together:
>>
>> 1. Get the possible passthrough msr slot index.
>>
>> 2. For x2APIC/PT/LBR msr, return -ENOENT.
>>
>> 3. For other msr, return the same -ENOENT, with a WARN.
>>
>> The semantics of the function look confusing.
>>
>> If the objective is to return passthrough msr slot, why return -ENOENT for
>> x2APIC/PT/LBR.
> 
> Because there is no "slot" for them in vmx_possible_passthrough_msrs, and the
> main purpose of the helpers is to get that slot in order to efficiently update
> the MSR bitmaps in response to userspace MSR filter changes.  The WARN is an extra
> sanity check to ensure that KVM doesn't start passing through an MSR without
> adding the MSR to vmx_possible_passthrough_msrs (or special casing it a la XAPIC,
> PT, and LBR MSRS).
> 
>> Why both x2APIC/PT/LBR and other MSRs return the same -ENOENT, while the other
>> MSRs may trigger WARN. (I know this is because the other MSRs do not belong to
>> any passthrough MSRs).
> 
> The x2APIC/PT/LBR MSRs are given special treatment: KVM may pass them through to
> the guest, but unlike the "regular" passthrough MSRs, userspace is NOT allowed to
> override that behavior via MSR filters.
> 
> And so as mentioned above, they don't have a slot in vmx_possible_passthrough_msrs.

Thank you very much for the explanation! This looks good to me.

Dongli Zhang