Re: [PATCH v15 13/20] KVM: SEV: Implement gmem hook for initializing private pages

[Huang, Kai]

On Wed, 2024-05-01 at 03:52 -0500, Michael Roth wrote:
> This will handle the RMP table updates needed to put a page into a
> private state before mapping it into an SEV-SNP guest.
> 
> 

[...]

> +int sev_gmem_prepare(struct kvm *kvm, kvm_pfn_t pfn, gfn_t gfn, int max_order)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	kvm_pfn_t pfn_aligned;
> +	gfn_t gfn_aligned;
> +	int level, rc;
> +	bool assigned;
> +
> +	if (!sev_snp_guest(kvm))
> +		return 0;
> +
> +	rc = snp_lookup_rmpentry(pfn, &assigned, &level);
> +	if (rc) {
> +		pr_err_ratelimited("SEV: Failed to look up RMP entry: GFN %llx PFN %llx error %d\n",
> +				   gfn, pfn, rc);
> +		return -ENOENT;
> +	}
> +
> +	if (assigned) {
> +		pr_debug("%s: already assigned: gfn %llx pfn %llx max_order %d level %d\n",
> +			 __func__, gfn, pfn, max_order, level);
> +		return 0;
> +	}
> +
> +	if (is_large_rmp_possible(kvm, pfn, max_order)) {
> +		level = PG_LEVEL_2M;
> +		pfn_aligned = ALIGN_DOWN(pfn, PTRS_PER_PMD);
> +		gfn_aligned = ALIGN_DOWN(gfn, PTRS_PER_PMD);
> +	} else {
> +		level = PG_LEVEL_4K;
> +		pfn_aligned = pfn;
> +		gfn_aligned = gfn;
> +	}
> +
> +	rc = rmp_make_private(pfn_aligned, gfn_to_gpa(gfn_aligned), level, sev->asid, false);
> +	if (rc) {
> +		pr_err_ratelimited("SEV: Failed to update RMP entry: GFN %llx PFN %llx level %d error %d\n",
> +				   gfn, pfn, level, rc);
> +		return -EINVAL;
> +	}
> +
> +	pr_debug("%s: updated: gfn %llx pfn %llx pfn_aligned %llx max_order %d level %d\n",
> +		 __func__, gfn, pfn, pfn_aligned, max_order, level);
> +
> +	return 0;
> +}
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b70556608e8d..60783e9f2ae8 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -5085,6 +5085,8 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
>  	.vcpu_deliver_sipi_vector = svm_vcpu_deliver_sipi_vector,
>  	.vcpu_get_apicv_inhibit_reasons = avic_vcpu_get_apicv_inhibit_reasons,
>  	.alloc_apic_backing_page = svm_alloc_apic_backing_page,
> +
> +	.gmem_prepare = sev_gmem_prepare,
>  };
>  
> 

+Rick, Isaku,

I am wondering whether this can be done in the KVM page fault handler?

The reason that I am asking is KVM will introduce several new
kvm_x86_ops::xx_private_spte() ops for TDX to handle setting up the
private mapping, and I am wondering whether SNP can just reuse some of
them so we can avoid having this .gmem_prepare():

        /* Add a page as page table page into private page table */
        int (*link_private_spt)(struct kvm *kvm, gfn_t gfn, 
			enum pg_level level, void *private_spt);
        /*
         * Free a page table page of private page table.
	 * ...
         */
        int (*free_private_spt)(struct kvm *kvm, gfn_t gfn, 
			enum pg_level level, void *private_spt);

        /* Add a guest private page into private page table */
        int (*set_private_spte)(struct kvm *kvm, gfn_t gfn, 
			enum pg_level level, kvm_pfn_t pfn);

        /* Remove a guest private page from private page table*/
        int (*remove_private_spte)(struct kvm *kvm, gfn_t gfn, 
			enum pg_level level, kvm_pfn_t pfn);
        /*
         * Keep a guest private page mapped in private page table, 
	 * but clear its present bit
         */
        int (*zap_private_spte)(struct kvm *kvm, gfn_t gfn,
			enum pg_level level);

The idea behind these is in the fault handler:

	bool use_private_pt = fault->is_private && 
			kvm_use_private_pt(kvm);

	root_pt = use_private_pt ? mmu->private_root_hpa : mmu->root_hpa;

	tdp_mmu_for_each_pte(&iter, root_pt, gfn, gfn+1, ..) {

		if (use_private_pt)
			kvm_x86_ops->xx_private_spte();
		else
			// normal TDP MMU ops
	}

Which means: if the fault is for private GPA, _AND_ when the VM has a
separate private table, use the specific xx_private_spte() ops to handle
private mapping.

But I am thinking we can use those hooks for SNP too, because
"conceptually", SNP also has concept of "private GPA" and must at least
issue some command to update the RMP table when private mapping is
setup/torn down.

So if we change the above logic to use fault->is_private, but not
'use_private_pt' to decide whether to invoke the
kvm_x86_ops::xx_private_spte(), then we can also implement SNP commands in
those callbacks IIUC:

	if (fault->is_private && kvm_x86_ops::xx_private_spte())
		kvm_x86_ops::xx_private_spte();
	else
		// normal TDP MMU operation

For SNP, these callbacks will operate on normal page table using the
normal TDP MMU code, but can do additional things like issuing commands as
shown in this patch.

My understanding is SNP doesn't need specific handling for middle level
page table, but should be able to utilize the ops when setting up /
tearing down the leaf SPTE?