Message ID | 20210719112136.57018-11-yang.zhong@intel.com |
---|---|
State | New |
Headers | show |
Series | Qemu SGX virtualization | expand |
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 > +
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 > > +
On 9/16/21 8:08 AM, Yang Zhong wrote: > 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. Could you add a comment (and reference if possible) about them please? Thanks, Phil.
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 + static void x86_cpu_reset(DeviceState *dev) { CPUState *s = CPU(dev); @@ -5804,6 +5814,8 @@ static void x86_cpu_reset(DeviceState *dev) if (kvm_enabled()) { kvm_arch_reset_vcpu(cpu); } + + x86_cpu_set_sgxlepubkeyhash(env); #endif } @@ -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 + /* * note: the call to the framework needs to happen after feature expansion, * but before the checks/modifications to ucode_rev, mwait, phys_bits. @@ -6871,7 +6887,6 @@ static const TypeInfo x86_cpu_type_info = { .class_init = x86_cpu_common_class_init, }; - /* "base" CPU model, used by query-cpu-model-expansion */ static void x86_cpu_base_class_init(ObjectClass *oc, void *data) { diff --git a/target/i386/cpu.h b/target/i386/cpu.h index e4d46cca80..892c0dfab4 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1499,6 +1499,7 @@ typedef struct CPUX86State { uint64_t mcg_status; uint64_t msr_ia32_misc_enable; uint64_t msr_ia32_feature_control; + uint64_t msr_ia32_sgxlepubkeyhash[4]; uint64_t msr_fixed_ctr_ctrl; uint64_t msr_global_ctrl; diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index 59ed8327ac..d4bf054ebe 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -3096,6 +3096,17 @@ static int kvm_put_msrs(X86CPU *cpu, int level) } } + if (env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_SGX_LC) { + kvm_msr_entry_add(cpu, MSR_IA32_SGXLEPUBKEYHASH0, + env->msr_ia32_sgxlepubkeyhash[0]); + kvm_msr_entry_add(cpu, MSR_IA32_SGXLEPUBKEYHASH1, + env->msr_ia32_sgxlepubkeyhash[1]); + kvm_msr_entry_add(cpu, MSR_IA32_SGXLEPUBKEYHASH2, + env->msr_ia32_sgxlepubkeyhash[2]); + kvm_msr_entry_add(cpu, MSR_IA32_SGXLEPUBKEYHASH3, + env->msr_ia32_sgxlepubkeyhash[3]); + } + /* Note: MSR_IA32_FEATURE_CONTROL is written separately, see * kvm_put_msr_feature_control. */ } @@ -3435,6 +3446,13 @@ static int kvm_get_msrs(X86CPU *cpu) } } + if (env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_SGX_LC) { + kvm_msr_entry_add(cpu, MSR_IA32_SGXLEPUBKEYHASH0, 0); + kvm_msr_entry_add(cpu, MSR_IA32_SGXLEPUBKEYHASH1, 0); + kvm_msr_entry_add(cpu, MSR_IA32_SGXLEPUBKEYHASH2, 0); + kvm_msr_entry_add(cpu, MSR_IA32_SGXLEPUBKEYHASH3, 0); + } + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, cpu->kvm_msr_buf); if (ret < 0) { return ret; @@ -3724,6 +3742,10 @@ static int kvm_get_msrs(X86CPU *cpu) case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B: env->msr_rtit_addrs[index - MSR_IA32_RTIT_ADDR0_A] = msrs[i].data; break; + case MSR_IA32_SGXLEPUBKEYHASH0 ... MSR_IA32_SGXLEPUBKEYHASH3: + env->msr_ia32_sgxlepubkeyhash[index - MSR_IA32_SGXLEPUBKEYHASH0] = + msrs[i].data; + break; } } diff --git a/target/i386/machine.c b/target/i386/machine.c index f6f094f1c9..099a4c36f7 100644 --- a/target/i386/machine.c +++ b/target/i386/machine.c @@ -1396,6 +1396,25 @@ static const VMStateDescription vmstate_msr_tsx_ctrl = { } }; +static bool intel_sgx_msrs_needed(void *opaque) +{ + X86CPU *cpu = opaque; + CPUX86State *env = &cpu->env; + + return !!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_SGX_LC); +} + +static const VMStateDescription vmstate_msr_intel_sgx = { + .name = "cpu/intel_sgx", + .version_id = 1, + .minimum_version_id = 1, + .needed = intel_sgx_msrs_needed, + .fields = (VMStateField[]) { + VMSTATE_UINT64_ARRAY(env.msr_ia32_sgxlepubkeyhash, X86CPU, 4), + VMSTATE_END_OF_LIST() + } +}; + const VMStateDescription vmstate_x86_cpu = { .name = "cpu", .version_id = 12, @@ -1531,6 +1550,7 @@ const VMStateDescription vmstate_x86_cpu = { &vmstate_nested_state, #endif &vmstate_msr_tsx_ctrl, + &vmstate_msr_intel_sgx, NULL } };