From: Ashish Kalra <ashish.kalra@amd.com>
When leaking certain page types, such as Hypervisor Fixed (HV_FIXED)
pages, it does not make sense to dump RMP contents for the 2MB range of
the page(s) being leaked. In the case of HV_FIXED pages, this is not an
error situation where the surrounding 2MB page RMP entries can provide
debug information.
Add new quiet parameter to snp_leak_pages(), to continue adding pages
to the snp_leaked_pages_list but not issue dump_rmpentry().
All existing users pass quiet=false parameter maintaining current
behavior. No functional changes.
Suggested-by: Thomas Lendacky <Thomas.Lendacky@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
arch/x86/include/asm/sev.h | 4 ++--
arch/x86/kvm/svm/sev.c | 4 ++--
arch/x86/virt/svm/sev.c | 5 +++--
drivers/crypto/ccp/sev-dev.c | 2 +-
4 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 02236962fdb1..8fc03f6c3026 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -616,7 +616,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, u32 asid, bool immutable);
int rmp_make_shared(u64 pfn, enum pg_level level);
-void snp_leak_pages(u64 pfn, unsigned int npages);
+void snp_leak_pages(u64 pfn, unsigned int npages, bool quiet);
void kdump_sev_callback(void);
void snp_fixup_e820_tables(void);
@@ -649,7 +649,7 @@ static inline int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 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) {}
+static inline void snp_leak_pages(u64 pfn, unsigned int npages, bool quiet) {}
static inline void kdump_sev_callback(void) { }
static inline void snp_fixup_e820_tables(void) {}
static inline void sev_evict_cache(void *va, int npages) {}
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 2fbdebf79fbb..a7db96a5f56d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -271,7 +271,7 @@ static void sev_decommission(unsigned int handle)
static int kvm_rmp_make_shared(struct kvm *kvm, u64 pfn, enum pg_level level)
{
if (KVM_BUG_ON(rmp_make_shared(pfn, level), kvm)) {
- snp_leak_pages(pfn, page_level_size(level) >> PAGE_SHIFT);
+ snp_leak_pages(pfn, page_level_size(level) >> PAGE_SHIFT, false);
return -EIO;
}
@@ -300,7 +300,7 @@ static int snp_page_reclaim(struct kvm *kvm, u64 pfn)
data.paddr = __sme_set(pfn << PAGE_SHIFT);
rc = sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, &data, &fw_err);
if (KVM_BUG(rc, kvm, "Failed to reclaim PFN %llx, rc %d fw_err %d", pfn, rc, fw_err)) {
- snp_leak_pages(pfn, 1);
+ snp_leak_pages(pfn, 1, false);
return -EIO;
}
diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 942372e69b4d..d75659859a07 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -1029,7 +1029,7 @@ int rmp_make_shared(u64 pfn, enum pg_level level)
}
EXPORT_SYMBOL_GPL(rmp_make_shared);
-void snp_leak_pages(u64 pfn, unsigned int npages)
+void snp_leak_pages(u64 pfn, unsigned int npages, bool quiet)
{
struct page *page = pfn_to_page(pfn);
@@ -1052,7 +1052,8 @@ void snp_leak_pages(u64 pfn, unsigned int npages)
(PageHead(page) && compound_nr(page) <= npages))
list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
- dump_rmpentry(pfn);
+ if (!quiet)
+ dump_rmpentry(pfn);
snp_nr_leaked_pages++;
pfn++;
page++;
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 4f000dc2e639..203a43a2df63 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -408,7 +408,7 @@ static int snp_reclaim_pages(unsigned long paddr, unsigned int npages, bool lock
* If there was a failure reclaiming the page then it is no longer safe
* to release it back to the system; leak it instead.
*/
- snp_leak_pages(__phys_to_pfn(paddr), npages - i);
+ snp_leak_pages(__phys_to_pfn(paddr), npages - i, false);
return ret;
}
--
2.34.1On Mon, Aug 18, 2025, Ashish Kalra wrote:
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 2fbdebf79fbb..a7db96a5f56d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -271,7 +271,7 @@ static void sev_decommission(unsigned int handle)
> static int kvm_rmp_make_shared(struct kvm *kvm, u64 pfn, enum pg_level level)
> {
> if (KVM_BUG_ON(rmp_make_shared(pfn, level), kvm)) {
> - snp_leak_pages(pfn, page_level_size(level) >> PAGE_SHIFT);
> + snp_leak_pages(pfn, page_level_size(level) >> PAGE_SHIFT, false);
> return -EIO;
> }
>
> @@ -300,7 +300,7 @@ static int snp_page_reclaim(struct kvm *kvm, u64 pfn)
> data.paddr = __sme_set(pfn << PAGE_SHIFT);
> rc = sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, &data, &fw_err);
> if (KVM_BUG(rc, kvm, "Failed to reclaim PFN %llx, rc %d fw_err %d", pfn, rc, fw_err)) {
> - snp_leak_pages(pfn, 1);
> + snp_leak_pages(pfn, 1, false);
Open coded true/false literals are ugly, e.g. now I have to go look at the
declaration (or even definition) of snp_leak_pages() to understand what %false
controls.
Assuming "don't dump the RMP entry" is the rare case, then craft the APIs to
reflect that, i.e. make snp_leak_pages() a wrapper for the common case. As a
bonus, you don't need to churn any extra code either.
void __snp_leak_pages(u64 pfn, unsigned int npages, bool dump_rmp);
static inline void snp_leak_pages(u64 pfn, unsigned int npages)
{
__snp_leak_pages(pfn, npages, true);
}
> return -EIO;
> }
>
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index 942372e69b4d..d75659859a07 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -1029,7 +1029,7 @@ int rmp_make_shared(u64 pfn, enum pg_level level)
> }
> EXPORT_SYMBOL_GPL(rmp_make_shared);
>
> -void snp_leak_pages(u64 pfn, unsigned int npages)
> +void snp_leak_pages(u64 pfn, unsigned int npages, bool quiet)
> {
> struct page *page = pfn_to_page(pfn);
>
> @@ -1052,7 +1052,8 @@ void snp_leak_pages(u64 pfn, unsigned int npages)
> (PageHead(page) && compound_nr(page) <= npages))
> list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
>
> - dump_rmpentry(pfn);
> + if (!quiet)
The polarity is arbitrarily odd, and "quiet" is annoyingly ambiguous and arguably
misleading, e.g. one could expect "quiet=true" to suppress the pr_warn() too, but
it does not.
pr_warn("Leaking PFN range 0x%llx-0x%llx\n", pfn, pfn + npages)
If you call it "bool dump_rmp" then it's more precise, self-explanatory, and
doesn't need to be inverted.
> + dump_rmpentry(pfn);
> snp_nr_leaked_pages++;
> pfn++;
> page++;
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 4f000dc2e639..203a43a2df63 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -408,7 +408,7 @@ static int snp_reclaim_pages(unsigned long paddr, unsigned int npages, bool lock
> * If there was a failure reclaiming the page then it is no longer safe
> * to release it back to the system; leak it instead.
> */
> - snp_leak_pages(__phys_to_pfn(paddr), npages - i);
> + snp_leak_pages(__phys_to_pfn(paddr), npages - i, false);
> return ret;
> }
>
> --
> 2.34.1
>
Hello Sean,
On 8/18/2025 4:14 PM, Sean Christopherson wrote:
> On Mon, Aug 18, 2025, Ashish Kalra wrote:
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 2fbdebf79fbb..a7db96a5f56d 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -271,7 +271,7 @@ static void sev_decommission(unsigned int handle)
>> static int kvm_rmp_make_shared(struct kvm *kvm, u64 pfn, enum pg_level level)
>> {
>> if (KVM_BUG_ON(rmp_make_shared(pfn, level), kvm)) {
>> - snp_leak_pages(pfn, page_level_size(level) >> PAGE_SHIFT);
>> + snp_leak_pages(pfn, page_level_size(level) >> PAGE_SHIFT, false);
>> return -EIO;
>> }
>>
>> @@ -300,7 +300,7 @@ static int snp_page_reclaim(struct kvm *kvm, u64 pfn)
>> data.paddr = __sme_set(pfn << PAGE_SHIFT);
>> rc = sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, &data, &fw_err);
>> if (KVM_BUG(rc, kvm, "Failed to reclaim PFN %llx, rc %d fw_err %d", pfn, rc, fw_err)) {
>> - snp_leak_pages(pfn, 1);
>> + snp_leak_pages(pfn, 1, false);
>
> Open coded true/false literals are ugly, e.g. now I have to go look at the
> declaration (or even definition) of snp_leak_pages() to understand what %false
> controls.
>
> Assuming "don't dump the RMP entry" is the rare case, then craft the APIs to
> reflect that, i.e. make snp_leak_pages() a wrapper for the common case. As a
> bonus, you don't need to churn any extra code either.
>
> void __snp_leak_pages(u64 pfn, unsigned int npages, bool dump_rmp);
>
> static inline void snp_leak_pages(u64 pfn, unsigned int npages)
> {
> __snp_leak_pages(pfn, npages, true);
> }
>
>> return -EIO;
>> }
>>
>> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
>> index 942372e69b4d..d75659859a07 100644
>> --- a/arch/x86/virt/svm/sev.c
>> +++ b/arch/x86/virt/svm/sev.c
>> @@ -1029,7 +1029,7 @@ int rmp_make_shared(u64 pfn, enum pg_level level)
>> }
>> EXPORT_SYMBOL_GPL(rmp_make_shared);
>>
>> -void snp_leak_pages(u64 pfn, unsigned int npages)
>> +void snp_leak_pages(u64 pfn, unsigned int npages, bool quiet)
>> {
>> struct page *page = pfn_to_page(pfn);
>>
>> @@ -1052,7 +1052,8 @@ void snp_leak_pages(u64 pfn, unsigned int npages)
>> (PageHead(page) && compound_nr(page) <= npages))
>> list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
>>
>> - dump_rmpentry(pfn);
>> + if (!quiet)
>
> The polarity is arbitrarily odd, and "quiet" is annoyingly ambiguous and arguably
> misleading, e.g. one could expect "quiet=true" to suppress the pr_warn() too, but
> it does not.
>
> pr_warn("Leaking PFN range 0x%llx-0x%llx\n", pfn, pfn + npages)
>
> If you call it "bool dump_rmp" then it's more precise, self-explanatory, and
> doesn't need to be inverted.
Thanks, i will re-work this accordingly.
Ashish
>
>> + dump_rmpentry(pfn);
>> snp_nr_leaked_pages++;
>> pfn++;
>> page++;
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 4f000dc2e639..203a43a2df63 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -408,7 +408,7 @@ static int snp_reclaim_pages(unsigned long paddr, unsigned int npages, bool lock
>> * If there was a failure reclaiming the page then it is no longer safe
>> * to release it back to the system; leak it instead.
>> */
>> - snp_leak_pages(__phys_to_pfn(paddr), npages - i);
>> + snp_leak_pages(__phys_to_pfn(paddr), npages - i, false);
>> return ret;
>> }
>>
>> --
>> 2.34.1
>>
© 2016 - 2025 Red Hat, Inc.