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