Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

[Isaku Yamahata]

On Wed, Aug 14, 2024 at 09:20:06AM +0800,
Yuan Yao <yuan.yao@xxxxxxxxxxxxxxx> wrote:

> > > > +		ret = -EIO;
> > > > +		pr_tdx_error(TDH_VP_CREATE, err);
> > > > +		goto free_tdvpx;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < tdx_sysinfo_nr_tdcx_pages(); i++) {
> > > > +		va = __get_free_page(GFP_KERNEL_ACCOUNT);
> > > > +		if (!va) {
> > > > +			ret = -ENOMEM;
> > > > +			goto free_tdvpx;
> > >
> > > It's possible that some pages already added into TD by
> > > tdh_vp_addcx() below and they won't be handled by
> > > tdx_vcpu_free() if goto free_tdvpx here;
> >
> > Due to TDX TD state check, we can't free partially assigned TDCS pages.
> > TDX module seems to assume that TDH.VP.ADDCX() won't fail in the middle.
> 
> The already partially added TDCX pages are initialized by
> MOVDIR64 with the TD's private HKID in TDX module, the above
> 'goto free_tdvpx' frees them back to kernel directly w/o
> take back the ownership with shared HKID. This violates the
> rule that a page's ownership should be taken back with shared
> HKID before release to kernel if they were initialized by any
> private HKID before.
> 
> How about do tdh_vp_addcx() afer allocated all TDCX pages
> and give WARN_ON_ONCE() to the return value of
> tdh_vp_addcx() if the tdh_vp_addcx() won't fail except some
> BUG inside TDX module in our current usage ?

Yes, that makes sense.  Those error recovery paths need to be simplified.
-- 
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>