| Message ID | 8f63961f00fd170ba0e561f499292175f3155d26.1655761627.git.ashish.kalra@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) | expand |
On 6/20/22 16:02, Ashish Kalra wrote: > +/* > + * The RMP entry format is not architectural. The format is defined in PPR > + * Family 19h Model 01h, Rev B1 processor. > + */ Let's say that Family 20h comes out and has a new RMP entry format. What keeps an old kernel from attempting to use this old format on that new CPU?
[AMD Official Use Only - General] >> +/* >> + * The RMP entry format is not architectural. The format is defined >> +in PPR >> + * Family 19h Model 01h, Rev B1 processor. >> + */ >Let's say that Family 20h comes out and has a new RMP entry format. >What keeps an old kernel from attempting to use this old format on that new CPU? As I replied previously on the same subject: Architectural implies that it is defined in the APM and shouldn't change in such a way as to not be backward compatible. I probably think the wording here should be architecture independent or more precisely platform independent. Thanks, Ashish
On 6/22/22 07:22, Kalra, Ashish wrote: > As I replied previously on the same subject: Architectural implies > that it is defined in the APM and shouldn't change in such a way as > to not be backward compatible. I probably think the wording here > should be architecture independent or more precisely platform > independent. Yeah, arch-independent and non-architectural are quite different concepts. At Intel, at least, when someone says "not architectural" mean that the behavior is implementation-specific. That, combined with the model/family/stepping gave me the wrong impression about what was going on. Some more clarity would be greatly appreciated.
[Public] On 6/22/22 07:22, Kalra, Ashish wrote: >> As I replied previously on the same subject: Architectural implies >> that it is defined in the APM and shouldn't change in such a way as to >> not be backward compatible. I probably think the wording here should >> be architecture independent or more precisely platform independent. >Yeah, arch-independent and non-architectural are quite different concepts. >At Intel, at least, when someone says "not architectural" mean that the behavior is implementation-specific. That, combined with the model/family/stepping gave me the wrong impression about what was going on. >Some more clarity would be greatly appreciated. Actually, the PPR for family 19h Model 01h, Rev B1 defines the RMP entry format as below: 2.1.4.2 RMP Entry Format 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. Each RMP entry is 16B in size and is formatted as follows. Software should not rely on any field definitions not specified in this table and the format of an RMP entry may change in future processors. Architectural implies that it is defined in the APM and shouldn't change in such a way as to not be backward compatible. So non-architectural in this context means that it is only defined in our PPR. So actually this RPM entry definition is platform dependent and will need to be changed for different AMD processors and that change has to be handled correspondingly in the dump_rmpentry() code. Thanks, Ashish
On 6/22/22 11:15, Kalra, Ashish wrote: > So actually this RPM entry definition is platform dependent and will > need to be changed for different AMD processors and that change has > to be handled correspondingly in the dump_rmpentry() code. So, if the RMP entry format changes in future processors, how do we make sure that the kernel does not try to use *this* code on those processors?
[AMD Official Use Only - General] -----Original Message----- From: Dave Hansen <dave.hansen@intel.com> Sent: Wednesday, June 22, 2022 1:18 PM To: Kalra, Ashish <Ashish.Kalra@amd.com>; x86@kernel.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org; linux-coco@lists.linux.dev; linux-mm@kvack.org; linux-crypto@vger.kernel.org Cc: tglx@linutronix.de; mingo@redhat.com; jroedel@suse.de; Lendacky, Thomas <Thomas.Lendacky@amd.com>; hpa@zytor.com; ardb@kernel.org; pbonzini@redhat.com; seanjc@google.com; vkuznets@redhat.com; jmattson@google.com; luto@kernel.org; dave.hansen@linux.intel.com; slp@redhat.com; pgonda@google.com; peterz@infradead.org; srinivas.pandruvada@linux.intel.com; rientjes@google.com; dovmurik@linux.ibm.com; tobin@ibm.com; bp@alien8.de; Roth, Michael <Michael.Roth@amd.com>; vbabka@suse.cz; kirill@shutemov.name; ak@linux.intel.com; tony.luck@intel.com; marcorr@google.com; sathyanarayanan.kuppuswamy@linux.intel.com; alpergun@google.com; dgilbert@redhat.com; jarkko@kernel.org Subject: Re: [PATCH Part2 v6 05/49] x86/sev: Add RMP entry lookup helpers On 6/22/22 11:15, Kalra, Ashish wrote: > So actually this RPM entry definition is platform dependent and will > need to be changed for different AMD processors and that change has to > be handled correspondingly in the dump_rmpentry() code. >So, if the RMP entry format changes in future processors, how do we make sure that the kernel does not try to use *this* code on those processors? Functions snp_lookup_rmpentry() and dump_rmpentry() which rely on this structure definition will need to handle it accordingly. Thanks, Ashish
On 6/22/22 11:34, Kalra, Ashish wrote: >> So, if the RMP entry format changes in future processors, how do we >> make sure that the kernel does not try to use *this* code on those >> processors? > Functions snp_lookup_rmpentry() and dump_rmpentry() which rely on > this structure definition will need to handle it accordingly. In other words, old kernels will break on new hardware? I think that needs to be fixed. It should be as simple as a model/family check, though. If someone (for example) attempts to use SNP (and thus snp_lookup_rmpentry() and dump_rmpentry()) code on a newer CPU, the kernel should refuse.
[AMD Official Use Only - General] -----Original Message----- From: Dave Hansen <dave.hansen@intel.com> Sent: Wednesday, June 22, 2022 1:43 PM To: Kalra, Ashish <Ashish.Kalra@amd.com>; x86@kernel.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org; linux-coco@lists.linux.dev; linux-mm@kvack.org; linux-crypto@vger.kernel.org Cc: tglx@linutronix.de; mingo@redhat.com; jroedel@suse.de; Lendacky, Thomas <Thomas.Lendacky@amd.com>; hpa@zytor.com; ardb@kernel.org; pbonzini@redhat.com; seanjc@google.com; vkuznets@redhat.com; jmattson@google.com; luto@kernel.org; dave.hansen@linux.intel.com; slp@redhat.com; pgonda@google.com; peterz@infradead.org; srinivas.pandruvada@linux.intel.com; rientjes@google.com; dovmurik@linux.ibm.com; tobin@ibm.com; bp@alien8.de; Roth, Michael <Michael.Roth@amd.com>; vbabka@suse.cz; kirill@shutemov.name; ak@linux.intel.com; tony.luck@intel.com; marcorr@google.com; sathyanarayanan.kuppuswamy@linux.intel.com; alpergun@google.com; dgilbert@redhat.com; jarkko@kernel.org Subject: Re: [PATCH Part2 v6 05/49] x86/sev: Add RMP entry lookup helpers On 6/22/22 11:34, Kalra, Ashish wrote: >>> So, if the RMP entry format changes in future processors, how do we >>> make sure that the kernel does not try to use *this* code on those >>> processors? >> Functions snp_lookup_rmpentry() and dump_rmpentry() which rely on this >> structure definition will need to handle it accordingly. >In other words, old kernels will break on new hardware? >I think that needs to be fixed. It should be as simple as a model/family check, though. If someone (for example) attempts to use SNP (and thus snp_lookup_rmpentry() and dump_rmpentry()) code on a newer CPU, the kernel should refuse. More specifically I am thinking of adding RMP entry field accessors so that they can do this cpu model/family check and return the correct field as per processor architecture. Thanks, Ashish
On 6/22/22 12:43, Kalra, Ashish wrote: >> I think that needs to be fixed. It should be as simple as a >> model/family check, though. If someone (for example) attempts to >> use SNP (and thus snp_lookup_rmpentry() and dump_rmpentry()) code >> on a newer CPU, the kernel should refuse. > More specifically I am thinking of adding RMP entry field accessors > so that they can do this cpu model/family check and return the > correct field as per processor architecture. That will be helpful down the road when there's more than one format. But, the real issue is that the kernel doesn't *support* a different RMP format. So, the SNP support should be disabled when encountering a model/family other than the known good one.
[AMD Official Use Only - General] From: Dave Hansen <dave.hansen@intel.com> Sent: Wednesday, June 22, 2022 2:50 PM To: Kalra, Ashish <Ashish.Kalra@amd.com>; x86@kernel.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org; linux-coco@lists.linux.dev; linux-mm@kvack.org; linux-crypto@vger.kernel.org Cc: tglx@linutronix.de; mingo@redhat.com; jroedel@suse.de; Lendacky, Thomas <Thomas.Lendacky@amd.com>; hpa@zytor.com; ardb@kernel.org; pbonzini@redhat.com; seanjc@google.com; vkuznets@redhat.com; jmattson@google.com; luto@kernel.org; dave.hansen@linux.intel.com; slp@redhat.com; pgonda@google.com; peterz@infradead.org; srinivas.pandruvada@linux.intel.com; rientjes@google.com; dovmurik@linux.ibm.com; tobin@ibm.com; bp@alien8.de; Roth, Michael <Michael.Roth@amd.com>; vbabka@suse.cz; kirill@shutemov.name; ak@linux.intel.com; tony.luck@intel.com; marcorr@google.com; sathyanarayanan.kuppuswamy@linux.intel.com; alpergun@google.com; dgilbert@redhat.com; jarkko@kernel.org Subject: Re: [PATCH Part2 v6 05/49] x86/sev: Add RMP entry lookup helpers On 6/22/22 12:43, Kalra, Ashish wrote: >>> I think that needs to be fixed. It should be as simple as a >>> model/family check, though. If someone (for example) attempts to use >>> SNP (and thus snp_lookup_rmpentry() and dump_rmpentry()) code on a >>> newer CPU, the kernel should refuse. >> More specifically I am thinking of adding RMP entry field accessors so >> that they can do this cpu model/family check and return the correct >> field as per processor architecture. >That will be helpful down the road when there's more than one format. >But, the real issue is that the kernel doesn't *support* a different RMP format. So, the SNP support should be disabled when encountering a model/family other than the known good one. Yes, that makes sense, will add an additional check in snp_rmptable_init(). Thanks, Ashish
[Public] From: Dave Hansen <dave.hansen@intel.com> Sent: Wednesday, June 22, 2022 2:50 PM To: Kalra, Ashish <Ashish.Kalra@amd.com>; x86@kernel.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org; linux-coco@lists.linux.dev; linux-mm@kvack.org; linux-crypto@vger.kernel.org Cc: tglx@linutronix.de; mingo@redhat.com; jroedel@suse.de; Lendacky, Thomas <Thomas.Lendacky@amd.com>; hpa@zytor.com; ardb@kernel.org; pbonzini@redhat.com; seanjc@google.com; vkuznets@redhat.com; jmattson@google.com; luto@kernel.org; dave.hansen@linux.intel.com; slp@redhat.com; pgonda@google.com; peterz@infradead.org; srinivas.pandruvada@linux.intel.com; rientjes@google.com; dovmurik@linux.ibm.com; tobin@ibm.com; bp@alien8.de; Roth, Michael <Michael.Roth@amd.com>; vbabka@suse.cz; kirill@shutemov.name; ak@linux.intel.com; tony.luck@intel.com; marcorr@google.com; sathyanarayanan.kuppuswamy@linux.intel.com; alpergun@google.com; dgilbert@redhat.com; jarkko@kernel.org Subject: Re: [PATCH Part2 v6 05/49] x86/sev: Add RMP entry lookup helpers On 6/22/22 12:43, Kalra, Ashish wrote: >>> I think that needs to be fixed. It should be as simple as a >>> model/family check, though. If someone (for example) attempts to >>> use SNP (and thus snp_lookup_rmpentry() and dump_rmpentry()) code on >>> a newer CPU, the kernel should refuse. >> More specifically I am thinking of adding RMP entry field accessors >> so that they can do this cpu model/family check and return the >> correct field as per processor architecture. >That will be helpful down the road when there's more than one format. >But, the real issue is that the kernel doesn't *support* a different RMP format. So, the SNP support should be disabled when encountering a model/family other than the known good one. >Yes, that makes sense, will add an additional check in snp_rmptable_init(). Also to add here, additionally we may create an architectural way to read the RMP entry in the future. Thanks, Ashish
On Mon, Jun 20, 2022 at 4:02 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote: > > From: Brijesh Singh <brijesh.singh@amd.com> > > 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://bugzilla.kernel.org/attachment.cgi?id=296015. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/include/asm/sev.h | 27 ++++++++++++++++++++++++ > arch/x86/kernel/sev.c | 43 ++++++++++++++++++++++++++++++++++++++ > include/linux/sev.h | 30 ++++++++++++++++++++++++++ > 3 files changed, 100 insertions(+) > create mode 100644 include/linux/sev.h > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h > index 9c2d33f1cfee..cb16f0e5b585 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> > #include <asm/insn.h> > #include <asm/sev-common.h> > #include <asm/bootparam.h> > @@ -84,6 +85,32 @@ extern bool handle_vc_boot_ghcb(struct pt_regs *regs); > > /* RMP page size */ > #define RMP_PG_SIZE_4K 0 > +#define RMP_TO_X86_PG_LEVEL(level) (((level) == RMP_PG_SIZE_4K) ? PG_LEVEL_4K : PG_LEVEL_2M) > + > +/* > + * The RMP entry format is not architectural. The format is defined in PPR > + * Family 19h Model 01h, Rev B1 processor. > + */ > +struct __packed rmpentry { > + union { > + struct { > + u64 assigned : 1, > + pagesize : 1, > + immutable : 1, > + rsvd1 : 9, > + gpa : 39, > + asid : 10, > + vmsa : 1, > + validated : 1, > + rsvd2 : 1; > + } info; > + u64 low; > + }; > + u64 high; > +}; > + > +#define rmpentry_assigned(x) ((x)->info.assigned) > +#define rmpentry_pagesize(x) ((x)->info.pagesize) > > #define RMPADJUST_VMSA_PAGE_BIT BIT(16) > > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c > index 25c7feb367f6..59e7ec6b0326 100644 > --- a/arch/x86/kernel/sev.c > +++ b/arch/x86/kernel/sev.c > @@ -65,6 +65,8 @@ > * bookkeeping, the range need to be added during the RMP entry lookup. > */ > #define RMPTABLE_CPU_BOOKKEEPING_SZ 0x4000 > +#define RMPENTRY_SHIFT 8 > +#define rmptable_page_offset(x) (RMPTABLE_CPU_BOOKKEEPING_SZ + (((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); > @@ -2386,3 +2388,44 @@ static int __init snp_rmptable_init(void) > * available after subsys_initcall(). > */ > fs_initcall(snp_rmptable_init); > + > +static struct rmpentry *__snp_lookup_rmpentry(u64 pfn, int *level) > +{ > + unsigned long vaddr, paddr = pfn << PAGE_SHIFT; > + struct rmpentry *entry, *large_entry; > + > + if (!pfn_valid(pfn)) > + return ERR_PTR(-EINVAL); > + > + if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) > + return ERR_PTR(-ENXIO); nit: I think we should check if SNP is enabled first, before doing anything else. In other words, I think we should move this check above the `!pfn_valid()` check. > + > + vaddr = rmptable_start + rmptable_page_offset(paddr); > + if (unlikely(vaddr > rmptable_end)) > + return ERR_PTR(-ENXIO); nit: It would be nice to use a different error code here, from the SNP feature check. That way, if this function fails, it's easier to diagnose where the function failed from the error code. > + > + 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(paddr & PMD_MASK); > + large_entry = (struct rmpentry *)vaddr; > + *level = RMP_TO_X86_PG_LEVEL(rmpentry_pagesize(large_entry)); > + > + return entry; > +} > + > +/* > + * Return 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(u64 pfn, int *level) > +{ > + struct rmpentry *e; > + > + e = __snp_lookup_rmpentry(pfn, level); > + if (IS_ERR(e)) > + return PTR_ERR(e); > + > + return !!rmpentry_assigned(e); > +} > +EXPORT_SYMBOL_GPL(snp_lookup_rmpentry); > diff --git a/include/linux/sev.h b/include/linux/sev.h > new file mode 100644 > index 000000000000..1a68842789e1 > --- /dev/null > +++ b/include/linux/sev.h > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * AMD Secure Encrypted Virtualization > + * > + * Author: Brijesh Singh <brijesh.singh@amd.com> > + */ > + > +#ifndef __LINUX_SEV_H > +#define __LINUX_SEV_H > + > +/* RMUPDATE detected 4K page and 2MB page overlap. */ > +#define RMPUPDATE_FAIL_OVERLAP 7 > + > +#ifdef CONFIG_AMD_MEM_ENCRYPT > +int snp_lookup_rmpentry(u64 pfn, int *level); > +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); nit: I think the declarations for `psmash()`, `rmp_make_private()`, and `rmp_make_shared()` should be introduced in the patches that have their definitions. > +#else > +static inline int snp_lookup_rmpentry(u64 pfn, int *level) { return 0; } > +static inline int psmash(u64 pfn) { return -ENXIO; } > +static inline int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, int asid, > + bool immutable) > +{ > + return -ENODEV; > +} > +static inline int rmp_make_shared(u64 pfn, enum pg_level level) { return -ENODEV; } > + > +#endif /* CONFIG_AMD_MEM_ENCRYPT */ > +#endif /* __LINUX_SEV_H */ > -- > 2.25.1 >
On Wed, Jun 22, 2022, Kalra, Ashish wrote: > On 6/22/22 12:43, Kalra, Ashish wrote: > >>> I think that needs to be fixed. It should be as simple as a > >>> model/family check, though. If someone (for example) attempts to use > >>> SNP (and thus snp_lookup_rmpentry() and dump_rmpentry()) code on a > >>> newer CPU, the kernel should refuse. > >> More specifically I am thinking of adding RMP entry field accessors so > >> that they can do this cpu model/family check and return the correct > >> field as per processor architecture. > > >That will be helpful down the road when there's more than one format. But, > >the real issue is that the kernel doesn't *support* a different RMP format. > >So, the SNP support should be disabled when encountering a model/family > >other than the known good one. > > Yes, that makes sense, will add an additional check in snp_rmptable_init(). And as I suggested in v5[*], bury the microarchitectural struct in sev.c so that nothing outside of the few bits of SNP code that absolutely need to know the layout of the struct should even be aware that there's a struct overlay for RMP entries. [*] https://lore.kernel.org/all/YPCAZaROOHNskGlO@google.com
[AMD Official Use Only - General] >> On 6/22/22 12:43, Kalra, Ashish wrote: >> >>> I think that needs to be fixed. It should be as simple as a >> >>> model/family check, though. If someone (for example) attempts to >> >>> use SNP (and thus snp_lookup_rmpentry() and dump_rmpentry()) code >> >>> on a newer CPU, the kernel should refuse. >> >> More specifically I am thinking of adding RMP entry field accessors >> >> so that they can do this cpu model/family check and return the >> >> correct field as per processor architecture. >> >> >That will be helpful down the road when there's more than one format. >> >But, the real issue is that the kernel doesn't *support* a different RMP format. >> >So, the SNP support should be disabled when encountering a >> >model/family other than the known good one. >> >> Yes, that makes sense, will add an additional check in snp_rmptable_init(). >And as I suggested in v5[*], bury the microarchitectural struct in sev.c so that nothing outside of the few bits of SNP code that absolutely need to know the layout of the struct should even be aware that there's a struct overlay for RMP entries. Yes, that's a nice way to hide it from the rest of the kernel which does not require access to this structure anyway, in essence, it becomes a private structure. Thanks, Ashish >[*] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2FYPCAZaROOHNskGlO%40google.com&data=05%7C01%7CAshish.Kalra%40amd.com%7Ce210ec383f654556348c08da5568ca81%>7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637916205851843411%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=6TOpchjhgFg%>2F5JTa%2FqSviiTuehNoZgvTVBuZv6JxsXc%3D&reserved=0
On Thu, Jun 23, 2022 at 10:43:40PM +0000, Kalra, Ashish wrote: > Yes, that's a nice way to hide it from the rest of the kernel which > does not require access to this structure anyway, in essence, it > becomes a private structure. So this whole discussion whether there should be a model check or not in case a new RMP format gets added in the future is moot - when a new model format comes along, *then* the distinction should be done and added in code - not earlier. This is nothing else but normal CPU enablement work - it should be done when it is really needed. Because the opposite can happen: you can add a model check which excludes future model X, future model X comes along but does *not* change the RMP format and then you're going to have to relax that model check again to fix SNP on the new model X. So pls add the model checks only when really needed. Thx.
On Mon, Jun 20, 2022 at 11:02:33PM +0000, Ashish Kalra wrote: > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c > index 25c7feb367f6..59e7ec6b0326 100644 > --- a/arch/x86/kernel/sev.c > +++ b/arch/x86/kernel/sev.c > @@ -65,6 +65,8 @@ > * bookkeeping, the range need to be added during the RMP entry lookup. > */ > #define RMPTABLE_CPU_BOOKKEEPING_SZ 0x4000 > +#define RMPENTRY_SHIFT 8 > +#define rmptable_page_offset(x) (RMPTABLE_CPU_BOOKKEEPING_SZ + (((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); > @@ -2386,3 +2388,44 @@ static int __init snp_rmptable_init(void) > * available after subsys_initcall(). > */ > fs_initcall(snp_rmptable_init); > + > +static struct rmpentry *__snp_lookup_rmpentry(u64 pfn, int *level) > +{ > + unsigned long vaddr, paddr = pfn << PAGE_SHIFT; > + struct rmpentry *entry, *large_entry; > + > + if (!pfn_valid(pfn)) > + return ERR_PTR(-EINVAL); > + > + if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) > + return ERR_PTR(-ENXIO); That test should happen first. > + vaddr = rmptable_start + rmptable_page_offset(paddr); Wait, what does that macro do? It takes the physical address and gives the offset from the beginning of the RMP table in VA space? So why don't you do entry = rmptable_entry(paddr) instead which simply gives you directly the entry in the RMP table with which you can work further? Instead of this macro doing half the work and then callers having to add the RMP start address and cast. And make it small function so that you can have typechecking too, while at it. > + if (unlikely(vaddr > rmptable_end)) > + return ERR_PTR(-ENXIO); > + > + 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(paddr & PMD_MASK); > + large_entry = (struct rmpentry *)vaddr; > + *level = RMP_TO_X86_PG_LEVEL(rmpentry_pagesize(large_entry)); > + > + return entry; > +} > + > +/* > + * Return 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(u64 pfn, int *level) > +{ > + struct rmpentry *e; > + > + e = __snp_lookup_rmpentry(pfn, level); > + if (IS_ERR(e)) > + return PTR_ERR(e); > + > + return !!rmpentry_assigned(e); > +} > +EXPORT_SYMBOL_GPL(snp_lookup_rmpentry); > diff --git a/include/linux/sev.h b/include/linux/sev.h > new file mode 100644 > index 000000000000..1a68842789e1 > --- /dev/null > +++ b/include/linux/sev.h Why is this header in the linux/ namespace and not in arch/x86/ ? All that stuff in here doesn't have any meaning outside of x86...
On Fri, Jul 22, 2022, Borislav Petkov wrote: > On Thu, Jun 23, 2022 at 10:43:40PM +0000, Kalra, Ashish wrote: > > Yes, that's a nice way to hide it from the rest of the kernel which > > does not require access to this structure anyway, in essence, it > > becomes a private structure. > > So this whole discussion whether there should be a model check or not > in case a new RMP format gets added in the future is moot - when a new > model format comes along, *then* the distinction should be done and > added in code - not earlier. I disagree. Running an old kernel on new hardware with a different RMP layout should refuse to use SNP, not read/write garbage and likely corrupt the RMP and/or host memory. And IMO, hiding the non-architectural RMP format in SNP-specific code so that we don't have to churn a bunch of call sites that don't _need_ access to the raw RMP format is a good idea regardless of whether we want to be optimistic or pessimistic about future formats. > This is nothing else but normal CPU enablement work - it should be done > when it is really needed. > > Because the opposite can happen: you can add a model check which > excludes future model X, future model X comes along but does *not* > change the RMP format and then you're going to have to relax that model > check again to fix SNP on the new model X. > > So pls add the model checks only when really needed. > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Jul 22, 2022 at 07:04:23PM +0000, Sean Christopherson wrote: > I disagree. Running an old kernel on new hardware with a different RMP layout > should refuse to use SNP, not read/write garbage and likely corrupt the RMP and/or > host memory. See my example below. > And IMO, hiding the non-architectural RMP format in SNP-specific code so that we > don't have to churn a bunch of call sites that don't _need_ access to the raw RMP > format is a good idea regardless of whether we want to be optimistic or pessimistic > about future formats. I don't think I ever objected to that. > > This is nothing else but normal CPU enablement work - it should be done > > when it is really needed. > > <--- this here. > > Because the opposite can happen: you can add a model check which > > excludes future model X, future model X comes along but does *not* > > change the RMP format and then you're going to have to relax that model > > check again to fix SNP on the new model X. So constantly adding new models to a list which support a certain version of the RMP format doesn't scale either. If you corrupt the RMP because your kernel is old, you'll crash and burn very visibly so that you'll be forced to have to look for an updated kernel regardless.
Btw, what could work is to spec only a *version* field somewhere in the HW or FW which says which version the RMP header has. Then, OS would check that field and if it doesn't support that certain version, it'll bail. I'd need to talk to folks first, though, what the whole story is behind not spec-ing the RMP format...
On Fri, Jul 22, 2022, Borislav Petkov wrote: > On Fri, Jul 22, 2022 at 07:04:23PM +0000, Sean Christopherson wrote: > > I disagree. Running an old kernel on new hardware with a different RMP layout > > should refuse to use SNP, not read/write garbage and likely corrupt the RMP and/or > > host memory. > > See my example below. > > > And IMO, hiding the non-architectural RMP format in SNP-specific code so that we > > don't have to churn a bunch of call sites that don't _need_ access to the raw RMP > > format is a good idea regardless of whether we want to be optimistic or pessimistic > > about future formats. > > I don't think I ever objected to that. Yar, just wanted to be make sure we're all on the same page, I wasn't entirely sure what was get nacked :-) > > > This is nothing else but normal CPU enablement work - it should be done > > > when it is really needed. > > > > > <--- this here. > > > > Because the opposite can happen: you can add a model check which > > > excludes future model X, future model X comes along but does *not* > > > change the RMP format and then you're going to have to relax that model > > > check again to fix SNP on the new model X. > > So constantly adding new models to a list which support a certain > version of the RMP format doesn't scale either. Yeah, but either we get AMD to give us an architectural layout or we'll have to eat that cost at some point in the future. > If you corrupt the RMP because your kernel is old, you'll crash and burn > very visibly so that you'll be forced to have to look for an updated > kernel regardless. Heh, you're definitely more optimistic than me. I can just see something truly ridiculous happening like moving the page size bit and then getting weird behavior only when KVM happens to need the page size for some edge case. Anyways, it's not a sticking point, and I certainly am not volunteering to maintain the FMS list...
On Fri, Jul 22, 2022 at 10:16:07PM +0000, Sean Christopherson wrote: > Yar, just wanted to be make sure we're all on the same page, I wasn't entirely > sure what was get nacked :-) Not nacked - we're all just talking here. :-) > Heh, you're definitely more optimistic than me. I can just see something truly > ridiculous happening like moving the page size bit and then getting weird behavior > only when KVM happens to need the page size for some edge case. > > Anyways, it's not a sticking point, and I certainly am not volunteering to > maintain the FMS list... Yeah, no need for it to be a sticking point because a pretty reliable birdie just told me that we're worrying for nothing and it all will solve itself. :-)
On Mon, Jun 20, 2022 at 11:02:33PM +0000, Ashish Kalra wrote: > +/* > + * The RMP entry format is not architectural. The format is defined in PPR > + * Family 19h Model 01h, Rev B1 processor. > + */ > +struct __packed rmpentry { That __packed goes... > + union { > + struct { > + u64 assigned : 1, > + pagesize : 1, > + immutable : 1, > + rsvd1 : 9, > + gpa : 39, > + asid : 10, > + vmsa : 1, > + validated : 1, > + rsvd2 : 1; > + } info; > + u64 low; > + }; > + u64 high; > +}; ... here, at the end.
[AMD Official Use Only - General] Hello Boris, >> +static struct rmpentry *__snp_lookup_rmpentry(u64 pfn, int *level) { >> + unsigned long vaddr, paddr = pfn << PAGE_SHIFT; >> + struct rmpentry *entry, *large_entry; >.> + >> + if (!pfn_valid(pfn)) >> + return ERR_PTR(-EINVAL); >> + >> + if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) >> + return ERR_PTR(-ENXIO); >That test should happen first. Ok. >> + vaddr = rmptable_start + rmptable_page_offset(paddr); >Wait, what does that macro do? >It takes the physical address and gives the offset from the beginning of the RMP table in VA space? >So why don't you do > entry = rmptable_entry(paddr) >instead which simply gives you directly the entry in the RMP table with which you can work further? >Instead of this macro doing half the work and then callers having to add the RMP start address and cast. >And make it small function so that you can have typechecking too, while at it. Ok, I will add a new corresponding rmptable_entry() function here, should be usable for getting the large RMP entry below too. >> + if (unlikely(vaddr > rmptable_end)) >> + return ERR_PTR(-ENXIO); >> + >> + 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(paddr & PMD_MASK); >> + large_entry = (struct rmpentry *)vaddr; >> + *level = RMP_TO_X86_PG_LEVEL(rmpentry_pagesize(large_entry)); >> + >> + return entry; >> +} >> + >> +/* >> diff --git a/include/linux/sev.h b/include/linux/sev.h new file mode >> 100644 index 000000000000..1a68842789e1 >> --- /dev/null >> +++ b/include/linux/sev.h >Why is this header in the linux/ namespace and not in arch/x86/ ? >All that stuff in here doesn't have any meaning outside of x86... Yes, makes sense to move it to arch/x86. Thanks, Ashish
[AMD Official Use Only - General] >> I disagree. Running an old kernel on new hardware with a different >> RMP layout should refuse to use SNP, not read/write garbage and likely >> corrupt the RMP and/or host memory. >See my example below. > And IMO, hiding the non-architectural RMP format in SNP-specific code > so that we don't have to churn a bunch of call sites that don't _need_ > access to the raw RMP format is a good idea regardless of whether we > want to be optimistic or pessimistic about future formats. >I don't think I ever objected to that. I agree with what Sean is recommending to do. As I mentioned earlier, in the long term and with respect to future platforms, we are going to add architectural support to read RMP table entries, so this structure will exist only for older platform support. Thanks, Ashish
[AMD Official Use Only - General]
As I mentioned before, in the future we will have architectural support to read RMP table entries, we will first check for
availability of this feature and use it always if it is supported and enabled, and only fallback to doing raw RMP table access
if this architectural support is not available.
Thanks,
Ashish
-----Original Message-----
From: Borislav Petkov <bp@alien8.de>
Sent: Friday, July 22, 2022 2:38 PM
To: Sean Christopherson <seanjc@google.com>
Cc: Kalra, Ashish <Ashish.Kalra@amd.com>; Dave Hansen <dave.hansen@intel.com>; x86@kernel.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org; linux-coco@lists.linux.dev; linux-mm@kvack.org; linux-crypto@vger.kernel.org; tglx@linutronix.de; mingo@redhat.com; jroedel@suse.de; Lendacky, Thomas <Thomas.Lendacky@amd.com>; hpa@zytor.com; ardb@kernel.org; pbonzini@redhat.com; vkuznets@redhat.com; jmattson@google.com; luto@kernel.org; dave.hansen@linux.intel.com; slp@redhat.com; pgonda@google.com; peterz@infradead.org; srinivas.pandruvada@linux.intel.com; rientjes@google.com; dovmurik@linux.ibm.com; tobin@ibm.com; Roth, Michael <Michael.Roth@amd.com>; vbabka@suse.cz; kirill@shutemov.name; ak@linux.intel.com; tony.luck@intel.com; marcorr@google.com; sathyanarayanan.kuppuswamy@linux.intel.com; alpergun@google.com; dgilbert@redhat.com; jarkko@kernel.org
Subject: Re: [PATCH Part2 v6 05/49] x86/sev: Add RMP entry lookup helpers
Btw,
what could work is to spec only a *version* field somewhere in the HW or FW which says which version the RMP header has.
Then, OS would check that field and if it doesn't support that certain version, it'll bail.
I'd need to talk to folks first, though, what the whole story is behind not spec-ing the RMP format...
--
Regards/Gruss,
Boris.
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=05%7C01%7CAshish.Kalra%40amd.com%7Cfc8ed4feddb346bbae8a08da6c19b7d6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637941154968117489%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=khiE7a%2FAW8C%2B0RilZHWxGvMtnlQkDTlC5UtU8Q3L1Lo%3D&reserved=0
[AMD Official Use Only - General] Hello Boris, >> + * The RMP entry format is not architectural. The format is defined in PPR >> + * Family 19h Model 01h, Rev B1 processor. >> + */ >> +struct __packed rmpentry { >That __packed goes... >> + union { >> + struct { >> + u64 assigned : 1, >> + pagesize : 1, >> + immutable : 1, >> + rsvd1 : 9, >> + gpa : 39, >> + asid : 10, >> + vmsa : 1, >> + validated : 1, >> + rsvd2 : 1; >> + } info; >> + u64 low; >> + }; >> + u64 high; >> +}; >... here, at the end. Yes, will fix that. Thanks, Ashish
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index 9c2d33f1cfee..cb16f0e5b585 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> #include <asm/insn.h> #include <asm/sev-common.h> #include <asm/bootparam.h> @@ -84,6 +85,32 @@ extern bool handle_vc_boot_ghcb(struct pt_regs *regs); /* RMP page size */ #define RMP_PG_SIZE_4K 0 +#define RMP_TO_X86_PG_LEVEL(level) (((level) == RMP_PG_SIZE_4K) ? PG_LEVEL_4K : PG_LEVEL_2M) + +/* + * The RMP entry format is not architectural. The format is defined in PPR + * Family 19h Model 01h, Rev B1 processor. + */ +struct __packed rmpentry { + union { + struct { + u64 assigned : 1, + pagesize : 1, + immutable : 1, + rsvd1 : 9, + gpa : 39, + asid : 10, + vmsa : 1, + validated : 1, + rsvd2 : 1; + } info; + u64 low; + }; + u64 high; +}; + +#define rmpentry_assigned(x) ((x)->info.assigned) +#define rmpentry_pagesize(x) ((x)->info.pagesize) #define RMPADJUST_VMSA_PAGE_BIT BIT(16) diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index 25c7feb367f6..59e7ec6b0326 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -65,6 +65,8 @@ * bookkeeping, the range need to be added during the RMP entry lookup. */ #define RMPTABLE_CPU_BOOKKEEPING_SZ 0x4000 +#define RMPENTRY_SHIFT 8 +#define rmptable_page_offset(x) (RMPTABLE_CPU_BOOKKEEPING_SZ + (((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); @@ -2386,3 +2388,44 @@ static int __init snp_rmptable_init(void) * available after subsys_initcall(). */ fs_initcall(snp_rmptable_init); + +static struct rmpentry *__snp_lookup_rmpentry(u64 pfn, int *level) +{ + unsigned long vaddr, paddr = pfn << PAGE_SHIFT; + struct rmpentry *entry, *large_entry; + + if (!pfn_valid(pfn)) + return ERR_PTR(-EINVAL); + + if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + return ERR_PTR(-ENXIO); + + vaddr = rmptable_start + rmptable_page_offset(paddr); + if (unlikely(vaddr > rmptable_end)) + return ERR_PTR(-ENXIO); + + 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(paddr & PMD_MASK); + large_entry = (struct rmpentry *)vaddr; + *level = RMP_TO_X86_PG_LEVEL(rmpentry_pagesize(large_entry)); + + return entry; +} + +/* + * Return 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(u64 pfn, int *level) +{ + struct rmpentry *e; + + e = __snp_lookup_rmpentry(pfn, level); + if (IS_ERR(e)) + return PTR_ERR(e); + + return !!rmpentry_assigned(e); +} +EXPORT_SYMBOL_GPL(snp_lookup_rmpentry); diff --git a/include/linux/sev.h b/include/linux/sev.h new file mode 100644 index 000000000000..1a68842789e1 --- /dev/null +++ b/include/linux/sev.h @@ -0,0 +1,30 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * AMD Secure Encrypted Virtualization + * + * Author: Brijesh Singh <brijesh.singh@amd.com> + */ + +#ifndef __LINUX_SEV_H +#define __LINUX_SEV_H + +/* RMUPDATE detected 4K page and 2MB page overlap. */ +#define RMPUPDATE_FAIL_OVERLAP 7 + +#ifdef CONFIG_AMD_MEM_ENCRYPT +int snp_lookup_rmpentry(u64 pfn, int *level); +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); +#else +static inline int snp_lookup_rmpentry(u64 pfn, int *level) { return 0; } +static inline int psmash(u64 pfn) { return -ENXIO; } +static inline int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, int asid, + bool immutable) +{ + return -ENODEV; +} +static inline int rmp_make_shared(u64 pfn, enum pg_level level) { return -ENODEV; } + +#endif /* CONFIG_AMD_MEM_ENCRYPT */ +#endif /* __LINUX_SEV_H */