| Message ID | 9d4fc3f8ea7b325aaa1879beab1286876f45d450.1631141919.git.thomas.lendacky@amd.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Implement generic cc_platform_has() helper function | expand |
On 9/8/21 10:58 PM, 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 <paulus@samba.org> > 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 > > diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig > index 5e037df2a3a1..2e57391e0778 100644 > --- a/arch/powerpc/platforms/pseries/Kconfig > +++ b/arch/powerpc/platforms/pseries/Kconfig > @@ -159,6 +159,7 @@ config PPC_SVM > select SWIOTLB > select ARCH_HAS_MEM_ENCRYPT > select ARCH_HAS_FORCE_DMA_UNENCRYPTED > + select ARCH_HAS_CC_PLATFORM > help > There are certain POWER platforms which support secure guests using > the Protected Execution Facility, with the help of an Ultravisor > diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile > index 4cda0ef87be0..41d8aee98da4 100644 > --- a/arch/powerpc/platforms/pseries/Makefile > +++ b/arch/powerpc/platforms/pseries/Makefile > @@ -31,3 +31,5 @@ obj-$(CONFIG_FA_DUMP) += rtas-fadump.o > > obj-$(CONFIG_SUSPEND) += suspend.o > obj-$(CONFIG_PPC_VAS) += vas.o > + > +obj-$(CONFIG_ARCH_HAS_CC_PLATFORM) += cc_platform.o > diff --git a/arch/powerpc/platforms/pseries/cc_platform.c b/arch/powerpc/platforms/pseries/cc_platform.c > new file mode 100644 > index 000000000000..e8021af83a19 > --- /dev/null > +++ b/arch/powerpc/platforms/pseries/cc_platform.c > @@ -0,0 +1,26 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Confidential Computing Platform Capability checks > + * > + * Copyright (C) 2021 Advanced Micro Devices, Inc. > + * > + * Author: Tom Lendacky <thomas.lendacky@amd.com> > + */ > + > +#include <linux/export.h> > +#include <linux/cc_platform.h> > + > +#include <asm/machdep.h> > +#include <asm/svm.h> > + > +bool cc_platform_has(enum cc_attr attr) > +{ Please keep this function inline as mem_encrypt_active() is > + switch (attr) { > + case CC_ATTR_MEM_ENCRYPT: > + return is_secure_guest(); > + > + default: > + return false; > + } > +} > +EXPORT_SYMBOL_GPL(cc_platform_has); >
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 <paulus@samba.org> > 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.
Le 14/09/2021 à 13:58, Borislav Petkov a écrit : > 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 <paulus@samba.org> >> 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. > Yes, see https://lore.kernel.org/linuxppc-dev/20210914123919.58203eef@canb.auug.org.au/T/#t
On Tue, Sep 14, 2021 at 04:47:41PM +0200, Christophe Leroy wrote:
> Yes, see https://lore.kernel.org/linuxppc-dev/20210914123919.58203eef@canb.auug.org.au/T/#t
Aha, more compiler magic stuff ;-\
Oh well, I guess that fix will land upstream soon.
Thx.
Borislav Petkov <bp@alien8.de> writes: > 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 <paulus@samba.org> >> 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 :) Yeah, hch thinks it'll cause a big mess otherwise: https://lore.kernel.org/lkml/YSScWvpXeVXw%2Fed5@infradead.org/ I guess less ifdeffery is nice too. > Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc) Thx. > Yeah, fixed in mainline today, thanks for trying to cross compile :) Always! :-)
Le 15/09/2021 à 12:08, Borislav Petkov a écrit : > 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 :) > > Yeah, hch thinks it'll cause a big mess otherwise: > > https://lore.kernel.org/lkml/YSScWvpXeVXw%2Fed5@infradead.org/ 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
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.
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.
Christoph Hellwig <hch@infradead.org> writes: > 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. 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.h similarity index 74% rename from arch/powerpc/platforms/pseries/cc_platform.c rename to arch/powerpc/include/asm/cc_platform.h index e8021af83a19..6285c3c385a6 100644 --- a/arch/powerpc/platforms/pseries/cc_platform.c +++ b/arch/powerpc/include/asm/cc_platform.h @@ -7,13 +7,10 @@ * Author: Tom Lendacky <thomas.lendacky@amd.com> */ -#include <linux/export.h> #include <linux/cc_platform.h> - -#include <asm/machdep.h> #include <asm/svm.h> -bool cc_platform_has(enum cc_attr attr) +static inline bool arch_cc_platform_has(enum cc_attr attr) { switch (attr) { case CC_ATTR_MEM_ENCRYPT: @@ -23,4 +20,3 @@ bool cc_platform_has(enum cc_attr attr) return false; } } -EXPORT_SYMBOL_GPL(cc_platform_has); diff --git a/arch/x86/include/asm/cc_platform.h b/arch/x86/include/asm/cc_platform.h new file mode 100644 index 000000000000..0a4220697043 --- /dev/null +++ b/arch/x86/include/asm/cc_platform.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_X86_CC_PLATFORM_H_ +#define _ASM_X86_CC_PLATFORM_H_ + +bool arch_cc_platform_has(enum cc_attr attr); + +#endif // _ASM_X86_CC_PLATFORM_H_ diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c index 3c9bacd3c3f3..77e8c3465979 100644 --- a/arch/x86/kernel/cc_platform.c +++ b/arch/x86/kernel/cc_platform.c @@ -11,11 +11,11 @@ #include <linux/cc_platform.h> #include <linux/mem_encrypt.h> -bool cc_platform_has(enum cc_attr attr) +bool arch_cc_platform_has(enum cc_attr attr) { if (sme_me_mask) return amd_cc_platform_has(attr); return false; } -EXPORT_SYMBOL_GPL(cc_platform_has); +EXPORT_SYMBOL_GPL(arch_cc_platform_has); diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h index 253f3ea66cd8..f3306647c5d9 100644 --- a/include/linux/cc_platform.h +++ b/include/linux/cc_platform.h @@ -65,6 +65,8 @@ enum cc_attr { #ifdef CONFIG_ARCH_HAS_CC_PLATFORM +#include <asm/cc_platform.h> + /** * cc_platform_has() - Checks if the specified cc_attr attribute is active * @attr: Confidential computing attribute to check @@ -77,7 +79,10 @@ enum cc_attr { * * TRUE - Specified Confidential Computing attribute is active * * FALSE - Specified Confidential Computing attribute is not active */ -bool cc_platform_has(enum cc_attr attr); +static inline bool cc_platform_has(enum cc_attr attr) +{ + return arch_cc_platform_has(attr); +} #else /* !CONFIG_ARCH_HAS_CC_PLATFORM */
diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig index 5e037df2a3a1..2e57391e0778 100644 --- a/arch/powerpc/platforms/pseries/Kconfig +++ b/arch/powerpc/platforms/pseries/Kconfig @@ -159,6 +159,7 @@ config PPC_SVM select SWIOTLB select ARCH_HAS_MEM_ENCRYPT select ARCH_HAS_FORCE_DMA_UNENCRYPTED + select ARCH_HAS_CC_PLATFORM help There are certain POWER platforms which support secure guests using the Protected Execution Facility, with the help of an Ultravisor diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile index 4cda0ef87be0..41d8aee98da4 100644 --- a/arch/powerpc/platforms/pseries/Makefile +++ b/arch/powerpc/platforms/pseries/Makefile @@ -31,3 +31,5 @@ obj-$(CONFIG_FA_DUMP) += rtas-fadump.o obj-$(CONFIG_SUSPEND) += suspend.o obj-$(CONFIG_PPC_VAS) += vas.o + +obj-$(CONFIG_ARCH_HAS_CC_PLATFORM) += cc_platform.o diff --git a/arch/powerpc/platforms/pseries/cc_platform.c b/arch/powerpc/platforms/pseries/cc_platform.c new file mode 100644 index 000000000000..e8021af83a19 --- /dev/null +++ b/arch/powerpc/platforms/pseries/cc_platform.c @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Confidential Computing Platform Capability checks + * + * Copyright (C) 2021 Advanced Micro Devices, Inc. + * + * Author: Tom Lendacky <thomas.lendacky@amd.com> + */ + +#include <linux/export.h> +#include <linux/cc_platform.h> + +#include <asm/machdep.h> +#include <asm/svm.h> + +bool cc_platform_has(enum cc_attr attr) +{ + switch (attr) { + case CC_ATTR_MEM_ENCRYPT: + return is_secure_guest(); + + default: + return false; + } +} +EXPORT_SYMBOL_GPL(cc_platform_has);
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 <paulus@samba.org> 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