Re: [PATCH v1 07/26] x86/fault: Add helper for dumping RMP entries

[Tom Lendacky <thomas.lendacky@xxxxxxx>]

On 1/10/24 05:13, Borislav Petkov wrote:
> On Sat, Dec 30, 2023 at 10:19:35AM -0600, Michael Roth wrote:
> > +	while (pfn_current < pfn_end) {
> > +		e = __snp_lookup_rmpentry(pfn_current, &level);
> > +		if (IS_ERR(e)) {
> > +			pfn_current++;
> > +			continue;
> > +		}
> > +
> > +		e_data = (u64 *)e;
> > +		if (e_data[0] || e_data[1]) {
> > +			pr_info("No assigned RMP entry for PFN 0x%llx, but the 2MB region contains populated RMP entries, e.g.: PFN 0x%llx: [high=0x%016llx low=0x%016llx]\n",
> > +				pfn, pfn_current, e_data[1], e_data[0]);
> > +			return;
> > +		}
> > +		pfn_current++;
> > +	}
> > +
> > +	pr_info("No populated RMP entries in the 2MB region containing PFN 0x%llx\n",
> > +		pfn);
> > +}
>
> Ok, I went and reworked this, see below.
>
> Yes, I think it is important - at least in the beginning - to dump the
> whole 2M PFN region for debugging purposes. If that output starts
> becoming too unwieldy and overflowing terminals or log files, we'd
> shorten it or put it behind a debug option or so.
>
> Thx.
>
> ---
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index a8cf33b7da71..259a1dd655a7 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c

> +	pr_info("PFN 0x%llx unassigned, dumping the whole 2M PFN region: [0x%llx - 0x%llx]\n",
> +		pfn, pfn_i, pfn_end);

How about saying "... dumping all non-zero entries in the whole ..."

and then removing the print below that prints the PFN and "..."

> +
> +	while (pfn_i < pfn_end) {
> +		e = __snp_lookup_rmpentry(pfn_i, &level);
>   		if (IS_ERR(e)) {
> -			pfn_current++;
> +			pr_err("Error %ld reading RMP entry for PFN 0x%llx\n",
> +				PTR_ERR(e), pfn_i);
> +			pfn_i++;
>   			continue;
>   		}
>   
> -		e_data = (u64 *)e;
> -		if (e_data[0] || e_data[1]) {
> -			pr_info("No assigned RMP entry for PFN 0x%llx, but the 2MB region contains populated RMP entries, e.g.: PFN 0x%llx: [high=0x%016llx low=0x%016llx]\n",
> -				pfn, pfn_current, e_data[1], e_data[0]);
> -			return;
> -		}
> -		pfn_current++;
> -	}
> +		if (e->lo || e->hi)
> +			pr_info("PFN: 0x%llx, [0x%016llx - 0x%016llx]\n", pfn_i, e->lo, e->hi);
> +		else
> +			pr_info("PFN: 0x%llx ...\n", pfn_i);

Remove this one.

That should cut down on excess output since you are really only concerned 
with non-zero RMP entries when the input PFN RMP entry is not assigned.

Thanks,
Tom

>   
> -	pr_info("No populated RMP entries in the 2MB region containing PFN 0x%llx\n",
> -		pfn);
> +		pfn_i++;
> +	}
>   }
>   
>   void snp_dump_hva_rmpentry(unsigned long hva)
> @@ -339,4 +343,3 @@ void snp_dump_hva_rmpentry(unsigned long hva)
>   
>   	dump_rmpentry(pte_pfn(*pte));
>   }
> -EXPORT_SYMBOL_GPL(snp_dump_hva_rmpentry);