[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BYAPR12MB27599015C77E3F1E164C01F48EB29@BYAPR12MB2759.namprd12.prod.outlook.com>
Date: Wed, 22 Jun 2022 18:04:40 +0000
From: "Kalra, Ashish" <Ashish.Kalra@....com>
To: Dave Hansen <dave.hansen@...el.com>,
"x86@...nel.org" <x86@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>,
"linux-mm@...ck.org" <linux-mm@...ck.org>,
"linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>
CC: "tglx@...utronix.de" <tglx@...utronix.de>,
"mingo@...hat.com" <mingo@...hat.com>,
"jroedel@...e.de" <jroedel@...e.de>,
"Lendacky, Thomas" <Thomas.Lendacky@....com>,
"hpa@...or.com" <hpa@...or.com>,
"ardb@...nel.org" <ardb@...nel.org>,
"pbonzini@...hat.com" <pbonzini@...hat.com>,
"seanjc@...gle.com" <seanjc@...gle.com>,
"vkuznets@...hat.com" <vkuznets@...hat.com>,
"jmattson@...gle.com" <jmattson@...gle.com>,
"luto@...nel.org" <luto@...nel.org>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"slp@...hat.com" <slp@...hat.com>,
"pgonda@...gle.com" <pgonda@...gle.com>,
"peterz@...radead.org" <peterz@...radead.org>,
"srinivas.pandruvada@...ux.intel.com"
<srinivas.pandruvada@...ux.intel.com>,
"rientjes@...gle.com" <rientjes@...gle.com>,
"dovmurik@...ux.ibm.com" <dovmurik@...ux.ibm.com>,
"tobin@....com" <tobin@....com>, "bp@...en8.de" <bp@...en8.de>,
"Roth, Michael" <Michael.Roth@....com>,
"vbabka@...e.cz" <vbabka@...e.cz>,
"kirill@...temov.name" <kirill@...temov.name>,
"ak@...ux.intel.com" <ak@...ux.intel.com>,
"tony.luck@...el.com" <tony.luck@...el.com>,
"marcorr@...gle.com" <marcorr@...gle.com>,
"sathyanarayanan.kuppuswamy@...ux.intel.com"
<sathyanarayanan.kuppuswamy@...ux.intel.com>,
"alpergun@...gle.com" <alpergun@...gle.com>,
"dgilbert@...hat.com" <dgilbert@...hat.com>,
"jarkko@...nel.org" <jarkko@...nel.org>
Subject: RE: [PATCH Part2 v6 06/49] x86/sev: Add helper functions for
RMPUPDATE and PSMASH instruction
[AMD Official Use Only - General]
>> +int psmash(u64 pfn)
>> +{
>> + unsigned long paddr = pfn << PAGE_SHIFT;
>> + int ret;
>> +
>> + if (!pfn_valid(pfn))
>> + return -EINVAL;
>> +
>> + if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
>> + return -ENXIO;
>> +
>> + /* Binutils version 2.36 supports the PSMASH mnemonic. */
>> + asm volatile(".byte 0xF3, 0x0F, 0x01, 0xFF"
>> + : "=a"(ret)
>> + : "a"(paddr)
>> + : "memory", "cc");
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(psmash);
>If a function gets an EXPORT_SYMBOL_GPL(), the least we can do is reasonably document it. We don't need full kerneldoc nonsense, but a one-line about what this does would be quite helpful. That goes for all the functions here.
>It would also be extremely helpful to have the changelog explain why these functions are exported and how the exports will be used.
I will add basic descriptions for all these exported functions.
Thanks,
Ashish
>As a general rule, please push cpu_feature_enabled() checks as early as you reasonably can. They are *VERY* cheap and can even enable the compiler to completely zap code like an #ifdef.
There also seem to be a lot of pfn_valid() checks in here that aren't very well thought out. For instance, there's a pfn_valid() check here:
+int rmp_make_shared(u64 pfn, enum pg_level level) {
+ struct rmpupdate val;
+
+ if (!pfn_valid(pfn))
+ return -EINVAL;
...
+ return rmpupdate(pfn, &val);
+}
and in rmpupdate():
+static int rmpupdate(u64 pfn, struct rmpupdate *val) {
+ unsigned long paddr = pfn << PAGE_SHIFT;
+ int ret;
+
+ if (!pfn_valid(pfn))
+ return -EINVAL;
...
This is (at best) wasteful. Could it be refactored?
Powered by blists - more mailing lists