On 2/22/19 11:46 AM, Paolo Bonzini wrote:
> On 15/02/19 18:24, 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;
>
> I am confused.
>
> As the comment says, the "return 1" will _skip_ emulation, so if you
> enter the "if" you are not going to do any emulation in the first place.
> You could get an infinite loop though.
>
If emulation is required and we are not able to read the instruction
bytes then we would like to restart the guest. For this particular
errata if its SEV guest then yes we will enter into infinite loop
hoping that the KVM_SHUTDOWN request will eventually kill the guest.
> What you want is, methinks,
>
> if (unlikely(insn && !insn_len)) {
> if (!kvm_x86_ops->need_emulation_on_page_fault(vcpu))
> return 1;
> }
>
> Is this name accurate? I am okay with keeping the if here; it is
> unlikely to happen, or even impossible on Intel, so there won't be a
> performance impact from the (never invoked) callback on Intel.
>
Yes, that seems a bit better name than what I have in the patch.
