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

[Yang Zhong <yang.zhong@intel.com>]

On Tue, Sep 14, 2021 at 08:38:59AM +0200, Philippe Mathieu-Daudé wrote:
> 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.
>

  Thanks for comments, since this function is pure void function, we can move this #ifdef
  into function.
 
> Where these values come from btw?

  Those MSR values are intel default values, which were defined in Skylake platform.

  Yang

> 
> > @@ -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
> > +