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

[Philippe Mathieu-Daudé]

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.