linux-kernel - Re: [RFC v1 26/26] x86/kvm: Use bounce buffers for TD guest

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: <1741d632-0ed0-6a5b-efae-4fdb2689e20f@intel.com>
Date:   Thu, 1 Apr 2021 14:17:54 -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:     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
Subject: Re: [RFC v1 26/26] x86/kvm: Use bounce buffers for TD guest

On 2/5/21 3:38 PM, Kuppuswamy Sathyanarayanan wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
> 
> TDX doesn't allow to perform DMA access to guest private memory.
> In order for DMA to work properly in TD guest, user SWIOTLB bounce
> buffers.
> 
> Move AMD SEV initialization into common code and adopt for TDX.

This would be best if it can draw a parallel between TDX and SEV.

>  arch/x86/kernel/pci-swiotlb.c    |  2 +-
>  arch/x86/kernel/tdx.c            |  3 +++
>  arch/x86/mm/mem_encrypt.c        | 44 -------------------------------
>  arch/x86/mm/mem_encrypt_common.c | 45 ++++++++++++++++++++++++++++++++
>  4 files changed, 49 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
> index c2cfa5e7c152..020e13749758 100644
> --- a/arch/x86/kernel/pci-swiotlb.c
> +++ b/arch/x86/kernel/pci-swiotlb.c
> @@ -49,7 +49,7 @@ int __init pci_swiotlb_detect_4gb(void)
>  	 * buffers are allocated and used for devices that do not support
>  	 * the addressing range required for the encryption mask.
>  	 */
> -	if (sme_active())
> +	if (sme_active() || is_tdx_guest())
>  		swiotlb = 1;
>  
>  	return swiotlb;
> diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
> index f51a19168adc..ccb9401bd706 100644
> --- a/arch/x86/kernel/tdx.c
> +++ b/arch/x86/kernel/tdx.c
> @@ -9,6 +9,7 @@
>  #include <asm/vmx.h>
>  #include <asm/insn.h>
>  #include <linux/sched/signal.h> /* force_sig_fault() */
> +#include <linux/swiotlb.h>
>  
>  #ifdef CONFIG_KVM_GUEST
>  #include "tdx-kvm.c"
> @@ -472,6 +473,8 @@ void __init tdx_early_init(void)
>  
>  	legacy_pic = &null_legacy_pic;
>  
> +	swiotlb_force = SWIOTLB_FORCE;

Dumb question time.  But, what is the difference between

	swiotlb = 1;

and

	swiotlb_force = SWIOTLB_FORCE;

It would be nice of the patch to enable me to be a lazy reviewer.

>  	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "tdx:cpu_hotplug",
>  			  NULL, tdx_cpu_offline_prepare);
>  
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 11a6a7b3af7e..7fbbb2f3d426 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c

Should we be renaming this to amd_mem_encrypt.c or something?

...
> -	 */
> -	if (sev_active())
> -		static_branch_enable(&sev_enable_key);
> -
> -	print_mem_encrypt_feature_info();
> -}
> -
> diff --git a/arch/x86/mm/mem_encrypt_common.c b/arch/x86/mm/mem_encrypt_common.c
> index b6d93b0c5dcf..6f3d90d4d68e 100644
> --- a/arch/x86/mm/mem_encrypt_common.c
> +++ b/arch/x86/mm/mem_encrypt_common.c
> @@ -10,6 +10,7 @@
>  #include <linux/mm.h>
>  #include <linux/mem_encrypt.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/swiotlb.h>
>  
>  /* Override for DMA direct allocation check - ARCH_HAS_FORCE_DMA_UNENCRYPTED */
>  bool force_dma_unencrypted(struct device *dev)
> @@ -36,3 +37,47 @@ bool force_dma_unencrypted(struct device *dev)
>  
>  	return false;
>  }
> +
> +static void print_mem_encrypt_feature_info(void)
> +{

This function is now named wrong IMNHO.  If it's about AMD only, it
needs AMD in the name.

> +	pr_info("AMD Memory Encryption Features active:");
> +
> +	/* Secure Memory Encryption */
> +	if (sme_active()) {
> +		/*
> +		 * SME is mutually exclusive with any of the SEV
> +		 * features below.
> +		 */
> +		pr_cont(" SME\n");
> +		return;
> +	}
> +
> +	/* Secure Encrypted Virtualization */
> +	if (sev_active())
> +		pr_cont(" SEV");
> +
> +	/* Encrypted Register State */
> +	if (sev_es_active())
> +		pr_cont(" SEV-ES");
> +
> +	pr_cont("\n");
> +}

I'm really tempted to say this needs to be off in arch/x86/kernel/cpu/amd.c

> +/* Architecture __weak replacement functions */
> +void __init mem_encrypt_init(void)
> +{
> +	if (!sme_me_mask && !is_tdx_guest())
> +		return;

The direct check of sme_me_mask looks odd now.  What does this *MEAN*?
Are we looking to jump out of here if no memory encryption is enabled?

I'd much rather this look more like:

	if (!x86_memory_encryption())
		return;

> +	/* Call into SWIOTLB to update the SWIOTLB DMA buffers */
> +	swiotlb_update_mem_attributes();
> +	/*
> +	 * With SEV, we need to unroll the rep string I/O instructions.
> +	 */
> +	if (sev_active())
> +		static_branch_enable(&sev_enable_key);
> +
> +	if (!is_tdx_guest())
> +		print_mem_encrypt_feature_info();
> +}