Re: [PATCH Part2 RFC v4 15/40] crypto: ccp: Handle the legacy TMR allocation when SNP is enabled

[Brijesh Singh <brijesh.singh@amd.com>]

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