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

["Kalra, Ashish" <ashish.kalra@amd.com>]

, 
On 10/23/2024 1:41 PM, Tom Lendacky wrote:
> The RMPREAD instruction returns an architecture defined format of an
> RMP entry. This is the preferred method for examining RMP entries.
> 
> In preparation for using the RMPREAD instruction, convert the existing
> code that directly accesses the RMP to map the raw RMP information into
> the architecture defined format.
> 
> RMPREAD output returns a status bit for the 2MB region status. If the
> input page address is 2MB aligned and any other pages within the 2MB
> region are assigned, then 2MB region status will be set to 1. Otherwise,
> the 2MB region status will be set to 0. For systems that do not support
> RMPREAD, calculating this value would require looping over all of the RMP
> table entries within that range until one is found with the assigned bit
> set. Since this bit is not defined in the current format, and so not used
> today, do not incur the overhead associated with calculating it.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  arch/x86/virt/svm/sev.c | 121 +++++++++++++++++++++++++++++-----------
>  1 file changed, 87 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index 0ce17766c0e5..4d095affdb4d 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -30,6 +30,22 @@
>  #include <asm/cmdline.h>
>  #include <asm/iommu.h>
>  
> +/*
> + * The RMP entry format as returned by the RMPREAD instruction.
> + */
> +struct rmpread {
> +	u64 gpa;
> +	u8  assigned		:1,
> +	    rsvd1		:7;
> +	u8  pagesize		:1,
> +	    hpage_region_status	:1,
> +	    rsvd2		:6;
> +	u8  immutable		:1,
> +	    rsvd3		:7;
> +	u8  rsvd4;
> +	u32 asid;
> +} __packed;
> +
>  /*
>   * The RMP entry format is not architectural. The format is defined in PPR
>   * Family 19h Model 01h, Rev B1 processor.
> @@ -270,48 +286,77 @@ static int __init snp_rmptable_init(void)
>   */
>  device_initcall(snp_rmptable_init);
>  
> -static struct rmpentry *get_rmpentry(u64 pfn)
> +static struct rmpentry *__get_rmpentry(u64 pfn)
>  {
> -	if (WARN_ON_ONCE(pfn > rmptable_max_pfn))
> -		return ERR_PTR(-EFAULT);
> -
> -	return &rmptable[pfn];
> -}
> -
> -static struct rmpentry *__snp_lookup_rmpentry(u64 pfn, int *level)
> -{
> -	struct rmpentry *large_entry, *entry;
> -
> -	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
> +	if (!rmptable)
>  		return ERR_PTR(-ENODEV);
>  
> -	entry = get_rmpentry(pfn);
> -	if (IS_ERR(entry))
> -		return entry;
> +	if (unlikely(pfn > rmptable_max_pfn))

rmptable_max_pfn is initialized in patch#5, shouldn't this patch be moved after patch #5 ?

> +		return ERR_PTR(-EFAULT);
> +
> +	return rmptable + pfn;

Again, rmptable is initialized/setup in patch#5, similarly shouldn't this patch be moved
after patch #5 ?

Thanks,
Ashish

> +}
> +
> +static int get_rmpentry(u64 pfn, struct rmpread *entry)
> +{
> +	struct rmpentry *e;
> +
> +	e = __get_rmpentry(pfn);
> +	if (IS_ERR(e))
> +		return PTR_ERR(e);
> +
> +	/*
> +	 * Map the RMP table entry onto the RMPREAD output format.
> +	 * The 2MB region status indicator (hpage_region_status field) is not
> +	 * calculated, since the overhead could be significant and the field
> +	 * is not used.
> +	 */
> +	memset(entry, 0, sizeof(*entry));
> +	entry->gpa       = e->gpa << PAGE_SHIFT;
> +	entry->asid      = e->asid;
> +	entry->assigned  = e->assigned;
> +	entry->pagesize  = e->pagesize;
> +	entry->immutable = e->immutable;
> +
> +	return 0;
> +}
> +
> +static int __snp_lookup_rmpentry(u64 pfn, struct rmpread *entry, int *level)
> +{
> +	struct rmpread large_entry;
> +	int ret;
> +
> +	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
> +		return -ENODEV;
> +
> +	ret = get_rmpentry(pfn, entry);
> +	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, &large_entry);
> +	if (ret)
> +		return ret;
>  
> -	*level = RMP_TO_PG_LEVEL(large_entry->pagesize);
> +	*level = RMP_TO_PG_LEVEL(large_entry.pagesize);
>  
> -	return entry;
> +	return 0;
>  }
>  
>  int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level)
>  {
> -	struct rmpentry *e;
> +	struct rmpread rmpread;
> +	int ret;
>  
> -	e = __snp_lookup_rmpentry(pfn, level);
> -	if (IS_ERR(e))
> -		return PTR_ERR(e);
> +	ret = __snp_lookup_rmpentry(pfn, &rmpread, level);
> +	if (ret)
> +		return ret;
>  
> -	*assigned = !!e->assigned;
> +	*assigned = !!rmpread.assigned;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(snp_lookup_rmpentry);
> @@ -324,18 +369,26 @@ EXPORT_SYMBOL_GPL(snp_lookup_rmpentry);
>   */
>  static void dump_rmpentry(u64 pfn)
>  {
> -	u64 pfn_i, pfn_end;
> +	struct rmpread rmpread;
>  	struct rmpentry *e;
> -	int level;
> +	u64 pfn_i, pfn_end;
> +	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, &rmpread, &level);
> +	if (ret) {
> +		pr_err("Failed to read RMP entry for PFN 0x%llx, error %d\n",
> +		       pfn, ret);
>  		return;
>  	}
>  
> -	if (e->assigned) {
> +	if (rmpread.assigned) {
> +		e = __get_rmpentry(pfn);
> +		if (IS_ERR(e)) {
> +			pr_err("Failed to read RMP contents for PFN 0x%llx, error %ld\n",
> +			       pfn, PTR_ERR(e));
> +			return;
> +		}
> +
>  		pr_info("PFN 0x%llx, RMP entry: [0x%016llx - 0x%016llx]\n",
>  			pfn, e->lo, e->hi);
>  		return;
> @@ -356,9 +409,9 @@ static void dump_rmpentry(u64 pfn)
>  		pfn, pfn_i, pfn_end);
>  
>  	while (pfn_i < pfn_end) {
> -		e = __snp_lookup_rmpentry(pfn_i, &level);
> +		e = __get_rmpentry(pfn_i);
>  		if (IS_ERR(e)) {
> -			pr_err("Error %ld reading RMP entry for PFN 0x%llx\n",
> +			pr_err("Error %ld reading RMP contents for PFN 0x%llx\n",
>  			       PTR_ERR(e), pfn_i);
>  			pfn_i++;
>  			continue;