Re: [PATCH v7 039/102] KVM: x86/mmu: Allow per-VM override of the TDP max page level

[Kai Huang]

On Mon, 2022-06-27 at 14:53 -0700, isaku.yamahata@xxxxxxxxx wrote:
> From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> 
> TODO: This is a transient workaround patch until the large page support for
> TDX is implemented.  Support large page for TDX and remove this patch.

I don't understand.  How does this patch have anything to do with what you are
talking about here?

If you want to remove this patch later, then why not just explain the reason to
remove when you actually have that patch?

> 
> At this point, large page for TDX isn't supported, and need to allow guest
> TD to work only with 4K pages.  On the other hand, conventional VMX VMs
> should continue to work with large page.  Allow per-VM override of the TDP
> max page level.

At which point/previous patch have you made/declared "large page for TDX isn't
supported"?

If you want to declare you don't want to support large page for TDX, IMHO just
declare it here, for instance:

"For simplicity, only support 4K page for TD guest."
  
> 
> In the existing x86 KVM MMU code, there is already max_level member in
> struct kvm_page_fault with KVM_MAX_HUGEPAGE_LEVEL initial value.  The KVM
> page fault handler denies page size larger than max_level.
> 
> Add per-VM member to indicate the allowed maximum page size with
> KVM_MAX_HUGEPAGE_LEVEL as default value and initialize max_level in struct
> kvm_page_fault with it.  For the guest TD, the set per-VM value for allows
> maximum page size to 4K page size.  Then only allowed page size is 4K.  It
> means large page is disabled.

To me it's overcomplicated.  You just need simple sentences for such simple
infrastructural patch.  For instance:

"TDX requires special handling to support large private page.  For simplicity,
only support 4K page for TD guest for now.  Add per-VM maximum page level
support to support different maximum page sizes for TD guest and conventional
VMX guest."

Just for your reference.

> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 +
>  arch/x86/kvm/mmu/mmu.c          | 1 +
>  arch/x86/kvm/mmu/mmu_internal.h | 2 +-
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 39215daa8576..f4d4ed41641b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1146,6 +1146,7 @@ struct kvm_arch {
>  	unsigned long n_requested_mmu_pages;
>  	unsigned long n_max_mmu_pages;
>  	unsigned int indirect_shadow_pages;
> +	int tdp_max_page_level;
>  	u8 mmu_valid_gen;
>  	struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
>  	struct list_head active_mmu_pages;
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index e0aa5ad3931d..80d7c7709af3 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5878,6 +5878,7 @@ int kvm_mmu_init_vm(struct kvm *kvm)
>  	node->track_write = kvm_mmu_pte_write;
>  	node->track_flush_slot = kvm_mmu_invalidate_zap_pages_in_memslot;
>  	kvm_page_track_register_notifier(kvm, node);
> +	kvm->arch.tdp_max_page_level = KVM_MAX_HUGEPAGE_LEVEL;
>  	kvm_mmu_set_mmio_spte_mask(kvm, shadow_default_mmio_mask,
>  				   shadow_default_mmio_mask,
>  				   ACC_WRITE_MASK | ACC_USER_MASK);
> diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
> index bd2a26897b97..44a04fad4bed 100644
> --- a/arch/x86/kvm/mmu/mmu_internal.h
> +++ b/arch/x86/kvm/mmu/mmu_internal.h
> @@ -244,7 +244,7 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  		.is_tdp = likely(vcpu->arch.mmu->page_fault == kvm_tdp_page_fault),
>  		.nx_huge_page_workaround_enabled = is_nx_huge_page_enabled(),
>  
> -		.max_level = KVM_MAX_HUGEPAGE_LEVEL,
> +		.max_level = vcpu->kvm->arch.tdp_max_page_level,
>  		.req_level = PG_LEVEL_4K,
>  		.goal_level = PG_LEVEL_4K,
>  	};