| Message ID | 7d55bac0cf2e73f53816bce3a3097877ed9663f3.1628873970.git.thomas.lendacky@amd.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Implement generic prot_guest_has() helper function | expand |
On Fri, Aug 13, 2021 at 11:59:22AM -0500, Tom Lendacky wrote: > diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h > new file mode 100644 > index 000000000000..51e4eefd9542 > --- /dev/null > +++ b/arch/x86/include/asm/protected_guest.h > @@ -0,0 +1,29 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Protected Guest (and Host) Capability checks > + * > + * Copyright (C) 2021 Advanced Micro Devices, Inc. > + * > + * Author: Tom Lendacky <thomas.lendacky@amd.com> > + */ > + > +#ifndef _X86_PROTECTED_GUEST_H > +#define _X86_PROTECTED_GUEST_H > + > +#include <linux/mem_encrypt.h> > + > +#ifndef __ASSEMBLY__ > + > +static inline bool prot_guest_has(unsigned int attr) > +{ > +#ifdef CONFIG_AMD_MEM_ENCRYPT > + if (sme_me_mask) > + return amd_prot_guest_has(attr); > +#endif > + > + return false; > +} > + > +#endif /* __ASSEMBLY__ */ > + > +#endif /* _X86_PROTECTED_GUEST_H */ I think this can be simplified more, diff ontop below: - no need for the ifdeffery as amd_prot_guest_has() has versions for both when CONFIG_AMD_MEM_ENCRYPT is set or not. - the sme_me_mask check is pushed there too. - and since this is vendor-specific, I'm checking the vendor bit. Yeah, yeah, cross-vendor but I don't really believe that. --- diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h index 51e4eefd9542..8541c76d5da4 100644 --- a/arch/x86/include/asm/protected_guest.h +++ b/arch/x86/include/asm/protected_guest.h @@ -12,18 +12,13 @@ #include <linux/mem_encrypt.h> -#ifndef __ASSEMBLY__ - static inline bool prot_guest_has(unsigned int attr) { -#ifdef CONFIG_AMD_MEM_ENCRYPT - if (sme_me_mask) + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD || + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) return amd_prot_guest_has(attr); -#endif return false; } -#endif /* __ASSEMBLY__ */ - #endif /* _X86_PROTECTED_GUEST_H */ diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c index edc67ddf065d..5a0442a6f072 100644 --- a/arch/x86/mm/mem_encrypt.c +++ b/arch/x86/mm/mem_encrypt.c @@ -392,6 +392,9 @@ bool noinstr sev_es_active(void) bool amd_prot_guest_has(unsigned int attr) { + if (!sme_me_mask) + return false; + switch (attr) { case PATTR_MEM_ENCRYPT: return sme_me_mask != 0;
On 8/14/21 2:08 PM, Borislav Petkov wrote: > On Fri, Aug 13, 2021 at 11:59:22AM -0500, Tom Lendacky wrote: >> diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h >> new file mode 100644 >> index 000000000000..51e4eefd9542 >> --- /dev/null >> +++ b/arch/x86/include/asm/protected_guest.h >> @@ -0,0 +1,29 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Protected Guest (and Host) Capability checks >> + * >> + * Copyright (C) 2021 Advanced Micro Devices, Inc. >> + * >> + * Author: Tom Lendacky <thomas.lendacky@amd.com> >> + */ >> + >> +#ifndef _X86_PROTECTED_GUEST_H >> +#define _X86_PROTECTED_GUEST_H >> + >> +#include <linux/mem_encrypt.h> >> + >> +#ifndef __ASSEMBLY__ >> + >> +static inline bool prot_guest_has(unsigned int attr) >> +{ >> +#ifdef CONFIG_AMD_MEM_ENCRYPT >> + if (sme_me_mask) >> + return amd_prot_guest_has(attr); >> +#endif >> + >> + return false; >> +} >> + >> +#endif /* __ASSEMBLY__ */ >> + >> +#endif /* _X86_PROTECTED_GUEST_H */ > > I think this can be simplified more, diff ontop below: > > - no need for the ifdeffery as amd_prot_guest_has() has versions for > both when CONFIG_AMD_MEM_ENCRYPT is set or not. Ugh, yeah, not sure why I put that in for this version since I have the static inline for when CONFIG_AMD_MEM_ENCRYPT is not set. > > - the sme_me_mask check is pushed there too. > > - and since this is vendor-specific, I'm checking the vendor bit. Yeah, > yeah, cross-vendor but I don't really believe that. It's not a cross-vendor thing as opposed to a KVM or other hypervisor thing where the family doesn't have to be reported as AMD or HYGON. That's why I made the if check be for sme_me_mask. I think that is the safer way to go. Thanks, Tom > > --- > diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h > index 51e4eefd9542..8541c76d5da4 100644 > --- a/arch/x86/include/asm/protected_guest.h > +++ b/arch/x86/include/asm/protected_guest.h > @@ -12,18 +12,13 @@ > > #include <linux/mem_encrypt.h> > > -#ifndef __ASSEMBLY__ > - > static inline bool prot_guest_has(unsigned int attr) > { > -#ifdef CONFIG_AMD_MEM_ENCRYPT > - if (sme_me_mask) > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD || > + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) > return amd_prot_guest_has(attr); > -#endif > > return false; > } > > -#endif /* __ASSEMBLY__ */ > - > #endif /* _X86_PROTECTED_GUEST_H */ > diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c > index edc67ddf065d..5a0442a6f072 100644 > --- a/arch/x86/mm/mem_encrypt.c > +++ b/arch/x86/mm/mem_encrypt.c > @@ -392,6 +392,9 @@ bool noinstr sev_es_active(void) > > bool amd_prot_guest_has(unsigned int attr) > { > + if (!sme_me_mask) > + return false; > + > switch (attr) { > case PATTR_MEM_ENCRYPT: > return sme_me_mask != 0; >
On Sun, Aug 15, 2021 at 08:53:31AM -0500, Tom Lendacky wrote: > It's not a cross-vendor thing as opposed to a KVM or other hypervisor > thing where the family doesn't have to be reported as AMD or HYGON. What would be the use case? A HV starts a guest which is supposed to be encrypted using the AMD's confidential guest technology but the HV tells the guest that it is not running on an AMD SVM HV but something else? Is that even an actual use case? Or am I way off? I know we have talked about this in the past but this still sounds insane.
On 8/15/21 9:39 AM, Borislav Petkov wrote: > On Sun, Aug 15, 2021 at 08:53:31AM -0500, Tom Lendacky wrote: >> It's not a cross-vendor thing as opposed to a KVM or other hypervisor >> thing where the family doesn't have to be reported as AMD or HYGON. > > What would be the use case? A HV starts a guest which is supposed to be > encrypted using the AMD's confidential guest technology but the HV tells > the guest that it is not running on an AMD SVM HV but something else? > > Is that even an actual use case? > > Or am I way off? > > I know we have talked about this in the past but this still sounds > insane. Maybe the KVM folks have a better understanding of it... I can change it to be an AMD/HYGON check... although, I'll have to check to see if any (very) early use of the function will work with that. At a minimum, the check in arch/x86/kernel/head64.c will have to be changed or removed. I'll take a closer look. Thanks, Tom >
On Tue, Aug 17, 2021 at 10:22:52AM -0500, Tom Lendacky wrote: > I can change it to be an AMD/HYGON check... although, I'll have to check > to see if any (very) early use of the function will work with that. We can always change it later if really needed. It is just that I'm not a fan of such "preemptive" changes. > At a minimum, the check in arch/x86/kernel/head64.c will have to be > changed or removed. I'll take a closer look. Yeah, sme_me_mask, already discussed on IRC. Thx.
On Fri, Aug 13, 2021 at 11:59:22AM -0500, Tom Lendacky wrote: > While the name suggests this is intended mainly for guests, it will > also be used for host memory encryption checks in place of sme_active(). Which suggest that the name is not good to start with. Maybe protected hardware, system or platform might be a better choice? > +static inline bool prot_guest_has(unsigned int attr) > +{ > +#ifdef CONFIG_AMD_MEM_ENCRYPT > + if (sme_me_mask) > + return amd_prot_guest_has(attr); > +#endif > + > + return false; > +} Shouldn't this be entirely out of line? > +/* 0x800 - 0x8ff reserved for AMD */ > +#define PATTR_SME 0x800 > +#define PATTR_SEV 0x801 > +#define PATTR_SEV_ES 0x802 Why do we need reservations for a purely in-kernel namespace? And why are you overoading a brand new generic API with weird details of a specific implementation like this?
On Thu, Aug 19, 2021 at 10:52:53AM +0100, Christoph Hellwig wrote: > Which suggest that the name is not good to start with. Maybe protected > hardware, system or platform might be a better choice? Yah, coming up with a proper name here hasn't been easy. prot_guest_has() is not the first variant. From all three things you suggest above, I guess calling it a "platform" is the closest. As in, this is a confidential computing platform which provides host and guest facilities etc. So calling it confidential_computing_platform_has() is obviously too long. ccp_has() clashes with the namespace of drivers/crypto/ccp/ which is used by the technology too. coco_platform_has() is too unserious. So I guess cc_platform_has() ain't all that bad. Unless you have a better idea, ofc.
On 8/19/21 4:52 AM, Christoph Hellwig wrote: > On Fri, Aug 13, 2021 at 11:59:22AM -0500, Tom Lendacky wrote: >> While the name suggests this is intended mainly for guests, it will >> also be used for host memory encryption checks in place of sme_active(). > > Which suggest that the name is not good to start with. Maybe protected > hardware, system or platform might be a better choice? > >> +static inline bool prot_guest_has(unsigned int attr) >> +{ >> +#ifdef CONFIG_AMD_MEM_ENCRYPT >> + if (sme_me_mask) >> + return amd_prot_guest_has(attr); >> +#endif >> + >> + return false; >> +} > > Shouldn't this be entirely out of line? I did it as inline originally because the presence of the function will be decided based on the ARCH_HAS_PROTECTED_GUEST config. For now, that is only selected by the AMD memory encryption support, so if I went out of line I could put in mem_encrypt.c. But with TDX wanting to also use it, it would have to be in an always built file with some #ifdefs or in its own file that is conditionally built based on the ARCH_HAS_PROTECTED_GUEST setting (they've already tried building with ARCH_HAS_PROTECTED_GUEST=y and AMD_MEM_ENCRYPT not set). 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. > >> +/* 0x800 - 0x8ff reserved for AMD */ >> +#define PATTR_SME 0x800 >> +#define PATTR_SEV 0x801 >> +#define PATTR_SEV_ES 0x802 > > Why do we need reservations for a purely in-kernel namespace? > > And why are you overoading a brand new generic API with weird details > of a specific implementation like this? There was some talk about this on the mailing list where TDX and SEV may need to be differentiated, so we wanted to reserve a range of values per technology. I guess I can remove them until they are actually needed. Thanks, Tom >
On 8/19/21 11:33 AM, Tom Lendacky wrote: > There was some talk about this on the mailing list where TDX and SEV may > need to be differentiated, so we wanted to reserve a range of values per > technology. I guess I can remove them until they are actually needed. In TDX also we have similar requirements and we need some flags for TDX specific checks. So I think it is fine to leave some space for vendor flags.
On Thu, Aug 19, 2021 at 01:33:09PM -0500, Tom Lendacky wrote: > I did it as inline originally because the presence of the function will be > decided based on the ARCH_HAS_PROTECTED_GUEST config. For now, that is > only selected by the AMD memory encryption support, so if I went out of > line I could put in mem_encrypt.c. But with TDX wanting to also use it, it > would have to be in an always built file with some #ifdefs or in its own > file that is conditionally built based on the ARCH_HAS_PROTECTED_GUEST > setting (they've already tried building with ARCH_HAS_PROTECTED_GUEST=y > and AMD_MEM_ENCRYPT not set). > > 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. > > > > >> +/* 0x800 - 0x8ff reserved for AMD */ > >> +#define PATTR_SME 0x800 > >> +#define PATTR_SEV 0x801 > >> +#define PATTR_SEV_ES 0x802 > > > > Why do we need reservations for a purely in-kernel namespace? > > > > And why are you overoading a brand new generic API with weird details > > of a specific implementation like this? > > There was some talk about this on the mailing list where TDX and SEV may > need to be differentiated, so we wanted to reserve a range of values per > technology. I guess I can remove them until they are actually needed. In that case add a flag for the differing behavior. And only add them when actually needed. And either way there is absolutely no need to reserve ranges.
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 421fa9e38c60..82e5fb713261 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -1514,6 +1514,7 @@ config AMD_MEM_ENCRYPT select ARCH_HAS_FORCE_DMA_UNENCRYPTED select INSTRUCTION_DECODER select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS + select ARCH_HAS_PROTECTED_GUEST help Say yes to enable support for the encryption of system memory. This requires an AMD processor that supports Secure Memory diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h index 9c80c68d75b5..a46d47662772 100644 --- a/arch/x86/include/asm/mem_encrypt.h +++ b/arch/x86/include/asm/mem_encrypt.h @@ -53,6 +53,7 @@ void __init sev_es_init_vc_handling(void); bool sme_active(void); bool sev_active(void); bool sev_es_active(void); +bool amd_prot_guest_has(unsigned int attr); #define __bss_decrypted __section(".bss..decrypted") @@ -78,6 +79,7 @@ static inline void sev_es_init_vc_handling(void) { } static inline bool sme_active(void) { return false; } static inline bool sev_active(void) { return false; } static inline bool sev_es_active(void) { return false; } +static inline bool amd_prot_guest_has(unsigned int attr) { return false; } static inline int __init early_set_memory_decrypted(unsigned long vaddr, unsigned long size) { return 0; } diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h new file mode 100644 index 000000000000..51e4eefd9542 --- /dev/null +++ b/arch/x86/include/asm/protected_guest.h @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Protected Guest (and Host) Capability checks + * + * Copyright (C) 2021 Advanced Micro Devices, Inc. + * + * Author: Tom Lendacky <thomas.lendacky@amd.com> + */ + +#ifndef _X86_PROTECTED_GUEST_H +#define _X86_PROTECTED_GUEST_H + +#include <linux/mem_encrypt.h> + +#ifndef __ASSEMBLY__ + +static inline bool prot_guest_has(unsigned int attr) +{ +#ifdef CONFIG_AMD_MEM_ENCRYPT + if (sme_me_mask) + return amd_prot_guest_has(attr); +#endif + + return false; +} + +#endif /* __ASSEMBLY__ */ + +#endif /* _X86_PROTECTED_GUEST_H */ diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c index ff08dc463634..edc67ddf065d 100644 --- a/arch/x86/mm/mem_encrypt.c +++ b/arch/x86/mm/mem_encrypt.c @@ -20,6 +20,7 @@ #include <linux/bitops.h> #include <linux/dma-mapping.h> #include <linux/virtio_config.h> +#include <linux/protected_guest.h> #include <asm/tlbflush.h> #include <asm/fixmap.h> @@ -389,6 +390,30 @@ bool noinstr sev_es_active(void) return sev_status & MSR_AMD64_SEV_ES_ENABLED; } +bool amd_prot_guest_has(unsigned int attr) +{ + switch (attr) { + case PATTR_MEM_ENCRYPT: + return sme_me_mask != 0; + + case PATTR_SME: + case PATTR_HOST_MEM_ENCRYPT: + return sme_active(); + + case PATTR_SEV: + case PATTR_GUEST_MEM_ENCRYPT: + return sev_active(); + + case PATTR_SEV_ES: + case PATTR_GUEST_PROT_STATE: + return sev_es_active(); + + default: + return false; + } +} +EXPORT_SYMBOL_GPL(amd_prot_guest_has); + /* Override for DMA direct allocation check - ARCH_HAS_FORCE_DMA_UNENCRYPTED */ bool force_dma_unencrypted(struct device *dev) { diff --git a/include/linux/protected_guest.h b/include/linux/protected_guest.h index 43d4dde94793..5ddef1b6a2ea 100644 --- a/include/linux/protected_guest.h +++ b/include/linux/protected_guest.h @@ -20,6 +20,11 @@ #define PATTR_GUEST_MEM_ENCRYPT 2 /* Guest encrypted memory */ #define PATTR_GUEST_PROT_STATE 3 /* Guest encrypted state */ +/* 0x800 - 0x8ff reserved for AMD */ +#define PATTR_SME 0x800 +#define PATTR_SEV 0x801 +#define PATTR_SEV_ES 0x802 + #ifdef CONFIG_ARCH_HAS_PROTECTED_GUEST #include <asm/protected_guest.h>