Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <d377806e-43af-4ac7-8e7a-291fb19a2091@intel.com>
Date:   Wed, 15 Nov 2023 16:31:25 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 24/25] KVM: nVMX: Introduce new VMX_BASIC bit for event
 error_code delivery to L1
Content-Language: en-US
To:     Chao Gao <chao.gao@intel.com>
CC:     <seanjc@google.com>, <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <dave.hansen@intel.com>,
        <peterz@infradead.org>, <rick.p.edgecombe@intel.com>,
        <john.allen@amd.com>
References: <20230914063325.85503-1-weijiang.yang@intel.com>
 <20230914063325.85503-25-weijiang.yang@intel.com>
 <ZUHSTEGpdWGjL93M@chao-email>
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZUHSTEGpdWGjL93M@chao-email>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dWVuRzh3ZERDMm9yajRBejhIVDdDaWE5dU5QamtmalVOaXRiWmhWRkQrVGRa?=
 =?utf-8?B?cFJFbXNKRkc5Zzl2ajNGSVg0L0NjVzVFczZvZCswMEpwSVdJL2lyV1JmQ2VQ?=
 =?utf-8?B?dGZJRUY3elZHdXdjK0ZzaGpDZFNrQ0dGZ3JwUEIzS3pRTkFXckdBWkVPU1lr?=
 =?utf-8?B?YzhYazF5ckplbGN1Wm44N1BDeXN5TGJOOGlQUGVtdVJILzFqeW5CanVzVXRZ?=
 =?utf-8?B?Wi9sbFhOcFZSUnBFeEVzVk16a05Xd0w1c0lOQ1FBdENCVk5adVppYW11R1hZ?=
 =?utf-8?B?WTNDTnNGaFlMeUgzQWtkR0F6RnFBQk5HRkxJWTNVWnJ1TkxoZTJoZVBwakY0?=
 =?utf-8?B?ZUgyNm03THo3d2o1Y2Q2d05HWHBxbWNrY20zVlBMa0hvMlhnUkFuUncyUXMv?=
 =?utf-8?B?UDV0MzJrb2o2QktqOW1vc0RPak5YS1VsMVVIZVpTemhkR0YxQSs3bFI5OGw5?=
 =?utf-8?B?R01BbDJsblAxckNMWXpBakZQaHI3aE5XM0RLSkUxV0JZTHRRVmxkZjFjUW5v?=
 =?utf-8?B?N0llTnVGeTRHK1FqekNFWnlVU1JqU3VKQzVBNzBtaDR5VDB6OWEranZwKzlq?=
 =?utf-8?B?RjlDR1BNSG1kUXEyOW9HRnNVTXFhUGRhZW9HMjFwZ2hSTmtqbVBObEljUTh2?=
 =?utf-8?B?NkNiSmRad1lhcnlqTzdmdUt2eW4yTzQvaUlkL2E1dFBOc1FjK2x3V0tHM1JB?=
 =?utf-8?B?VU84OHFEZUFUNGl3Y0FuWTM0ZVU5T3N6Q3c0OCtOMWZ6QXF2RURRbDRPSkFB?=
 =?utf-8?B?ZWNPa2RuMnZiZGZJMWs2WmcyYTVnOHJqRjlXLzE5YVF4SlZHeVRSSTI4ZXpv?=
 =?utf-8?B?ZE5OcWVMWGpuTDI5VG14dlZTV1c4ZGdBUVpBcUVnKzM2N203aXhCYUk2SXUw?=
 =?utf-8?B?WUZ3SlRHSFQ1Z0ZxMm9zdnJNNWlSMXN1NU9EYytPOExSQ3o1TjN0NUVkZWNQ?=
 =?utf-8?B?d0hsY1BDb2szaE42cnFQR2JVa21xcnIrSkgvUVFvNE95U3YyREZzMENSaWZZ?=
 =?utf-8?B?cC9tMDJhc3k3U0ZNQ2tvU3VaR2RkczNsYitoekd2dC91eHZYOWMvNVgxSXZX?=
 =?utf-8?B?ZzJjUy9WUjFEY1V2cXFOcmhIMC9IcWZBOW5mZzJxb1ZnVU9ubjhPdGR6M0N5?=
 =?utf-8?B?MnNOTGUwMXB0UzZFY2xWSThoRng0WmZUK0tjSWp3OEkxeVpZRnQwWEd1SWo2?=
 =?utf-8?B?ZUMxOXZMbUVSTFY5R3FTc2M5UEVVckxJTkVlR2JzQUljcURCY0RnU3FuTVlS?=
 =?utf-8?B?TmtPWk9CYWFMdWNWUm96dzh4VXU0NFhUdjV2MTV5aU16NXR6QmNCc3lXSTYw?=
 =?utf-8?B?UlZ0TSs2SnBreVoyd1lTYkNFaFJRb25tT09vSFhFRTdGOURNbUpXeWh5dGJP?=
 =?utf-8?B?YkVBWFpETEVKUlR3Q3dFT3BYdHdtTmMwcXdlYzRwd0dDa292ZWNiVSs3Vllu?=
 =?utf-8?B?MWVOSG52YUVZM1ZyNkoyZyt1ZDU3WmtnNlBJeHdjbGdxeGN4T1V0NnFJRFBm?=
 =?utf-8?B?cTlUWERGcFdLdU5BQ3R2TTdRUkhzZnl1cHdYQ2FUbGIyQ2ZKUWxobzl5WjdH?=
 =?utf-8?B?VHByTzJmRmNyMGIyQzBZclJnSGpORWR0NWFpK2swN3M0NzB2Yjh3RkQ4eGVQ?=
 =?utf-8?B?L2Q2UU5oY2NLeSs2aXNRR3BRYVBDMEpVRWFpUjl4T2VjNzdqT0R1KzJSYnZR?=
 =?utf-8?B?bzZOYzRqSkFUNVM0aXFtUFh1MkZuOUdBWGhaVmVVTnJiQ3VsSDFuVTZmdG9G?=
 =?utf-8?B?eUhSOGUrQStKZmFtSmRuNTJBZ09zdHZqQ0h1bkNvS0txRGFIdjV5eG5Hemt1?=
 =?utf-8?B?MExEcDFudjRWTXo0enFSZzRFdFV5VzV5MjZYRXRac3NLUjU4YnMxSzkvTjVr?=
 =?utf-8?B?V1NDR2FxeEFkbTFScjN1RnB2QU5XQU1rVWpuam9uSFlydHRkNENBb2VJV0xP?=
 =?utf-8?B?V29JNEc4dy9DaGJ4cTZNUnl3T1NIUEd4M1R6N2ZkNndvanNEMzAydWtiL2JK?=
 =?utf-8?B?dm9lTFBHYllndWVXS1RJZzY5M092bXNCdjR2NDZudy9OUkRiakZxZ3hSOXQ4?=
 =?utf-8?B?akNaRTFaY0FjdEtOZTJPUlZSeFcrczE0VUtKU29YL3VtY1RHTXRNeWJjM3E2?=
 =?utf-8?B?Q1llaER1QURVSVhjZ3pwTUhaUVEvdEE2cDBSS3BrSEpGcmxWMXRiZVMydUQ4?=
 =?utf-8?B?Z3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a00d6314-20f9-456f-1b3f-08dbe5b5470b
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Nov 2023 08:31:35.1560
 (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: jvYLFHZu+Xk2SoMcO9mXtwswn8eOGtuoSYdWGhXXd4qOdtCcWvDqkmAf4WDINoA61w9Y/2IsOkObL49qdRp5Bg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7309
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Wed, 15 Nov 2023 00:31:43 -0800 (PST)

On 11/1/2023 12:21 PM, Chao Gao wrote:
> On Thu, Sep 14, 2023 at 02:33:24AM -0400, Yang Weijiang wrote:
>> Per SDM description(Vol.3D, Appendix A.1):
>> "If bit 56 is read as 1, software can use VM entry to deliver a hardware
>> exception with or without an error code, regardless of vector"
>>
>> Modify has_error_code check before inject events to nested guest. Only
>> enforce the check when guest is in real mode, the exception is not hard
>> exception and the platform doesn't enumerate bit56 in VMX_BASIC, in all
>> other case ignore the check to make the logic consistent with SDM.
>>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>> arch/x86/kvm/vmx/nested.c | 22 ++++++++++++++--------
>> arch/x86/kvm/vmx/nested.h |  5 +++++
>> 2 files changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index c5ec0ef51ff7..78a3be394d00 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -1205,9 +1205,9 @@ static int vmx_restore_vmx_basic(struct vcpu_vmx *vmx, u64 data)
>> {
>> 	const u64 feature_and_reserved =
>> 		/* feature (except bit 48; see below) */
>> -		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) |
>> +		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) | BIT_ULL(56) |
>> 		/* reserved */
>> -		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 56);
>> +		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 57);
>> 	u64 vmx_basic = vmcs_config.nested.basic;
>>
>> 	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
>> @@ -2846,12 +2846,16 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
>> 		    CC(intr_type == INTR_TYPE_OTHER_EVENT && vector != 0))
>> 			return -EINVAL;
>>
>> -		/* VM-entry interruption-info field: deliver error code */
>> -		should_have_error_code =
>> -			intr_type == INTR_TYPE_HARD_EXCEPTION && prot_mode &&
>> -			x86_exception_has_error_code(vector);
>> -		if (CC(has_error_code != should_have_error_code))
>> -			return -EINVAL;
>> +		if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION ||
>> +		    !nested_cpu_has_no_hw_errcode_cc(vcpu)) {
>> +			/* VM-entry interruption-info field: deliver error code */
>> +			should_have_error_code =
>> +				intr_type == INTR_TYPE_HARD_EXCEPTION &&
>> +				prot_mode &&
>> +				x86_exception_has_error_code(vector);
>> +			if (CC(has_error_code != should_have_error_code))
>> +				return -EINVAL;
>> +		}
> prot_mode and intr_type are used twice, making the code a little hard to read.
>
> how about:
> 		/*
> 		 * Cannot deliver error code in real mode or if the
> 		 * interruption type is not hardware exception. For other
> 		 * cases, do the consistency check only if the vCPU doesn't
> 		 * enumerate VMX_BASIC_NO_HW_ERROR_CODE_CC.
> 		 */
> 		if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION) {
> 			if (CC(has_error_code))
> 				return -EINVAL;
> 		} else if (!nested_cpu_has_no_hw_errcode_cc(vcpu)) {
> 			if (CC(has_error_code != x86_exception_has_error_code(vector)))
> 				return -EINVAL;
> 		}
>
> and drop should_have_error_code.

The change looks clearer, I'll take it, thanks!