Messages in this thread | | | Date | Tue, 21 Feb 2023 09:31:01 -0600 | Subject | Re: [PATCH RFC v8 24/56] crypto: ccp: Handle the legacy TMR allocation when SNP is enabled | From | "Kalra, Ashish" <> |
| |
>> +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.
> 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; >> +} >> +
| |