Received-SPF: pass (google.com: domain of linux-kernel+bounces-16095-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: <ae83fa1b-99f2-40aa-ad2c-d31ff0d72e47@intel.com>
Date: Thu, 4 Jan 2024 00:03:01 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1] KVM: nVMX: Fix handling triple fault on RSM
 instruction
Content-Language: en-US
To: Sean Christopherson <seanjc@google.com>
CC: <pbonzini@redhat.com>, <tglx@linutronix.de>, <mingo@redhat.com>,
	<bp@alien8.de>, <dave.hansen@linux.intel.com>, <x86@kernel.org>,
	<hpa@zytor.com>, <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<zhi.a.wang@intel.com>, <artem.bityutskiy@linux.intel.com>,
	<yuan.yao@intel.com>, Zheyu Ma <zheyuma97@gmail.com>, Maxim Levitsky
	<mlevitsk@redhat.com>
References: <20231222164543.918037-1-michal.wilczynski@intel.com>
 <ZZRqptOaukCb7rO_@google.com>
From: "Wilczynski, Michal" <michal.wilczynski@intel.com>
In-Reply-To: <ZZRqptOaukCb7rO_@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?amhpcUtEdWVOVjVHYmN5cXhvV3FBdWN2d2ZZek5yVytrdWtBNVROR1RtOVJj?=
 =?utf-8?B?ZXR6UThuWU5mK0lBNUNiOHR1Y2FZakVMd0x6aUZyYTdoeDlEUER6TmQyK1RN?=
 =?utf-8?B?OTA5cnpOUTlmWFlqeTcySTMyeEFadHA2d0dNcC9qNURGWjY2TExRY2tPdW51?=
 =?utf-8?B?ZUJrYUlGVWRFcTRLTW5pZlFKOG9iQW9PK1pYUGl2bkwxUXJZdUtkNkRkeXoz?=
 =?utf-8?B?K2JhVlB1YlQxRlYrRFlMeGtlcmlIYmhIT3kzOTB6WjRzV2NxUzRIdmYvK00y?=
 =?utf-8?B?VHZGeGZGM2w2aDB0YmpEeEdLQ2E4NDlFMEI3Mk0vMytSUmpXd1N6akNtQ3hC?=
 =?utf-8?B?cVIxUjhTL2ZDZzAzckV0TmtiN0hnNHJxUXRjRGFCWmh6MmNtV1hSQmJWdm8w?=
 =?utf-8?B?cmNYcFFlb05IRlltd0JDcGZMMTBSdVk4dWp2UXJUZ3Z5bVRldEx6RkFKNDFP?=
 =?utf-8?B?YW9BQXAwalpUdEYxMU5kYVZVbkRadVB4RGdLVkU2eERLb2VRamlFVjB0VWN6?=
 =?utf-8?B?Rk1GWXBCN1dGa3FFUnJ2VjduSmY2SlFoeHpPMzhlM1NUd3NZQ002T0VERzd2?=
 =?utf-8?B?MnRnOGE1M2JGdWdEQ3FFczhDaFI2dElBQldHZEZBOFdFdG5YaDRkcENYUGRk?=
 =?utf-8?B?QmJWM2NOVW0vS21aSjA1QWVxb01QMUx2L3ZUQVVwWmlGbE9pNFo1MU4waGNv?=
 =?utf-8?B?Q01GR0NJSE1SWFpMMU83eC9xOENGTS9qVy9mU1NqUHpmR01rUW02VW9rQzdz?=
 =?utf-8?B?amlkNkU2WWZkeE9CWjIxRldwQlUvbzhCVGtGcFBTRTB3RmFkcEtRamtObE95?=
 =?utf-8?B?Wjc5ZmJoYmJVSGZScnFCR2lnV1M5RkZDSUZSemJsM3FMVEhRUUpGb2Jnc0l6?=
 =?utf-8?B?Y2lFT2tqdFhjM3ZBMDg3czJ6RVFUeDV4c3BGUTBXSnRVZnl5Ykc5VCtMNExq?=
 =?utf-8?B?R29sOWlMNW1aQkF4M3Q0dzdQcnFaZWpEbjlmNHRSQkwyK01TclI1Ujl1ZUFU?=
 =?utf-8?B?T0FQUVM1TXlZM3o2dVlUNUlYM0hVUUM1dGEyYnB4T3RyRXRraXVNSlpXN0lC?=
 =?utf-8?B?QnJFL1hORnFENGtoSVV0RG1QYkg1NTdPdFRaQmxXMUJ2TktpcjVJaUJOQk5k?=
 =?utf-8?B?Mm5mMXVCK3Y3UjFPV0Y1cTBQTWpuMm5VM0hmY0xlMHkxVnh0V0JHV3FTYVF1?=
 =?utf-8?B?a3J5ME9ETGN6Nlp4UWZrMGNJRFNzM2g1QUxieWl5WHZKSFdUUTdNVU9wYnRV?=
 =?utf-8?B?RmVwK3dzV2lzbzhsVGR2d0VPQ0lySmxXSzVlVWl6b2dRMkt3a1pFZW12OS9N?=
 =?utf-8?B?OEVlQ2g2bkc5R0htOG53UzFFODJLMGFXWnYydGtLbk16RGREWXM4cHVxSXc2?=
 =?utf-8?B?NjhVNWI1eDdIQVEza0VrM00zc1RXdk5YQzBuOVRHK2EzRGVJTitpVk1SN25q?=
 =?utf-8?B?WEJ5Z1BWR0o3eldKaDBTQXJXWlUveXZNam02WXU2cWlxZW1QZlNxUmVtVlJ3?=
 =?utf-8?B?YkRvNGZKT2RoNXdLazBrUURiYVdvOVY4NDRNT2xVczNBQjZhMDQwdFhYcS9o?=
 =?utf-8?B?R0hnRUFyRkFqajQrUGhuc2g0U1lDbUtxeWpCaTZSeHZQelZmNDVjNXB3K3pw?=
 =?utf-8?B?Rk5mYjhYUUJwSjhnUEtDWEJHcVdNcUZmN01SbU0zMWZvR2lzQTR0b0ErcHFJ?=
 =?utf-8?B?aE5nSm5wdGwyeUF1bCtHVUVqbjlkbEdJWlBBMXprSjE4VkQ3NllHeE90bk5t?=
 =?utf-8?B?WUhMalhNR1FCSG9xdSt2YnhqNHhyTkJmdGZ5VTZGb2xkRVZ0Qm9ZREZOZHNr?=
 =?utf-8?B?T0hBSGx5ZHMzN3V4ajQxQzlLdCtJTkpYL0pEZUk3Q29sbFBVSnIxc2xoT3lo?=
 =?utf-8?B?Y1NTcnQvemVjNTByUU43RDB6L2NKRTlYZG85RFExcllrY2FLVXFMUDZ6UTVB?=
 =?utf-8?B?amFPSWViOTJpZXc5NFlEdjNoUTNjbE5FQ3RKVVJ0UkVrOXhnbk5zYWZLdUE0?=
 =?utf-8?B?OXQxaFFZS0NYT012QklwZ1RYMVhJYVJEMG9XL0hac2d2MzVrcVY3c0FhT3RQ?=
 =?utf-8?B?c3FkNjRZTlg3cUQxWDFPQUY5T0dNZ3FwT2hia3FUVzZndDVNUXRnRjB6WE5t?=
 =?utf-8?B?RHpjT21ZYzA3S2dGSEdMNjh2T3cwN3pRVFFTcktaY3AyaWV5Z09HbFlIcGg3?=
 =?utf-8?B?dFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7c476d45-fda5-427b-0722-08dc0cb026cf
