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
