On 1/26/24 05:11, Michael Roth wrote:
> From: Ashish Kalra <ashish.kalra@xxxxxxx>
>
> Pages are unsafe to be released back to the page-allocator, if they
> have been transitioned to firmware/guest state and can't be reclaimed
> or transitioned back to hypervisor/shared state. In this case add
> them to an internal leaked pages list to ensure that they are not freed
> or touched/accessed to cause fatal page faults.
>
> Suggested-by: Vlastimil Babka <vbabka@xxxxxxx>
> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> [mdr: relocate to arch/x86/virt/svm/sev.c]
> Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx>
Some minor nitpicks:
> ---
> arch/x86/include/asm/sev.h | 2 ++
> arch/x86/virt/svm/sev.c | 34 ++++++++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+)
>
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index d3ccb7a0c7e9..435ba9bc4510 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -264,6 +264,7 @@ void snp_dump_hva_rmpentry(unsigned long address);
> int psmash(u64 pfn);
> int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, int asid, bool immutable);
> int rmp_make_shared(u64 pfn, enum pg_level level);
> +void snp_leak_pages(u64 pfn, unsigned int npages);
> #else
> static inline bool snp_probe_rmptable_info(void) { return false; }
> static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; }
> @@ -275,6 +276,7 @@ static inline int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, int as
> return -ENODEV;
> }
> static inline int rmp_make_shared(u64 pfn, enum pg_level level) { return -ENODEV; }
> +static inline void snp_leak_pages(u64 pfn, unsigned int npages) {}
> #endif
>
> #endif
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index 1a13eff78c9d..649ac1bb6b0e 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -65,6 +65,11 @@ static u64 probed_rmp_base, probed_rmp_size;
> static struct rmpentry *rmptable __ro_after_init;
> static u64 rmptable_max_pfn __ro_after_init;
>
> +static LIST_HEAD(snp_leaked_pages_list);
> +static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
> +
> +static unsigned long snp_nr_leaked_pages;
> +
> #undef pr_fmt
> #define pr_fmt(fmt) "SEV-SNP: " fmt
>
> @@ -505,3 +510,32 @@ int rmp_make_shared(u64 pfn, enum pg_level level)
> return rmpupdate(pfn, &state);
> }
> EXPORT_SYMBOL_GPL(rmp_make_shared);
> +
> +void snp_leak_pages(u64 pfn, unsigned int npages)
> +{
> + struct page *page = pfn_to_page(pfn);
> +
> + pr_warn("Leaking PFN range 0x%llx-0x%llx\n", pfn, pfn + npages);
> +
> + spin_lock(&snp_leaked_pages_list_lock);
> + while (npages--) {
> + /*
> + * Reuse the page's buddy list for chaining into the leaked
> + * pages list. This page should not be on a free list currently
> + * and is also unsafe to be added to a free list.
> + */
> + if (likely(!PageCompound(page)) ||
> + (PageHead(page) && compound_nr(page) <= npages))
> + /*
> + * Skip inserting tail pages of compound page as
> + * page->buddy_list of tail pages is not usable.
> + */
> + list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
Even though it's not necessary for correctness, with the comment I'd put the
whole block into { } to make easier to follow. Or just move the comment
above the if() itself?
> + dump_rmpentry(pfn);
> + snp_nr_leaked_pages++;
> + pfn++;
> + page++;
> + }
> + spin_unlock(&snp_leaked_pages_list_lock);
> +}
> +EXPORT_SYMBOL_GPL(snp_leak_pages);
