lkml.org 
[lkml]   [2017]   [Jul]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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)
    --

    \
     
     \ /
      Last update: 2017-07-26 06:29    [W:4.126 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site