Re: [PATCH v19 038/130] KVM: TDX: create/destroy VM structure

[Huang, Kai]

On 28/03/2024 11:53 am, Isaku Yamahata wrote:
> On Tue, Mar 26, 2024 at 02:43:54PM +1300,
> "Huang, Kai" <kai.huang@xxxxxxxxx> wrote:
>
> > ... continue the previous review ...
> >
> > > +
> > > +static void tdx_reclaim_control_page(unsigned long td_page_pa)
> > > +{
> > > +	WARN_ON_ONCE(!td_page_pa);
> >
> >  From the name 'td_page_pa' we cannot tell whether it is a control page, but
> > this function is only intended for control page AFAICT, so perhaps a more
> > specific name.
> >
> > > +
> > > +	/*
> > > +	 * TDCX are being reclaimed.  TDX module maps TDCX with HKID
> >
> > "are" -> "is".
> >
> > Are you sure it is TDCX, but not TDCS?
> >
> > AFAICT TDCX is the control structure for 'vcpu', but here you are handling
> > the control structure for the VM.
>
> TDCS, TDVPR, and TDCX.  Will update the comment.

But TDCX, TDVPR are vcpu-scoped.  Do you want to mention them _here_?

Otherwise you will have to explain them.

[...]

> > > +
> > > +void 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;
> > > +	u64 err;
> > > +	int i;
> > > +
> > > +	if (!is_hkid_assigned(kvm_tdx))
> > > +		return;
> > > +
> > > +	if (!is_td_created(kvm_tdx)) {
> > > +		tdx_hkid_free(kvm_tdx);
> > > +		return;
> > > +	}
> >
> > I lost tracking what does "td_created()" mean.
> >
> > I guess it means: KeyID has been allocated to the TDX guest, but not yet
> > programmed/configured.
> >
> > Perhaps add a comment to remind the reviewer?
>
> As Chao suggested, will introduce state machine for vm and vcpu.
>
> https://lore.kernel.org/kvm/ZfvI8t7SlfIsxbmT@chao-email/

Could you elaborate what will the state machine look like?

I need to understand it.


[...]


> How about this?
>
> /*
>   * We need three SEAMCALLs, TDH.MNG.VPFLUSHDONE(), TDH.PHYMEM.CACHE.WB(), and
>   * TDH.MNG.KEY.FREEID() to free the HKID.
>   * Other threads can remove pages from TD.  When the HKID is assigned, we need
>   * to use TDH.MEM.SEPT.REMOVE() or TDH.MEM.PAGE.REMOVE().
>   * TDH.PHYMEM.PAGE.RECLAIM() is needed when the HKID is free.  Get lock to not
>   * present transient state of HKID.
>   */

Could you elaborate why it is still possible to have other thread 
removing pages from TD?

I am probably missing something, but the thing I don't understand is why 
this function is triggered by MMU release?  All the things done in this 
function don't seem to be related to MMU at all.

IIUC, by reaching here, you must already have done VPFLUSHDONE, which 
should be called when you free vcpu?  Freeing vcpus is done in 
kvm_arch_destroy_vm(), which is _after_ mmu_notifier->release(), in 
which this tdx_mmu_release_keyid() is called?

But here we are depending vcpus to be freed before tdx_mmu_release_hkid()?

> > > +	/*
> > > +	 * In the case of error in tdx_do_tdh_phymem_cache_wb(), the following
> > > +	 * tdh_mng_key_freeid() will fail.
> > > +	 */
> > > +	err = tdh_mng_key_freeid(kvm_tdx->tdr_pa);
> > > +	if (WARN_ON_ONCE(err)) {
> >
> > I see KVM_BUG_ON() is normally used for SEAMCALL error.  Why this uses
> > WARN_ON_ONCE() here?
>
> Because vm_free() hook is (one of) the final steps to free struct kvm.  No one
> else touches this kvm.  Because it doesn't harm to use KVM_BUG_ON() here,
> I'll change it for consistency.

I am fine with either.  You can use KVM_BUG_ON() for SEAMCALLs at 
runtime, but use WARN_ON_ONCE() for those involved during VM creation.

[...]

> > > +	err = tdh_phymem_page_wbinvd(set_hkid_to_hpa(kvm_tdx->tdr_pa,
> > > +						     tdx_global_keyid));
> > > +	if (WARN_ON_ONCE(err)) {
> >
> > Again, KVM_BUG_ON()?
> >
> > Should't matter, though.
>
> Ok, let's use KVM_BUG_ON() consistently.

Ditto.