X-MS-Exchange-CrossTenant-AuthSource: CO6PR11MB5603.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jan 2024 23:03:08.7195
 (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: pGGV7S+mgDA0bD9hZ9kZRNKwHpog4NdkYr5q8wmhPN3Bsu6D24o+qrEceD4cj/ICK0ZMtpVddnoHB9FI4r1YIKrkZlSFzvZMeA6Wgq8ST+Q=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB8776
X-OriginatorOrg: intel.com


On 1/2/2024 8:57 PM, Sean Christopherson wrote:
>>
>> Additionally, while the proposed code fixes VMX specific issue, SVM also
>> might suffer from similar problem as it also uses it's own
>> nested_run_pending variable.
>>
>> Reported-by: Zheyu Ma <zheyuma97@gmail.com>
>> Closes: https://lore.kernel.org/all/CAMhUBjmXMYsEoVYw_M8hSZjBMHh24i88QYm-RY6HDta5YZ7Wgw@mail.gmail.com
> 
> Fixes: 759cbd59674a ("KVM: x86: nSVM/nVMX: set nested_run_pending on VM entry which is a result of RSM")

Thanks !

> 
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>> ---
>>  arch/x86/kvm/vmx/nested.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index c5ec0ef51ff7..44432e19eea6 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -4904,7 +4904,16 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
>>  
>>  static void nested_vmx_triple_fault(struct kvm_vcpu *vcpu)
>>  {
>> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
>> +
>>  	kvm_clear_request(KVM_REQ_TRIPLE_FAULT, vcpu);
>> +
>> +	/* In case of a triple fault, cancel the nested reentry. This may occur
> 
> 	/*
> 	 * Multi-line comments should look like this.  Blah blah blab blah blah
> 	 * blah blah blah blah.
> 	 */

Sorry, didn't notice, and checkpatch didn't complain. In other
subsystems e.g. networking this is not enforced. I will make sure to
remember about this next time.

> 
>> +	 * when the RSM instruction fails while attempting to restore the state
>> +	 * from SMRAM.
>> +	 */
>> +	vmx->nested.nested_run_pending = 0;
> 
> Argh.  KVM's handling of SMIs while L2 is active is complete garbage.  As explained
> by the comment in vmx_enter_smm(), the L2<->SMM transitions should have a completely
> custom flow and not piggyback/usurp nested VM-Exit/VM-Entry.
> 
> 	/*
> 	 * TODO: Implement custom flows for forcing the vCPU out/in of L2 on
> 	 * SMI and RSM.  Using the common VM-Exit + VM-Enter routines is wrong
> 	 * SMI and RSM only modify state that is saved and restored via SMRAM.
> 	 * E.g. most MSRs are left untouched, but many are modified by VM-Exit
> 	 * and VM-Enter, and thus L2's values may be corrupted on SMI+RSM.
> 	 */

I noticed this while working on the issue, and I would be very
interested to take this task and implement custom flows mentioned. Hope
you're fine with this.


> As a stop gap, something like this patch is not awful, though I would strongly
> prefer to be more precise and not clear it on all triple faults.  We've had KVM
> bugs where KVM prematurely synthesizes triple fault on an actual nested VM-Enter,
> and those would be covered up by this fix.
> 
> But due to nested_run_pending being (unnecessarily) buried in vendor structs, it
> might actually be easier to do a cleaner fix.  E.g. add yet another flag to track
> that a hardware VM-Enter needs to be completed in order to complete instruction
> emulation.

Sounds like a good idea. I will experiment with that approach.

> 
> And as alluded to above, there's another bug lurking.  Events that are *emulated*
> by KVM must not be emulated until KVM knows the vCPU is at an instruction boundary.
> Specifically, enter_smm() shouldn't be invoked while KVM is in the middle of
> instruction emulation (even if "emulation" is just setting registers and skipping
> the instruction).  Theoretically, that could be fixed by honoring the existing
> at_instruction_boundary flag for SMIs, but that'd be a rather large change and
> at_instruction_boundary is nowhere near accurate enough to use right now.
> 
> Anyways, before we do anything, I'd like to get Maxim's input on what exactly was
> addressed by 759cbd59674a.

Thank you very much for such a comprehensive review! I've learned a lot.
Will try to help with the mentioned problems.

Michał