Re: [PATCH 13/25] KVM: TDX: create/destroy VM structure

[Tony Lindgren]

On Mon, Aug 19, 2024 at 06:09:06PM +0300, Nikolay Borisov wrote:
> On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote:
> > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > +static u64 ____tdx_reclaim_page(hpa_t pa, u64 *rcx, u64 *rdx, u64 *r8)
> 
> Just inline this into its sole caller. Yes each specific function is rather
> small but if you have to go through several levels of indirection then
> there's no point in splitting it...

Makes sense, will do a patch for this.

> > +static inline u8 tdx_sysinfo_nr_tdcs_pages(void)
> > +{
> > +	return tdx_sysinfo->td_ctrl.tdcs_base_size / PAGE_SIZE;
> > +}
> 
> Just add a nr_tdcs_pages to struct tdx_sysinfo_td_ctrl and claculate this
> value in get_tdx_td_ctrl() rather than having this long-named non-sense.
> This value can't be calculated at compiletime anyway.

The struct tdx_sysinfo_td_ctrl is defined in the TDX module API json files.
Probably best to add nr_tdcs_pages to struct kvm_tdx.

> > +void tdx_vm_free(struct kvm *kvm)
> > +{
> > +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > +	u64 err;
> > +	int i;
> > +
> > +	/*
> > +	 * tdx_mmu_release_hkid() failed to reclaim HKID.  Something went wrong
> > +	 * heavily with TDX module.  Give up freeing TD pages.  As the function
> > +	 * already warned, don't warn it again.
> > +	 */
> > +	if (is_hkid_assigned(kvm_tdx))
> > +		return;
> > +
> > +	if (kvm_tdx->tdcs_pa) {
> > +		for (i = 0; i < tdx_sysinfo_nr_tdcs_pages(); i++) {
> > +			if (!kvm_tdx->tdcs_pa[i])
> > +				continue;
> > +
> > +			tdx_reclaim_control_page(kvm_tdx->tdcs_pa[i]);
> > +		}
> > +		kfree(kvm_tdx->tdcs_pa);
> > +		kvm_tdx->tdcs_pa = NULL;
> > +	}
> > +
> > +	if (!kvm_tdx->tdr_pa)
> > +		return;
> 
> Use is_td_created() helper. Also isn't this check redundant since you've
> already executed is_hkid_assigned() and if the VM is not properly created
> i.e __tdx_td_init() has failed for whatever reason then the is_hkid_assigned
> check will also fail?

On the error path __tdx_td_init() calls tdx_mmu_release_hkid().

I'll do a patch to change to use is_td_created(). The error path is a bit
hard to follow so likely needs some more patches :)

Regards,

Tony