On 9/14/20 5:08 PM, Sean Christopherson wrote:
> On Mon, Sep 14, 2020 at 03:15:35PM -0500, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@xxxxxxx>
>>
>> For SEV-ES guests, the interception of EFER write access is not
>> recommended. EFER interception occurs prior to EFER being modified and
>> the hypervisor is unable to modify EFER itself because the register is
>> located in the encrypted register state.
>>
>> SEV-ES guests introduce a new EFER write trap. This trap provides
>> intercept support of an EFER write after it has been modified. The new
>> EFER value is provided in the VMCB EXITINFO1 field, allowing the
>> hypervisor to track the setting of the guest EFER.
>>
>> Add support to track the value of the guest EFER value using the EFER
>> write trap so that the hypervisor understands the guest operating mode.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> ---
>> arch/x86/include/asm/kvm_host.h | 1 +
>> arch/x86/include/uapi/asm/svm.h | 2 ++
>> arch/x86/kvm/svm/svm.c | 12 ++++++++++++
>> arch/x86/kvm/x86.c | 12 ++++++++++++
>> 4 files changed, 27 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 7320a9c68a5a..b535b690eb66 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1427,6 +1427,7 @@ void kvm_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector);
>> int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
>> int reason, bool has_error_code, u32 error_code);
>>
>> +int kvm_track_efer(struct kvm_vcpu *vcpu, u64 efer);
>> int kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
>> int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
>> int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4);
>> diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
>> index 0bc3942ffdd3..ce937a242995 100644
>> --- a/arch/x86/include/uapi/asm/svm.h
>> +++ b/arch/x86/include/uapi/asm/svm.h
>> @@ -77,6 +77,7 @@
>> #define SVM_EXIT_MWAIT_COND 0x08c
>> #define SVM_EXIT_XSETBV 0x08d
>> #define SVM_EXIT_RDPRU 0x08e
>> +#define SVM_EXIT_EFER_WRITE_TRAP 0x08f
>> #define SVM_EXIT_NPF 0x400
>> #define SVM_EXIT_AVIC_INCOMPLETE_IPI 0x401
>> #define SVM_EXIT_AVIC_UNACCELERATED_ACCESS 0x402
>> @@ -183,6 +184,7 @@
>> { SVM_EXIT_MONITOR, "monitor" }, \
>> { SVM_EXIT_MWAIT, "mwait" }, \
>> { SVM_EXIT_XSETBV, "xsetbv" }, \
>> + { SVM_EXIT_EFER_WRITE_TRAP, "write_efer_trap" }, \
>> { SVM_EXIT_NPF, "npf" }, \
>> { SVM_EXIT_AVIC_INCOMPLETE_IPI, "avic_incomplete_ipi" }, \
>> { SVM_EXIT_AVIC_UNACCELERATED_ACCESS, "avic_unaccelerated_access" }, \
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index ac64a5b128b2..ac467225a51d 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -2466,6 +2466,17 @@ static int cr8_write_interception(struct vcpu_svm *svm)
>> return 0;
>> }
>>
>> +static int efer_trap(struct vcpu_svm *svm)
>> +{
>> + int ret;
>> +
>> + ret = kvm_track_efer(&svm->vcpu, svm->vmcb->control.exit_info_1);
>> + if (ret)
>
> Shouldn't this be a WARN or something? E.g. KVM thinks the WRMSR has faulted,
> while it obviously hasn't, which means KVM's internal model is now out of sync.
Makes sense, I can add something here.
>
>> + return ret;
>> +
>> + return kvm_complete_insn_gp(&svm->vcpu, 0);
>> +}
>> +
>> static int svm_get_msr_feature(struct kvm_msr_entry *msr)
>> {
>> msr->data = 0;
>> @@ -2944,6 +2955,7 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>> [SVM_EXIT_MWAIT] = mwait_interception,
>> [SVM_EXIT_XSETBV] = xsetbv_interception,
>> [SVM_EXIT_RDPRU] = rdpru_interception,
>> + [SVM_EXIT_EFER_WRITE_TRAP] = efer_trap,
>> [SVM_EXIT_NPF] = npf_interception,
>> [SVM_EXIT_RSM] = rsm_interception,
>> [SVM_EXIT_AVIC_INCOMPLETE_IPI] = avic_incomplete_ipi_interception,
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 674719d801d2..b65bd0c986d4 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1480,6 +1480,18 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> return 0;
>> }
>>
>> +int kvm_track_efer(struct kvm_vcpu *vcpu, u64 efer)
>> +{
>> + struct msr_data msr_info;
>> +
>> + msr_info.host_initiated = false;
>> + msr_info.index = MSR_EFER;
>> + msr_info.data = efer;
>> +
>> + return set_efer(vcpu, &msr_info);
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_track_efer);
>
> I don't see any reason to put this in x86.c, just copy-paste the guts into
> efer_trap() and s/set_efer/kvm_set_msr_common.
Ok, I can do that. I'll add a comment to indicate that the result of doing
that is that set_efer() is ultimately invoked through that path.
Thanks,
Tom
>
>> +
>> void kvm_enable_efer_bits(u64 mask)
>> {
>> efer_reserved_bits &= ~mask;
>> --
>> 2.28.0
>>
