Re: [PATCH v3 04/11] x86: Introduce generic protected guest abstraction

[Kuppuswamy, Sathyanarayanan]

On 6/24/21 8:01 AM, Borislav Petkov wrote:
> On Fri, Jun 18, 2021 at 03:57:48PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > Add a generic way to check if we run with an encrypted guest,
>
> Please use passive voice in your commit message: no "we" or "I", etc,
> and describe your changes in imperative mood.
>
> Also, pls read section "2) Describe your changes" in
> Documentation/process/submitting-patches.rst for more details.
>
> Bottom line is: personal pronouns are ambiguous in text, especially with
> so many parties/companies/etc developing the kernel so let's avoid them
> please.

I will fix this in next version. I will make sure to follow it in future
submissions.

> > without requiring x86 specific ifdefs. This can then be used in
> > non architecture specific code.
>
> "... in arch-independent code." or so.

I will fix this in next version.

> > prot_guest_has() is used to check for protected guest feature
> > flags.
> >
> > Originally-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > ---
> >
> > Change since v1:
> >   * Introduced PR_GUEST_TDX and PR_GUEST_SEV vendor flags as per
> >     Boris suggestion.
> >   * Replaced is_tdx_guest() with if (boot_cpu_data.x86_vendor ==
> >     X86_VENDOR_INTEL) in prot_guest_has().
> >   * Modified tdx_protected_guest_has() and sev_protected_guest_has()
> >     to support vendor flags.
>
> ...
>
> > diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h
> > new file mode 100644
> > index 000000000000..d47668dee6c2
> > --- /dev/null
> > +++ b/arch/x86/include/asm/protected_guest.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/* Copyright (C) 2020 Intel Corporation */
> > +#ifndef _ASM_PROTECTED_GUEST_H
> > +#define _ASM_PROTECTED_GUEST_H 1
>
> #define _ASM_X86_PROTECTED_GUEST_H
>
> > +
> > +#include <asm/processor.h>
> > +#include <asm/tdx.h>
> > +#include <asm/sev.h>
> > +
> > +static inline bool prot_guest_has(unsigned long flag)
> > +{
> > +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> > +		return tdx_protected_guest_has(flag);
> > +	else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> > +		return sev_protected_guest_has(flag);
>
> s/protected/prot/
>
> tdx_prot_guest_has
> sev_prot_guest_has

Ok. I will make this change in next version.

> ...
>
> > @@ -18,6 +20,21 @@ static inline bool cpuid_has_tdx_guest(void)
> >   	return !memcmp("IntelTDX    ", sig, 12);
> >   }
> >   
> > +bool tdx_protected_guest_has(unsigned long flag)
> > +{
> > +	switch (flag) {
> > +	case PR_GUEST_MEM_ENCRYPT:
> > +	case PR_GUEST_MEM_ENCRYPT_ACTIVE:
> > +	case PR_GUEST_UNROLL_STRING_IO:
> > +	case PR_GUEST_SHARED_MAPPING_INIT:
> > +	case PR_GUEST_TDX:
> > +		return static_cpu_has(X86_FEATURE_TDX_GUEST);
>
> 		return cpu_feature_enabled(...)

I will use it in next version.

>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer