Messages in this thread | | | Date | Tue, 21 Feb 2023 16:06:06 -0600 | Subject | Re: [PATCH RFC v8 24/56] crypto: ccp: Handle the legacy TMR allocation when SNP is enabled | From | "Kalra, Ashish" <> |
| |
On 2/21/2023 3:15 PM, Zhi Wang wrote: > On Tue, 21 Feb 2023 09:31:01 -0600 > "Kalra, Ashish" <ashish.kalra@amd.com> wrote: > >>>> +static int snp_reclaim_pages(unsigned long paddr, unsigned int npages, bool locked) >>>> +{ >>>> + /* Cbit maybe set in the paddr */ >>> >>> This is confusing. >>> >>> I suppose C-bit is treated as a attribute of PTE in the kernel not part of the >>> PA. It means only a PTE might carry a C-bit. >>> >> >> snp_reclaim_pages() is also called for reclaiming guest memory, in which >> case the (guest) paddr will have the C-bit set. Hence this C-bit >> handling is done within snp_reclaim_pages() so that the callers don't >> need to handle it explicitly. > > Thanks for the explanation. > > Do you mean it will be used like that in the later patch? Sorry if it is in the > later patch as I was making progress slowly. It is quite a big patch set. >
Yes, these are callers in later patches, like the following code path in patch 25:
static int unmap_firmware_writeable(u64 *paddr, u32 len, bool guest, struct snp_host_map *map) { unsigned int npages = PAGE_ALIGN(len) >> PAGE_SHIFT; ... /* If paddr points to a guest memory then restore the page state to hypervisor. */ if (guest) { if (snp_reclaim_pages(*paddr, npages, true)) return -EFAULT;
goto done; }
... ...
Or, the following as part of patch 52:
int snp_guest_dbg_decrypt_page(u64 gctx_pfn, u64 src_pfn, u64 dst_pfn, int *error) { ... data.gctx_paddr = sme_me_mask | (gctx_pfn << PAGE_SHIFT); data.src_addr = sme_me_mask | (src_pfn << PAGE_SHIFT); data.dst_addr = sme_me_mask | (dst_pfn << PAGE_SHIFT);
/* The destination page must be in the firmware state. */ if (rmp_mark_pages_firmware(data.dst_addr, 1, false)) return -EIO;
ret = sev_do_cmd(SEV_CMD_SNP_DBG_DECRYPT, &data, error);
/* Restore the page state */ if (snp_reclaim_pages(data.dst_addr, 1, false)) ... ...
Thanks, Ashish
> At least, I don't see that kind of usage in the current patch. Feel free to > correct me if I am wrong. > > The call chains: > > __snp_free_firmware_page() > snp_reclaim_pages(); > > As __snp_free_firmware_page() takes struct page*, all the follwing coversion > from it would not carry C-bit. > > __snp_alloc_firmware_pages() > rmp_mark_pages_firmware() > snp_reclaim_pages() > > As __snp_alloc_firmware_page() allocates page with struct page*, the same > conclusion as above. > >> >> >>> The paddr is from __pa(page_address()). It is not extracted from a PTE. Thus, the >>> return from them should never have a C-bit. >>> >>> BTW: Wouldn't it be better to have pfn as input param instead of paddr? >>> >>> The caller has struct page, calling snp_reclaim_pages(page_to_pfn(page), xxxxx) >>> would be much clearer than the current conversion: >>> page_address() (struct page is converted to VA), __pa() (VA is converted to PA) >>> in the caller and then PA is converted to pfn here. >>> >>>> + unsigned long pfn = __sme_clr(paddr) >> PAGE_SHIFT; >>>> + int ret, err, i, n = 0; >>>> + >>> >>> should be unsigned int i, n; as the input param npage is unsigned int. >>> >>>> + if (!pfn_valid(pfn)) { >>>> + pr_err("%s: Invalid PFN %lx\n", __func__, pfn); >>>> + return 0; >>>> + } >>>> + >>>> + for (i = 0; i < npages; i++, pfn++, n++) { >>>> + paddr = pfn << PAGE_SHIFT; >>>> + >>>> + if (locked) >>>> + ret = __sev_do_cmd_locked(SEV_CMD_SNP_PAGE_RECLAIM, &paddr, &err); >>>> + else >>>> + ret = sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, &paddr, &err); >>>> + >>>> + if (ret) >>>> + goto cleanup; >>>> + >>>> + ret = rmp_make_shared(pfn, PG_LEVEL_4K); >>>> + if (ret) >>>> + goto cleanup; >>>> + } >>>> + >>>> + return 0; >>>> + >>>> +cleanup: >>>> + /* >>>> + * If failed to reclaim the page then page is no longer safe to >>>> + * be release back to the system, leak it. >>>> + */ >>>> + snp_mark_pages_offline(pfn, npages - n); >>>> + return ret; >>>> +} >>>> + >>>> +static int rmp_mark_pages_firmware(unsigned long paddr, unsigned int npages, bool locked) >>> >>> The same comment as above. Better take pfn or page instead of paddr with >>> redundant conversions. >>> >> >> Again, the paddr can point to guest memory so it can have C-bit set. >> >> Thanks, >> Ashish >> >>>> +{ >>>> + /* Cbit maybe set in the paddr */ >>>> + unsigned long pfn = __sme_clr(paddr) >> PAGE_SHIFT; >>>> + int rc, n = 0, i; >>>> + >>>> + for (i = 0; i < npages; i++, n++, pfn++) { >>>> + rc = rmp_make_private(pfn, 0, PG_LEVEL_4K, 0, true); >>>> + if (rc) >>>> + goto cleanup; >>>> + } >>>> + >>>> + return 0; >>>> + >>>> +cleanup: >>>> + /* >>>> + * Try unrolling the firmware state changes by >>>> + * reclaiming the pages which were already changed to the >>>> + * firmware state. >>>> + */ >>>> + snp_reclaim_pages(paddr, n, locked); >>>> + >>>> + return rc; >>>> +} >>>> + >
| |