Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
On 9/24/21 4:51 AM, Borislav Petkov wrote: 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: 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. 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 :-) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
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: > > 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. > > 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. :-) -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
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: > > 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
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.
---
diff --git a/arch/x86/include/asm/mem_encrypt.h
b/arch/x86/include/asm/mem_encrypt.h
index a73712b6ee0e..2d4f5c17d79c 100644
--- a/arch/x86/include/asm/mem_encrypt.h
+++ b/arch/x86/include/asm/mem_encrypt.h
@@ -51,7 +51,6 @@ void __init mem_encrypt_free_decrypted_mem(void);
void __init mem_encrypt_init(void);
void __init sev_es_init_vc_handling(void);
-bool amd_cc_platform_has(enum cc_attr attr);
#define __bss_decrypted __section(".bss..decrypted")
@@ -74,7 +73,6 @@ static inline void __init sme_encrypt_kernel(struct
boot_params *bp) { }
static inline void __init sme_enable(struct boot_params *bp) { }
static inline void sev_es_init_vc_handling(void) { }
-static inline bool amd_cc_platform_has(enum cc_attr attr) { return false; }
static inline int __init
early_set_memory_decrypted(unsigned long vaddr, unsigned long size) { return
0; }
@@ -103,12 +101,6 @@ static inline u64 sme_get_me_mask(void)
return sme_me_mask;
}
-#if defined(CONFIG_CPU_SUP_INTEL) && defined(CONFIG_ARCH_HAS_CC_PLATFORM)
-bool intel_cc_platform_has(enum cc_attr attr);
-#else
-static inline bool intel_cc_platform_has(enum cc_attr attr) { return false; }
-#endif
-
#endif /* __ASSEMBLY__ */
#endif /* __X86_MEM_ENCRYPT_H__ */
diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
index da54a1805211..97ede7052f77 100644
--- a/arch/x86/kernel/cc_platform.c
+++ b/arch/x86/kernel/cc_platform.c
@@ -13,6 +13,52 @@
#include
+static bool intel_cc_platform_has(enum cc_attr attr)
+{
+#ifdef CONFIG_INTEL_TDX_GUEST
+ return false;
+#else
+ return false;
+#endif
+}
+
+/*
+ * SME and SEV are very similar but they are not the same, so there are
+ * times that the kernel will need to distinguish between SME and SEV. The
+ * cc_platform_has() function is used for this. When a distinction isn't
+ * needed, the CC_ATTR_MEM_ENCRYPT attribute can be used.
+ *
+ * The trampoline code is a good example for this requirement. Before
+ * paging is activated, SME will access all memory as decrypted, but SEV
+ * will access all memory as encrypted. So, when APs are being brought
+ * up under SME the trampoline area cannot be encrypted, whereas under SEV
+ * the trampoline area must be encrypted.
+ */
+static bool amd_cc_platform_has(enum cc_attr attr)
+{
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+ switch (attr) {
+ case CC_ATTR_MEM_ENCRYPT:
+ return sme_me_mask;
+
+ case CC_ATTR_HOST_MEM_ENCRYPT:
+ return sme_me_mask && !(sev_status & MSR_AMD64_SEV_ENABLED);
+
+ case CC_ATTR_GUEST_MEM_ENCRYPT:
+ return sev_status & MSR_AMD64_SEV_ENABLED;
+
+ case CC_ATTR_GUEST_STATE_ENCRYPT:
+ return sev_status & MSR_AMD64_SEV_ES_ENABLED;
+
+ default:
+ return false;
+ }
+#else
+ return false;
+#endif
+}
+
+
bool cc_platform_has(enum cc_attr attr)
{
if (sme_me_mask)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 53756ff12295..8321c43554a1 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -60,13 +60,6 @@ static u64 msr_test_ctrl_cache __ro_after_init;
*/
static bool cpu_model_supports_sld __ro_after_init;
-#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
-bool intel_cc_platform_has(enum cc_attr attr)
-{
- return false;
-}
-#endif
-
/*
* Processors which have self-snooping capability can handle conflicting
* memory type across CPUs by snooping its own cache. However, there exists
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 9417d404ea92..23d54b810f08 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -361,38 +361,6 @@ int __init early_set_memory_encrypted(unsigned long vaddr,
unsigned long size)
return early_set_memory_enc_dec(vaddr, size, true);
}
-/*
- * SME and SEV are very similar but they are not the same, so there are
- * times that the kernel will need to distinguish between SME and SEV. The
- * cc_platform_has() function is used for this. When a distinction isn't
- * needed, the CC_ATTR_MEM_ENCRYPT attribute can be used.
- *
- * The trampoline code is a good example for this requirement. Before
- * paging is activated, SME will access all memory as decrypted, but SEV
- * will access all memory
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
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:
> > 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
On Wed, Sep 22, 2021 at 05:30:15PM +0300, Kirill A. Shutemov wrote: > 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? -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
On Wed, Sep 22, 2021 at 08:40:43AM -0500, Tom Lendacky wrote: > On 9/21/21 4:58 PM, Kirill A. Shutemov wrote: > > On Tue, Sep 21, 2021 at 04:43:59PM -0500, Tom Lendacky wrote: > > > On 9/21/21 4:34 PM, Kirill A. Shutemov wrote: > > > > 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: > > > > > > 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? > > > > You can find broken vmlinux and bzImage here: > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdrive.google.com%2Fdrive%2Ffolders%2F1n74vUQHOGebnF70Im32qLFY8iS3wvjIs%3Fusp%3Dsharingdata=04%7C01%7Cthomas.lendacky%40amd.com%7C1c7adf380cbe4c1a6bb708d97d4af6ff%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637678583935705530%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=gA30x%2Bfu97tUx0p2UqI8HgjiL8bxDbK1GqgJBbUrUE4%3Dreserved=0 > > > > Let me know when I can remove 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
On 9/21/21 4:58 PM, Kirill A. Shutemov wrote: On Tue, Sep 21, 2021 at 04:43:59PM -0500, Tom Lendacky wrote: On 9/21/21 4:34 PM, Kirill A. Shutemov wrote: 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: 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? You can find broken vmlinux and bzImage here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdrive.google.com%2Fdrive%2Ffolders%2F1n74vUQHOGebnF70Im32qLFY8iS3wvjIs%3Fusp%3Dsharingdata=04%7C01%7Cthomas.lendacky%40amd.com%7C1c7adf380cbe4c1a6bb708d97d4af6ff%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637678583935705530%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=gA30x%2Bfu97tUx0p2UqI8HgjiL8bxDbK1GqgJBbUrUE4%3Dreserved=0 Let me know when I can remove 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. 8124f880 : 8124f880: e8 bb 64 06 00 callq 812b5d40 <__fentry__> 8124f885: e8 36 ca 42 00 callq 8167c2c0 <__sanitizer_cov_trace_pc> 8124f88a: 31 c0 xor%eax,%eax 8124f88c: c3 retq 8167c2c0 <__sanitizer_cov_trace_pc>: 8167c2c0: 65 8b 05 39 ad 9a 7emov%gs:0x7e9aad39(%rip),%eax # 27000 <__preempt_count> 8167c2c7: 89 c6 mov%eax,%esi 8167c2c9: 48 8b 0c 24 mov(%rsp),%rcx 8167c2cd: 81 e6 00 01 00 00 and$0x100,%esi 8167c2d3: 65 48 8b 14 25 40 70mov%gs:0x27040,%rdx Thanks, Tom ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
On Tue, Sep 21, 2021 at 04:43:59PM -0500, Tom Lendacky wrote: > On 9/21/21 4:34 PM, Kirill A. Shutemov wrote: > > 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: > > > > 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? You can find broken vmlinux and bzImage here: https://drive.google.com/drive/folders/1n74vUQHOGebnF70Im32qLFY8iS3wvjIs?usp=sharing Let me know when I can remove it. -- Kirill A. Shutemov ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
On 9/21/21 4:34 PM, Kirill A. Shutemov wrote: 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: 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
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: > > 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
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:
> > 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.
>
> And here's the lineup I have so far, I'd appreciate it if ppc and s390 folks
> could run it too:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=rc2-cc
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:
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index f98c76a1d16c..e9110a44bf1b 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -285,7 +285,7 @@ unsigned long __head __startup_64(unsigned long physaddr,
* there is no need to zero it after changing the memory encryption
* attribute.
*/
- if (cc_platform_has(CC_ATTR_MEM_ENCRYPT)) {
+ if (0 && cc_platform_has(CC_ATTR_MEM_ENCRYPT)) {
vaddr = (unsigned long)__start_bss_decrypted;
vaddr_end = (unsigned long)__end_bss_decrypted;
for (; vaddr < vaddr_end; vaddr += PMD_SIZE) {
diff --git a/arch/x86/mm/mem_encrypt_identity.c
b/arch/x86/mm/mem_encrypt_identity.c
index eff4d19f9cb4..91638ed0b1db 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -288,7 +288,7 @@ void __init sme_encrypt_kernel(struct boot_params *bp)
unsigned long pgtable_area_len;
unsigned long decrypted_base;
- if (!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
+ if (1 || !cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
return;
/*
--
Kirill A. Shutemov
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
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. And here's the lineup I have so far, I'd appreciate it if ppc and s390 folks could run it too: https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=rc2-cc Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
On 9/20/21 2:23 PM, Kirill A. Shutemov wrote: On Wed, Sep 08, 2021 at 05:58:36PM -0500, Tom Lendacky wrote: diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c index 470b20208430..eff4d19f9cb4 100644 --- a/arch/x86/mm/mem_encrypt_identity.c +++ b/arch/x86/mm/mem_encrypt_identity.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -287,7 +288,7 @@ void __init sme_encrypt_kernel(struct boot_params *bp) unsigned long pgtable_area_len; unsigned long decrypted_base; - if (!sme_active()) + if (!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) return; /* 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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
On Wed, Sep 08, 2021 at 05:58:36PM -0500, Tom Lendacky wrote: > diff --git a/arch/x86/mm/mem_encrypt_identity.c > b/arch/x86/mm/mem_encrypt_identity.c > index 470b20208430..eff4d19f9cb4 100644 > --- a/arch/x86/mm/mem_encrypt_identity.c > +++ b/arch/x86/mm/mem_encrypt_identity.c > @@ -30,6 +30,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -287,7 +288,7 @@ void __init sme_encrypt_kernel(struct boot_params *bp) > unsigned long pgtable_area_len; > unsigned long decrypted_base; > > - if (!sme_active()) > + if (!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) > return; > > /* 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 5/8] x86/sme: Replace occurrences of sme_active() with cc_platform_has()
On Wed, Sep 08, 2021 at 05:58:36PM -0500, Tom Lendacky wrote:
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 18fe19916bc3..4b54a2377821 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -144,7 +144,7 @@ void __init sme_unmap_bootdata(char *real_mode_data)
> struct boot_params *boot_data;
> unsigned long cmdline_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 */
> @@ -164,7 +164,7 @@ void __init sme_map_bootdata(char *real_mode_data)
> struct boot_params *boot_data;
> unsigned long cmdline_paddr;
>
> - if (!sme_active())
> + if (!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
> return;
>
> __sme_early_map_unmap_mem(real_mode_data, sizeof(boot_params), true);
> @@ -377,11 +377,6 @@ bool sev_active(void)
> {
> return sev_status & MSR_AMD64_SEV_ENABLED;
> }
> -
> -bool sme_active(void)
> -{
> - return sme_me_mask && !sev_active();
> -}
> EXPORT_SYMBOL_GPL(sev_active);
>
> /* Needs to be called from non-instrumentable code */
You forgot this hunk:
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 5635ca9a1fbe..a3a2396362a5 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -364,8 +364,9 @@ int __init early_set_memory_encrypted(unsigned long vaddr,
unsigned long size)
/*
* SME and SEV are very similar but they are not the same, so there are
* times that the kernel will need to distinguish between SME and SEV. The
- * sme_active() and sev_active() functions are used for this. When a
- * distinction isn't needed, the mem_encrypt_active() function can be used.
+ * PATTR_HOST_MEM_ENCRYPT and PATTR_GUEST_MEM_ENCRYPT flags to
+ * amd_prot_guest_has() are used for this. When a distinction isn't needed,
+ * the mem_encrypt_active() function can be used.
*
* The trampoline code is a good example for this requirement. Before
* paging is activated, SME will access all memory as decrypted, but SEV
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
