On 2/15/19 3:32 PM, Sean Christopherson wrote:
> On Fri, Feb 15, 2019 at 05:24:12PM +0000, Singh, Brijesh wrote:
>> Errata#1096:
>>
>> On a nested data page fault when CR.SMAP=1 and the guest data read
>> generates a SMAP violation, GuestInstrBytes field of the VMCB on a
>> VMEXIT will incorrectly return 0h instead the correct guest
>> instruction bytes .
>>
>> Recommend Workaround:
>>
>> To determine what instruction the guest was executing the hypervisor
>> will have to decode the instruction at the instruction pointer.
>>
>> The recommended workaround can not be implemented for the SEV
>> guest because guest memory is encrypted with the guest specific key,
>> and instruction decoder will not be able to decode the instruction
>> bytes. If we hit this errata in the SEV guest then log the message
>> and request a guest shutdown.
>>
>> Reported-by: Venkatesh Srinivas <venkateshs@xxxxxxxxxx>
>> Cc: Jim Mattson <jmattson@xxxxxxxxxx>
>> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> Cc: Borislav Petkov <bp@xxxxxxxxx>
>> Cc: Joerg Roedel <joro@xxxxxxxxxx>
>> Cc: "Radim Krčmář" <rkrcmar@xxxxxxxxxx>
>> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
>> ---
>>
>> Changes since v1:
>> * request to shutdown the guest instead of injecting #GP
>> * use the pr_err_ratelimited to supress the host dmesg
>>
>> arch/x86/include/asm/kvm_host.h | 2 ++
>> arch/x86/kvm/mmu.c | 6 ++++--
>> arch/x86/kvm/svm.c | 32 ++++++++++++++++++++++++++++++++
>> arch/x86/kvm/vmx/vmx.c | 6 ++++++
>> 4 files changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 4660ce90de7f..536acf622af9 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1194,6 +1194,8 @@ struct kvm_x86_ops {
>> int (*nested_enable_evmcs)(struct kvm_vcpu *vcpu,
>> uint16_t *vmcs_version);
>> uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu);
>> +
>> + bool (*emulate_instruction_possible)(struct kvm_vcpu *vcpu);
>> };
>>
>> struct kvm_arch_async_pf {
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index da9c42349b1f..8ebc1bfcabd3 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -5384,8 +5384,10 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code,
>> * page is not present in memory). In those cases we simply restart the
>> * guest.
>> */
>> - if (unlikely(insn && !insn_len))
>> - return 1;
>> + if (unlikely(insn && !insn_len)) {
>> + if (!kvm_x86_ops->emulate_instruction_possible(vcpu))
>> + return 1;
>> + }
>
> IMO "insn && !insn_len" belongs in emulate_instruction_possible(). The
> cost of the indirect function call can be avoided by making the callback
> optional and only attaching it when necessary, e.g. if the CPU is
> affected by the errata. E.g.:
>
Looking at the recent commits, it seems that the preference it to
avoid making callback optional. I don't have strong opinion on it.
Since most of time the insn_len is *not* zero, IMO we should keep
the unlikely macro check here to avoid unnecessary function call.
> For brevity, maybe emulation_impossible() as the ops' name? E.g.:
>
> if (kvm_x86_ops->emulation_impossible &&
> kvm_x86_ops->emulation_impossible(vcpu, insn, insn_len))
> return 1;
>
>>
>> er = x86_emulate_instruction(vcpu, cr2, emulation_type, insn, insn_len);
