lkml.org 
[lkml]   [2021]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH Part2 RFC v4 15/40] crypto: ccp: Handle the legacy TMR allocation when SNP is enabled
From
Date


On 7/14/21 8:22 AM, Marc Orr wrote:
>>
>> +static int snp_reclaim_page(struct page *page, bool locked)
>> +{
>> + struct sev_data_snp_page_reclaim data = {};
>
> Hmmm.. according to some things I read online, an empty initializer
> list is not legal in C. For example:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstackoverflow.com%2Fquestions%2F17589533%2Fis-an-empty-initializer-list-valid-c-code&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cda82a72de9ab40237b1208d946ca78e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637618657748568732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zrK%2BUfXYGFVB5MfsmIIM0LtPDQ9UsAJxksCunosP9MY%3D&reserved=0
> I'm sure this is compiling. Should we change this to `{0}`, which I
> believe will initialize all fields in this struct to zero, according
> to: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstackoverflow.com%2Fquestions%2F11152160%2Finitializing-a-struct-to-0&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cda82a72de9ab40237b1208d946ca78e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637618657748568732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=vpyAtB%2BZ6b%2BXD3VthQy2b8JtYzYnMceWb9cdj5UGlPg%3D&reserved=0?
>

Ah, good point. I will fix in next version.


>
> Should this return a non-zero value -- maybe `-ENODEV`? Otherwise, the
> `snp_alloc_firmware_page()` API will return a page that the caller
> believes is suitable to use with FW. My concern is that someone
> decides to use this API to stash a page very early on during kernel
> boot and that page becomes a time bomb.

But that means the caller now need to know that SNP is enabled before
calling the APIs. The idea behind the API was that caller does not need
to know whether the firmware is in the INIT state. If the firmware has
initialized the SNP, then it will transparently set the immutable bit in
the RMP table.

>
> If we initialize `rc` to `-ENODEV` (or something similar), then every
> return in this function can be `return rc`.
>
>> +
>> + /* If SEV-SNP is initialized then add the page in RMP table. */
>> + sev = psp->sev_data;
>> + if (!sev->snp_inited)
>> + return 0;
>
> Ditto. Should this turn a non-zero value?
>
>> +
>> + while (pfn < pfn_end) {
>> + if (need_reclaim)
>> + if (snp_reclaim_page(pfn_to_page(pfn), locked))
>> + return -EFAULT;
>> +
>> + rc = rmpupdate(pfn_to_page(pfn), val);
>> + if (rc)
>> + return rc;
>> +
>> + pfn++;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct page *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order, bool locked)
>> +{
>> + struct rmpupdate val = {};
>
> `{}` -> `{0}`? (Not sure, see my previous comment.)
>
>> + unsigned long paddr;
>> + struct page *page;
>> +
>> + page = alloc_pages(gfp_mask, order);
>> + if (!page)
>> + return NULL;
>> +
>> + val.assigned = 1;
>> + val.immutable = 1;
>> + paddr = __pa((unsigned long)page_address(page));
>> +
>> + if (snp_set_rmptable_state(paddr, 1 << order, &val, locked, false)) {
>> + pr_warn("Failed to set page state (leaking it)\n");
>
> Maybe `WARN_ONCE` instead of `pr_warn`? It's both a big attention
> grabber and also rate limited.

Noted.

>
>> + return NULL;
>> + }
>> +
>> + return page;
>> +}
>> +
>> +void *snp_alloc_firmware_page(gfp_t gfp_mask)
>> +{
>> + struct page *page;
>> +
>> + page = __snp_alloc_firmware_pages(gfp_mask, 0, false);
>> +
>> + return page ? page_address(page) : NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(snp_alloc_firmware_page);
>>
>> +static void __snp_free_firmware_pages(struct page *page, int order, bool locked)
>> +{
>> + struct rmpupdate val = {};
>
> `{}` -> `{0}`? (Not sure, see my previous comment.)
>
>> + unsigned long paddr;
>> +
>> + if (!page)
>> + return;
>> +
>> + paddr = __pa((unsigned long)page_address(page));
>> +
>> + if (snp_set_rmptable_state(paddr, 1 << order, &val, locked, true)) {
>> + pr_warn("Failed to set page state (leaking it)\n");
>
> WARN_ONCE?

Noted.

thanks

\
 
 \ /
  Last update: 2021-07-14 18:47    [W:0.264 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site