Re: [PATCH v6 1/8] x86/sev: Prepare for using the RMPREAD instruction to access the RMP

[Borislav Petkov]

On Mon, Dec 02, 2024 at 02:50:46PM -0600, Tom Lendacky wrote:
> +static int __snp_lookup_rmpentry(u64 pfn, struct rmpentry *e, int *level)
> +{
> +	struct rmpentry e_large;
> +	int ret;
> +
> +	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))

Btw, just a side note: this is AMD-specific and x86 code so we probably should
use:

	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))

For another series.

> +		return -ENODEV;
> +
> +	ret = get_rmpentry(pfn, e);
> +	if (ret)
> +		return ret;
>  
>  	/*
>  	 * Find the authoritative RMP entry for a PFN. This can be either a 4K
>  	 * RMP entry or a special large RMP entry that is authoritative for a
>  	 * whole 2M area.
>  	 */
> -	large_entry = get_rmpentry(pfn & PFN_PMD_MASK);
> -	if (IS_ERR(large_entry))
> -		return large_entry;
> +	ret = get_rmpentry(pfn & PFN_PMD_MASK, &e_large);
> +	if (ret)
> +		return ret;
>  
> -	*level = RMP_TO_PG_LEVEL(large_entry->pagesize);
> +	*level = RMP_TO_PG_LEVEL(e_large.pagesize);
>  
> -	return entry;
> +	return 0;
>  }

...

>  static void dump_rmpentry(u64 pfn)
>  {
> +	struct rmpentry_raw *e_raw;
>  	u64 pfn_i, pfn_end;
> -	struct rmpentry *e;
> -	int level;
> +	struct rmpentry e;
> +	int level, ret;
>  
> -	e = __snp_lookup_rmpentry(pfn, &level);
> -	if (IS_ERR(e)) {
> -		pr_err("Failed to read RMP entry for PFN 0x%llx, error %ld\n",
> -		       pfn, PTR_ERR(e));
> +	ret = __snp_lookup_rmpentry(pfn, &e, &level);
> +	if (ret) {
> +		pr_err("Failed to read RMP entry for PFN 0x%llx, error %d\n",
> +		       pfn, ret);
>  		return;
>  	}
>  
> -	if (e->assigned) {
> +	if (e.assigned) {
> +		e_raw = get_raw_rmpentry(pfn);
> +		if (IS_ERR(e_raw)) {
> +			pr_err("Failed to read RMP contents for PFN 0x%llx, error %ld\n",
> +			       pfn, PTR_ERR(e_raw));
> +			return;
> +		}
> +
>  		pr_info("PFN 0x%llx, RMP entry: [0x%016llx - 0x%016llx]\n",
> -			pfn, e->lo, e->hi);
> +			pfn, e_raw->lo, e_raw->hi);
>  		return;
>  	}

Do I see it correctly that we don't really need to call that
get_raw_rmpentry() again for that @pfn because __snp_lookup_rmpentry()
returned the whole thing in @e already?

IOW:

diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index cf64e9384ea0..2e1833426b08 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -387,15 +387,8 @@ static void dump_rmpentry(u64 pfn)
 	}
 
 	if (e.assigned) {
-		e_raw = get_raw_rmpentry(pfn);
-		if (IS_ERR(e_raw)) {
-			pr_err("Failed to read RMP contents for PFN 0x%llx, error %ld\n",
-			       pfn, PTR_ERR(e_raw));
-			return;
-		}
-
-		pr_info("PFN 0x%llx, RMP entry: [0x%016llx - 0x%016llx]\n",
-			pfn, e_raw->lo, e_raw->hi);
+		pr_info("PFN 0x%llx, RMP entry: [ASID: 0x%x, pagesize: 0x%x, immutable: %d]\n",
+			e.gpa, e.asid, e.pagesize, e.immutable);
 		return;
 	}
 
-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette