Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <296c5c68-d03b-65bd-bfb6-41e6046ed389@intel.com>
Date:   Mon, 24 Jul 2023 22:16:56 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v4 11/20] KVM:x86: Save and reload GUEST_SSP to/from SMRAM
Content-Language: en-US
To:     Chao Gao <chao.gao@intel.com>
CC:     <seanjc@google.com>, <pbonzini@redhat.com>, <peterz@infradead.org>,
        <john.allen@amd.com>, <kvm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <rick.p.edgecombe@intel.com>,
        <binbin.wu@linux.intel.com>
References: <20230721030352.72414-1-weijiang.yang@intel.com>
 <20230721030352.72414-12-weijiang.yang@intel.com>
 <ZL5AwOBYN1JV7I4W@chao-email>
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZL5AwOBYN1JV7I4W@chao-email>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bWIzcGYzU1IvMDRENUt3ekk5R3dCWTJ6d05NK3U2OUxBWjlzMUV4b3ZNSVc4?=
 =?utf-8?B?WEdYcG9lTFdYTTZQV2ZhNlBHVGFtMENqNW9PMXZEdWtQUFdlKzJqMFdYeGl0?=
 =?utf-8?B?MVdieDBQdTNwck1xb0ViNER0OGkyMzNoNGtMOWFGK1k2Si8zUmZhcXN4Nk9G?=
 =?utf-8?B?bnFYcXNCbGxicFNYcnY2UkdoT1JqSEZiNkwwbHBHdnErY0ZnT0JIQlBjODJl?=
 =?utf-8?B?ckFURStjRlIvTUUyWlpNR01TWDJHS2lpNVJRaHdiVGpqVm1mRzM1ZXpHUTdP?=
 =?utf-8?B?V0c5TUdXM3FXV1NvcVRnNUFVNlJKc3cxeEN6MEVpVVVabW11WGIwRTZwZkt4?=
 =?utf-8?B?VmpKSGp2cVhJQ1VkNDAwUForSG9xbHNGSTc3THZMSi9TSkdPbHJXYTIvdG5P?=
 =?utf-8?B?NEdzVUhzTmw3bnljcGExSzBLYVRhUm5RcFJsclVXejByZlJRWEV1V3NLV3ox?=
 =?utf-8?B?UkdoU0Rrd0xFT2lQWExVY3UyZmg0ZXFtRWJ0Zk8xa3VCT2dsVFA2QWszbHRB?=
 =?utf-8?B?WlBmODdzc0RjKysvTkRnb3Y4Mi9nY3d1d1U3TDErTjhVaThVRks3c1NtRVpY?=
 =?utf-8?B?UDcrWnA4N1hQYU1JeGpnM2xSNnFzZTZKdmNkNndvN1NZSDR6TXNPaHVLZWVG?=
 =?utf-8?B?VFlLV2dtRkZBOUU1ZGU4NzdzQllmS29tUXltVGxMZnJDaGVvM1lHNkwxTzZN?=
 =?utf-8?B?VW5xRGprQUZ0dFpHVGp4eDJoRG1BRHpXRUZZZmNWamxoOXpISC95ei9zYUlk?=
 =?utf-8?B?S2s4d2RrUGo1MjREY3pKclQyKzhQaGRldStNRFljQ0pQZmEvallocnpqTXdR?=
 =?utf-8?B?blFEWGlzcEVjMlFkaW1sVTNxdzJlaFZFMDJ6bWV0ZmE1bWtSTkszekpUR3Fw?=
 =?utf-8?B?dWcvazZ4NFdDUFFaVDc1eUJJUTdudERNRE5yaUZkdExoa3Jtb1NOSGx5Z1Zq?=
 =?utf-8?B?a3R0eHFWUkNGYUtWWDk0aldBS1FscVFUQWJ2WHJsTEc5SUhmUEczT05ibTc1?=
 =?utf-8?B?UGNhRTd2MFhpazhCcGtuREpDYVI2YUVrTG9UcFhENVdiUVFyNVd0YXhtZlJm?=
 =?utf-8?B?K3NRbE9XcG42eWNkenUrOXNjeWM1QlBEa3FtS3Q0TlhvYXRKSEdiMTlTVkU4?=
 =?utf-8?B?ZlE2eUNoK2hQcFc2dUpvNWswTGkvVmx2RG5MM0h3bU5XR3AwWU9CVlBpTzY0?=
 =?utf-8?B?K3hYbzNKL3BsYk56Z2FqL3Q4Ukx1REZmaUNtMTRYRlQwR05XWlpoL3ZyWGk2?=
 =?utf-8?B?NVh1TTJrYjRFdHBiM3FnT2hDUG5CVUFlemRvY1RTNmlYL3E2M1JwdXJDTGU5?=
 =?utf-8?B?UGU2N1hkeU41YTA3TEZVSzZObXhCK0x4Y1JhTk8yd0MvemhmaThnRHBkTWEr?=
 =?utf-8?B?eGdOcE9ZUTRkd2RWbEgzSEJUTFM3VzJMd1ZUNkxIVFpMa2lYYWI4V1BZM3BJ?=
 =?utf-8?B?ZTBiT1hCSDZIdlArTHo3MmMvUCtEN3hqRCtqWENDZjI4WkpjVDVra2Q3TGZ2?=
 =?utf-8?B?ZmdYZ0gza2oxNlZTSGRTUXF4L1FvVnB0MUdaM3RvZlhYRWdlN3FMVzV6dGEw?=
 =?utf-8?B?eEY0TE5oNnVMNUhuSEhCSWV5MHcrRWt0SzNLSVNEOCtZZ1NJelMxcUtsU0Nt?=
 =?utf-8?B?STV6amRaTlF5ODJyZnVlSVR5UUpEalBZa3pNV2lhS0p2cXBDSGEweWxDM256?=
 =?utf-8?B?Zk9JWnMxY3lQR2N3UHk1WUQ2eUFPNCtyU3NZd2RlbUpiZFQyVjh3RkZ6ZWJU?=
 =?utf-8?B?Q0xhZEpPSnhkaW1PcGRVdnRUMXRXckhCWHU3YkdCMk93RDN2RG9SSm0zM3NU?=
 =?utf-8?B?b0x5MGUrT29pc0p1ejhrd21NOWF4elVSeEIxQmNNaERRb0xENWN1dnYyVkNr?=
 =?utf-8?B?VVhyaXEwKzY2c0VRMVFYZTE1cGRIbm9PUUVtNHpXSXJKaXNwbHgxak9YTzVs?=
 =?utf-8?B?WU1tMVBVSW51VWtDSUp4Mno4azZKYVUxVTA0OHdNazZZaWs3NW5kanUwejZJ?=
 =?utf-8?B?RjhwQlVpdDlDbFFXd1dyTlpnSmVyMmQ5TXNMSVcxUS83RXVWYkRkdUYwQzFI?=
 =?utf-8?B?WXl0WGZOVUtwM3VFSHlHTnVlSTdEU1FRNDJ0bU1FYkZDVndFYjhZajZKdUhK?=
 =?utf-8?B?VWNIYTlRMWh2Si91QjZQZWhXSm16aVhWWG12RDUxR3dOK2lCQ2ZxOHd6ZGtp?=
 =?utf-8?B?OGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 826434f7-46a1-446e-9aea-08db8c50a972
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Jul 2023 14:17:07.3815
 (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: fnFRYEXa3/C2IVCpmBC0Zsqrkuf/NA858+Ce5VGmWKBUreRW2ZKGoC0EeqiRbMBOpvfx3AXfE9JgG5cTGz23eg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6200
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,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


On 7/24/2023 5:13 PM, Chao Gao wrote:
> On Thu, Jul 20, 2023 at 11:03:43PM -0400, Yang Weijiang wrote:
>> Save GUEST_SSP to SMRAM on SMI and reload it on RSM.
>> KVM emulates architectural behavior when guest enters/leaves SMM
>> mode, i.e., save registers to SMRAM at the entry of SMM and reload
>> them at the exit of SMM. Per SDM, GUEST_SSP is defined as one of
> To me, GUEST_SSP is confusing here. From QEMU's perspective, it reads/writes
> the SSP register. People may confuse it with the GUEST_SSP in nVMCS field.
> I prefer to rename it to MSR_KVM_SSP.

Hmm, looks a bit, I'll change it, thanks!

>
>> the fields in SMRAM for 64-bit mode, so handle the state accordingly.
>>
>> Check HF_SMM_MASK to determine whether kvm_cet_is_msr_accessible()
>> is called in SMM mode so that kvm_{set,get}_msr() works in SMM mode.
>>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>> arch/x86/kvm/smm.c | 17 +++++++++++++++++
>> arch/x86/kvm/smm.h |  2 +-
>> arch/x86/kvm/x86.c | 12 +++++++++++-
>> 3 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c
>> index b42111a24cc2..a4e19d72224f 100644
>> --- a/arch/x86/kvm/smm.c
>> +++ b/arch/x86/kvm/smm.c
>> @@ -309,6 +309,15 @@ void enter_smm(struct kvm_vcpu *vcpu)
>>
>> 	kvm_smm_changed(vcpu, true);
>>
>> +#ifdef CONFIG_X86_64
>> +	if (guest_can_use(vcpu, X86_FEATURE_SHSTK)) {
>> +		u64 data;
>> +
>> +		if (!kvm_get_msr(vcpu, MSR_KVM_GUEST_SSP, &data))
>> +			smram.smram64.ssp = data;
> I don't think it is correct to continue if kvm fails to read the MSR.
>
> how about:
> 		if (kvm_get_msr(vcpu, MSR_KVM_GUEST_SSP, &smram.smram64.ssp))
> 			goto error;

Agree,  will change it.

>> +	}
>> +#endif
>> +
>> 	if (kvm_vcpu_write_guest(vcpu, vcpu->arch.smbase + 0xfe00, &smram, sizeof(smram)))
>> 		goto error;
>>
>> @@ -586,6 +595,14 @@ int emulator_leave_smm(struct x86_emulate_ctxt *ctxt)
>> 	if ((vcpu->arch.hflags & HF_SMM_INSIDE_NMI_MASK) == 0)
>> 		static_call(kvm_x86_set_nmi_mask)(vcpu, false);
>>
>> +#ifdef CONFIG_X86_64
>> +	if (guest_can_use(vcpu, X86_FEATURE_SHSTK)) {
>> +		u64 data = smram.smram64.ssp;
>> +
>> +		if (is_noncanonical_address(data, vcpu) && IS_ALIGNED(data, 4))
> shouldn't the checks be already done inside kvm_set_msr()?

Nice catch, will remove them.


>
>> +			kvm_set_msr(vcpu, MSR_KVM_GUEST_SSP, data);
> please handle the failure case. Probably just return X86EMUL_UNHANDLEABLE like other
> failure paths in this function.

OK, VM should be shutdown if this field cannot be written successfully.

>
>> +	}
>> +#endif
>> 	kvm_smm_changed(vcpu, false);
>>
>> 	/*
>> diff --git a/arch/x86/kvm/smm.h b/arch/x86/kvm/smm.h
>> index a1cf2ac5bd78..b3efef7cb1dc 100644
>> --- a/arch/x86/kvm/smm.h
>> +++ b/arch/x86/kvm/smm.h
>> @@ -116,7 +116,7 @@ struct kvm_smram_state_64 {
>> 	u32 smbase;
>> 	u32 reserved4[5];
>>
>> -	/* ssp and svm_* fields below are not implemented by KVM */
>> +	/* svm_* fields below are not implemented by KVM */
> move this comment one line downward

OK

>
>> 	u64 ssp;
>> 	u64 svm_guest_pat;
>> 	u64 svm_host_efer;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index f7558f0f6fc0..70d7c80889d6 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -3653,8 +3653,18 @@ static bool kvm_cet_is_msr_accessible(struct kvm_vcpu *vcpu,
>> 		if (!kvm_cpu_cap_has(X86_FEATURE_SHSTK))
>> 			return false;
>>
>> -		if (msr->index == MSR_KVM_GUEST_SSP)
>> +		/*
>> +		 * This MSR is synthesized mainly for userspace access during
>> +		 * Live Migration, it also can be accessed in SMM mode by VMM.
>> +		 * Guest is not allowed to access this MSR.
>> +		 */
>> +		if (msr->index == MSR_KVM_GUEST_SSP) {
>> +			if (IS_ENABLED(CONFIG_X86_64) &&
>> +			    !!(vcpu->arch.hflags & HF_SMM_MASK))
> use is_smm() instead.

OK.

>
>> +				return true;
>> +
>> 			return msr->host_initiated;
>> +		}
>>
>> 		return msr->host_initiated ||
>> 			guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
>> -- 
>> 2.27.0
>>