Re: [PATCH RFC v8 24/56] crypto: ccp: Handle the legacy TMR allocation when SNP is enabled

[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@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;
> > > > +}
> > > > +