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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZtWD5G0ZUp5Ui1Zp@tlindgre-MOBL1>
Date: Mon, 2 Sep 2024 12:22:44 +0300
From: Tony Lindgren <tony.lindgren@...ux.intel.com>
To: Nikolay Borisov <nik.borisov@...e.com>
Cc: Rick Edgecombe <rick.p.edgecombe@...el.com>, seanjc@...gle.com,
	pbonzini@...hat.com, kvm@...r.kernel.org, kai.huang@...el.com,
	isaku.yamahata@...il.com, xiaoyao.li@...el.com,
	linux-kernel@...r.kernel.org,
	Isaku Yamahata <isaku.yamahata@...el.com>,
	Sean Christopherson <sean.j.christopherson@...el.com>,
	Yan Zhao <yan.y.zhao@...el.com>
Subject: Re: [PATCH 13/25] KVM: TDX: create/destroy VM structure

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@...el.com>
> > +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