Re: [RFC PATCH 21/35] KVM: SVM: Add support for EFER write traps for an SEV-ES guest

[Tom Lendacky <thomas.lendacky@amd.com>]

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@amd.com>
>>
>> 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@amd.com>
>> ---
>>  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
>>