Re: [Qemu-devel] [PATCH v10 25/28] cpu/i386: populate CPUID 0x8000_001F when SEV is active

[Eduardo Habkost <ehabkost@redhat.com>]

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