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

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