Messages in this thread | | | Subject | Re: [RFC v1 26/26] x86/kvm: Use bounce buffers for TD guest | From | Dave Hansen <> | Date | Thu, 1 Apr 2021 14:17:54 -0700 |
| |
On 2/5/21 3:38 PM, Kuppuswamy Sathyanarayanan wrote: > From: "Kirill A. Shutemov" <kirill.shutemov@linux.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(); > +}
| |