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

[Tom Lendacky]

On 12/3/24 10:01, Borislav Petkov wrote:
> 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?

The idea is to dump the actual RMP contents that exist in memory. The @e
value only contains what the RMPREAD instruction chooses to expose.

Thanks,
Tom

> 
> 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;
>  	}
>