linux-kernel - Re: [RFC v2-fix 1/1] x86/tdx: Wire up KVM hypercalls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d669889f-23e0-5f21-60ab-b550d5934364@linux.intel.com>
Date:   Tue, 18 May 2021 13:12:16 -0700
From:   "Kuppuswamy, Sathyanarayanan" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
To:     Dave Hansen <dave.hansen@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>
Cc:     Tony Luck <tony.luck@...el.com>, Andi Kleen <ak@...ux.intel.com>,
        Kirill Shutemov <kirill.shutemov@...ux.intel.com>,
        Kuppuswamy Sathyanarayanan <knsathya@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Raj Ashok <ashok.raj@...el.com>,
        Sean Christopherson <seanjc@...gle.com>,
        linux-kernel@...r.kernel.org,
        Isaku Yamahata <isaku.yamahata@...el.com>
Subject: Re: [RFC v2-fix 1/1] x86/tdx: Wire up KVM hypercalls



On 5/18/21 8:51 AM, Dave Hansen wrote:
> Question for KVM folks: Should all of these guest patches say:
> "x86/tdx/guest:" or something?  It seems like that would put us all in
> the right frame of mind as we review these.  It's kinda easy (for me at
> least) to get lost about which side I'm looking at sometimes.
> 
> On 5/17/21 5:15 PM, Kuppuswamy Sathyanarayanan wrote:
>> From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
>>
>> KVM hypercalls use the "vmcall" or "vmmcall" instructions.
>> Although the ABI is similar, those instructions no longer
>> function for TDX guests. Make vendor specififc TDVMCALLs
> 
> 				"vendor-specific"
> 
> 		    Hyphen and spelling ^

I will fix it next version.

> 
>> instead of VMCALL.
> 
> This would also be a great place to say:
> 
> This enables TDX guests to run with KVM acting as the hypervisor.  TDX
> guests running under other hypervisors will continue to use those
> hypervisors hypercalls.

I will include it.

> 
>> [Isaku: proposed KVM VENDOR string]
>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
>> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
>> Reviewed-by: Andi Kleen <ak@...ux.intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> 
> This SoB chain is odd.  Kirill wrote this, sent it to Isaku, who sent it
> to Sathya?

Initially we have used "0" as vendor ID for KVM. But Isaku proposed a new
value for it and sent a patch to fix it. But, I did not want to carry it as
separate patch (for one line change). So I have merged his change with
this patch, and added his signed-off with comment ([Isaku: proposed KVM VENDOR string])

+#define TDVMCALL_VENDOR_KVM            0x4d564b2e584454 /* "TDX.KVM" */


> 
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 9e0e0ff76bab..768df1b98487 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -886,6 +886,12 @@ config INTEL_TDX_GUEST
>>   	  run in a CPU mode that protects the confidentiality of TD memory
>>   	  contents and the TD’s CPU state from other software, including VMM.
>>   
>> +config INTEL_TDX_GUEST_KVM
>> +	def_bool y
>> +	depends on KVM_GUEST && INTEL_TDX_GUEST
>> +	help
>> +	 This option enables KVM specific hypercalls in TDX guest.
> 
> For something that's not user-visible, I'd probably just add a Kconfig
> comment rather than help text.

If it is the preferred approach, I can remove it.

> 
> ...
>> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>> index 7966c10ea8d1..a90fec004844 100644
>> --- a/arch/x86/kernel/Makefile
>> +++ b/arch/x86/kernel/Makefile
>> @@ -128,6 +128,7 @@ obj-$(CONFIG_X86_PMEM_LEGACY_DEVICE) += pmem.o
>>   
>>   obj-$(CONFIG_JAILHOUSE_GUEST)	+= jailhouse.o
>>   obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdcall.o tdx.o
>> +obj-$(CONFIG_INTEL_TDX_GUEST_KVM) += tdx-kvm.o
> 
> Is the indentation consistent with the other items near "tdx-kvm.o" in
> the Makefile?

Yes. For longer config names, common indentation is not maintained. Please
check the PMEM example.

126 obj-$(CONFIG_PARAVIRT_CLOCK)    += pvclock.o
127 obj-$(CONFIG_X86_PMEM_LEGACY_DEVICE) += pmem.o
128
129 obj-$(CONFIG_JAILHOUSE_GUEST)   += jailhouse.o
130 obj-$(CONFIG_INTEL_TDX_GUEST)   += tdcall.o tdx.o
131 obj-$(CONFIG_INTEL_TDX_GUEST_KVM) += tdx-kvm.o


> 
> ...
>> +/* Used by kvm_hypercall4() to trigger hypercall in TDX guest */
>> +long tdx_kvm_hypercall4(unsigned int nr, unsigned long p1, unsigned long p2,
>> +		unsigned long p3, unsigned long p4)
>> +{
>> +	return tdx_kvm_hypercall(nr, p1, p2, p3, p4);
>> +}
>> +EXPORT_SYMBOL_GPL(tdx_kvm_hypercall4);
> 
> I always forget that KVM code is goofy and needs to have things in C
> files so you can export the symbols.  Could you add a sentence to the
> changelog to this effect?
> 
> Code-wise, this is fine.  Just a few tweaks and I'll be happy to ack
> this one.

Will add it.

     Since KVM hypercall functions can be included and called
     from kernel modules, export tdx_kvm_hypercall*() functions
     to avoid symbol errors


> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer