On Wed, Feb 28, 2018 at 03:10:25PM -0600, Brijesh Singh wrote:
> When SEV is enabled, CPUID 0x8000_001F should provide additional
> information regarding the feature (such as which page table bit is used
> to mark the pages as encrypted etc).
>
> 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 | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b5e431e769da..7a3cec59402b 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -26,6 +26,7 @@
> #include "sysemu/hvf.h"
> #include "sysemu/cpus.h"
> #include "kvm_i386.h"
> +#include "sev_i386.h"
>
> #include "qemu/error-report.h"
> #include "qemu/option.h"
> @@ -3612,6 +3613,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> *ecx = 0;
> *edx = 0;
> break;
> + case 0x8000001F:
What I'm worried about here is ABI stability and migration safety
guarantees. Let's see:
> + *eax = sev_enabled() ? 0x2 : 0;
sev_enabled() just checks if sev_state is set.
sev_state is only set by sev_guest_init().
sev_guest_init() is only called if MachineState::memory_encryption is set.
The value is a function of command-line options only. Good.
> + *ebx = sev_get_cbit_position();
This is 0 if sev_state is NULL (see above). Good.
If sev_state is set, it returns sev_state->cbitpos.
s->cbitpos is set based on the "cbitpos" property of
QSevGuestInfo only (it's only validated against host CPUID). The
property has no host-dependent default and needs to be set
explicitly.
This means sev_get_cbit_position() is a function of command-line
options only, too. Good.
> + *ebx |= sev_get_reduced_phys_bits() << 6;
Logic is very similar to sev_get_cbit_position(), and depends on
the "reduced-phys-bits" property of QSevGuestInfo only. Good.
> + *ecx = 0;
> + *edx = 0;
> + break;
> default:
> /* reserved values: zero */
> *eax = 0;
> @@ -4041,6 +4049,11 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
> 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 (sev_enabled()) {
> + x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001F);
> + }
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> }
>
> /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
> --
> 2.14.3
>
>
--
Eduardo