Re: [PATCH v4 10/33] i386: Add get/set/migrate support for SGX_LEPUBKEYHASH MSRs

[Philippe Mathieu-Daudé]

On 7/19/21 1:21 PM, Yang Zhong wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> On real hardware, on systems that supports SGX Launch Control, those
> MSRs are initialized to digest of Intel's signing key; on systems that
> don't support SGX Launch Control, those MSRs are not available but
> hardware always uses digest of Intel's signing key in EINIT.
> 
> KVM advertises SGX LC via CPUID if and only if the MSRs are writable.
> Unconditionally initialize those MSRs to digest of Intel's signing key
> when CPU is realized and reset to reflect the fact. This avoids
> potential bug in case kvm_arch_put_registers() is called before
> kvm_arch_get_registers() is called, in which case guest's virtual
> SGX_LEPUBKEYHASH MSRs will be set to 0, although KVM initializes those
> to digest of Intel's signing key by default, since KVM allows those MSRs
> to be updated by Qemu to support live migration.
> 
> Save/restore the SGX Launch Enclave Public Key Hash MSRs if SGX Launch
> Control (LC) is exposed to the guest. Likewise, migrate the MSRs if they
> are writable by the guest.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  target/i386/cpu.c     | 17 ++++++++++++++++-
>  target/i386/cpu.h     |  1 +
>  target/i386/kvm/kvm.c | 22 ++++++++++++++++++++++
>  target/i386/machine.c | 20 ++++++++++++++++++++
>  4 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 840f825431..cea4307930 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5673,6 +5673,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> uint32_t count,
>      }
>  }
>  
> +#ifndef CONFIG_USER_ONLY
> +static void x86_cpu_set_sgxlepubkeyhash(CPUX86State *env)
> +{
> +    env->msr_ia32_sgxlepubkeyhash[0] = 0xa6053e051270b7acULL;
> +    env->msr_ia32_sgxlepubkeyhash[1] = 0x6cfbe8ba8b3b413dULL;
> +    env->msr_ia32_sgxlepubkeyhash[2] = 0xc4916d99f2b3735dULL;
> +    env->msr_ia32_sgxlepubkeyhash[3] = 0xd4f8c05909f9bb3bULL;
> +}
> +#endif

Maybe easier to move the #ifdef'ry inside the function.

Where these values come from btw?

> @@ -6186,6 +6198,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>             & CPUID_EXT2_AMD_ALIASES);
>      }
>  
> +#ifndef CONFIG_USER_ONLY
> +    x86_cpu_set_sgxlepubkeyhash(env);
> +#endif
> +