Re: [PATCH Part2 RFC v4 05/40] x86/sev: Add RMP entry lookup helpers
From: Brijesh Singh
Date: Thu Jul 15 2021 - 15:52:39 EST
On 7/15/21 1:37 PM, Sean Christopherson wrote:
On Wed, Jul 07, 2021, Brijesh Singh wrote:
The snp_lookup_page_in_rmptable() can be used by the host to read the RMP
entry for a given page. The RMP entry format is documented in AMD PPR, see
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fattachment.cgi%3Fid%3D296015&data=04%7C01%7Cbrijesh.singh%40amd.com%7C2140214b3fbd4a71617008d947bf9ae7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637619710568694335%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=AkCyolw0P%2BrRFF%2FAnRozld4GkegQ0hR%2F523DI48jB4g%3D&reserved=0.
Ewwwwww, the RMP format isn't architectural!?
Architecturally the format of RMP entries are not specified in APM. In order
to assist software, the following table specifies select portions of the RMP
entry format for this specific product.
Unfortunately yes.
But the documented fields in the RMP entry is architectural. The entry
fields are documented in the APM section 15.36. So, in future we are
guaranteed to have those fields available. If we are reading the RMP
table directly, then architecture should provide some other means to get
to fields from the RMP entry.
I know we generally don't want to add infrastructure without good reason, but on
the other hand exposing a microarchitectural data structure to the kernel at large
is going to be a disaster if the format does change on a future processor.
Looking at the future patches, dump_rmpentry() is the only power user, e.g.
everything else mostly looks at "assigned" and "level" (and one ratelimited warn
on "validated" in snp_make_page_shared(), but I suspect that particular check
can and should be dropped).
Yes, we need "assigned" and "level" and other entries are mainly for the
debug purposes.
So, what about hiding "struct rmpentry" and possibly renaming it to something
scary/microarchitectural, e.g. something like
Yes, it will work fine.
/*
* Returns 1 if the RMP entry is assigned, 0 if it exists but is not assigned,
* and -errno if there is no corresponding RMP entry.
*/
int snp_lookup_rmpentry(struct page *page, int *level)
{
unsigned long phys = page_to_pfn(page) << PAGE_SHIFT;
struct rmpentry *entry, *large_entry;
unsigned long vaddr;
if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
return -ENXIO;
vaddr = rmptable_start + rmptable_page_offset(phys);
if (unlikely(vaddr > rmptable_end))
return -EXNIO;
entry = (struct rmpentry *)vaddr;
/* Read a large RMP entry to get the correct page level used in RMP entry. */
vaddr = rmptable_start + rmptable_page_offset(phys & PMD_MASK);
large_entry = (struct rmpentry *)vaddr;
*level = RMP_TO_X86_PG_LEVEL(rmpentry_pagesize(large_entry));
return !!entry->assigned;
}
And then move dump_rmpentry() (or add a helper) in sev.c so that "struct rmpentry"
can be declared in sev.c.
Ack.
Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
---
arch/x86/include/asm/sev.h | 4 +--
arch/x86/kernel/sev.c | 26 +++++++++++++++++++
include/linux/sev.h | 51 ++++++++++++++++++++++++++++++++++++++
3 files changed, 78 insertions(+), 3 deletions(-)
create mode 100644 include/linux/sev.h
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 6c23e694a109..9e7e7e737f55 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -9,6 +9,7 @@
#define __ASM_ENCRYPTED_STATE_H
#include <linux/types.h>
+#include <linux/sev.h>
Why move things to linux/sev.h? AFAICT, even at the end of the series, the only
users of anything in this file all reside somewhere in arch/x86.
If we go with approach where the 'struct rmpentry' is not visible
outside the arch/x86/kernel/sev.c then there is no need to define all
these bit fields in linux/sev.h. I kept in linux/sev.h because driver
(KVM, and PSP) uses the rmpentry_xxx() to read the fields.
#include <asm/insn.h>
#include <asm/sev-common.h>
#include <asm/bootparam.h>
@@ -75,9 +76,6 @@ extern bool handle_vc_boot_ghcb(struct pt_regs *regs);
/* Software defined (when rFlags.CF = 1) */
#define PVALIDATE_FAIL_NOUPDATE 255
-/* RMP page size */
-#define RMP_PG_SIZE_4K 0
-
#define RMPADJUST_VMSA_PAGE_BIT BIT(16)
#ifdef CONFIG_AMD_MEM_ENCRYPT
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index f9d813d498fa..1aed3d53f59f 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -49,6 +49,8 @@
#define DR7_RESET_VALUE 0x400
#define RMPTABLE_ENTRIES_OFFSET 0x4000
+#define RMPENTRY_SHIFT 8
+#define rmptable_page_offset(x) (RMPTABLE_ENTRIES_OFFSET + (((unsigned long)x) >> RMPENTRY_SHIFT))
/* For early boot hypervisor communication in SEV-ES enabled guests */
static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
@@ -2319,3 +2321,27 @@ static int __init snp_rmptable_init(void)
* passthough state, and it is available after subsys_initcall().
*/
fs_initcall(snp_rmptable_init);
+
+struct rmpentry *snp_lookup_page_in_rmptable(struct page *page, int *level)
Maybe just snp_get_rmpentry? Or snp_lookup_rmpentry? I'm guessing the name was
chosen to align with e.g. lookup_address_in_mm, but IMO the lookup_address helpers
are oddly named.
Yes, it was mostly choose to align with it. Dave recommended dropping
the 'struct page *' arg from it and accept the pfn directly. Based on
your feedbacks, I am going to add
int snp_lookup_rmpentry(unsigned long pfn, int *level);
thanks