Re: [PATCH 13/21] KVM: TDX: Handle TLB tracking for TDX

[Yan Zhao <yan.y.zhao@intel.com>]

On Tue, Sep 10, 2024 at 10:16:27AM +0200, Paolo Bonzini wrote:
> On 9/4/24 05:07, Rick Edgecombe wrote:
> > +static void vt_flush_tlb_all(struct kvm_vcpu *vcpu)
> > +{
> > +	/*
> > +	 * TDX calls tdx_track() in tdx_sept_remove_private_spte() to ensure
> > +	 * private EPT will be flushed on the next TD enter.
> > +	 * No need to call tdx_track() here again even when this callback is as
> > +	 * a result of zapping private EPT.
> > +	 * Just invoke invept() directly here to work for both shared EPT and
> > +	 * private EPT.
> > +	 */
> > +	if (is_td_vcpu(vcpu)) {
> > +		ept_sync_global();
> > +		return;
> > +	}
> > +
> > +	vmx_flush_tlb_all(vcpu);
> > +}
> > +
> > +static void vt_flush_tlb_current(struct kvm_vcpu *vcpu)
> > +{
> > +	if (is_td_vcpu(vcpu)) {
> > +		tdx_flush_tlb_current(vcpu);
> > +		return;
> > +	}
> > +
> > +	vmx_flush_tlb_current(vcpu);
> > +}
> > +
> 
> I'd do it slightly different:
> 
> static void vt_flush_tlb_all(struct kvm_vcpu *vcpu)
> {
> 	if (is_td_vcpu(vcpu)) {
> 		tdx_flush_tlb_all(vcpu);
> 		return;
> 	}
> 
> 	vmx_flush_tlb_all(vcpu);
> }
Thanks!
This is better.

> 
> static void vt_flush_tlb_current(struct kvm_vcpu *vcpu)
> {
> 	if (is_td_vcpu(vcpu)) {
> 		/*
> 		 * flush_tlb_current() is used only the first time for
> 		 * the vcpu runs, since TDX supports neither shadow
> 		 * nested paging nor SMM.  Keep this function simple.
> 		 */
> 		tdx_flush_tlb_all(vcpu);
Could we still keep tdx_flush_tlb_current()?
Though both tdx_flush_tlb_all() and tdx_flush_tlb_current() simply invoke
ept_sync_global(), their purposes are different:

- The ept_sync_global() in tdx_flush_tlb_current() is intended to avoid
  retrieving private EPTP required for the single-context invalidation for
  shared EPT;
- while the ept_sync_global() in tdx_flush_tlb_all() is right for shared EPT.

Adding a tdx_flush_tlb_current() can help document the differences in tdx.c.

like this:

void tdx_flush_tlb_current(struct kvm_vcpu *vcpu)
{
        /*
         * flush_tlb_current() is invoked when the first time for the vcpu to
         * run or when root of shared EPT is invalidated.
         * KVM only needs to flush the TLB for shared EPT because the TDX module
         * handles TLB invalidation for private EPT in tdh_vp_enter();
         *
         * A single context invalidation for shared EPT can be performed here.
         * However, this single context invalidation requires the private EPTP
         * rather than the shared EPTP to flush TLB for shared EPT, as shared
         * EPT uses private EPTP as its ASID for TLB invalidation.
         *
         * To avoid reading back private EPTP, perform a global invalidation for
         * shared EPT instead to keep this function simple.
         */
        ept_sync_global();
}

void tdx_flush_tlb_all(struct kvm_vcpu *vcpu)
{
        /*
         * TDX has called tdx_track() in tdx_sept_remove_private_spte() to
         * ensure that private EPT will be flushed on the next TD enter. No need
         * to call tdx_track() here again even when this callback is a result of
         * zapping private EPT.
         *
         * Due to the lack of the context to determine which EPT has been
         * affected by zapping, invoke invept() directly here for both shared
         * EPT and private EPT for simplicity, though it's not necessary for
         * private EPT.          *
         */
        ept_sync_global();
}



> 		return;
> 	}
> 
> 	vmx_flush_tlb_current(vcpu);
> }
> 

> and put the implementation details close to tdx_track:
> void tdx_flush_tlb_all(struct kvm_vcpu *vcpu)
> {
> 	/*
> 	 * TDX calls tdx_track() in tdx_sept_remove_private_spte() to
> 	 * ensure private EPT will be flushed on the next TD enter.
> 	 * No need to call tdx_track() here again, even when this
> 	 * callback is a result of zapping private EPT.  Just
> 	 * invoke invept() directly here, which works for both shared
> 	 * EPT and private EPT.
> 	 */
> 	ept_sync_global();
> }
Got it!