Messages in this thread | | | Date | Wed, 26 Jul 2017 06:28:40 +0200 | From | Borislav Petkov <> | Subject | Re: [RFC Part1 PATCH v3 03/17] x86/mm: Secure Encrypted Virtualization (SEV) support |
| |
On Mon, Jul 24, 2017 at 02:07:43PM -0500, Brijesh Singh wrote: > From: Tom Lendacky <thomas.lendacky@amd.com> > > Provide support for Secure Encyrpted Virtualization (SEV). This initial
Your subject misses a verb and patch subjects should have an active verb denoting what the patch does. The sentence above is a good example.
> support defines a flag that is used by the kernel to determine if it is > running with SEV active. > > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/include/asm/mem_encrypt.h | 2 ++ > arch/x86/mm/mem_encrypt.c | 3 +++ > include/linux/mem_encrypt.h | 8 +++++++- > 3 files changed, 12 insertions(+), 1 deletion(-)
...
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c > index 0fbd092..1e4643e 100644 > --- a/arch/x86/mm/mem_encrypt.c > +++ b/arch/x86/mm/mem_encrypt.c > @@ -40,6 +40,9 @@ static char sme_cmdline_off[] __initdata = "off"; > unsigned long sme_me_mask __section(.data) = 0; > EXPORT_SYMBOL_GPL(sme_me_mask); > > +unsigned int sev_enabled __section(.data) = 0; > +EXPORT_SYMBOL_GPL(sev_enabled);
So sev_enabled is a pure bool used only in bool context, not like sme_me_mask whose value is read too. Which means, you can make the former static and query it only through accessor functions.
> /* Buffer used for early in-place encryption by BSP, no locking needed */ > static char sme_early_buffer[PAGE_SIZE] __aligned(PAGE_SIZE); > > diff --git a/include/linux/mem_encrypt.h b/include/linux/mem_encrypt.h > index 1255f09..ea0831a 100644 > --- a/include/linux/mem_encrypt.h > +++ b/include/linux/mem_encrypt.h > @@ -22,12 +22,18 @@ > #else /* !CONFIG_ARCH_HAS_MEM_ENCRYPT */ > > #define sme_me_mask 0UL > +#define sev_enabled 0 > > #endif /* CONFIG_ARCH_HAS_MEM_ENCRYPT */ > > static inline bool sme_active(void) > { > - return !!sme_me_mask; > + return (sme_me_mask && !sev_enabled);
You don't need the brackets. Below too.
> +} > + > +static inline bool sev_active(void) > +{ > + return (sme_me_mask && sev_enabled); > }
So this is confusing, TBH. SME and SEV are not mutually exclusive and yet the logic here says so. Why?
I mean, in the hypervisor context, sme_active() is still true.
/me is confused.
-- Regards/Gruss, Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
| |