Re: [PATCH v8 05/28] target/i386: add memory encryption feature cpuid support

[Brijesh Singh <brijesh.singh@amd.com>]

On 2/12/18 12:38 PM, Eduardo Habkost wrote:
> On Mon, Feb 12, 2018 at 09:36:52AM -0600, Brijesh Singh wrote:
>> AMD EPYC processors support memory encryption feature. The feature
>> is reported through CPUID 8000_001F[EAX].
>>
>> Fn8000_001F [EAX]:
>>  Bit 0   Secure Memory Encryption (SME) supported
>>  Bit 1   Secure Encrypted Virtualization (SEV) supported
>>  Bit 2   Page flush MSR supported
>>  Bit 3   Ecrypted State (SEV-ES) support
>>
>> when memory encryption feature is reported, CPUID 8000_001F[EBX] should
>> provide additional information regarding the feature (such as which page
>> table bit is used to mark pages as encrypted etc). The information in EBX
>> and ECX may vary from one family to another hence we use the host cpuid
>> to populate the EBX information.
>>
>> The details for memory encryption CPUID is available in AMD APM
>> (https://support.amd.com/TechDocs/24594.pdf) Section E.4.17
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  target/i386/cpu.c | 36 ++++++++++++++++++++++++++++++++++++
>>  target/i386/cpu.h |  3 +++
>>  2 files changed, 39 insertions(+)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index b5e431e769da..475d98a44880 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -235,6 +235,7 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
>>  #define TCG_EXT4_FEATURES 0
>>  #define TCG_SVM_FEATURES 0
>>  #define TCG_KVM_FEATURES 0
>> +#define TCG_MEM_ENCRYPT_FEATURES 0
>>  #define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP | \
>>            CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ADX | \
>>            CPUID_7_0_EBX_PCOMMIT | CPUID_7_0_EBX_CLFLUSHOPT |            \
>> @@ -546,6 +547,20 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>>          .cpuid_reg = R_EDX,
>>          .tcg_features = ~0U,
>>      },
>> +    [FEAT_MEM_ENCRYPT] = {
>> +        .feat_names = {
>> +            NULL, "sev", NULL, NULL,
>> +            NULL, NULL, NULL, NULL,
>> +            NULL, NULL, NULL, NULL,
>> +            NULL, NULL, NULL, NULL,
>> +            NULL, NULL, NULL, NULL,
>> +            NULL, NULL, NULL, NULL,
>> +            NULL, NULL, NULL, NULL,
>> +            NULL, NULL, NULL, NULL,
>> +        },
>> +        .cpuid_eax = 0x8000001F, .cpuid_reg = R_EAX,
>> +        .tcg_features = TCG_MEM_ENCRYPT_FEATURES,
>> +    }
> What should happen if "-cpu ...,+sev" is used without the
> appropriate SEV configuration on the command-line?
>

 If host supports SEV feature then we should communicate guest that SEV
feature is available but not enabled. At least that's what I am doing
today. Having said that, I am flexible to change if you all things
otherwise. We could abort the launch if SEV configuration is missing and
feature is enabled.

In current implementation, when -cpu ...,+sev is passed without
appropriate SEV configuration then we populate the Fn8000_001F CPUID but
VMCB will not have SEV bit set hence a guest will be launched as
non-SEV. The presence of Fn8000_001F provides the hint that HW supports
the SEV feature but does not mean that it is enabled. A guest OS need to
call MSR_AMD64_SEV (0xc001_0131) to determine if SEV is enabled. The
MSR_AMD64_SEV is non interceptable read-only MSR. This MSR is set by the
HW when SEV bit is enabled in VMCB. SEV bit in VMCB is enabled only when
qemu adds those extra SEV configuration (i.e KVM_SEV_INIT is success)

The steps used by guest OS to determine SEV active is documented here [1]

[1]
https://git.kernel.org/pub/scm/virt/kvm/kvm.git/tree/Documentation/x86/amd-memory-encryption.txt?h=linux-next#n57


