| Message ID | 20240126041126.1927228-1-michael.roth@amd.com |
|---|---|
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Initialization Support | expand |
On Fri, Jan 26, 2024 at 04:34:51PM +0100, Borislav Petkov wrote: > On Thu, Jan 25, 2024 at 10:11:11PM -0600, Michael Roth wrote: > > +static int adjust_direct_map(u64 pfn, int rmp_level) > > +{ > > + unsigned long vaddr = (unsigned long)pfn_to_kaddr(pfn); > > + unsigned int level; > > + int npages, ret; > > + pte_t *pte; > > Again, something I asked the last time but no reply: > > Looking at Documentation/arch/x86/x86_64/mm.rst, the direct map starts > at page_offset_base so this here should at least check > > if (vaddr < __PAGE_OFFSET) > return 0; vaddr comes from pfn_to_kaddr(pfn), i.e. __va(paddr), so it will necessarily be a direct-mapped address above __PAGE_OFFSET. > > I'm not sure about the upper end. Right now, the adjusting should not For upper-end, a pfn_valid(pfn) check might suffice, since only a valid PFN would have a possibly-valid mapping wthin the directmap range. > happen only for the direct map but also for the whole kernel address > space range because we don't want to cause any mismatch between page > mappings anywhere. > > Which means, this function should be called adjust_kernel_map() or so... These are PFNs that are owned/allocated-to the caller. Due to the nature of the directmap it's possible non-owners would write to a mapping that overlaps, but vmalloc()/etc. would not create mappings for any pages that were not specifically part of an allocation that belongs to the caller, so I don't see where there's any chance for an overlap there. And the caller of these functions would not be adjusting directmap for PFNs that might be mapped into other kernel address ranges like kernel-text/etc unless the caller was specifically making SNP-aware adjustments to those ranges, in which case it would be responsible for making those other adjustments, or implementing the necessary helpers/etc. I'm not aware of such cases in the current code, and I don't think it makes sense to attempt to try to handle them here generically until such a case arises, since it will likely involve more specific requirements than what we can anticipate from a theoretical/generic standpoint. -Mike > > Hmmm. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Jan 26, 2024 at 11:04:15AM -0600, Michael Roth wrote: > vaddr comes from pfn_to_kaddr(pfn), i.e. __va(paddr), so it will > necessarily be a direct-mapped address above __PAGE_OFFSET. Ah, true. > For upper-end, a pfn_valid(pfn) check might suffice, since only a valid > PFN would have a possibly-valid mapping wthin the directmap range. Looking at it, yap, that could be a sensible thing to check. > These are PFNs that are owned/allocated-to the caller. Due to the nature > of the directmap it's possible non-owners would write to a mapping that > overlaps, but vmalloc()/etc. would not create mappings for any pages that > were not specifically part of an allocation that belongs to the caller, > so I don't see where there's any chance for an overlap there. And the caller > of these functions would not be adjusting directmap for PFNs that might be > mapped into other kernel address ranges like kernel-text/etc unless the > caller was specifically making SNP-aware adjustments to those ranges, in > which case it would be responsible for making those other adjustments, > or implementing the necessary helpers/etc. Why does any of that matter? If you can make this helper as generic as possible now, why don't you? > I'm not aware of such cases in the current code, and I don't think it makes > sense to attempt to try to handle them here generically until such a case > arises, since it will likely involve more specific requirements than what > we can anticipate from a theoretical/generic standpoint. Then that's a different story. If it will likely involve more specific handling, then that function should deal with pfns for which it can DTRT and for others warn loudly so that the code gets fixed in time. IOW, then it should check for the upper pfn of the direct map here and we have two, depending on the page sizes used... Thx.
On Fri, Jan 26, 2024 at 07:43:40PM +0100, Borislav Petkov wrote: > On Fri, Jan 26, 2024 at 11:04:15AM -0600, Michael Roth wrote: > > vaddr comes from pfn_to_kaddr(pfn), i.e. __va(paddr), so it will > > necessarily be a direct-mapped address above __PAGE_OFFSET. > > Ah, true. > > > For upper-end, a pfn_valid(pfn) check might suffice, since only a valid > > PFN would have a possibly-valid mapping wthin the directmap range. > > Looking at it, yap, that could be a sensible thing to check. > > > These are PFNs that are owned/allocated-to the caller. Due to the nature > > of the directmap it's possible non-owners would write to a mapping that > > overlaps, but vmalloc()/etc. would not create mappings for any pages that > > were not specifically part of an allocation that belongs to the caller, > > so I don't see where there's any chance for an overlap there. And the caller > > of these functions would not be adjusting directmap for PFNs that might be > > mapped into other kernel address ranges like kernel-text/etc unless the > > caller was specifically making SNP-aware adjustments to those ranges, in > > which case it would be responsible for making those other adjustments, > > or implementing the necessary helpers/etc. > > Why does any of that matter? > > If you can make this helper as generic as possible now, why don't you? In this case, it would make it more difficult to handle things efficiently and implement proper bounds-checking/etc. For instance, if the caller *knows* they are doing something different like splitting a kernel-text mapping, then we could implement proper bounds-checking based on expected ranges, and implement any special handling associated with that use-case, and capture that in a nice/understandable adjust_kernel_text_mapping() helper. Or maybe if these are adjustments for non-static/non-linear mappings, it makes more sense to be given an HVA rather than PFN, etc., since we might not have any sort of reverse-map structure/function than can be used to do the PFN->HVA lookups efficiently. It's just a lot to guess at. And just the directmap splitting itself has been the source of so much discussion, investigation, re-work, benchmarking, etc., that it hints that implementing similar handling for other use-cases really needs to have a clear and necessary purpose and set of requirements hat can be evaluated/tested before enabling them and reasonably expecting them to work as expected. > > > I'm not aware of such cases in the current code, and I don't think it makes > > sense to attempt to try to handle them here generically until such a case > > arises, since it will likely involve more specific requirements than what > > we can anticipate from a theoretical/generic standpoint. > > Then that's a different story. If it will likely involve more specific > handling, then that function should deal with pfns for which it can DTRT > and for others warn loudly so that the code gets fixed in time. > > IOW, then it should check for the upper pfn of the direct map here and > we have two, depending on the page sizes used... Is something like this close to what you're thinking? I've re-tested with SNP guests and it seems to work as expected. diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c index 846e9e53dff0..c09497487c08 100644 --- a/arch/x86/virt/svm/sev.c +++ b/arch/x86/virt/svm/sev.c @@ -421,7 +421,12 @@ static int adjust_direct_map(u64 pfn, int rmp_level) if (WARN_ON_ONCE(rmp_level > PG_LEVEL_2M)) return -EINVAL; - if (WARN_ON_ONCE(rmp_level == PG_LEVEL_2M && !IS_ALIGNED(pfn, PTRS_PER_PMD))) + if (!pfn_valid(pfn)) + return -EINVAL; + + if (rmp_level == PG_LEVEL_2M && + (!IS_ALIGNED(pfn, PTRS_PER_PMD) || + !pfn_valid(pfn + PTRS_PER_PMD - 1))) return -EINVAL; Note that I removed the WARN_ON_ONCE(), which I think was a bit overzealous for this check. The one above it for rmp_level > PG_LEVEL_2M I think is more warranted, since it returns error for a use-case that might in theory become valid on future hardware, but would result in unecessary 1GB->2MB directmap splitting if we were to try to implement handling for that before it's actually needed (which may well be never). -Mike > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Jan 26, 2024 at 05:54:20PM -0600, Michael Roth wrote: > Is something like this close to what you're thinking? I've re-tested with > SNP guests and it seems to work as expected. > > diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c > index 846e9e53dff0..c09497487c08 100644 > --- a/arch/x86/virt/svm/sev.c > +++ b/arch/x86/virt/svm/sev.c > @@ -421,7 +421,12 @@ static int adjust_direct_map(u64 pfn, int rmp_level) > if (WARN_ON_ONCE(rmp_level > PG_LEVEL_2M)) > return -EINVAL; > > - if (WARN_ON_ONCE(rmp_level == PG_LEVEL_2M && !IS_ALIGNED(pfn, PTRS_PER_PMD))) > + if (!pfn_valid(pfn)) _text at VA 0xffffffff81000000 is also a valid pfn so no, this is not enough. Either this function should not have "direct map" in the name as it converts *any* valid pfn not just the direct map ones or it should check whether the pfn belongs to the direct map range. Thx.
On Sat, Jan 27, 2024 at 12:42:07PM +0100, Borislav Petkov wrote: > On Fri, Jan 26, 2024 at 05:54:20PM -0600, Michael Roth wrote: > > Is something like this close to what you're thinking? I've re-tested with > > SNP guests and it seems to work as expected. > > > > diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c > > index 846e9e53dff0..c09497487c08 100644 > > --- a/arch/x86/virt/svm/sev.c > > +++ b/arch/x86/virt/svm/sev.c > > @@ -421,7 +421,12 @@ static int adjust_direct_map(u64 pfn, int rmp_level) > > if (WARN_ON_ONCE(rmp_level > PG_LEVEL_2M)) > > return -EINVAL; > > > > - if (WARN_ON_ONCE(rmp_level == PG_LEVEL_2M && !IS_ALIGNED(pfn, PTRS_PER_PMD))) > > + if (!pfn_valid(pfn)) > > _text at VA 0xffffffff81000000 is also a valid pfn so no, this is not > enough. directmap maps all physical memory accessible by kernel, including text pages, so those are valid PFNs as far as this function is concerned. We can't generally guard against the caller passing in any random PFN that might also be mapped into additional address ranges, similarly to how we can't guard against something doing a write to some random PFN __va(0x1234) and scribbling over memory that it doesn't own, or just unmapping the entire directmap range and blowing up the kernel. The expectation is that the caller is aware of what PFNs it is passing in, whether those PFNs have additional mappings, and if those mappings are of concern, implement the necessary handlers if new use-cases are ever introducted, like the adjust_kernel_text_mapping() example I mentioned earlier. > > Either this function should not have "direct map" in the name as it > converts *any* valid pfn not just the direct map ones or it should check > whether the pfn belongs to the direct map range. This function only splits mappings in the 0xffff888000000000 directmap range. It would be inaccurate to name it in such a way that suggests that it does anything else. If a use-case ever arises for splitting _text mappings at 0xffffffff81000000, or any other ranges, those too would best served by dedicated helpers adjust_kernel_text_mapping() that *actually* modify mappings for those virtual ranges, and implement bounds-checking appropriate for those physical/virtual ranges. The directmap range maps all kernel-accessible physical memory, so it's appropriate that our bounds-checking for the purpose of adjust_direct_map() is all kernel-accessible physical memory. If that includes PFNs mapped to other virtual ranges as well, the caller needs to consider that and implement additional helpers as necessary, but they'd likely *still* need to call adjust_direct_map() to adjust the directmap range in addition to those other ranges, so even if were possible to do so reliably, we shouldn't try to selectively reject any PFN ranges beyond what mm.rst suggests is valid for 0xffff888000000000. -Mike > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette >
On Sat, Jan 27, 2024 at 09:45:06AM -0600, Michael Roth wrote: > directmap maps all physical memory accessible by kernel, including text > pages, so those are valid PFNs as far as this function is concerned. Why don't you have a look at Documentation/arch/x86/x86_64/mm.rst to sync up on the nomenclature first? ffff888000000000 | -119.5 TB | ffffc87fffffffff | 64 TB | direct mapping of all physical memory (page_offset_base) ... ffffffff80000000 | -2 GB | ffffffff9fffffff | 512 MB | kernel text mapping, mapped to physical address 0 and so on. > The expectation is that the caller is aware of what PFNs it is passing in, There are no expectations. Have you written them down somewhere? > This function only splits mappings in the 0xffff888000000000 directmap > range. This function takes any PFN it gets passed in as it is. I don't care who its users are now or in the future and whether they pay attention what they pass into - it needs to be properly defined. Mike, please get on with the program. Use the right naming for the function and basta. IOW, this: diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c index 0a8f9334ec6e..652ee63e87fd 100644 --- a/arch/x86/virt/svm/sev.c +++ b/arch/x86/virt/svm/sev.c @@ -394,7 +394,7 @@ EXPORT_SYMBOL_GPL(psmash); * More specifics on how these checks are carried out can be found in APM * Volume 2, "RMP and VMPL Access Checks". */ -static int adjust_direct_map(u64 pfn, int rmp_level) +static int split_pfn(u64 pfn, int rmp_level) { unsigned long vaddr = (unsigned long)pfn_to_kaddr(pfn); unsigned int level; @@ -405,7 +405,12 @@ static int adjust_direct_map(u64 pfn, int rmp_level) if (WARN_ON_ONCE(rmp_level > PG_LEVEL_2M)) return -EINVAL; - if (WARN_ON_ONCE(rmp_level == PG_LEVEL_2M && !IS_ALIGNED(pfn, PTRS_PER_PMD))) + if (!pfn_valid(pfn)) + return -EINVAL; + + if (rmp_level == PG_LEVEL_2M && + (!IS_ALIGNED(pfn, PTRS_PER_PMD) || + !pfn_valid(pfn + PTRS_PER_PMD - 1))) return -EINVAL; /* @@ -456,7 +461,7 @@ static int rmpupdate(u64 pfn, struct rmp_state *state) level = RMP_TO_PG_LEVEL(state->pagesize); - if (adjust_direct_map(pfn, level)) + if (split_pfn(pfn, level)) return -EFAULT; do {
On Sat, Jan 27, 2024 at 05:02:49PM +0100, Borislav Petkov wrote: > This function takes any PFN it gets passed in as it is. I don't care > who its users are now or in the future and whether they pay attention > what they pass into - it needs to be properly defined. Ok, we solved it offlist, here's the final version I have. It has a comment explaining what I was asking. --- From: Michael Roth <michael.roth@amd.com> Date: Thu, 25 Jan 2024 22:11:11 -0600 Subject: [PATCH] x86/sev: Adjust the directmap to avoid inadvertent RMP faults If the kernel uses a 2MB or larger directmap mapping to write to an address, and that mapping contains any 4KB pages that are set to private in the RMP table, an RMP #PF will trigger and cause a host crash. SNP-aware code that owns the private PFNs will never attempt such a write, but other kernel tasks writing to other PFNs in the range may trigger these checks inadvertently due to writing to those other PFNs via a large directmap mapping that happens to also map a private PFN. Prevent this by splitting any 2MB+ mappings that might end up containing a mix of private/shared PFNs as a result of a subsequent RMPUPDATE for the PFN/rmp_level passed in. Another way to handle this would be to limit the directmap to 4K mappings in the case of hosts that support SNP, but there is potential risk for performance regressions of certain host workloads. Handling it as-needed results in the directmap being slowly split over time, which lessens the risk of a performance regression since the more the directmap gets split as a result of running SNP guests, the more likely the host is being used primarily to run SNP guests, where a mostly-split directmap is actually beneficial since there is less chance of TLB flushing and cpa_lock contention being needed to perform these splits. Cases where a host knows in advance it wants to primarily run SNP guests and wishes to pre-split the directmap can be handled by adding a tuneable in the future, but preliminary testing has shown this to not provide a signficant benefit in the common case of guests that are backed primarily by 2MB THPs, so it does not seem to be warranted currently and can be added later if a need arises in the future. Signed-off-by: Michael Roth <michael.roth@amd.com> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> Link: https://lore.kernel.org/r/20240126041126.1927228-12-michael.roth@amd.com --- arch/x86/virt/svm/sev.c | 86 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 84 insertions(+), 2 deletions(-) diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c index c0b4c2306e8d..8da9c5330ff0 100644 --- a/arch/x86/virt/svm/sev.c +++ b/arch/x86/virt/svm/sev.c @@ -368,6 +368,82 @@ int psmash(u64 pfn) } EXPORT_SYMBOL_GPL(psmash); +/* + * If the kernel uses a 2MB or larger directmap mapping to write to an address, + * and that mapping contains any 4KB pages that are set to private in the RMP + * table, an RMP #PF will trigger and cause a host crash. Hypervisor code that + * owns the PFNs being transitioned will never attempt such a write, but other + * kernel tasks writing to other PFNs in the range may trigger these checks + * inadvertently due a large directmap mapping that happens to overlap such a + * PFN. + * + * Prevent this by splitting any 2MB+ mappings that might end up containing a + * mix of private/shared PFNs as a result of a subsequent RMPUPDATE for the + * PFN/rmp_level passed in. + * + * Note that there is no attempt here to scan all the RMP entries for the 2MB + * physical range, since it would only be worthwhile in determining if a + * subsequent RMPUPDATE for a 4KB PFN would result in all the entries being of + * the same shared/private state, thus avoiding the need to split the mapping. + * But that would mean the entries are currently in a mixed state, and so the + * mapping would have already been split as a result of prior transitions. + * And since the 4K split is only done if the mapping is 2MB+, and there isn't + * currently a mechanism in place to restore 2MB+ mappings, such a check would + * not provide any usable benefit. + * + * More specifics on how these checks are carried out can be found in APM + * Volume 2, "RMP and VMPL Access Checks". + */ +static int adjust_direct_map(u64 pfn, int rmp_level) +{ + unsigned long vaddr; + unsigned int level; + int npages, ret; + pte_t *pte; + + /* + * pfn_to_kaddr() will return a vaddr only within the direct + * map range. + */ + vaddr = (unsigned long)pfn_to_kaddr(pfn); + + /* Only 4KB/2MB RMP entries are supported by current hardware. */ + if (WARN_ON_ONCE(rmp_level > PG_LEVEL_2M)) + return -EINVAL; + + if (!pfn_valid(pfn)) + return -EINVAL; + + if (rmp_level == PG_LEVEL_2M && + (!IS_ALIGNED(pfn, PTRS_PER_PMD) || + !pfn_valid(pfn + PTRS_PER_PMD - 1))) + return -EINVAL; + + /* + * If an entire 2MB physical range is being transitioned, then there is + * no risk of RMP #PFs due to write accesses from overlapping mappings, + * since even accesses from 1GB mappings will be treated as 2MB accesses + * as far as RMP table checks are concerned. + */ + if (rmp_level == PG_LEVEL_2M) + return 0; + + pte = lookup_address(vaddr, &level); + if (!pte || pte_none(*pte)) + return 0; + + if (level == PG_LEVEL_4K) + return 0; + + npages = page_level_size(rmp_level) / PAGE_SIZE; + ret = set_memory_4k(vaddr, npages); + if (ret) + pr_warn("Failed to split direct map for PFN 0x%llx, ret: %d\n", + pfn, ret); + + return ret; +} + /* * It is expected that those operations are seldom enough so that no mutual * exclusion of updaters is needed and thus the overlap error condition below @@ -384,11 +460,16 @@ EXPORT_SYMBOL_GPL(psmash); static int rmpupdate(u64 pfn, struct rmp_state *state) { unsigned long paddr = pfn << PAGE_SHIFT; - int ret; + int ret, level; if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) return -ENODEV; + level = RMP_TO_PG_LEVEL(state->pagesize); + + if (adjust_direct_map(pfn, level)) + return -EFAULT; + do { /* Binutils version 2.36 supports the RMPUPDATE mnemonic. */ asm volatile(".byte 0xF2, 0x0F, 0x01, 0xFE" @@ -398,7 +479,8 @@ static int rmpupdate(u64 pfn, struct rmp_state *state) } while (ret == RMPUPDATE_FAIL_OVERLAP); if (ret) { - pr_err("RMPUPDATE failed for PFN %llx, ret: %d\n", pfn, ret); + pr_err("RMPUPDATE failed for PFN %llx, pg_level: %d, ret: %d\n", + pfn, level, ret); dump_rmpentry(pfn); dump_stack(); return -EFAULT;
On 1/29/24 12:59, Borislav Petkov wrote: > On Sat, Jan 27, 2024 at 05:02:49PM +0100, Borislav Petkov wrote: >> This function takes any PFN it gets passed in as it is. I don't care >> who its users are now or in the future and whether they pay attention >> what they pass into - it needs to be properly defined. > > Ok, we solved it offlist, here's the final version I have. It has > a comment explaining what I was asking. > > --- > From: Michael Roth <michael.roth@amd.com> > Date: Thu, 25 Jan 2024 22:11:11 -0600 > Subject: [PATCH] x86/sev: Adjust the directmap to avoid inadvertent RMP faults > > If the kernel uses a 2MB or larger directmap mapping to write to an > address, and that mapping contains any 4KB pages that are set to private > in the RMP table, an RMP #PF will trigger and cause a host crash. > > SNP-aware code that owns the private PFNs will never attempt such > a write, but other kernel tasks writing to other PFNs in the range may > trigger these checks inadvertently due to writing to those other PFNs > via a large directmap mapping that happens to also map a private PFN. > > Prevent this by splitting any 2MB+ mappings that might end up containing > a mix of private/shared PFNs as a result of a subsequent RMPUPDATE for > the PFN/rmp_level passed in. > > Another way to handle this would be to limit the directmap to 4K > mappings in the case of hosts that support SNP, but there is potential > risk for performance regressions of certain host workloads. > > Handling it as-needed results in the directmap being slowly split over > time, which lessens the risk of a performance regression since the more > the directmap gets split as a result of running SNP guests, the more > likely the host is being used primarily to run SNP guests, where > a mostly-split directmap is actually beneficial since there is less > chance of TLB flushing and cpa_lock contention being needed to perform > these splits. > > Cases where a host knows in advance it wants to primarily run SNP guests > and wishes to pre-split the directmap can be handled by adding > a tuneable in the future, but preliminary testing has shown this to not > provide a signficant benefit in the common case of guests that are > backed primarily by 2MB THPs, so it does not seem to be warranted > currently and can be added later if a need arises in the future. > > Signed-off-by: Michael Roth <michael.roth@amd.com> > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > Link: https://lore.kernel.org/r/20240126041126.1927228-12-michael.roth@amd.com Acked-by: Vlastimil Babka <vbabka@suse.cz> Thanks!
On Thu, Jan 25, 2024 at 10:11:00PM -0600, Michael Roth wrote: > This patchset is also available at: > > https://github.com/amdese/linux/commits/snp-host-init-v2 > > and is based on top of linux-next tag next-20240125 Ok, I've queued this now after some testing. As of now https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=x86/sev is frozen and fixes should come ontop so that Paolo/Sean can use that branch to base the remaining SNP host stuff ontop. Thx.
This patchset is also available at: https://github.com/amdese/linux/commits/snp-host-init-v2 and is based on top of linux-next tag next-20240125 These patches were originally included in v10 of the SNP KVM/hypervisor patches[1], but have been split off from the general KVM support for easier review and eventual merging into the x86 tree. They are based on linux-next to help stay in sync with both tip and kvm-next. There is 1 KVM-specific patch here since it is needed to avoid regressions when running legacy SEV guests while the RMP table is enabled. == OVERVIEW == AMD EPYC systems utilizing Zen 3 and newer microarchitectures add support for a new feature called SEV-SNP, which adds Secure Nested Paging support on top of the SEV/SEV-ES support already present on existing EPYC systems. One of the main features of SNP is the addition of an RMP (Reverse Map) table to enforce additional security protections for private guest memory. This series primarily focuses on the various host initialization requirements for enabling SNP on the system, while the actual KVM support for running SNP guests is added as a separate series based on top of these patches. The basic requirements to initialize SNP support on a host when the feature has been enabled in the BIOS are: - Discovering and initializing the RMP table - Initializing various MSRs to enable the capability across CPUs - Various tasks to maintain legacy functionality on the system, such as: - Setting up hooks for handling RMP-related changes for IOMMU pages - Initializing SNP in the firmware via the CCP driver, and implement additional requirements needed for continued operation of legacy SEV/SEV-ES guests Additionally some basic SEV ioctl interfaces are added to configure various aspects of SNP-enabled firmwares via the CCP driver. More details are available in the SEV-SNP Firmware ABI[2]. Feedback/review is very much appreciated! -Mike [1] https://lore.kernel.org/kvm/20231016132819.1002933-1-michael.roth@amd.com/ [2] https://www.amd.com/en/developer/sev.html Changes since v1: * rebased on linux-next tag next-202401125 * patch 26: crypto: ccp: Add the SNP_SET_CONFIG command - documentation updates (Boris) * patch 25: crypto: ccp: Add the SNP_COMMIT command - s/len/length/ in SNP_COMMIT kernel-doc (Boris) - s/length/len/ in struct for consistency with other cmds/structs * patch 24: crypto: ccp: Add the SNP_PLATFORM_STATUS command - add comments regarding the need for page reclaim of status page (Boris) * patch 21: crypto: ccp: Add panic notifier for SEV/SNP firmware shutdown on kdump - enable crash_kexec_post_notifiers by default for SNP (Ashish) - squash in refactorings from Boris, but keep wbinvd_on_all_cpus() so that non-SNP/non-panic case is still handled as it was prior to SNP. * patch 20: crypto: ccp: Add debug support for decrypting pages - not utilized in current code, dropped it for now (Sean) * patch 18: crypto: ccp: Handle legacy SEV commands when SNP is enabled - drop uneeded index variables used when rolling back descriptor mappings when failure occurs (Boris) - error message fixups (Boris) - fix indentation to align each argument description line to same column as recommended by kernel-doc documentation - attempt to unmap/reclaim all descriptors even if a previous unmap/reclaim failed - s/restored to/restored to hypervisor-owned/ in commit message - fix reclaim clobbering return value when a command failure occurs - reclaim cmd buffers and descriptors separately so that active cmd buffers can be marked !inuse even when reclaim for a descriptor fails * patch 17: crypto: ccp: Handle non-volatile INIT_EX data when SNP is enabled - drop usage of vmap() to access INIT_EX non-volatile data buffer, no longer needed now that rmpupdate() splits directmap rather than removes mappings * patch 15: x86/sev: Introduce snp leaked pages list - add Suggested-by: Vlastimil Babka <vbabka@suse.cz> - avoid the need to allocate memory when leaking unreclaimable pages (Vlastimil, Ashish) - use pr_warn() instead of pr_debug() when leaking (Sean) * patch 14: crypto: ccp: Provide API to issue SEV and SNP commands - s/SEV/SEV device/ in kernel-doc for sev_do_cmd() (Boris) * patch 13: crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP - account for non-page-aligned HV-Fixed ranges when calling SNP_INIT (Ashish) - various comment/commit fixups (Boris) * patch 12: crypto: ccp: Define the SEV-SNP commands - fix alignment of SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX enum - add "Incoming Migration Image" in all places that mention of IMI for purposes of documentation (Boris) - fix indentation to align each argument description line to same column in accordance with Documentation/doc-guide/kernel-doc.rst - rename sev_data_snp_addr.paddr_gctx to more appropriate sev_data_snp_addr.address, fix up affected patches - len/length in kernel-doc descriptions * patch 11: x86/sev: Invalidate pages from the direct map when adding them to the RMP table - replace old patch/handling with an implementation based on set_memory_4k() that splits directmap as-needed rather than adding/removing mappings in response to shared/private conversions. - lookup/confirm the mappings in advance of splitting, and avoid unecessary splits for 2M private ranges to reduce unecessary contention / TLB flushing (Dave, Boris, Tom, Mike, Vlastimil) * patch 10: x86/sev: Add helper functions for RMPUPDATE and PSMASH instruction - add additional comments clarifying RMPUPDATE retry logic (Boris) * patch 07: x86/fault: Add helper for dumping RMP entries - fix dump logic for 2MB+ pages (Tom) - dump full 2MB range if particular 4K RMP entry is not populated (Boris) - drop EXPORT_SYMBOL_GPL() until actually needed by a module (Boris) * patch 04: x86/sev: Add the host SEV-SNP initialization support - macro alignment fixups (Boris) - commit message cleanups (Boris) - work in Zen5 patch (Boris) - squash __snp_init_rmptable in helper back in (Boris) * patch 03: iommu/amd: Don't rely on external callers to enable IOMMU SNP support - add Joerg's acked-by - error message cleanups (Boris) Changes since being split off from v10 hypervisor patches: * Move all host initialization patches to beginning of overall series and post as a separate patchset. Include only KVM patches that are necessary for maintaining legacy SVM/SEV functionality with SNP enabled. (Paolo, Boris) * Don't enable X86_FEATURE_SEV_SNP until all patches are in place to maintain legacy SVM/SEV/SEV-ES functionality when RMP table and SNP firmware support are enabled. (Paolo) * Re-write how firmware-owned buffers are handled when dealing with legacy SEV commands. Allocate on-demand rather than relying on pre-allocated pool, use a descriptor format for handling nested/pointer params instead of relying on macros. (Boris) * Don't introduce sev-host.h, re-use sev.h (Boris) * Various renames, cleanups, refactorings throughout the tree (Boris) * Rework leaked pages handling (Vlastimil) * Fix kernel-doc errors introduced by series (Boris) * Fix warnings when onlining/offlining CPUs (Jeremi, Ashish) * Ensure X86_FEATURE_SEV_SNP is cleared early enough that AutoIBRS will still be enabled if RMP table support is not available/configured. (Tom) * Only read the RMP base/end MSR values once via BSP (Boris) * Handle IOMMU SNP setup automatically based on state machine rather than via external caller (Boris) ---------------------------------------------------------------- Ashish Kalra (5): iommu/amd: Don't rely on external callers to enable IOMMU SNP support x86/mtrr: Don't print errors if MtrrFixDramModEn is set when SNP enabled x86/sev: Introduce snp leaked pages list iommu/amd: Clean up RMP entries for IOMMU pages during SNP shutdown crypto: ccp: Add panic notifier for SEV/SNP firmware shutdown on kdump Brijesh Singh (14): x86/cpufeatures: Add SEV-SNP CPU feature x86/sev: Add the host SEV-SNP initialization support x86/sev: Add RMP entry lookup helpers x86/fault: Add helper for dumping RMP entries x86/traps: Define RMP violation #PF error code x86/sev: Add helper functions for RMPUPDATE and PSMASH instruction crypto: ccp: Define the SEV-SNP commands crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP crypto: ccp: Provide API to issue SEV and SNP commands crypto: ccp: Handle the legacy TMR allocation when SNP is enabled crypto: ccp: Handle legacy SEV commands when SNP is enabled KVM: SEV: Make AVIC backing, VMSA and VMCB memory allocation SNP safe crypto: ccp: Add the SNP_PLATFORM_STATUS command crypto: ccp: Add the SNP_SET_CONFIG command Kim Phillips (1): x86/speculation: Do not enable Automatic IBRS if SEV SNP is enabled Michael Roth (3): x86/fault: Dump RMP table information when RMP page faults occur x86/sev: Adjust directmap to avoid inadvertant RMP faults x86/cpufeatures: Enable/unmask SEV-SNP CPU feature Tom Lendacky (2): crypto: ccp: Handle non-volatile INIT_EX data when SNP is enabled crypto: ccp: Add the SNP_COMMIT command Documentation/virt/coco/sev-guest.rst | 51 ++ arch/x86/Kbuild | 2 + arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/disabled-features.h | 8 +- arch/x86/include/asm/iommu.h | 1 + arch/x86/include/asm/kvm-x86-ops.h | 1 + arch/x86/include/asm/kvm_host.h | 1 + arch/x86/include/asm/msr-index.h | 11 +- arch/x86/include/asm/sev.h | 38 + arch/x86/include/asm/trap_pf.h | 20 +- arch/x86/kernel/cpu/amd.c | 21 +- arch/x86/kernel/cpu/common.c | 7 +- arch/x86/kernel/cpu/mtrr/generic.c | 3 + arch/x86/kernel/crash.c | 3 + arch/x86/kernel/sev.c | 10 + arch/x86/kvm/lapic.c | 5 +- arch/x86/kvm/svm/nested.c | 2 +- arch/x86/kvm/svm/sev.c | 37 +- arch/x86/kvm/svm/svm.c | 17 +- arch/x86/kvm/svm/svm.h | 1 + arch/x86/mm/fault.c | 5 + arch/x86/virt/svm/Makefile | 3 + arch/x86/virt/svm/sev.c | 547 +++++++++++++++ drivers/crypto/ccp/sev-dev.c | 1122 ++++++++++++++++++++++++++++-- drivers/crypto/ccp/sev-dev.h | 5 + drivers/iommu/amd/amd_iommu.h | 1 - drivers/iommu/amd/init.c | 120 +++- include/linux/amd-iommu.h | 6 +- include/linux/psp-sev.h | 319 ++++++++- include/uapi/linux/psp-sev.h | 59 ++ tools/arch/x86/include/asm/cpufeatures.h | 1 + 31 files changed, 2303 insertions(+), 125 deletions(-) create mode 100644 arch/x86/virt/svm/Makefile create mode 100644 arch/x86/virt/svm/sev.c