Re: [PATCH v4 11/17] KVM: TDX: Honor the guest's accept level contained in an EPT violation

[Yan Zhao]

On Sat, Oct 10, 2026 at 06:00:01AM +0800, Edgecombe, Rick P wrote:
> On Fri, 2026-10-09 at 17:33 +0800, Yan Zhao wrote:
> > On Thu, Oct 08, 2026 at 09:05:32AM +0800, Edgecombe, Rick P wrote:
> > > On Mon, 2026-09-28 at 17:11 +0800, Yan Zhao wrote:
> > > 
> > > > +
> > > > +	/*
> > > > +	 * No TLB flush is required, as the "BLOCK + TRACK + kick off vCPUs"
> > > > +	 * sequence required by the TDX-Module includes a TLB flush.
> > > > +	 */
> > > 
> > > I think the TLB flush is actually more obvious than the level + 1/2 bits below.
> > > It is worried about inhibiting 1GB mappings, why?
> > Hmm, including inhibiting 1GB mappings can make the logic more generic and more
> > correct in concept. That is, when the guest accept level is 2MB, 1GB mappings
> > should be inhibited conceptually. This help remove the code dependency on the
> > assumption that 1GB mappings are impossible with the initial implementation,
> > given that inhibiting 1GB mappings as well is simple.
> 
> I strongly think the code or comments should not prep for 1GB S-EPT pages or
> promotion. Warning if 1GB S-EPT pages is tried is good. But I don't see how
> silently handling this particular corner of huge pages helps anything.
Hmm. I didn't realize inhibiting 1GB S-EPT pages would be regarded as a prep for
1GB S-EPT pages enabling or promotion.

> If/when we get to 1GB mappings the code could have evolved at that point. So
> introducing code or comments now will add constraints on other work that is
> done. We don't want to think about it.
I'll drop the 1GB inhibition if it causes confusion for the reader.

> > > > +	hugepage_set_guest_inhibit(slot, gfn, level + 1);
> > > > +	if (level == PG_LEVEL_4K)
> > > > +		hugepage_set_guest_inhibit(slot, gfn, level + 2);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > If you have a strong opinion to drop inhibiting 1GB mappings for now, I can add
> > the following diff:
> > 
> > @@ -2183,7 +2172,13 @@ static int tdx_handle_guest_accept_ept_violation(struct kvm_vcpu *vcpu, gfn_t gf
> >                 return 0;
> > 
> >         level = tdx_get_ept_violation_level(vcpu);
> > -       if (level > PG_LEVEL_2M)
> > +       KVM_BUG_ON(level != PG_LEVEL_2M && level != PG_LEVEL_4K, kvm);
> > +
> > +       /*
> > +        * No need to inhibit 1GB mappings since they are unsupported with
> > +        * the initial huge page implementation.
> 
> This also implies 1GB is coming...
What about simply commenting as
"No need to inhibit 1GB mappings since they are unsupported." ?

> > +        */
> > +       if (level == PG_LEVEL_2M)
> >                 return 0;
> > 
> >         if (hugepage_test_guest_inhibit(slot, gfn, level + 1))
> > @@ -2203,8 +2198,6 @@ static int tdx_handle_guest_accept_ept_violation(struct kvm_vcpu *vcpu, gfn_t gf
> >          * sequence required by the TDX-Module includes a TLB flush.
> >          */
> >         hugepage_set_guest_inhibit(slot, gfn, level + 1);
> > -       if (level == PG_LEVEL_4K)
> > -               hugepage_set_guest_inhibit(slot, gfn, level + 2);
> > 
> >         return 0;
> >  }
> > 
> 
> There are a lot of possible things that are not handled if a 1GB mapping sneaks
> through.
> 
> Why not KVM_BUG_ON() if something tries to map 1GB in the appropriate
> tdx_sept_set_private_spte() sub functions. tdx_sept_split_leaf_spte() should be
> covered already, but not tdx_sept_map_leaf_spte() looks like.
Hmm, there's an implicit KVM_BUG_ON() inside tdx_sept_map_leaf_spte() about the
mapping level: both tdx_mem_page_aug() and tdx_mem_page_add() return error if
the mapping level is 1GB.