From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2021-09-08 22:59:01
This patch series provides a generic helper function, cc_platform_has(),
to replace the sme_active(), sev_active(), sev_es_active() and
mem_encrypt_active() functions.
It is expected that as new confidential computing technologies are
added to the kernel, they can all be covered by a single function call
instead of a collection of specific function calls all called from the
same locations.
The powerpc and s390 patches have been compile tested only. Can the
folks copied on this series verify that nothing breaks for them. Also,
a new file, arch/powerpc/platforms/pseries/cc_platform.c, has been
created for powerpc to hold the out of line function.
Cc: Andi Kleen <redacted>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Baoquan He <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Christian Borntraeger <redacted>
Cc: Daniel Vetter <redacted>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dave Young <redacted>
Cc: David Airlie <redacted>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Paul Mackerras <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <redacted>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: VMware Graphics <redacted>
Cc: Will Deacon <will@kernel.org>
Cc: Christoph Hellwig <hch@infradead.org>
---
Patches based on:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
4b93c544e90e ("thunderbolt: test: split up test cases in tb_test_credit_alloc_all")
Changes since v2:
- Changed the name from prot_guest_has() to cc_platform_has()
- Took the cc_platform_has() function out of line. Created two new files,
cc_platform.c, in both x86 and ppc to implment the function. As a
result, also changed the attribute defines into enums.
- Removed any received Reviewed-by's and Acked-by's given changes in this
version.
- Added removal of new instances of mem_encrypt_active() usage in powerpc
arch.
- Based on latest Linux tree to pick up powerpc changes related to the
mem_encrypt_active() function.
Changes since v1:
- Moved some arch ioremap functions within #ifdef CONFIG_AMD_MEM_ENCRYPT
in prep for use of prot_guest_has() by TDX.
- Added type includes to the the protected_guest.h header file to prevent
build errors outside of x86.
- Made amd_prot_guest_has() EXPORT_SYMBOL_GPL
- Used amd_prot_guest_has() in place of checking sme_me_mask in the
arch/x86/mm/mem_encrypt.c file.
Tom Lendacky (8):
x86/ioremap: Selectively build arch override encryption functions
mm: Introduce a function to check for confidential computing features
x86/sev: Add an x86 version of cc_platform_has()
powerpc/pseries/svm: Add a powerpc version of cc_platform_has()
x86/sme: Replace occurrences of sme_active() with cc_platform_has()
x86/sev: Replace occurrences of sev_active() with cc_platform_has()
x86/sev: Replace occurrences of sev_es_active() with cc_platform_has()
treewide: Replace the use of mem_encrypt_active() with
cc_platform_has()
arch/Kconfig | 3 +
arch/powerpc/include/asm/mem_encrypt.h | 5 --
arch/powerpc/platforms/pseries/Kconfig | 1 +
arch/powerpc/platforms/pseries/Makefile | 2 +
arch/powerpc/platforms/pseries/cc_platform.c | 26 ++++++
arch/powerpc/platforms/pseries/svm.c | 5 +-
arch/s390/include/asm/mem_encrypt.h | 2 -
arch/x86/Kconfig | 1 +
arch/x86/include/asm/io.h | 8 ++
arch/x86/include/asm/kexec.h | 2 +-
arch/x86/include/asm/mem_encrypt.h | 14 +---
arch/x86/kernel/Makefile | 3 +
arch/x86/kernel/cc_platform.c | 21 +++++
arch/x86/kernel/crash_dump_64.c | 4 +-
arch/x86/kernel/head64.c | 4 +-
arch/x86/kernel/kvm.c | 3 +-
arch/x86/kernel/kvmclock.c | 4 +-
arch/x86/kernel/machine_kexec_64.c | 19 +++--
arch/x86/kernel/pci-swiotlb.c | 9 +-
arch/x86/kernel/relocate_kernel_64.S | 2 +-
arch/x86/kernel/sev.c | 6 +-
arch/x86/kvm/svm/svm.c | 3 +-
arch/x86/mm/ioremap.c | 18 ++--
arch/x86/mm/mem_encrypt.c | 57 +++++++------
arch/x86/mm/mem_encrypt_identity.c | 3 +-
arch/x86/mm/pat/set_memory.c | 3 +-
arch/x86/platform/efi/efi_64.c | 9 +-
arch/x86/realmode/init.c | 8 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 +-
drivers/gpu/drm/drm_cache.c | 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 6 +-
drivers/iommu/amd/init.c | 7 +-
drivers/iommu/amd/iommu.c | 3 +-
drivers/iommu/amd/iommu_v2.c | 3 +-
drivers/iommu/iommu.c | 3 +-
fs/proc/vmcore.c | 6 +-
include/linux/cc_platform.h | 88 ++++++++++++++++++++
include/linux/mem_encrypt.h | 4 -
kernel/dma/swiotlb.c | 4 +-
40 files changed, 267 insertions(+), 114 deletions(-)
create mode 100644 arch/powerpc/platforms/pseries/cc_platform.c
create mode 100644 arch/x86/kernel/cc_platform.c
create mode 100644 include/linux/cc_platform.h
base-commit: 4b93c544e90e2b28326182d31ee008eb80e02074
--
2.33.0
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2021-09-08 22:59:06
In prep for other uses of the cc_platform_has() function besides AMD's
memory encryption support, selectively build the AMD memory encryption
architecture override functions only when CONFIG_AMD_MEM_ENCRYPT=y. These
functions are:
- early_memremap_pgprot_adjust()
- arch_memremap_can_ram_remap()
Additionally, routines that are only invoked by these architecture
override functions can also be conditionally built. These functions are:
- memremap_should_map_decrypted()
- memremap_is_efi_data()
- memremap_is_setup_data()
- early_memremap_is_setup_data()
And finally, phys_mem_access_encrypted() is conditionally built as well,
but requires a static inline version of it when CONFIG_AMD_MEM_ENCRYPT is
not set.
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
arch/x86/include/asm/io.h | 8 ++++++++
arch/x86/mm/ioremap.c | 2 +-
2 files changed, 9 insertions(+), 1 deletion(-)
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2021-09-08 22:59:18
In prep for other confidential computing technologies, introduce a generic
helper function, cc_platform_has(), that can be used to check for specific
active confidential computing attributes, like memory encryption. This is
intended to eliminate having to add multiple technology-specific checks to
the code (e.g. if (sev_active() || tdx_active())).
Co-developed-by: Andi Kleen <redacted>
Signed-off-by: Andi Kleen <redacted>
Co-developed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
arch/Kconfig | 3 ++
include/linux/cc_platform.h | 88 +++++++++++++++++++++++++++++++++++++
2 files changed, 91 insertions(+)
create mode 100644 include/linux/cc_platform.h
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2021-09-08 22:59:30
Introduce a powerpc version of the cc_platform_has() function. This will
be used to replace the powerpc mem_encrypt_active() implementation, so
the implementation will initially only support the CC_ATTR_MEM_ENCRYPT
attribute.
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
arch/powerpc/platforms/pseries/Kconfig | 1 +
arch/powerpc/platforms/pseries/Makefile | 2 ++
arch/powerpc/platforms/pseries/cc_platform.c | 26 ++++++++++++++++++++
3 files changed, 29 insertions(+)
create mode 100644 arch/powerpc/platforms/pseries/cc_platform.c
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2021-09-08 22:59:45
Replace uses of sme_active() with the more generic cc_platform_has()
using CC_ATTR_HOST_MEM_ENCRYPT. If future support is added for other
memory encryption technologies, the use of CC_ATTR_HOST_MEM_ENCRYPT
can be updated, as required.
This also replaces two usages of sev_active() that are really geared
towards detecting if SME is active.
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
arch/x86/include/asm/kexec.h | 2 +-
arch/x86/include/asm/mem_encrypt.h | 2 --
arch/x86/kernel/machine_kexec_64.c | 15 ++++++++-------
arch/x86/kernel/pci-swiotlb.c | 9 ++++-----
arch/x86/kernel/relocate_kernel_64.S | 2 +-
arch/x86/mm/ioremap.c | 6 +++---
arch/x86/mm/mem_encrypt.c | 15 +++++----------
arch/x86/mm/mem_encrypt_identity.c | 3 ++-
arch/x86/realmode/init.c | 5 +++--
drivers/iommu/amd/init.c | 7 ++++---
10 files changed, 31 insertions(+), 35 deletions(-)
@@ -144,7 +144,7 @@ void __init sme_unmap_bootdata(char *real_mode_data)structboot_params*boot_data;unsignedlongcmdline_paddr;-if(!sme_active())+if(!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))return;/* Get the command line address before unmapping the real_mode_data */
@@ -377,11 +377,6 @@ bool sev_active(void){returnsev_status&MSR_AMD64_SEV_ENABLED;}--boolsme_active(void)-{-returnsme_me_mask&&!sev_active();-}EXPORT_SYMBOL_GPL(sev_active);/* Needs to be called from non-instrumentable code */
@@ -964,7 +964,7 @@ static bool copy_device_table(void)pr_err("The address of old device table is above 4G, not trustworthy!\n");returnfalse;}-old_devtb=(sme_active()&&is_kdump_kernel())+old_devtb=(cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)&&is_kdump_kernel())?(__forcevoid*)ioremap_encrypted(old_devtb_phys,dev_table_size):memremap(old_devtb_phys,dev_table_size,MEMREMAP_WB);
@@ -3024,7 +3024,8 @@ static int __init amd_iommu_init(void)staticboolamd_iommu_sme_check(void){-if(!sme_active()||(boot_cpu_data.x86!=0x17))+if(!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)||+(boot_cpu_data.x86!=0x17))returntrue;/* For Fam17h, a specific level of support is required */
@@ -457,7 +458,7 @@ static int has_svm(void)return0;}-if(sev_active()){+if(cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)){pr_info("KVM is unsupported when running as an SEV guest\n");return0;}
@@ -92,7 +92,7 @@ static unsigned int __ioremap_check_ram(struct resource *res)*/staticunsignedint__ioremap_check_encrypted(structresource*res){-if(!sev_active())+if(!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))return0;switch(res->desc){
@@ -112,7 +112,7 @@ static unsigned int __ioremap_check_encrypted(struct resource *res)*/staticvoid__ioremap_check_other(resource_size_taddr,structioremap_desc*desc){-if(!sev_active())+if(!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))return;if(!IS_ENABLED(CONFIG_EFI))
@@ -556,7 +556,7 @@ static bool memremap_should_map_decrypted(resource_size_t phys_addr,caseE820_TYPE_NVS:caseE820_TYPE_UNUSABLE:/* For SEV, these areas are encrypted */-if(sev_active())+if(cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))break;fallthrough;
@@ -364,8 +364,8 @@ int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size)/**SMEandSEVareverysimilarbuttheyarenotthesame,sothereare*timesthatthekernelwillneedtodistinguishbetweenSMEandSEV.The-*sme_active()andsev_active()functionsareusedforthis.Whena-*distinctionisn'tneeded,themem_encrypt_active()functioncanbeused.+*cc_platform_has()functionisusedforthis.Whenadistinctionisn't+*needed,theCC_ATTR_MEM_ENCRYPTattributecanbeused.**Thetrampolinecodeisagoodexampleforthisrequirement.Before*pagingisactivated,SMEwillaccessallmemoryasdecrypted,butSEV
@@ -373,11 +373,6 @@ int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size)*upunderSMEthetrampolineareacannotbeencrypted,whereasunderSEV*thetrampolineareamustbeencrypted.*/-boolsev_active(void)-{-returnsev_status&MSR_AMD64_SEV_ENABLED;-}-EXPORT_SYMBOL_GPL(sev_active);/* Needs to be called from non-instrumentable code */boolnoinstrsev_es_active(void)
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2021-09-08 23:00:48
Replace uses of sev_es_active() with the more generic cc_platform_has()
using CC_ATTR_GUEST_STATE_ENCRYPT. If future support is added for other
memory encyrption techonologies, the use of CC_ATTR_GUEST_STATE_ENCRYPT
can be updated, as required.
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
arch/x86/include/asm/mem_encrypt.h | 2 --
arch/x86/kernel/sev.c | 6 +++---
arch/x86/mm/mem_encrypt.c | 14 ++++----------
arch/x86/realmode/init.c | 3 +--
4 files changed, 8 insertions(+), 17 deletions(-)
@@ -373,13 +373,6 @@ int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size)*upunderSMEthetrampolineareacannotbeencrypted,whereasunderSEV*thetrampolineareamustbeencrypted.*/--/* Needs to be called from non-instrumentable code */-boolnoinstrsev_es_active(void)-{-returnsev_status&MSR_AMD64_SEV_ES_ENABLED;-}-boolamd_cc_platform_has(enumcc_attrattr){switch(attr){
@@ -76,7 +77,7 @@ int set_memory_encrypted(unsigned long addr, int numpages)intset_memory_decrypted(unsignedlongaddr,intnumpages){-if(!mem_encrypt_active())+if(!cc_platform_has(CC_ATTR_MEM_ENCRYPT))return0;if(!PAGE_ALIGNED(addr))
@@ -285,7 +285,7 @@ unsigned long __head __startup_64(unsigned long physaddr,*thereisnoneedtozeroitafterchangingthememoryencryption*attribute.*/-if(mem_encrypt_active()){+if(cc_platform_has(CC_ATTR_MEM_ENCRYPT)){vaddr=(unsignedlong)__start_bss_decrypted;vaddr_end=(unsignedlong)__end_bss_decrypted;for(;vaddr<vaddr_end;vaddr+=PMD_SIZE){
@@ -1986,7 +1987,7 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)intret;/* Nothing to do if memory encryption is not active */-if(!mem_encrypt_active())+if(!cc_platform_has(CC_ATTR_MEM_ENCRYPT))return0;/* Should not be working on unaligned addresses */
@@ -1252,7 +1253,8 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,*however,SMErequiresanindirectIOMMUmappingbecausetheencryption*bitisbeyondtheDMAmaskofthechip.*/-if(mem_encrypt_active()&&((flags&AMD_ASIC_MASK)==CHIP_RAVEN)){+if(cc_platform_has(CC_ATTR_MEM_ENCRYPT)&&+((flags&AMD_ASIC_MASK)==CHIP_RAVEN)){dev_info(&pdev->dev,"SME is not compatible with RAVEN\n");return-ENOTSUPP;
@@ -666,7 +666,7 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)[vmw_dma_map_bind]="Giving up DMA mappings early."};/* TTM currently doesn't fully support SEV encryption. */-if(mem_encrypt_active())+if(cc_platform_has(CC_ATTR_MEM_ENCRYPT))return-EINVAL;if(vmw_force_coherent)
@@ -742,7 +743,7 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)*Whenmemoryencryptionisactivethedeviceislikelynotina*direct-mappeddomain.ForbidusingIOMMUv2functionalityfornow.*/-if(mem_encrypt_active())+if(cc_platform_has(CC_ATTR_MEM_ENCRYPT))return-ENODEV;if(!amd_iommu_v2_supported())
@@ -552,7 +552,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,if(!mem)panic("Can not allocate SWIOTLB buffer earlier and can't now provide you with the DMA bounce buffer");-if(mem_encrypt_active())+if(cc_platform_has(CC_ATTR_MEM_ENCRYPT))pr_warn_once("Memory encryption is active and system is using DMA bounce buffers\n");if(mapping_size>alloc_size){
@@ -76,7 +77,7 @@ int set_memory_encrypted(unsigned long addr, int numpages)intset_memory_decrypted(unsignedlongaddr,intnumpages){-if(!mem_encrypt_active())+if(!cc_platform_has(CC_ATTR_MEM_ENCRYPT))return0;if(!PAGE_ALIGNED(addr))
From: Christian Borntraeger <hidden> Date: 2021-09-09 07:33:52
On 09.09.21 00:58, Tom Lendacky wrote:
This patch series provides a generic helper function, cc_platform_has(),
to replace the sme_active(), sev_active(), sev_es_active() and
mem_encrypt_active() functions.
It is expected that as new confidential computing technologies are
added to the kernel, they can all be covered by a single function call
instead of a collection of specific function calls all called from the
same locations.
The powerpc and s390 patches have been compile tested only. Can the
folks copied on this series verify that nothing breaks for them.
Is there a tree somewhere?
Also,
a new file, arch/powerpc/platforms/pseries/cc_platform.c, has been
created for powerpc to hold the out of line function.
Cc: Andi Kleen <redacted>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Baoquan He <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Christian Borntraeger <redacted>
Cc: Daniel Vetter <redacted>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dave Young <redacted>
Cc: David Airlie <redacted>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Paul Mackerras <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <redacted>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: VMware Graphics <redacted>
Cc: Will Deacon <will@kernel.org>
Cc: Christoph Hellwig <hch@infradead.org>
---
Patches based on:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
4b93c544e90e ("thunderbolt: test: split up test cases in tb_test_credit_alloc_all")
Changes since v2:
- Changed the name from prot_guest_has() to cc_platform_has()
- Took the cc_platform_has() function out of line. Created two new files,
cc_platform.c, in both x86 and ppc to implment the function. As a
result, also changed the attribute defines into enums.
- Removed any received Reviewed-by's and Acked-by's given changes in this
version.
- Added removal of new instances of mem_encrypt_active() usage in powerpc
arch.
- Based on latest Linux tree to pick up powerpc changes related to the
mem_encrypt_active() function.
Changes since v1:
- Moved some arch ioremap functions within #ifdef CONFIG_AMD_MEM_ENCRYPT
in prep for use of prot_guest_has() by TDX.
- Added type includes to the the protected_guest.h header file to prevent
build errors outside of x86.
- Made amd_prot_guest_has() EXPORT_SYMBOL_GPL
- Used amd_prot_guest_has() in place of checking sme_me_mask in the
arch/x86/mm/mem_encrypt.c file.
Tom Lendacky (8):
x86/ioremap: Selectively build arch override encryption functions
mm: Introduce a function to check for confidential computing features
x86/sev: Add an x86 version of cc_platform_has()
powerpc/pseries/svm: Add a powerpc version of cc_platform_has()
x86/sme: Replace occurrences of sme_active() with cc_platform_has()
x86/sev: Replace occurrences of sev_active() with cc_platform_has()
x86/sev: Replace occurrences of sev_es_active() with cc_platform_has()
treewide: Replace the use of mem_encrypt_active() with
cc_platform_has()
arch/Kconfig | 3 +
arch/powerpc/include/asm/mem_encrypt.h | 5 --
arch/powerpc/platforms/pseries/Kconfig | 1 +
arch/powerpc/platforms/pseries/Makefile | 2 +
arch/powerpc/platforms/pseries/cc_platform.c | 26 ++++++
arch/powerpc/platforms/pseries/svm.c | 5 +-
arch/s390/include/asm/mem_encrypt.h | 2 -
arch/x86/Kconfig | 1 +
arch/x86/include/asm/io.h | 8 ++
arch/x86/include/asm/kexec.h | 2 +-
arch/x86/include/asm/mem_encrypt.h | 14 +---
arch/x86/kernel/Makefile | 3 +
arch/x86/kernel/cc_platform.c | 21 +++++
arch/x86/kernel/crash_dump_64.c | 4 +-
arch/x86/kernel/head64.c | 4 +-
arch/x86/kernel/kvm.c | 3 +-
arch/x86/kernel/kvmclock.c | 4 +-
arch/x86/kernel/machine_kexec_64.c | 19 +++--
arch/x86/kernel/pci-swiotlb.c | 9 +-
arch/x86/kernel/relocate_kernel_64.S | 2 +-
arch/x86/kernel/sev.c | 6 +-
arch/x86/kvm/svm/svm.c | 3 +-
arch/x86/mm/ioremap.c | 18 ++--
arch/x86/mm/mem_encrypt.c | 57 +++++++------
arch/x86/mm/mem_encrypt_identity.c | 3 +-
arch/x86/mm/pat/set_memory.c | 3 +-
arch/x86/platform/efi/efi_64.c | 9 +-
arch/x86/realmode/init.c | 8 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 +-
drivers/gpu/drm/drm_cache.c | 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 6 +-
drivers/iommu/amd/init.c | 7 +-
drivers/iommu/amd/iommu.c | 3 +-
drivers/iommu/amd/iommu_v2.c | 3 +-
drivers/iommu/iommu.c | 3 +-
fs/proc/vmcore.c | 6 +-
include/linux/cc_platform.h | 88 ++++++++++++++++++++
include/linux/mem_encrypt.h | 4 -
kernel/dma/swiotlb.c | 4 +-
40 files changed, 267 insertions(+), 114 deletions(-)
create mode 100644 arch/powerpc/platforms/pseries/cc_platform.c
create mode 100644 arch/x86/kernel/cc_platform.c
create mode 100644 include/linux/cc_platform.h
base-commit: 4b93c544e90e2b28326182d31ee008eb80e02074
This declaration make it impossible for architectures to define this
function inline.
For such function, having it inline would make more sense as it would
allow GCC to perform constant folding and avoid the overhead of calling
a sub-function.
Introduce a powerpc version of the cc_platform_has() function. This will
be used to replace the powerpc mem_encrypt_active() implementation, so
the implementation will initially only support the CC_ATTR_MEM_ENCRYPT
attribute.
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
arch/powerpc/platforms/pseries/Kconfig | 1 +
arch/powerpc/platforms/pseries/Makefile | 2 ++
arch/powerpc/platforms/pseries/cc_platform.c | 26 ++++++++++++++++++++
3 files changed, 29 insertions(+)
create mode 100644 arch/powerpc/platforms/pseries/cc_platform.c
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2021-09-09 13:16:55
On 9/9/21 2:32 AM, Christian Borntraeger wrote:
On 09.09.21 00:58, Tom Lendacky wrote:
quoted
This patch series provides a generic helper function, cc_platform_has(),
to replace the sme_active(), sev_active(), sev_es_active() and
mem_encrypt_active() functions.
It is expected that as new confidential computing technologies are
added to the kernel, they can all be covered by a single function call
instead of a collection of specific function calls all called from the
same locations.
The powerpc and s390 patches have been compile tested only. Can the
folks copied on this series verify that nothing breaks for them.
a new file, arch/powerpc/platforms/pseries/cc_platform.c, has been
created for powerpc to hold the out of line function.
Cc: Andi Kleen <redacted>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Baoquan He <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Christian Borntraeger <redacted>
Cc: Daniel Vetter <redacted>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dave Young <redacted>
Cc: David Airlie <redacted>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Paul Mackerras <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <redacted>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: VMware Graphics <redacted>
Cc: Will Deacon <will@kernel.org>
Cc: Christoph Hellwig <hch@infradead.org>
---
Patches based on:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git&data=04%7C01%7Cthomas.lendacky%40amd.com%7C5cd71ef2c2ce4b90060708d973640358%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637667695657121432%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=FVngrPSxCCRKutAaIMtU2Nk8WArFQB1dEE2wN7v8RgA%3D&reserved=0
master
4b93c544e90e ("thunderbolt: test: split up test cases in
tb_test_credit_alloc_all")
Changes since v2:
- Changed the name from prot_guest_has() to cc_platform_has()
- Took the cc_platform_has() function out of line. Created two new files,
cc_platform.c, in both x86 and ppc to implment the function. As a
result, also changed the attribute defines into enums.
- Removed any received Reviewed-by's and Acked-by's given changes in this
version.
- Added removal of new instances of mem_encrypt_active() usage in powerpc
arch.
- Based on latest Linux tree to pick up powerpc changes related to the
mem_encrypt_active() function.
Changes since v1:
- Moved some arch ioremap functions within #ifdef CONFIG_AMD_MEM_ENCRYPT
in prep for use of prot_guest_has() by TDX.
- Added type includes to the the protected_guest.h header file to prevent
build errors outside of x86.
- Made amd_prot_guest_has() EXPORT_SYMBOL_GPL
- Used amd_prot_guest_has() in place of checking sme_me_mask in the
arch/x86/mm/mem_encrypt.c file.
Tom Lendacky (8):
x86/ioremap: Selectively build arch override encryption functions
mm: Introduce a function to check for confidential computing features
x86/sev: Add an x86 version of cc_platform_has()
powerpc/pseries/svm: Add a powerpc version of cc_platform_has()
x86/sme: Replace occurrences of sme_active() with cc_platform_has()
x86/sev: Replace occurrences of sev_active() with cc_platform_has()
x86/sev: Replace occurrences of sev_es_active() with cc_platform_has()
treewide: Replace the use of mem_encrypt_active() with
cc_platform_has()
arch/Kconfig | 3 +
arch/powerpc/include/asm/mem_encrypt.h | 5 --
arch/powerpc/platforms/pseries/Kconfig | 1 +
arch/powerpc/platforms/pseries/Makefile | 2 +
arch/powerpc/platforms/pseries/cc_platform.c | 26 ++++++
arch/powerpc/platforms/pseries/svm.c | 5 +-
arch/s390/include/asm/mem_encrypt.h | 2 -
arch/x86/Kconfig | 1 +
arch/x86/include/asm/io.h | 8 ++
arch/x86/include/asm/kexec.h | 2 +-
arch/x86/include/asm/mem_encrypt.h | 14 +---
arch/x86/kernel/Makefile | 3 +
arch/x86/kernel/cc_platform.c | 21 +++++
arch/x86/kernel/crash_dump_64.c | 4 +-
arch/x86/kernel/head64.c | 4 +-
arch/x86/kernel/kvm.c | 3 +-
arch/x86/kernel/kvmclock.c | 4 +-
arch/x86/kernel/machine_kexec_64.c | 19 +++--
arch/x86/kernel/pci-swiotlb.c | 9 +-
arch/x86/kernel/relocate_kernel_64.S | 2 +-
arch/x86/kernel/sev.c | 6 +-
arch/x86/kvm/svm/svm.c | 3 +-
arch/x86/mm/ioremap.c | 18 ++--
arch/x86/mm/mem_encrypt.c | 57 +++++++------
arch/x86/mm/mem_encrypt_identity.c | 3 +-
arch/x86/mm/pat/set_memory.c | 3 +-
arch/x86/platform/efi/efi_64.c | 9 +-
arch/x86/realmode/init.c | 8 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 +-
drivers/gpu/drm/drm_cache.c | 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 6 +-
drivers/iommu/amd/init.c | 7 +-
drivers/iommu/amd/iommu.c | 3 +-
drivers/iommu/amd/iommu_v2.c | 3 +-
drivers/iommu/iommu.c | 3 +-
fs/proc/vmcore.c | 6 +-
include/linux/cc_platform.h | 88 ++++++++++++++++++++
include/linux/mem_encrypt.h | 4 -
kernel/dma/swiotlb.c | 4 +-
40 files changed, 267 insertions(+), 114 deletions(-)
create mode 100644 arch/powerpc/platforms/pseries/cc_platform.c
create mode 100644 arch/x86/kernel/cc_platform.c
create mode 100644 include/linux/cc_platform.h
base-commit: 4b93c544e90e2b28326182d31ee008eb80e02074
int set_memory_encrypted(unsigned long addr, int numpages)
{
- if (!mem_encrypt_active())
+ if (!cc_platform_has(CC_ATTR_MEM_ENCRYPT))
return 0;
if (!PAGE_ALIGNED(addr))
@@ -76,7 +77,7 @@ int set_memory_encrypted(unsigned long addr, int
numpages)
int set_memory_decrypted(unsigned long addr, int numpages)
{
- if (!mem_encrypt_active())
+ if (!cc_platform_has(CC_ATTR_MEM_ENCRYPT))
return 0;
if (!PAGE_ALIGNED(addr))
This change unnecessarily complexifies the two functions. This is due to
cc_platform_has() being out-line. It should really remain inline.
On Wed, Sep 08, 2021 at 05:58:33PM -0500, Tom Lendacky wrote:
In prep for other confidential computing technologies, introduce a generic
preparation
helper function, cc_platform_has(), that can be used to check for specific
active confidential computing attributes, like memory encryption. This is
intended to eliminate having to add multiple technology-specific checks to
the code (e.g. if (sev_active() || tdx_active())).
On Wed, Sep 08, 2021 at 05:58:35PM -0500, Tom Lendacky wrote:
Introduce a powerpc version of the cc_platform_has() function. This will
be used to replace the powerpc mem_encrypt_active() implementation, so
the implementation will initially only support the CC_ATTR_MEM_ENCRYPT
attribute.
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
arch/powerpc/platforms/pseries/Kconfig | 1 +
arch/powerpc/platforms/pseries/Makefile | 2 ++
arch/powerpc/platforms/pseries/cc_platform.c | 26 ++++++++++++++++++++
3 files changed, 29 insertions(+)
create mode 100644 arch/powerpc/platforms/pseries/cc_platform.c
Michael,
can I get an ACK for the ppc bits to carry them through the tip tree
pls?
Btw, on a related note, cross-compiling this throws the following error here:
$ make CROSS_COMPILE=/home/share/src/crosstool/gcc-9.4.0-nolibc/powerpc64-linux/bin/powerpc64-linux- V=1 ARCH=powerpc
...
/home/share/src/crosstool/gcc-9.4.0-nolibc/powerpc64-linux/bin/powerpc64-linux-gcc -Wp,-MD,arch/powerpc/boot/.crt0.o.d -D__ASSEMBLY__ -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -O2 -msoft-float -mno-altivec -mno-vsx -pipe -fomit-frame-pointer -fno-builtin -fPIC -nostdinc -include ./include/linux/compiler_attributes.h -I./arch/powerpc/include -I./arch/powerpc/include/generated -I./include -I./arch/powerpc/include/uapi -I./arch/powerpc/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -m32 -isystem /home/share/src/crosstool/gcc-9.4.0-nolibc/powerpc64-linux/bin/../lib/gcc/powerpc64-linux/9.4.0/include -mbig-endian -nostdinc -c -o arch/powerpc/boot/crt0.o arch/powerpc/boot/crt0.S
In file included from <command-line>:
././include/linux/compiler_attributes.h:62:5: warning: "__has_attribute" is not defined, evaluates to 0 [-Wundef]
62 | #if __has_attribute(__assume_aligned__)
| ^~~~~~~~~~~~~~~
././include/linux/compiler_attributes.h:62:20: error: missing binary operator before token "("
62 | #if __has_attribute(__assume_aligned__)
| ^
././include/linux/compiler_attributes.h:88:5: warning: "__has_attribute" is not defined, evaluates to 0 [-Wundef]
88 | #if __has_attribute(__copy__)
| ^~~~~~~~~~~~~~~
...
Known issue?
This __has_attribute() thing is supposed to be supported
in gcc since 5.1 and I'm using the crosstool stuff from
https://www.kernel.org/pub/tools/crosstool/ and gcc-9.4 above is pretty
new so that should not happen actually.
But it does...
Hmmm.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Sep 08, 2021 at 05:58:35PM -0500, Tom Lendacky wrote:
quoted
Introduce a powerpc version of the cc_platform_has() function. This will
be used to replace the powerpc mem_encrypt_active() implementation, so
the implementation will initially only support the CC_ATTR_MEM_ENCRYPT
attribute.
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
arch/powerpc/platforms/pseries/Kconfig | 1 +
arch/powerpc/platforms/pseries/Makefile | 2 ++
arch/powerpc/platforms/pseries/cc_platform.c | 26 ++++++++++++++++++++
3 files changed, 29 insertions(+)
create mode 100644 arch/powerpc/platforms/pseries/cc_platform.c
Michael,
can I get an ACK for the ppc bits to carry them through the tip tree
pls?
Btw, on a related note, cross-compiling this throws the following error here:
$ make CROSS_COMPILE=/home/share/src/crosstool/gcc-9.4.0-nolibc/powerpc64-linux/bin/powerpc64-linux- V=1 ARCH=powerpc
...
/home/share/src/crosstool/gcc-9.4.0-nolibc/powerpc64-linux/bin/powerpc64-linux-gcc -Wp,-MD,arch/powerpc/boot/.crt0.o.d -D__ASSEMBLY__ -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -O2 -msoft-float -mno-altivec -mno-vsx -pipe -fomit-frame-pointer -fno-builtin -fPIC -nostdinc -include ./include/linux/compiler_attributes.h -I./arch/powerpc/include -I./arch/powerpc/include/generated -I./include -I./arch/powerpc/include/uapi -I./arch/powerpc/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -m32 -isystem /home/share/src/crosstool/gcc-9.4.0-nolibc/powerpc64-linux/bin/../lib/gcc/powerpc64-linux/9.4.0/include -mbig-endian -nostdinc -c -o arch/powerpc/boot/crt0.o arch/powerpc/boot/crt0.S
In file included from <command-line>:
././include/linux/compiler_attributes.h:62:5: warning: "__has_attribute" is not defined, evaluates to 0 [-Wundef]
62 | #if __has_attribute(__assume_aligned__)
| ^~~~~~~~~~~~~~~
././include/linux/compiler_attributes.h:62:20: error: missing binary operator before token "("
62 | #if __has_attribute(__assume_aligned__)
| ^
././include/linux/compiler_attributes.h:88:5: warning: "__has_attribute" is not defined, evaluates to 0 [-Wundef]
88 | #if __has_attribute(__copy__)
| ^~~~~~~~~~~~~~~
...
Known issue?
This __has_attribute() thing is supposed to be supported
in gcc since 5.1 and I'm using the crosstool stuff from
https://www.kernel.org/pub/tools/crosstool/ and gcc-9.4 above is pretty
new so that should not happen actually.
But it does...
Hmmm.
@@ -144,7 +144,7 @@ void __init sme_unmap_bootdata(char *real_mode_data)structboot_params*boot_data;unsignedlongcmdline_paddr;-if(!sme_active())+if(!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))return;/* Get the command line address before unmapping the real_mode_data */
@@ -377,11 +377,6 @@ bool sev_active(void){returnsev_status&MSR_AMD64_SEV_ENABLED;}--boolsme_active(void)-{-returnsme_me_mask&&!sev_active();-}EXPORT_SYMBOL_GPL(sev_active);/* Needs to be called from non-instrumentable code */
@@ -364,8 +364,9 @@ int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size)/**SMEandSEVareverysimilarbuttheyarenotthesame,sothereare*timesthatthekernelwillneedtodistinguishbetweenSMEandSEV.The-*sme_active()andsev_active()functionsareusedforthis.Whena-*distinctionisn'tneeded,themem_encrypt_active()functioncanbeused.+*PATTR_HOST_MEM_ENCRYPTandPATTR_GUEST_MEM_ENCRYPTflagsto+*amd_prot_guest_has()areusedforthis.Whenadistinctionisn'tneeded,+*themem_encrypt_active()functioncanbeused.**Thetrampolinecodeisagoodexampleforthisrequirement.Before*pagingisactivated,SMEwillaccessallmemoryasdecrypted,butSEV
because there's still a sme_active() mentioned there:
$ git grep sme_active
arch/x86/mm/mem_encrypt.c:367: * sme_active() and sev_active() functions are used for this. When a
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-09-15 00:29:07
Borislav Petkov [off-list ref] writes:
On Wed, Sep 08, 2021 at 05:58:35PM -0500, Tom Lendacky wrote:
quoted
Introduce a powerpc version of the cc_platform_has() function. This will
be used to replace the powerpc mem_encrypt_active() implementation, so
the implementation will initially only support the CC_ATTR_MEM_ENCRYPT
attribute.
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
arch/powerpc/platforms/pseries/Kconfig | 1 +
arch/powerpc/platforms/pseries/Makefile | 2 ++
arch/powerpc/platforms/pseries/cc_platform.c | 26 ++++++++++++++++++++
3 files changed, 29 insertions(+)
create mode 100644 arch/powerpc/platforms/pseries/cc_platform.c
Michael,
can I get an ACK for the ppc bits to carry them through the tip tree
pls?
Yeah.
I don't love it, a new C file and an out-of-line call to then call back
to a static inline that for most configuration will return false ... but
whatever :)
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
Btw, on a related note, cross-compiling this throws the following error here:
$ make CROSS_COMPILE=/home/share/src/crosstool/gcc-9.4.0-nolibc/powerpc64-linux/bin/powerpc64-linux- V=1 ARCH=powerpc
...
/home/share/src/crosstool/gcc-9.4.0-nolibc/powerpc64-linux/bin/powerpc64-linux-gcc -Wp,-MD,arch/powerpc/boot/.crt0.o.d -D__ASSEMBLY__ -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -O2 -msoft-float -mno-altivec -mno-vsx -pipe -fomit-frame-pointer -fno-builtin -fPIC -nostdinc -include ./include/linux/compiler_attributes.h -I./arch/powerpc/include -I./arch/powerpc/include/generated -I./include -I./arch/powerpc/include/uapi -I./arch/powerpc/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -m32 -isystem /home/share/src/crosstool/gcc-9.4.0-nolibc/powerpc64-linux/bin/../lib/gcc/powerpc64-linux/9.4.0/include -mbig-endian -nostdinc -c -o arch/powerpc/boot/crt0.o arch/powerpc/boot/crt0.S
In file included from <command-line>:
././include/linux/compiler_attributes.h:62:5: warning: "__has_attribute" is not defined, evaluates to 0 [-Wundef]
62 | #if __has_attribute(__assume_aligned__)
| ^~~~~~~~~~~~~~~
././include/linux/compiler_attributes.h:62:20: error: missing binary operator before token "("
62 | #if __has_attribute(__assume_aligned__)
| ^
././include/linux/compiler_attributes.h:88:5: warning: "__has_attribute" is not defined, evaluates to 0 [-Wundef]
88 | #if __has_attribute(__copy__)
| ^~~~~~~~~~~~~~~
...
Known issue?
Yeah, fixed in mainline today, thanks for trying to cross compile :)
cheers
On Wed, Sep 15, 2021 at 10:28:59AM +1000, Michael Ellerman wrote:
I don't love it, a new C file and an out-of-line call to then call back
to a static inline that for most configuration will return false ... but
whatever :)
On Wed, Sep 08, 2021 at 05:58:31PM -0500, Tom Lendacky wrote:
This patch series provides a generic helper function, cc_platform_has(),
to replace the sme_active(), sev_active(), sev_es_active() and
mem_encrypt_active() functions.
It is expected that as new confidential computing technologies are
added to the kernel, they can all be covered by a single function call
instead of a collection of specific function calls all called from the
same locations.
The powerpc and s390 patches have been compile tested only. Can the
folks copied on this series verify that nothing breaks for them. Also,
a new file, arch/powerpc/platforms/pseries/cc_platform.c, has been
created for powerpc to hold the out of line function.
...
Tom Lendacky (8):
x86/ioremap: Selectively build arch override encryption functions
mm: Introduce a function to check for confidential computing features
x86/sev: Add an x86 version of cc_platform_has()
powerpc/pseries/svm: Add a powerpc version of cc_platform_has()
x86/sme: Replace occurrences of sme_active() with cc_platform_has()
x86/sev: Replace occurrences of sev_active() with cc_platform_has()
x86/sev: Replace occurrences of sev_es_active() with cc_platform_has()
treewide: Replace the use of mem_encrypt_active() with
cc_platform_has()
Ok, modulo the minor things the plan is to take this through tip after
-rc2 releases in order to pick up the powerpc build fix and have a clean
base (-rc2) to base stuff on, at the same time.
Pls holler if something's still amiss.
Sathya,
if you want to prepare the Intel variant intel_cc_platform_has() ontop
of those and send it to me, that would be good because then I can
integrate it all in one branch which can be used to base future work
ontop.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Sep 15, 2021 at 10:28:59AM +1000, Michael Ellerman wrote:
quoted
I don't love it, a new C file and an out-of-line call to then call back
to a static inline that for most configuration will return false ... but
whatever :)
Could you please provide more explicit explanation why inlining such an
helper is considered as bad practice and messy ?
Because as demonstrated in my previous response some days ago, taking
that outline ends up with an unneccessary ugly generated code and we
don't benefit front GCC's capability to fold in and opt out unreachable
code.
As pointed by Michael in most cases the function will just return false
so behind the performance concern, there is also the code size and code
coverage topic that is to be taken into account. And even when the
function doesn't return false, the only thing it does folds into a
single powerpc instruction so there is really no point in making a
dedicated out-of-line fonction for that and suffer the cost and the size
of a function call and to justify the addition of a dedicated C file.
I guess less ifdeffery is nice too.
I can't see your point here. Inlining the function wouldn't add any
ifdeffery as far as I can see.
So, would you mind reconsidering your approach and allow architectures
to provide inline implementation by just not enforcing a generic
prototype ? Or otherwise provide more details and exemple of why the
cons are more important versus the pros ?
Thanks
Christophe
Sathya,
if you want to prepare the Intel variant intel_cc_platform_has() ontop
of those and send it to me, that would be good because then I can
integrate it all in one branch which can be used to base future work
ontop.
I have a Intel variant patch (please check following patch). But it includes
TDX changes as well. Shall I move TDX changes to different patch and just
create a separate patch for adding intel_cc_platform_has()?
commit fc5f98a0ed94629d903827c5b44ee9295f835831
Author: Kuppuswamy Sathyanarayanan [off-list ref]
Date: Wed May 12 11:35:13 2021 -0700
x86/tdx: Add confidential guest support for TDX guest
TDX architecture provides a way for VM guests to be highly secure and
isolated (from untrusted VMM). To achieve this requirement, any data
coming from VMM cannot be completely trusted. TDX guest fixes this
issue by hardening the IO drivers against the attack from the VMM.
So, when adding hardening fixes to the generic drivers, to protect
custom fixes use cc_platform_has() API.
Also add TDX guest support to cc_platform_has() API to protect the
TDX specific fixes.
Signed-off-by: Kuppuswamy Sathyanarayanan [off-list ref]
On Wed, Sep 15, 2021 at 07:18:34PM +0200, Christophe Leroy wrote:
Could you please provide more explicit explanation why inlining such an
helper is considered as bad practice and messy ?
Tom already told you to look at the previous threads. Let's read them
together. This one, for example:
https://lore.kernel.org/lkml/YSScWvpXeVXw%2Fed5@infradead.org/
| > To take it out of line, I'm leaning towards the latter, creating a new
| > file that is built based on the ARCH_HAS_PROTECTED_GUEST setting.
|
| Yes. In general everytime architectures have to provide the prototype
| and not just the implementation of something we end up with a giant mess
| sooner or later. In a few cases that is still warranted due to
| performance concerns, but i don't think that is the case here.
So I think what Christoph means here is that you want to have the
generic prototype defined in a header and arches get to implement it
exactly to the letter so that there's no mess.
As to what mess exactly, I'd let him explain that.
Because as demonstrated in my previous response some days ago, taking that
outline ends up with an unneccessary ugly generated code and we don't
benefit front GCC's capability to fold in and opt out unreachable code.
And this is real fast path where a couple of instructions matter or what?
set_memory_encrypted/_decrypted doesn't look like one to me.
I can't see your point here. Inlining the function wouldn't add any
ifdeffery as far as I can see.
If the function is touching defines etc, they all need to be visible.
If that function needs to call other functions - which is the case on
x86, perhaps not so much on power - then you need to either ifdef around
them or provide stubs with ifdeffery in the headers. And you need to
make them global functions instead of keeping them static to the same
compilation unit, etc, etc.
With a separate compilation unit, you don't need any of that and it is
all kept in that single file.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
From: Christoph Hellwig <hch@infradead.org> Date: 2021-09-16 07:37:21
On Wed, Sep 15, 2021 at 07:18:34PM +0200, Christophe Leroy wrote:
Could you please provide more explicit explanation why inlining such an
helper is considered as bad practice and messy ?
Because now we get architectures to all subly differ. Look at the mess
for ioremap and the ioremap* variant.
The only good reason to allow for inlines if if they are used in a hot
path. Which cc_platform_has is not, especially not on powerpc.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-09-16 11:51:42
Christoph Hellwig [off-list ref] writes:
On Wed, Sep 15, 2021 at 07:18:34PM +0200, Christophe Leroy wrote:
quoted
Could you please provide more explicit explanation why inlining such an
helper is considered as bad practice and messy ?
Because now we get architectures to all subly differ. Look at the mess
for ioremap and the ioremap* variant.
The only good reason to allow for inlines if if they are used in a hot
path. Which cc_platform_has is not, especially not on powerpc.
Yes I agree, it's not a hot path so it doesn't really matter, which is
why I Acked it.
I think it is possible to do both, share the declaration across arches
but also give arches flexibility to use an inline if they prefer, see
patch below.
I'm not suggesting we actually do that for this series now, but I think
it would solve the problem if we ever needed to in future.
cheers
diff --git a/arch/powerpc/platforms/pseries/cc_platform.c b/arch/powerpc/include/asm/cc_platform.hsimilarity index 74%rename from arch/powerpc/platforms/pseries/cc_platform.crename to arch/powerpc/include/asm/cc_platform.hindex e8021af83a19..6285c3c385a6 100644--- a/arch/powerpc/platforms/pseries/cc_platform.c+++ b/arch/powerpc/include/asm/cc_platform.h
On Wed, Sep 15, 2021 at 10:26:06AM -0700, Kuppuswamy, Sathyanarayanan wrote:
I have a Intel variant patch (please check following patch). But it includes
TDX changes as well. Shall I move TDX changes to different patch and just
create a separate patch for adding intel_cc_platform_has()?
Yes, please, so that I can expedite that stuff separately and so that it
can go in early in order for future work to be based ontop.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Sep 15, 2021 at 10:26:06AM -0700, Kuppuswamy, Sathyanarayanan wrote:
quoted
I have a Intel variant patch (please check following patch). But it includes
TDX changes as well. Shall I move TDX changes to different patch and just
create a separate patch for adding intel_cc_platform_has()?
Yes, please, so that I can expedite that stuff separately and so that it
can go in early in order for future work to be based ontop.
This change break boot for me (in KVM on Intel host). It only reproduces
with allyesconfig. More reasonable config works fine, but I didn't try to
find exact cause in config.
Convertion to cc_platform_has() in __startup_64() in 8/8 has the same
effect.
I believe it caused by sme_me_mask access from __startup_64() without
fixup_pointer() magic. I think __startup_64() requires special treatement
and we should avoid cc_platform_has() there (or have a special version of
the helper). Note that only AMD requires these cc_platform_has() to return
true.
--
Kirill A. Shutemov
This change break boot for me (in KVM on Intel host). It only reproduces
with allyesconfig. More reasonable config works fine, but I didn't try to
find exact cause in config.
Looks like instrumentation during early boot. I worked with Boris offline
to exclude arch/x86/kernel/cc_platform.c from some of the instrumentation
and that allowed an allyesconfig to boot.
Thanks,
Tom
Convertion to cc_platform_has() in __startup_64() in 8/8 has the same
effect.
I believe it caused by sme_me_mask access from __startup_64() without
fixup_pointer() magic. I think __startup_64() requires special treatement
and we should avoid cc_platform_has() there (or have a special version of
the helper). Note that only AMD requires these cc_platform_has() to return
true.
On Tue, Sep 21, 2021 at 12:04:58PM -0500, Tom Lendacky wrote:
Looks like instrumentation during early boot. I worked with Boris offline to
exclude arch/x86/kernel/cc_platform.c from some of the instrumentation and
that allowed an allyesconfig to boot.
From: Kirill A. Shutemov <hidden> Date: 2021-09-21 21:22:55
On Tue, Sep 21, 2021 at 07:47:15PM +0200, Borislav Petkov wrote:
On Tue, Sep 21, 2021 at 12:04:58PM -0500, Tom Lendacky wrote:
quoted
Looks like instrumentation during early boot. I worked with Boris offline to
exclude arch/x86/kernel/cc_platform.c from some of the instrumentation and
that allowed an allyesconfig to boot.
Still broken for me with allyesconfig.
gcc version 11.2.0 (Gentoo 11.2.0 p1)
GNU ld (Gentoo 2.37_p1 p0) 2.37
I still believe calling cc_platform_has() from __startup_64() is totally
broken as it lacks proper wrapping while accessing global variables.
I think sme_get_me_mask() has the same problem. I just happened to work
(until next compiler update).
This hack makes kernel boot again:
@@ -285,7 +285,7 @@ unsigned long __head __startup_64(unsigned long physaddr,*thereisnoneedtozeroitafterchangingthememoryencryption*attribute.*/-if(cc_platform_has(CC_ATTR_MEM_ENCRYPT)){+if(0&&cc_platform_has(CC_ATTR_MEM_ENCRYPT)){vaddr=(unsignedlong)__start_bss_decrypted;vaddr_end=(unsignedlong)__end_bss_decrypted;for(;vaddr<vaddr_end;vaddr+=PMD_SIZE){
On Wed, Sep 22, 2021 at 12:20:59AM +0300, Kirill A. Shutemov wrote:
I still believe calling cc_platform_has() from __startup_64() is totally
broken as it lacks proper wrapping while accessing global variables.
Well, one of the issues on the AMD side was using boot_cpu_data too
early and the Intel side uses it too. Can you replace those checks with
is_tdx_guest() or whatever was the helper's name which would check
whether the the kernel is running as a TDX guest, and see if that helps?
Thx.
--
Regards/Gruss,
Boris.
From: Kirill A. Shutemov <hidden> Date: 2021-09-21 21:34:06
On Tue, Sep 21, 2021 at 11:27:17PM +0200, Borislav Petkov wrote:
On Wed, Sep 22, 2021 at 12:20:59AM +0300, Kirill A. Shutemov wrote:
quoted
I still believe calling cc_platform_has() from __startup_64() is totally
broken as it lacks proper wrapping while accessing global variables.
Well, one of the issues on the AMD side was using boot_cpu_data too
early and the Intel side uses it too. Can you replace those checks with
is_tdx_guest() or whatever was the helper's name which would check
whether the the kernel is running as a TDX guest, and see if that helps?
There's no need in Intel check this early. Only AMD need it. Maybe just
opencode them?
--
Kirill A. Shutemov
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2021-09-21 21:44:09
On 9/21/21 4:34 PM, Kirill A. Shutemov wrote:
On Tue, Sep 21, 2021 at 11:27:17PM +0200, Borislav Petkov wrote:
quoted
On Wed, Sep 22, 2021 at 12:20:59AM +0300, Kirill A. Shutemov wrote:
quoted
I still believe calling cc_platform_has() from __startup_64() is totally
broken as it lacks proper wrapping while accessing global variables.
Well, one of the issues on the AMD side was using boot_cpu_data too
early and the Intel side uses it too. Can you replace those checks with
is_tdx_guest() or whatever was the helper's name which would check
whether the the kernel is running as a TDX guest, and see if that helps?
There's no need in Intel check this early. Only AMD need it. Maybe just
opencode them?
Any way you can put a gzipped/bzipped copy of your vmlinux file somewhere
I can grab it from and take a look at it?
Thanks,
Tom
From: Kirill A. Shutemov <hidden> Date: 2021-09-21 21:58:38
On Tue, Sep 21, 2021 at 04:43:59PM -0500, Tom Lendacky wrote:
On 9/21/21 4:34 PM, Kirill A. Shutemov wrote:
quoted
On Tue, Sep 21, 2021 at 11:27:17PM +0200, Borislav Petkov wrote:
quoted
On Wed, Sep 22, 2021 at 12:20:59AM +0300, Kirill A. Shutemov wrote:
quoted
I still believe calling cc_platform_has() from __startup_64() is totally
broken as it lacks proper wrapping while accessing global variables.
Well, one of the issues on the AMD side was using boot_cpu_data too
early and the Intel side uses it too. Can you replace those checks with
is_tdx_guest() or whatever was the helper's name which would check
whether the the kernel is running as a TDX guest, and see if that helps?
There's no need in Intel check this early. Only AMD need it. Maybe just
opencode them?
Any way you can put a gzipped/bzipped copy of your vmlinux file somewhere I
can grab it from and take a look at it?
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2021-09-22 13:41:09
On 9/21/21 4:58 PM, Kirill A. Shutemov wrote:
On Tue, Sep 21, 2021 at 04:43:59PM -0500, Tom Lendacky wrote:
quoted
On 9/21/21 4:34 PM, Kirill A. Shutemov wrote:
quoted
On Tue, Sep 21, 2021 at 11:27:17PM +0200, Borislav Petkov wrote:
quoted
On Wed, Sep 22, 2021 at 12:20:59AM +0300, Kirill A. Shutemov wrote:
quoted
I still believe calling cc_platform_has() from __startup_64() is totally
broken as it lacks proper wrapping while accessing global variables.
Well, one of the issues on the AMD side was using boot_cpu_data too
early and the Intel side uses it too. Can you replace those checks with
is_tdx_guest() or whatever was the helper's name which would check
whether the the kernel is running as a TDX guest, and see if that helps?
There's no need in Intel check this early. Only AMD need it. Maybe just
opencode them?
Any way you can put a gzipped/bzipped copy of your vmlinux file somewhere I
can grab it from and take a look at it?
Looking at everything, it is all RIP relative addressing, so those
accesses should be fine. Your image has the intel_cc_platform_has()
function, does it work if you remove that call? Because I think it may be
the early call into that function which looks like it has instrumentation
that uses %gs in __sanitizer_cov_trace_pc and %gs is not setup properly
yet. And since boot_cpu_data.x86_vendor will likely be zero this early it
will match X86_VENDOR_INTEL and call into that function.
ffffffff8124f880 <intel_cc_platform_has>:
ffffffff8124f880: e8 bb 64 06 00 callq ffffffff812b5d40 <__fentry__>
ffffffff8124f885: e8 36 ca 42 00 callq ffffffff8167c2c0 <__sanitizer_cov_trace_pc>
ffffffff8124f88a: 31 c0 xor %eax,%eax
ffffffff8124f88c: c3 retq
ffffffff8167c2c0 <__sanitizer_cov_trace_pc>:
ffffffff8167c2c0: 65 8b 05 39 ad 9a 7e mov %gs:0x7e9aad39(%rip),%eax # 27000 <__preempt_count>
ffffffff8167c2c7: 89 c6 mov %eax,%esi
ffffffff8167c2c9: 48 8b 0c 24 mov (%rsp),%rcx
ffffffff8167c2cd: 81 e6 00 01 00 00 and $0x100,%esi
ffffffff8167c2d3: 65 48 8b 14 25 40 70 mov %gs:0x27040,%rdx
Thanks,
Tom
From: Kirill A. Shutemov <hidden> Date: 2021-09-22 14:31:55
On Wed, Sep 22, 2021 at 08:40:43AM -0500, Tom Lendacky wrote:
On 9/21/21 4:58 PM, Kirill A. Shutemov wrote:
quoted
On Tue, Sep 21, 2021 at 04:43:59PM -0500, Tom Lendacky wrote:
quoted
On 9/21/21 4:34 PM, Kirill A. Shutemov wrote:
quoted
On Tue, Sep 21, 2021 at 11:27:17PM +0200, Borislav Petkov wrote:
quoted
On Wed, Sep 22, 2021 at 12:20:59AM +0300, Kirill A. Shutemov wrote:
quoted
I still believe calling cc_platform_has() from __startup_64() is totally
broken as it lacks proper wrapping while accessing global variables.
Well, one of the issues on the AMD side was using boot_cpu_data too
early and the Intel side uses it too. Can you replace those checks with
is_tdx_guest() or whatever was the helper's name which would check
whether the the kernel is running as a TDX guest, and see if that helps?
There's no need in Intel check this early. Only AMD need it. Maybe just
opencode them?
Any way you can put a gzipped/bzipped copy of your vmlinux file somewhere I
can grab it from and take a look at it?
Looking at everything, it is all RIP relative addressing, so those
accesses should be fine.
Not fine, but waiting to blowup with random build environment change.
Your image has the intel_cc_platform_has()
function, does it work if you remove that call? Because I think it may be
the early call into that function which looks like it has instrumentation
that uses %gs in __sanitizer_cov_trace_pc and %gs is not setup properly
yet. And since boot_cpu_data.x86_vendor will likely be zero this early it
will match X86_VENDOR_INTEL and call into that function.
Right removing call to intel_cc_platform_has() or moving it to
cc_platform.c fixes the issue.
--
Kirill A. Shutemov
From: Kirill A. Shutemov <hidden> Date: 2021-09-22 21:06:01
On Wed, Sep 22, 2021 at 09:52:07PM +0200, Borislav Petkov wrote:
On Wed, Sep 22, 2021 at 05:30:15PM +0300, Kirill A. Shutemov wrote:
quoted
Not fine, but waiting to blowup with random build environment change.
Why is it not fine?
Are you suspecting that the compiler might generate something else and
not a rip-relative access?
Yes. We had it before for __supported_pte_mask and other users of
fixup_pointer().
See for instance 4a09f0210c8b ("x86/boot/64/clang: Use fixup_pointer() to
access '__supported_pte_mask'")
Unless we find other way to guarantee RIP-relative access, we must use
fixup_pointer() to access any global variables.
--
Kirill A. Shutemov
On Thu, Sep 23, 2021 at 12:05:58AM +0300, Kirill A. Shutemov wrote:
Unless we find other way to guarantee RIP-relative access, we must use
fixup_pointer() to access any global variables.
Yah, I've asked compiler folks about any guarantees we have wrt
rip-relative addresses but it doesn't look good. Worst case, we'd have
to do the fixup_pointer() thing.
In the meantime, Tom and I did some more poking at this and here's a
diff ontop.
The direction being that we'll stick both the AMD and Intel
*cc_platform_has() call into cc_platform.c for which instrumentation
will be disabled so no issues with that.
And that will keep all that querying all together in a single file.
---
@@ -361,38 +361,6 @@ int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size)returnearly_set_memory_enc_dec(vaddr,size,true);}-/*-*SMEandSEVareverysimilarbuttheyarenotthesame,sothereare-*timesthatthekernelwillneedtodistinguishbetweenSMEandSEV.The-*cc_platform_has()functionisusedforthis.Whenadistinctionisn't-*needed,theCC_ATTR_MEM_ENCRYPTattributecanbeused.-*-*Thetrampolinecodeisagoodexampleforthisrequirement.Before-*pagingisactivated,SMEwillaccessallmemoryasdecrypted,butSEV-*willaccessallmemoryasencrypted.So,whenAPsarebeingbrought-*upunderSMEthetrampolineareacannotbeencrypted,whereasunderSEV-*thetrampolineareamustbeencrypted.-*/-boolamd_cc_platform_has(enumcc_attrattr)-{-switch(attr){-caseCC_ATTR_MEM_ENCRYPT:-returnsme_me_mask;--caseCC_ATTR_HOST_MEM_ENCRYPT:-returnsme_me_mask&&!(sev_status&MSR_AMD64_SEV_ENABLED);--caseCC_ATTR_GUEST_MEM_ENCRYPT:-returnsev_status&MSR_AMD64_SEV_ENABLED;--caseCC_ATTR_GUEST_STATE_ENCRYPT:-returnsev_status&MSR_AMD64_SEV_ES_ENABLED;--default:-returnfalse;-}-}-/* Override for DMA direct allocation check - ARCH_HAS_FORCE_DMA_UNENCRYPTED */boolforce_dma_unencrypted(structdevice*dev){
From: Kirill A. Shutemov <hidden> Date: 2021-09-24 09:41:38
On Thu, Sep 23, 2021 at 08:21:03PM +0200, Borislav Petkov wrote:
On Thu, Sep 23, 2021 at 12:05:58AM +0300, Kirill A. Shutemov wrote:
quoted
Unless we find other way to guarantee RIP-relative access, we must use
fixup_pointer() to access any global variables.
Yah, I've asked compiler folks about any guarantees we have wrt
rip-relative addresses but it doesn't look good. Worst case, we'd have
to do the fixup_pointer() thing.
In the meantime, Tom and I did some more poking at this and here's a
diff ontop.
The direction being that we'll stick both the AMD and Intel
*cc_platform_has() call into cc_platform.c for which instrumentation
will be disabled so no issues with that.
And that will keep all that querying all together in a single file.
And still do cc_platform_has() calls in __startup_64() codepath?
It's broken.
Intel detection in cc_platform_has() relies on boot_cpu_data.x86_vendor
which is not initialized until early_cpu_init() in setup_arch(). Given
that X86_VENDOR_INTEL is 0 it leads to false-positive.
I think opencode these two calls is the way forward. Maybe also move the
check from sme_encrypt_kernel() to __startup_64().
--
Kirill A. Shutemov
On Fri, Sep 24, 2021 at 12:41:32PM +0300, Kirill A. Shutemov wrote:
On Thu, Sep 23, 2021 at 08:21:03PM +0200, Borislav Petkov wrote:
quoted
On Thu, Sep 23, 2021 at 12:05:58AM +0300, Kirill A. Shutemov wrote:
quoted
Unless we find other way to guarantee RIP-relative access, we must use
fixup_pointer() to access any global variables.
Yah, I've asked compiler folks about any guarantees we have wrt
rip-relative addresses but it doesn't look good. Worst case, we'd have
to do the fixup_pointer() thing.
In the meantime, Tom and I did some more poking at this and here's a
diff ontop.
The direction being that we'll stick both the AMD and Intel
*cc_platform_has() call into cc_platform.c for which instrumentation
will be disabled so no issues with that.
And that will keep all that querying all together in a single file.
And still do cc_platform_has() calls in __startup_64() codepath?
It's broken.
Intel detection in cc_platform_has() relies on boot_cpu_data.x86_vendor
which is not initialized until early_cpu_init() in setup_arch(). Given
that X86_VENDOR_INTEL is 0 it leads to false-positive.
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2021-09-24 13:31:58
On 9/24/21 4:51 AM, Borislav Petkov wrote:
On Fri, Sep 24, 2021 at 12:41:32PM +0300, Kirill A. Shutemov wrote:
quoted
On Thu, Sep 23, 2021 at 08:21:03PM +0200, Borislav Petkov wrote:
quoted
On Thu, Sep 23, 2021 at 12:05:58AM +0300, Kirill A. Shutemov wrote:
quoted
Unless we find other way to guarantee RIP-relative access, we must use
fixup_pointer() to access any global variables.
Yah, I've asked compiler folks about any guarantees we have wrt
rip-relative addresses but it doesn't look good. Worst case, we'd have
to do the fixup_pointer() thing.
In the meantime, Tom and I did some more poking at this and here's a
diff ontop.
The direction being that we'll stick both the AMD and Intel
*cc_platform_has() call into cc_platform.c for which instrumentation
will be disabled so no issues with that.
And that will keep all that querying all together in a single file.
And still do cc_platform_has() calls in __startup_64() codepath?
It's broken.
Intel detection in cc_platform_has() relies on boot_cpu_data.x86_vendor
which is not initialized until early_cpu_init() in setup_arch(). Given
that X86_VENDOR_INTEL is 0 it leads to false-positive.
Yeah, Tom, I had the same question yesterday.
/me looks in his direction.
Yup, all the things we talked about.
But we also know that cc_platform_has() gets called a few other times
before boot_cpu_data is initialized - so more false-positives. For
cc_platform_has() to work properly, given the desire to consolidate the
calls, IMO, Intel will have to come up with some early setting that can be
enabled and checked in place of boot_cpu_data or else you live with
false-positives.
Thanks,
Tom