Received-SPF: pass (google.com: domain of linux-kernel+bounces-143036-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <0c3efffa-8dd5-4231-8e90-e0241f058a20@intel.com>
Date: Fri, 12 Apr 2024 09:15:29 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 087/130] KVM: TDX: handle vcpu migration over logical
 processor
To: <isaku.yamahata@intel.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
CC: <isaku.yamahata@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>,
	<erdemaktas@google.com>, Sean Christopherson <seanjc@google.com>, Sagi Shahar
	<sagis@google.com>, Kai Huang <kai.huang@intel.com>, <chen.bo@intel.com>,
	<hang.yuan@intel.com>, <tina.zhang@intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <b9fe57ceeaabe650f0aecb21db56ef2b1456dcfe.1708933498.git.isaku.yamahata@intel.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <b9fe57ceeaabe650f0aecb21db56ef2b1456dcfe.1708933498.git.isaku.yamahata@intel.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?dmdNd0ptdCtjVThCQm9qMlQ1UVo0WUNVT3BlRXlwTXJJV1cvOE12S2NwdXVD?=
 =?utf-8?B?TDA4QUpGMklrWWZnV1VyNnJxZkIrWExJVnBYU2FKN2RYNEtZK0JQR0FxbDJC?=
 =?utf-8?B?clliazV1ZDIxVTZDUVpTaVN5ejF3YnEzc2x2N09xUm5mdVFtK2tVbkJLMnlW?=
 =?utf-8?B?UGM3dmt6b3lzMXpWODJkWGVKazFUMnpNRHRYNzB2clc4dHJwR1BkdFpJQjZi?=
 =?utf-8?B?Umk5TmZuY0NpOHV2ZnMrdXEvOGhKNHZ6ZjNsYTNqTURxTTRXdjFPVlRuUGNF?=
 =?utf-8?B?ZzNJbTlMOXFESzdVRElJc09selEyVzVTdG1sVmFjcmd1eTU2YVladlFBR0Nz?=
 =?utf-8?B?U05RWVRHaHJ6aTBma1dEdmdVWFo5VzNONzFubUJPZzFEZGkzUEJJcWEvMmpn?=
 =?utf-8?B?aDBQMTBDeUFmVE8vMnM1OFFBcENKbTRHTWNZUE9ocFl6RHMxZ0F4djV1dGpW?=
 =?utf-8?B?WXRxL0c1aHFiTkZKSjhnQVFnVVU1c2lwWWlYYUtTUzRoTVBsOC9QWU54T2Zt?=
 =?utf-8?B?aCtIZms5M1h0V1Q2djJZa1FhcmFJRTFGR0tac2VZOWFFTzJydE1aaEdBaHZh?=
 =?utf-8?B?RExhTWszQUFwYTQzT0dFVlRkUWJZU3d0d0ptdFROMG5aVDZIekV3cVhsaHlY?=
 =?utf-8?B?T2FMZXVSejB0a2Zlb29IclJLYWg4QzJTeElOVmttamRNQ09kd1FXSloxZkk3?=
 =?utf-8?B?RFBINlJzbmFoamN0TzNEeUFpNFFJNTF4VkRQZWxBazBHVFI0RDEzQkUvUnFG?=
 =?utf-8?B?SHpFS0FhcXpyU3M2U2NyZ2l2VWN4bStRMzVyYmNrbm1Ta2JUWWtMQ1p3akdQ?=
 =?utf-8?B?TnowMzhhVWdHQk1aS1Z4aHFtQXlIRVN6c3RVbk51bHVrV0RhVTRRU3dxMm5s?=
 =?utf-8?B?SWhmSVFsUWtUR3ZaUEU5QXpBT3Bmc1UvUUdXTG92K2QvVjBFOEtIQkh4QWFY?=
 =?utf-8?B?SVlIQ3MrcWJkRlM2K0NWODQ3a0N0MFJJZm9WcW5KQnZ2TnNEalF1aitiZFAv?=
 =?utf-8?B?Tml3bGtWZStWVWFNR2QrOEVhcCtyc2oyeHlWRXp1bDRLLzlHZm5ReGdTb2V3?=
 =?utf-8?B?aWQrSHFkWm8wcDZnZjV3WGRicnJVWWo4ZmJMZzRrSGFLaXZMZTN1cjdodCtJ?=
 =?utf-8?B?OTRHNGo1NlcvVzFHZmRQY3pVUlZqcTMvcS85SHZQQmVQT0RHVm1YUXQySVlT?=
 =?utf-8?B?Nm52Z1JDTytDUXU3NFhaekpmZFY0dFlzNURsU0hTUW1PQWh0WkFmUnBMbVhk?=
 =?utf-8?B?a0d0SkpqeUJCc2RjSUJxbkVTQkhzMGovSWtVVDhVMkU2bTZtLzZGQmhUYXlW?=
 =?utf-8?B?VUFwdW9kVVRXWnhxSWQwVmtncmFtT0p4T2o0MlJtN0VrQnN6T0pVM0xkRXlQ?=
 =?utf-8?B?MllBcWh6RThYbzFrTXJWT0FkT3JHSVkwNUdqUU83SjlpVzgrMWw2dlBVeGJR?=
 =?utf-8?B?MWdoQU1CalRteDFLOWZEc0d6ODhna3ZlbXYwdmg4aSthMTF0YTNWOWw2bVJv?=
 =?utf-8?B?QmJ6M1hlZ3IxK0JmMkc5OW5MdG1PYWhRMGhiWEJiY1pZWlljLzI0cFRNek5h?=
 =?utf-8?B?aUcvZm5TZlptYUVGNWk5dk55VHFVZTVRMlMzUHhDdUhRdFBMcFlFcHl6bUx4?=
 =?utf-8?B?RGUrWUwwMldWNWFQYnZYTlhDOVowN2c5SGc1QnB5Z01NT3VVd21SY0hJRHN6?=
 =?utf-8?B?NHllRmxwdXArTmFVQUxuRXRVb3NmL0taOHJtZWhiT0syMnFYZE9jc2FiZWUr?=
 =?utf-8?B?NXBPbjhiUisxaG9iNzd5d1Y1VzhUR3BjbUxPbmRmS1o1TEdMMVdSa0J0OHh2?=
 =?utf-8?B?b1k3VjloL3o3emZWejJkQURDbkNLbmk1ZllSQjM5bGJVZEJlWUtsekJwWmE5?=
 =?utf-8?B?Z2ltNE11blU5MnlCZ2piM3JMd2ZMTWtMS3VNekVzeUtrY2lSYUZ3c3FnR3hj?=
 =?utf-8?B?bDViUDNIVnJKdDlnOTJWZlAyMzk1cGZiZmR5WDZYY0x2VCtWVWViVk95U29w?=
 =?utf-8?B?NnU3V1E0L2VhS1E1cmVRRzN1QS9OZzMxV3pOcmwyUml5d0V0OFIyMXhGSFJ3?=
 =?utf-8?B?aGtmaHRoRVpsK0NacWFFaVZ4SzdWZWFZbENFR0E0S281elpUYmR2ampUNno1?=
 =?utf-8?B?VUtTOElUc2xXaFJUd2E3TWZDT2pMT1ZPOTJYVEZkSlVidHB1MWpiWmMvR1k2?=
 =?utf-8?B?b0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 615f4d59-f883-47d5-e6d7-08dc5b0bc6a2
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Apr 2024 16:15:31.6893
 (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: EDa6nC7ZzupGMT7o99Gf1UGm6tZEpdCaG6fauhEwrzLoxpv1lF3rzyxI/fcGLLN3f74RbIPTZUgdYimgjftoLDkpLgHZGes5+Cd2SiVzLos=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4708
X-OriginatorOrg: intel.com

Hi Isaku,

On 2/26/2024 12:26 AM, isaku.yamahata@intel.com wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>

..

> @@ -218,6 +257,87 @@ static void tdx_reclaim_control_page(unsigned long td_page_pa)
>  	free_page((unsigned long)__va(td_page_pa));
>  }
>  
> +struct tdx_flush_vp_arg {
> +	struct kvm_vcpu *vcpu;
> +	u64 err;
> +};
> +
> +static void tdx_flush_vp(void *arg_)
> +{
> +	struct tdx_flush_vp_arg *arg = arg_;
> +	struct kvm_vcpu *vcpu = arg->vcpu;
> +	u64 err;
> +
> +	arg->err = 0;
> +	lockdep_assert_irqs_disabled();
> +
> +	/* Task migration can race with CPU offlining. */
> +	if (unlikely(vcpu->cpu != raw_smp_processor_id()))
> +		return;
> +
> +	/*
> +	 * No need to do TDH_VP_FLUSH if the vCPU hasn't been initialized.  The
> +	 * list tracking still needs to be updated so that it's correct if/when
> +	 * the vCPU does get initialized.
> +	 */
> +	if (is_td_vcpu_created(to_tdx(vcpu))) {
> +		/*
> +		 * No need to retry.  TDX Resources needed for TDH.VP.FLUSH are,
> +		 * TDVPR as exclusive, TDR as shared, and TDCS as shared.  This
> +		 * vp flush function is called when destructing vcpu/TD or vcpu
> +		 * migration.  No other thread uses TDVPR in those cases.
> +		 */

(I have comment later that refer back to this comment about needing retry.)

..

> @@ -233,26 +353,31 @@ static void tdx_do_tdh_phymem_cache_wb(void *unused)
>  		pr_tdx_error(TDH_PHYMEM_CACHE_WB, err, NULL);
>  }
>  
> -void tdx_mmu_release_hkid(struct kvm *kvm)
> +static int __tdx_mmu_release_hkid(struct kvm *kvm)
>  {
>  	bool packages_allocated, targets_allocated;
>  	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
>  	cpumask_var_t packages, targets;
> +	struct kvm_vcpu *vcpu;
> +	unsigned long j;
> +	int i, ret = 0;
>  	u64 err;
> -	int i;
>  
>  	if (!is_hkid_assigned(kvm_tdx))
> -		return;
> +		return 0;
>  
>  	if (!is_td_created(kvm_tdx)) {
>  		tdx_hkid_free(kvm_tdx);
> -		return;
> +		return 0;
>  	}
>  
>  	packages_allocated = zalloc_cpumask_var(&packages, GFP_KERNEL);
>  	targets_allocated = zalloc_cpumask_var(&targets, GFP_KERNEL);
>  	cpus_read_lock();
>  
> +	kvm_for_each_vcpu(j, vcpu, kvm)
> +		tdx_flush_vp_on_cpu(vcpu);
> +
>  	/*
>  	 * We can destroy multiple guest TDs simultaneously.  Prevent
>  	 * tdh_phymem_cache_wb from returning TDX_BUSY by serialization.
> @@ -270,6 +395,19 @@ void tdx_mmu_release_hkid(struct kvm *kvm)
>  	 */
>  	write_lock(&kvm->mmu_lock);
>  
> +	err = tdh_mng_vpflushdone(kvm_tdx->tdr_pa);
> +	if (err == TDX_FLUSHVP_NOT_DONE) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +	if (WARN_ON_ONCE(err)) {
> +		pr_tdx_error(TDH_MNG_VPFLUSHDONE, err, NULL);
> +		pr_err("tdh_mng_vpflushdone() failed. HKID %d is leaked.\n",
> +		       kvm_tdx->hkid);
> +		ret = -EIO;
> +		goto out;
> +	}
> +
>  	for_each_online_cpu(i) {
>  		if (packages_allocated &&
>  		    cpumask_test_and_set_cpu(topology_physical_package_id(i),
> @@ -291,14 +429,24 @@ void tdx_mmu_release_hkid(struct kvm *kvm)
>  		pr_tdx_error(TDH_MNG_KEY_FREEID, err, NULL);
>  		pr_err("tdh_mng_key_freeid() failed. HKID %d is leaked.\n",
>  		       kvm_tdx->hkid);
> +		ret = -EIO;
>  	} else
>  		tdx_hkid_free(kvm_tdx);
>  
> +out:
>  	write_unlock(&kvm->mmu_lock);
>  	mutex_unlock(&tdx_lock);
>  	cpus_read_unlock();
>  	free_cpumask_var(targets);
>  	free_cpumask_var(packages);
> +
> +	return ret;
> +}
> +
> +void tdx_mmu_release_hkid(struct kvm *kvm)
> +{
> +	while (__tdx_mmu_release_hkid(kvm) == -EBUSY)
> +		;
>  }

As I understand, __tdx_mmu_release_hkid() returns -EBUSY
after TDH.VP.FLUSH has been sent for every vCPU followed by
TDH.MNG.VPFLUSHDONE, which returns TDX_FLUSHVP_NOT_DONE.

Considering earlier comment that a retry of TDH.VP.FLUSH is not
needed, why is this while() loop here that sends the
TDH.VP.FLUSH again to all vCPUs instead of just a loop within
__tdx_mmu_release_hkid() to _just_ resend TDH.MNG.VPFLUSHDONE?

Could it be possible for a vCPU to appear during this time, thus
be missed in one TDH.VP.FLUSH cycle, to require a new cycle of
TDH.VP.FLUSH?

I note that TDX_FLUSHVP_NOT_DONE is distinct from TDX_OPERAND_BUSY
that can also be returned from TDH.MNG.VPFLUSHDONE and
wonder if a retry may be needed in that case also/instead? It looks like
TDH.MNG.VPFLUSHDONE needs exclusive access to all operands and I
do not know enough yet if this is the case here.

Reinette