| Message ID | 20210707183616.5620-16-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support | expand |
On Wed, Jul 7, 2021 at 11:37 AM Brijesh Singh <brijesh.singh@amd.com> wrote: > > The behavior and requirement for the SEV-legacy command is altered when > the SNP firmware is in the INIT state. See SEV-SNP firmware specification > for more details. > > When SNP is INIT state, all the SEV-legacy commands that cause the > firmware to write memory must be in the firmware state. The TMR memory > is allocated by the host but updated by the firmware, so, it must be > in the firmware state. Additionally, the TMR memory must be a 2MB aligned > instead of the 1MB, and the TMR length need to be 2MB instead of 1MB. > The helper __snp_{alloc,free}_firmware_pages() can be used for allocating > and freeing the memory used by the firmware. > > While at it, provide API that can be used by others to allocate a page > that can be used by the firmware. The immediate user for this API will > be the KVM driver. The KVM driver to need to allocate a firmware context > page during the guest creation. The context page need to be updated > by the firmware. See the SEV-SNP specification for further details. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > drivers/crypto/ccp/sev-dev.c | 144 +++++++++++++++++++++++++++++++---- > include/linux/psp-sev.h | 11 +++ > 2 files changed, 142 insertions(+), 13 deletions(-) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index ad9a0c8111e0..bb07c68834a6 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -54,6 +54,14 @@ static int psp_timeout; > #define SEV_ES_TMR_SIZE (1024 * 1024) > static void *sev_es_tmr; > > +/* When SEV-SNP is enabled the TMR need to be 2MB aligned and 2MB size. */ nit: "the TMR need" -> "the TMR needs" > +#define SEV_SNP_ES_TMR_SIZE (2 * 1024 * 1024) > + > +static size_t sev_es_tmr_size = SEV_ES_TMR_SIZE; > + > +static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret); > +static int sev_do_cmd(int cmd, void *data, int *psp_ret); > + > static inline bool sev_version_greater_or_equal(u8 maj, u8 min) > { > struct sev_device *sev = psp_master->sev_data; > @@ -151,6 +159,112 @@ static int sev_cmd_buffer_len(int cmd) > return 0; > } > > +static int snp_reclaim_page(struct page *page, bool locked) > +{ > + struct sev_data_snp_page_reclaim data = {}; Hmmm.. according to some things I read online, an empty initializer list is not legal in C. For example: https://stackoverflow.com/questions/17589533/is-an-empty-initializer-list-valid-c-code I'm sure this is compiling. Should we change this to `{0}`, which I believe will initialize all fields in this struct to zero, according to: https://stackoverflow.com/questions/11152160/initializing-a-struct-to-0? > + int ret, err; > + > + data.paddr = page_to_pfn(page) << PAGE_SHIFT; > + > + if (locked) > + ret = __sev_do_cmd_locked(SEV_CMD_SNP_PAGE_RECLAIM, &data, &err); > + else > + ret = sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, &data, &err); > + > + return ret; > +} > + > +static int snp_set_rmptable_state(unsigned long paddr, int npages, > + struct rmpupdate *val, bool locked, bool need_reclaim) > +{ > + unsigned long pfn = __sme_clr(paddr) >> PAGE_SHIFT; > + unsigned long pfn_end = pfn + npages; > + struct psp_device *psp = psp_master; > + struct sev_device *sev; > + int rc; > + > + if (!psp || !psp->sev_data) > + return 0; Should this return a non-zero value -- maybe `-ENODEV`? Otherwise, the `snp_alloc_firmware_page()` API will return a page that the caller believes is suitable to use with FW. My concern is that someone decides to use this API to stash a page very early on during kernel boot and that page becomes a time bomb. If we initialize `rc` to `-ENODEV` (or something similar), then every return in this function can be `return rc`. > + > + /* If SEV-SNP is initialized then add the page in RMP table. */ > + sev = psp->sev_data; > + if (!sev->snp_inited) > + return 0; Ditto. Should this turn a non-zero value? > + > + while (pfn < pfn_end) { > + if (need_reclaim) > + if (snp_reclaim_page(pfn_to_page(pfn), locked)) > + return -EFAULT; > + > + rc = rmpupdate(pfn_to_page(pfn), val); > + if (rc) > + return rc; > + > + pfn++; > + } > + > + return 0; > +} > + > +static struct page *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order, bool locked) > +{ > + struct rmpupdate val = {}; `{}` -> `{0}`? (Not sure, see my previous comment.) > + unsigned long paddr; > + struct page *page; > + > + page = alloc_pages(gfp_mask, order); > + if (!page) > + return NULL; > + > + val.assigned = 1; > + val.immutable = 1; > + paddr = __pa((unsigned long)page_address(page)); > + > + if (snp_set_rmptable_state(paddr, 1 << order, &val, locked, false)) { > + pr_warn("Failed to set page state (leaking it)\n"); Maybe `WARN_ONCE` instead of `pr_warn`? It's both a big attention grabber and also rate limited. > + return NULL; > + } > + > + return page; > +} > + > +void *snp_alloc_firmware_page(gfp_t gfp_mask) > +{ > + struct page *page; > + > + page = __snp_alloc_firmware_pages(gfp_mask, 0, false); > + > + return page ? page_address(page) : NULL; > +} > +EXPORT_SYMBOL_GPL(snp_alloc_firmware_page); > > +static void __snp_free_firmware_pages(struct page *page, int order, bool locked) > +{ > + struct rmpupdate val = {}; `{}` -> `{0}`? (Not sure, see my previous comment.) > + unsigned long paddr; > + > + if (!page) > + return; > + > + paddr = __pa((unsigned long)page_address(page)); > + > + if (snp_set_rmptable_state(paddr, 1 << order, &val, locked, true)) { > + pr_warn("Failed to set page state (leaking it)\n"); WARN_ONCE? > + return; > + } > + > + __free_pages(page, order); > +} > + > +void snp_free_firmware_page(void *addr) > +{ > + if (!addr) > + return; > + > + __snp_free_firmware_pages(virt_to_page(addr), 0, false); > +} > +EXPORT_SYMBOL(snp_free_firmware_page); > + > static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) > { > struct psp_device *psp = psp_master; > @@ -273,7 +387,7 @@ static int __sev_platform_init_locked(int *error) > > data.flags |= SEV_INIT_FLAGS_SEV_ES; > data.tmr_address = tmr_pa; > - data.tmr_len = SEV_ES_TMR_SIZE; > + data.tmr_len = sev_es_tmr_size; > } > > rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error); > @@ -630,6 +744,8 @@ static int __sev_snp_init_locked(int *error) > sev->snp_inited = true; > dev_dbg(sev->dev, "SEV-SNP firmware initialized\n"); > > + sev_es_tmr_size = SEV_SNP_ES_TMR_SIZE; > + > return rc; > } > > @@ -1153,8 +1269,10 @@ static void sev_firmware_shutdown(struct sev_device *sev) > /* The TMR area was encrypted, flush it from the cache */ > wbinvd_on_all_cpus(); > > - free_pages((unsigned long)sev_es_tmr, > - get_order(SEV_ES_TMR_SIZE)); > + > + __snp_free_firmware_pages(virt_to_page(sev_es_tmr), > + get_order(sev_es_tmr_size), > + false); > sev_es_tmr = NULL; > } > > @@ -1204,16 +1322,6 @@ void sev_pci_init(void) > sev_update_firmware(sev->dev) == 0) > sev_get_api_version(); > > - /* Obtain the TMR memory area for SEV-ES use */ > - tmr_page = alloc_pages(GFP_KERNEL, get_order(SEV_ES_TMR_SIZE)); > - if (tmr_page) { > - sev_es_tmr = page_address(tmr_page); > - } else { > - sev_es_tmr = NULL; > - dev_warn(sev->dev, > - "SEV: TMR allocation failed, SEV-ES support unavailable\n"); > - } > - > /* > * If boot CPU supports the SNP, then first attempt to initialize > * the SNP firmware. > @@ -1229,6 +1337,16 @@ void sev_pci_init(void) > } > } > > + /* Obtain the TMR memory area for SEV-ES use */ > + tmr_page = __snp_alloc_firmware_pages(GFP_KERNEL, get_order(sev_es_tmr_size), false); > + if (tmr_page) { > + sev_es_tmr = page_address(tmr_page); > + } else { > + sev_es_tmr = NULL; > + dev_warn(sev->dev, > + "SEV: TMR allocation failed, SEV-ES support unavailable\n"); > + } > + > /* Initialize the platform */ > rc = sev_platform_init(&error); > if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) { > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > index 63ef766cbd7a..b72a74f6a4e9 100644 > --- a/include/linux/psp-sev.h > +++ b/include/linux/psp-sev.h > @@ -12,6 +12,8 @@ > #ifndef __PSP_SEV_H__ > #define __PSP_SEV_H__ > > +#include <linux/sev.h> > + > #include <uapi/linux/psp-sev.h> > > #ifdef CONFIG_X86 > @@ -920,6 +922,8 @@ int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *error); > > > void *psp_copy_user_blob(u64 uaddr, u32 len); > +void *snp_alloc_firmware_page(gfp_t mask); > +void snp_free_firmware_page(void *addr); > > #else /* !CONFIG_CRYPTO_DEV_SP_PSP */ > > @@ -961,6 +965,13 @@ static inline int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *erro > return -ENODEV; > } > > +static inline void *snp_alloc_firmware_page(gfp_t mask) > +{ > + return NULL; > +} > + > +static inline void snp_free_firmware_page(void *addr) { } > + > #endif /* CONFIG_CRYPTO_DEV_SP_PSP */ > > #endif /* __PSP_SEV_H__ */ > -- > 2.17.1 > >
On 7/14/21 8:22 AM, Marc Orr wrote: >> >> +static int snp_reclaim_page(struct page *page, bool locked) >> +{ >> + struct sev_data_snp_page_reclaim data = {}; > > Hmmm.. according to some things I read online, an empty initializer > list is not legal in C. For example: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstackoverflow.com%2Fquestions%2F17589533%2Fis-an-empty-initializer-list-valid-c-code&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cda82a72de9ab40237b1208d946ca78e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637618657748568732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zrK%2BUfXYGFVB5MfsmIIM0LtPDQ9UsAJxksCunosP9MY%3D&reserved=0 > I'm sure this is compiling. Should we change this to `{0}`, which I > believe will initialize all fields in this struct to zero, according > to: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstackoverflow.com%2Fquestions%2F11152160%2Finitializing-a-struct-to-0&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cda82a72de9ab40237b1208d946ca78e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637618657748568732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=vpyAtB%2BZ6b%2BXD3VthQy2b8JtYzYnMceWb9cdj5UGlPg%3D&reserved=0? > Ah, good point. I will fix in next version. > > Should this return a non-zero value -- maybe `-ENODEV`? Otherwise, the > `snp_alloc_firmware_page()` API will return a page that the caller > believes is suitable to use with FW. My concern is that someone > decides to use this API to stash a page very early on during kernel > boot and that page becomes a time bomb. But that means the caller now need to know that SNP is enabled before calling the APIs. The idea behind the API was that caller does not need to know whether the firmware is in the INIT state. If the firmware has initialized the SNP, then it will transparently set the immutable bit in the RMP table. > > If we initialize `rc` to `-ENODEV` (or something similar), then every > return in this function can be `return rc`. > >> + >> + /* If SEV-SNP is initialized then add the page in RMP table. */ >> + sev = psp->sev_data; >> + if (!sev->snp_inited) >> + return 0; > > Ditto. Should this turn a non-zero value? > >> + >> + while (pfn < pfn_end) { >> + if (need_reclaim) >> + if (snp_reclaim_page(pfn_to_page(pfn), locked)) >> + return -EFAULT; >> + >> + rc = rmpupdate(pfn_to_page(pfn), val); >> + if (rc) >> + return rc; >> + >> + pfn++; >> + } >> + >> + return 0; >> +} >> + >> +static struct page *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order, bool locked) >> +{ >> + struct rmpupdate val = {}; > > `{}` -> `{0}`? (Not sure, see my previous comment.) > >> + unsigned long paddr; >> + struct page *page; >> + >> + page = alloc_pages(gfp_mask, order); >> + if (!page) >> + return NULL; >> + >> + val.assigned = 1; >> + val.immutable = 1; >> + paddr = __pa((unsigned long)page_address(page)); >> + >> + if (snp_set_rmptable_state(paddr, 1 << order, &val, locked, false)) { >> + pr_warn("Failed to set page state (leaking it)\n"); > > Maybe `WARN_ONCE` instead of `pr_warn`? It's both a big attention > grabber and also rate limited. Noted. > >> + return NULL; >> + } >> + >> + return page; >> +} >> + >> +void *snp_alloc_firmware_page(gfp_t gfp_mask) >> +{ >> + struct page *page; >> + >> + page = __snp_alloc_firmware_pages(gfp_mask, 0, false); >> + >> + return page ? page_address(page) : NULL; >> +} >> +EXPORT_SYMBOL_GPL(snp_alloc_firmware_page); >> >> +static void __snp_free_firmware_pages(struct page *page, int order, bool locked) >> +{ >> + struct rmpupdate val = {}; > > `{}` -> `{0}`? (Not sure, see my previous comment.) > >> + unsigned long paddr; >> + >> + if (!page) >> + return; >> + >> + paddr = __pa((unsigned long)page_address(page)); >> + >> + if (snp_set_rmptable_state(paddr, 1 << order, &val, locked, true)) { >> + pr_warn("Failed to set page state (leaking it)\n"); > > WARN_ONCE? Noted. thanks
> > Should this return a non-zero value -- maybe `-ENODEV`? Otherwise, the > > `snp_alloc_firmware_page()` API will return a page that the caller > > believes is suitable to use with FW. My concern is that someone > > decides to use this API to stash a page very early on during kernel > > boot and that page becomes a time bomb. > > But that means the caller now need to know that SNP is enabled before > calling the APIs. The idea behind the API was that caller does not need > to know whether the firmware is in the INIT state. If the firmware has > initialized the SNP, then it will transparently set the immutable bit in > the RMP table. For SNP, isn't that already the case? There are three scenarios: #1: The PSP driver is loaded and `snp_inited` is `true`: These returns are never hit. #2: The PSP driver is not loaded. The first return, `!psp || !psp->sev_data` fires. As written, it returns `0`, indicating success. However, we never called RMPUPDATE on the page. Thus, later, when the PSP driver is loaded, the page that was previously returned as usable with FW is in fact not usable with FW. Unless SNP is disabled (e.g., SEV, SEV-ES only). In which case I guess the page is OK. #3 The PSP driver is loaded but the SNP_INIT command has not been issued. Looking at this again, I guess `return 0` is OK. Because if we got this far, then `sev_pci_init()` has been called, and the SNP_INIT command has been issued if we're supporting SNP VMs. So in summary, I think we should change the first return to return an error and leave the 2nd return as is. > > If we initialize `rc` to `-ENODEV` (or something similar), then every > > return in this function can be `return rc`. > > > >> + > >> + /* If SEV-SNP is initialized then add the page in RMP table. */ > >> + sev = psp->sev_data; > >> + if (!sev->snp_inited) > >> + return 0; > > > > Ditto. Should this turn a non-zero value? > > > >> + > >> + while (pfn < pfn_end) { > >> + if (need_reclaim) > >> + if (snp_reclaim_page(pfn_to_page(pfn), locked)) > >> + return -EFAULT; > >> + > >> + rc = rmpupdate(pfn_to_page(pfn), val); > >> + if (rc) > >> + return rc; > >> + > >> + pfn++; > >> + } > >> + > >> + return 0; > >> +}
On Wed, Jul 07, 2021, Brijesh Singh wrote: > The behavior and requirement for the SEV-legacy command is altered when > the SNP firmware is in the INIT state. See SEV-SNP firmware specification > for more details. > > When SNP is INIT state, all the SEV-legacy commands that cause the > firmware to write memory must be in the firmware state. The TMR memory It'd be helpful to spell out Trusted Memory Region, I hadn't seen that term before and for some reason my brain immediately thought "xAPIC register!". > is allocated by the host but updated by the firmware, so, it must be > in the firmware state. Additionally, the TMR memory must be a 2MB aligned > instead of the 1MB, and the TMR length need to be 2MB instead of 1MB. > The helper __snp_{alloc,free}_firmware_pages() can be used for allocating > and freeing the memory used by the firmware. None of this actually states what the patch does, e.g. it's not clear whether all allocations are being converted to 2mb or just the SNP. Looks like it's just SNP. Something like this? Allocate the Trusted Memory Region (TMR) as a 2mb sized/aligned region when SNP is enabled to satisfy new requirements for SNP. Continue allocating a 1mb region for !SNP configuration. > While at it, provide API that can be used by others to allocate a page > that can be used by the firmware. The immediate user for this API will > be the KVM driver. The KVM driver to need to allocate a firmware context > page during the guest creation. The context page need to be updated > by the firmware. See the SEV-SNP specification for further details. ... > @@ -1153,8 +1269,10 @@ static void sev_firmware_shutdown(struct sev_device *sev) > /* The TMR area was encrypted, flush it from the cache */ > wbinvd_on_all_cpus(); > > - free_pages((unsigned long)sev_es_tmr, > - get_order(SEV_ES_TMR_SIZE)); > + > + __snp_free_firmware_pages(virt_to_page(sev_es_tmr), > + get_order(sev_es_tmr_size), > + false); > sev_es_tmr = NULL; > } > > @@ -1204,16 +1322,6 @@ void sev_pci_init(void) > sev_update_firmware(sev->dev) == 0) > sev_get_api_version(); > > - /* Obtain the TMR memory area for SEV-ES use */ > - tmr_page = alloc_pages(GFP_KERNEL, get_order(SEV_ES_TMR_SIZE)); > - if (tmr_page) { > - sev_es_tmr = page_address(tmr_page); > - } else { > - sev_es_tmr = NULL; > - dev_warn(sev->dev, > - "SEV: TMR allocation failed, SEV-ES support unavailable\n"); > - } > - > /* > * If boot CPU supports the SNP, then first attempt to initialize > * the SNP firmware. > @@ -1229,6 +1337,16 @@ void sev_pci_init(void) > } > } > > + /* Obtain the TMR memory area for SEV-ES use */ > + tmr_page = __snp_alloc_firmware_pages(GFP_KERNEL, get_order(sev_es_tmr_size), false); > + if (tmr_page) { > + sev_es_tmr = page_address(tmr_page); > + } else { > + sev_es_tmr = NULL; > + dev_warn(sev->dev, > + "SEV: TMR allocation failed, SEV-ES support unavailable\n"); > + } I think your patch ordering got a bit wonky. AFAICT, the chunk that added sev_snp_init() and friends in the previous patch 14 should have landed above the TMR allocation, i.e. the code movement here should be unnecessary. > /* Initialize the platform */ > rc = sev_platform_init(&error); > if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) { ... > @@ -961,6 +965,13 @@ static inline int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *erro > return -ENODEV; > } > > +static inline void *snp_alloc_firmware_page(gfp_t mask) > +{ > + return NULL; > +} > + > +static inline void snp_free_firmware_page(void *addr) { } Hmm, I think we should probably bite the bullet and #ifdef and/or stub out large swaths of svm/sev.c before adding SNP support. sev.c is getting quite massive, and we're accumulating more and more stubs outside of KVM because its SEV code is compiled unconditionally.
On 7/15/21 6:48 PM, Sean Christopherson wrote: > On Wed, Jul 07, 2021, Brijesh Singh wrote: >> The behavior and requirement for the SEV-legacy command is altered when >> the SNP firmware is in the INIT state. See SEV-SNP firmware specification >> for more details. >> >> When SNP is INIT state, all the SEV-legacy commands that cause the >> firmware to write memory must be in the firmware state. The TMR memory > It'd be helpful to spell out Trusted Memory Region, I hadn't seen that > term before and for some reason my brain immediately thought "xAPIC register!". Noted. > >> is allocated by the host but updated by the firmware, so, it must be >> in the firmware state. Additionally, the TMR memory must be a 2MB aligned >> instead of the 1MB, and the TMR length need to be 2MB instead of 1MB. >> The helper __snp_{alloc,free}_firmware_pages() can be used for allocating >> and freeing the memory used by the firmware. > None of this actually states what the patch does, e.g. it's not clear whether > all allocations are being converted to 2mb or just the SNP. Looks like it's > just SNP. Something like this? > > Allocate the Trusted Memory Region (TMR) as a 2mb sized/aligned region when > SNP is enabled to satisfy new requirements for SNP. Continue allocating a > 1mb region for !SNP configuration. > Only the TMR allocation is converted to use the 2mb when SNP is enabled. >> While at it, provide API that can be used by others to allocate a page >> that can be used by the firmware. The immediate user for this API will >> be the KVM driver. The KVM driver to need to allocate a firmware context >> page during the guest creation. The context page need to be updated >> by the firmware. See the SEV-SNP specification for further details. > ... > >> @@ -1153,8 +1269,10 @@ static void sev_firmware_shutdown(struct sev_device *sev) >> /* The TMR area was encrypted, flush it from the cache */ >> wbinvd_on_all_cpus(); >> >> - free_pages((unsigned long)sev_es_tmr, >> - get_order(SEV_ES_TMR_SIZE)); >> + >> + __snp_free_firmware_pages(virt_to_page(sev_es_tmr), >> + get_order(sev_es_tmr_size), >> + false); >> sev_es_tmr = NULL; >> } >> >> @@ -1204,16 +1322,6 @@ void sev_pci_init(void) >> sev_update_firmware(sev->dev) == 0) >> sev_get_api_version(); >> >> - /* Obtain the TMR memory area for SEV-ES use */ >> - tmr_page = alloc_pages(GFP_KERNEL, get_order(SEV_ES_TMR_SIZE)); >> - if (tmr_page) { >> - sev_es_tmr = page_address(tmr_page); >> - } else { >> - sev_es_tmr = NULL; >> - dev_warn(sev->dev, >> - "SEV: TMR allocation failed, SEV-ES support unavailable\n"); >> - } >> - >> /* >> * If boot CPU supports the SNP, then first attempt to initialize >> * the SNP firmware. >> @@ -1229,6 +1337,16 @@ void sev_pci_init(void) >> } >> } >> >> + /* Obtain the TMR memory area for SEV-ES use */ >> + tmr_page = __snp_alloc_firmware_pages(GFP_KERNEL, get_order(sev_es_tmr_size), false); >> + if (tmr_page) { >> + sev_es_tmr = page_address(tmr_page); >> + } else { >> + sev_es_tmr = NULL; >> + dev_warn(sev->dev, >> + "SEV: TMR allocation failed, SEV-ES support unavailable\n"); >> + } > I think your patch ordering got a bit wonky. AFAICT, the chunk that added > sev_snp_init() and friends in the previous patch 14 should have landed above > the TMR allocation, i.e. the code movement here should be unnecessary. I was debating about it whether to include all the SNP supports in one patch or divide it up. If I had included all legacy support new requirement in the same patch which adds the SNP then it will be a big patch. I had feeling that others may ask me to split it. So my approach is: * In the first patch adds SNP support only * Improve the legacy SEV/ES for the requirement when SNP is enabled. Once SNP is enabled, there are two new requirement for the legacy SEV/ES guests 1) TMR must be 2mb 2) The buffer given to the firmware for the write must be in the firmware state. I also divided both of the new requirement in separate patches so that its easy to review. > >> /* Initialize the platform */ >> rc = sev_platform_init(&error); >> if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) { > ... > >> @@ -961,6 +965,13 @@ static inline int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *erro >> return -ENODEV; >> } >> >> +static inline void *snp_alloc_firmware_page(gfp_t mask) >> +{ >> + return NULL; >> +} >> + >> +static inline void snp_free_firmware_page(void *addr) { } > Hmm, I think we should probably bite the bullet and #ifdef and/or stub out large > swaths of svm/sev.c before adding SNP support. sev.c is getting quite massive, > and we're accumulating more and more stubs outside of KVM because its SEV code > is compiled unconditionally.
On Fri, Jul 16, 2021, Brijesh Singh wrote: > > On 7/15/21 6:48 PM, Sean Christopherson wrote: > > On Wed, Jul 07, 2021, Brijesh Singh wrote: > >> @@ -1204,16 +1322,6 @@ void sev_pci_init(void) > >> sev_update_firmware(sev->dev) == 0) > >> sev_get_api_version(); > >> > >> - /* Obtain the TMR memory area for SEV-ES use */ > >> - tmr_page = alloc_pages(GFP_KERNEL, get_order(SEV_ES_TMR_SIZE)); > >> - if (tmr_page) { > >> - sev_es_tmr = page_address(tmr_page); > >> - } else { > >> - sev_es_tmr = NULL; > >> - dev_warn(sev->dev, > >> - "SEV: TMR allocation failed, SEV-ES support unavailable\n"); > >> - } > >> - > >> /* > >> * If boot CPU supports the SNP, then first attempt to initialize > >> * the SNP firmware. > >> @@ -1229,6 +1337,16 @@ void sev_pci_init(void) > >> } > >> } > >> > >> + /* Obtain the TMR memory area for SEV-ES use */ > >> + tmr_page = __snp_alloc_firmware_pages(GFP_KERNEL, get_order(sev_es_tmr_size), false); > >> + if (tmr_page) { > >> + sev_es_tmr = page_address(tmr_page); > >> + } else { > >> + sev_es_tmr = NULL; > >> + dev_warn(sev->dev, > >> + "SEV: TMR allocation failed, SEV-ES support unavailable\n"); > >> + } > > I think your patch ordering got a bit wonky. AFAICT, the chunk that added > > sev_snp_init() and friends in the previous patch 14 should have landed above > > the TMR allocation, i.e. the code movement here should be unnecessary. > > I was debating about it whether to include all the SNP supports in one > patch or divide it up. If I had included all legacy support new > requirement in the same patch which adds the SNP then it will be a big > patch. I had feeling that others may ask me to split it. It wasn't comment on the patch organization, rather that the code added in patch 14 appears to have landed in the wrong location within the code. The above diff shows that the TMR allocation is being moved around the SNP initialization code that was added in patch 14 (the immediately prior patch). Presumably the required order doesn't magically change just because the TMR is now being allocated as a 2mb blob, so either the code movement is unnecessary churn or the original location was wrong. In either case, landing the SNP initialization code above the TMR allocation in patch 14 would eliminate the above code movement.
On 7/16/21 10:35 AM, Sean Christopherson wrote: > It wasn't comment on the patch organization, rather that the code added in patch 14 > appears to have landed in the wrong location within the code. The above diff shows > that the TMR allocation is being moved around the SNP initialization code that was > added in patch 14 (the immediately prior patch). Presumably the required order > doesn't magically change just because the TMR is now being allocated as a 2mb blob, > so either the code movement is unnecessary churn or the original location was wrong. > In either case, landing the SNP initialization code above the TMR allocation in > patch 14 would eliminate the above code movement. Got it, I'll rearrange things in the previous patch to avoid this hunk. thanks
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index ad9a0c8111e0..bb07c68834a6 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -54,6 +54,14 @@ static int psp_timeout; #define SEV_ES_TMR_SIZE (1024 * 1024) static void *sev_es_tmr; +/* When SEV-SNP is enabled the TMR need to be 2MB aligned and 2MB size. */ +#define SEV_SNP_ES_TMR_SIZE (2 * 1024 * 1024) + +static size_t sev_es_tmr_size = SEV_ES_TMR_SIZE; + +static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret); +static int sev_do_cmd(int cmd, void *data, int *psp_ret); + static inline bool sev_version_greater_or_equal(u8 maj, u8 min) { struct sev_device *sev = psp_master->sev_data; @@ -151,6 +159,112 @@ static int sev_cmd_buffer_len(int cmd) return 0; } +static int snp_reclaim_page(struct page *page, bool locked) +{ + struct sev_data_snp_page_reclaim data = {}; + int ret, err; + + data.paddr = page_to_pfn(page) << PAGE_SHIFT; + + if (locked) + ret = __sev_do_cmd_locked(SEV_CMD_SNP_PAGE_RECLAIM, &data, &err); + else + ret = sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, &data, &err); + + return ret; +} + +static int snp_set_rmptable_state(unsigned long paddr, int npages, + struct rmpupdate *val, bool locked, bool need_reclaim) +{ + unsigned long pfn = __sme_clr(paddr) >> PAGE_SHIFT; + unsigned long pfn_end = pfn + npages; + struct psp_device *psp = psp_master; + struct sev_device *sev; + int rc; + + if (!psp || !psp->sev_data) + return 0; + + /* If SEV-SNP is initialized then add the page in RMP table. */ + sev = psp->sev_data; + if (!sev->snp_inited) + return 0; + + while (pfn < pfn_end) { + if (need_reclaim) + if (snp_reclaim_page(pfn_to_page(pfn), locked)) + return -EFAULT; + + rc = rmpupdate(pfn_to_page(pfn), val); + if (rc) + return rc; + + pfn++; + } + + return 0; +} + +static struct page *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order, bool locked) +{ + struct rmpupdate val = {}; + unsigned long paddr; + struct page *page; + + page = alloc_pages(gfp_mask, order); + if (!page) + return NULL; + + val.assigned = 1; + val.immutable = 1; + paddr = __pa((unsigned long)page_address(page)); + + if (snp_set_rmptable_state(paddr, 1 << order, &val, locked, false)) { + pr_warn("Failed to set page state (leaking it)\n"); + return NULL; + } + + return page; +} + +void *snp_alloc_firmware_page(gfp_t gfp_mask) +{ + struct page *page; + + page = __snp_alloc_firmware_pages(gfp_mask, 0, false); + + return page ? page_address(page) : NULL; +} +EXPORT_SYMBOL_GPL(snp_alloc_firmware_page); + +static void __snp_free_firmware_pages(struct page *page, int order, bool locked) +{ + struct rmpupdate val = {}; + unsigned long paddr; + + if (!page) + return; + + paddr = __pa((unsigned long)page_address(page)); + + if (snp_set_rmptable_state(paddr, 1 << order, &val, locked, true)) { + pr_warn("Failed to set page state (leaking it)\n"); + return; + } + + __free_pages(page, order); +} + +void snp_free_firmware_page(void *addr) +{ + if (!addr) + return; + + __snp_free_firmware_pages(virt_to_page(addr), 0, false); +} +EXPORT_SYMBOL(snp_free_firmware_page); + static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) { struct psp_device *psp = psp_master; @@ -273,7 +387,7 @@ static int __sev_platform_init_locked(int *error) data.flags |= SEV_INIT_FLAGS_SEV_ES; data.tmr_address = tmr_pa; - data.tmr_len = SEV_ES_TMR_SIZE; + data.tmr_len = sev_es_tmr_size; } rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error); @@ -630,6 +744,8 @@ static int __sev_snp_init_locked(int *error) sev->snp_inited = true; dev_dbg(sev->dev, "SEV-SNP firmware initialized\n"); + sev_es_tmr_size = SEV_SNP_ES_TMR_SIZE; + return rc; } @@ -1153,8 +1269,10 @@ static void sev_firmware_shutdown(struct sev_device *sev) /* The TMR area was encrypted, flush it from the cache */ wbinvd_on_all_cpus(); - free_pages((unsigned long)sev_es_tmr, - get_order(SEV_ES_TMR_SIZE)); + + __snp_free_firmware_pages(virt_to_page(sev_es_tmr), + get_order(sev_es_tmr_size), + false); sev_es_tmr = NULL; } @@ -1204,16 +1322,6 @@ void sev_pci_init(void) sev_update_firmware(sev->dev) == 0) sev_get_api_version(); - /* Obtain the TMR memory area for SEV-ES use */ - tmr_page = alloc_pages(GFP_KERNEL, get_order(SEV_ES_TMR_SIZE)); - if (tmr_page) { - sev_es_tmr = page_address(tmr_page); - } else { - sev_es_tmr = NULL; - dev_warn(sev->dev, - "SEV: TMR allocation failed, SEV-ES support unavailable\n"); - } - /* * If boot CPU supports the SNP, then first attempt to initialize * the SNP firmware. @@ -1229,6 +1337,16 @@ void sev_pci_init(void) } } + /* Obtain the TMR memory area for SEV-ES use */ + tmr_page = __snp_alloc_firmware_pages(GFP_KERNEL, get_order(sev_es_tmr_size), false); + if (tmr_page) { + sev_es_tmr = page_address(tmr_page); + } else { + sev_es_tmr = NULL; + dev_warn(sev->dev, + "SEV: TMR allocation failed, SEV-ES support unavailable\n"); + } + /* Initialize the platform */ rc = sev_platform_init(&error); if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) { diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index 63ef766cbd7a..b72a74f6a4e9 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -12,6 +12,8 @@ #ifndef __PSP_SEV_H__ #define __PSP_SEV_H__ +#include <linux/sev.h> + #include <uapi/linux/psp-sev.h> #ifdef CONFIG_X86 @@ -920,6 +922,8 @@ int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *error); void *psp_copy_user_blob(u64 uaddr, u32 len); +void *snp_alloc_firmware_page(gfp_t mask); +void snp_free_firmware_page(void *addr); #else /* !CONFIG_CRYPTO_DEV_SP_PSP */ @@ -961,6 +965,13 @@ static inline int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *erro return -ENODEV; } +static inline void *snp_alloc_firmware_page(gfp_t mask) +{ + return NULL; +} + +static inline void snp_free_firmware_page(void *addr) { } + #endif /* CONFIG_CRYPTO_DEV_SP_PSP */ #endif /* __PSP_SEV_H__ */
The behavior and requirement for the SEV-legacy command is altered when the SNP firmware is in the INIT state. See SEV-SNP firmware specification for more details. When SNP is INIT state, all the SEV-legacy commands that cause the firmware to write memory must be in the firmware state. The TMR memory is allocated by the host but updated by the firmware, so, it must be in the firmware state. Additionally, the TMR memory must be a 2MB aligned instead of the 1MB, and the TMR length need to be 2MB instead of 1MB. The helper __snp_{alloc,free}_firmware_pages() can be used for allocating and freeing the memory used by the firmware. While at it, provide API that can be used by others to allocate a page that can be used by the firmware. The immediate user for this API will be the KVM driver. The KVM driver to need to allocate a firmware context page during the guest creation. The context page need to be updated by the firmware. See the SEV-SNP specification for further details. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- drivers/crypto/ccp/sev-dev.c | 144 +++++++++++++++++++++++++++++++---- include/linux/psp-sev.h | 11 +++ 2 files changed, 142 insertions(+), 13 deletions(-)