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!
