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
