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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 18 May 2021 08:51:53 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.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

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 ^

> 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.

> [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?

> 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.

...
> 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?

...
> +/* 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.