lkml.org 
[lkml]   [2023]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH RFC v8 24/56] crypto: ccp: Handle the legacy TMR allocation when SNP is enabled
From
>> +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;
>> +}
>> +

\
 
 \ /
  Last update: 2023-03-27 00:30    [W:0.240 / U:1.652 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site