>
>>  };
>>  
>>  typedef struct X86RegisterInfo32 {
>> @@ -1966,6 +1981,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
>>              CPUID_XSAVE_XGETBV1,
>>          .features[FEAT_6_EAX] =
>>              CPUID_6_EAX_ARAT,
>> +        /* Missing: SEV_ES */
>> +        .features[FEAT_MEM_ENCRYPT] =
>> +            CPUID_8000_001F_EAX_SEV,
> Changing existing CPU models is possible only on very specific
> circumstances (where VMs that are currently runnable would always
> stay runnable), and would require compat_props entries to keep
> compatibility on existing machine-types.

Ah I didn't consider that case. What is recommendation, should we create
a new CPU Model (EPYC-SEV) ?

> I don't think this is one case where adding the feature will be
> safe (even if adding compat code).  What about existing VMs that
> are running on hosts that don't return SEV on KVM_GET_SUPPORTED_CPUID?
> "-cpu EPYC" would become not runnable on those hosts.
>
> (BTW, do you have a pointer to the KVM patches that enable SEV on
> KVM_GET_SUPPORTED_CPUID?  I couldn't find them.)
>

https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?h=linux-next&id=8765d75329a386dd7742f94a1ea5fdcdea8d93d0


>>          .xlevel = 0x8000000A,
>>          .model_id = "AMD EPYC Processor",
>>      },
>> @@ -3590,6 +3608,19 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>              *edx = 0;
>>          }
>>          break;
>> +    case 0x8000001F:
>> +        if (env->features[FEAT_MEM_ENCRYPT] & CPUID_8000_001F_EAX_SEV) {
>> +            *eax = env->features[FEAT_MEM_ENCRYPT];
>> +            host_cpuid(0x8000001F, 0, NULL, ebx, NULL, NULL);
> This still have the same problem as v6.  The CPUID data shouldn't
> come from the host CPU, but from the QEMU configuration.

During SEV guest configuration parsing we validate cbit position passed
through QEMU configure against the host and assert if verification fails
hence I thought it was OK to call host_cpuid() later in the code path.

Please note that EBX contains two information 1# C-bit position and 2#
physical address bit reduction. Both of these information need to filled
for the SEV guest. The physical address bit reduction depends on C-bit
position and I have  expose only C-bit position configuration to qemu
command line. I was not sure if we should  expose physical address bit
reduction in qemu cmdline and then construct EBX using those two
information.


> The same QEMU command-line must expose the same machine to the
> guest on any host (except on the "host" and "max" CPU models,
> that are the only non-migration-safe CPU models).
>
>
>> +            *ecx = 0;
>> +            *edx = 0;
>> +        } else {
>> +            *eax = 0;
>> +            *ebx = 0;
>> +            *ecx = 0;
>> +            *edx = 0;
>> +        }
>> +        break;
>>      case 0xC0000000:
>>          *eax = env->cpuid_xlevel2;
>>          *ebx = 0;
>> @@ -4037,10 +4068,15 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>>          x86_cpu_adjust_feat_level(cpu, FEAT_C000_0001_EDX);
>>          x86_cpu_adjust_feat_level(cpu, FEAT_SVM);
>>          x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE);
>> +        x86_cpu_adjust_feat_level(cpu, FEAT_MEM_ENCRYPT);
> This call here will automatically increase xlevel to 0x8000001F
> if any FEAT_MEM_ENCRYPT feature is set, so...
>
>
>>          /* SVM requires CPUID[0x8000000A] */
>>          if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
>>              x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000000A);
>>          }
>> +        /* SEV requires CPUID[0x8000001F] */
>> +        if ((env->features[FEAT_MEM_ENCRYPT] & CPUID_8000_001F_EAX_SEV)) {
>> +            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001F);
> ...this code here seems unnecessary.
>
>
>> +        }
>>      }
>>  
>>      /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index f91e37d25dea..448b30f893fa 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -483,6 +483,7 @@ typedef enum FeatureWord {
>>      FEAT_6_EAX,         /* CPUID[6].EAX */
>>      FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
>>      FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
>> +    FEAT_MEM_ENCRYPT,   /* CPUID[8000_001F].EAX */
>>      FEATURE_WORDS,
>>  } FeatureWord;
>>  
>> @@ -679,6 +680,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>>  
>>  #define CPUID_6_EAX_ARAT       (1U << 2)
>>  
>> +#define CPUID_8000_001F_EAX_SEV             (1U << 1) /* SEV */
>> +
>>  /* CPUID[0x80000007].EDX flags: */
>>  #define CPUID_APM_INVTSC       (1U << 8)
>>  
>> -- 
>> 2.14.3
>>