On 2/21/2023 3:15 PM, Zhi Wang wrote:
On Tue, 21 Feb 2023 09:31:01 -0600
"Kalra, Ashish" <ashish.kalra@xxxxxxx> 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;
+}
+
