On 2/26/19 2:21 PM, Jim Mattson wrote:
> On Tue, Feb 26, 2019 at 11:50 AM Singh, Brijesh <brijesh.singh@xxxxxxx> wrote:
>>
>>
>> On 2/26/19 1:04 PM, Jim Mattson wrote:
>>> On Tue, Feb 26, 2019 at 10:17 AM Singh, Brijesh <brijesh.singh@xxxxxxx> wrote:
>>>>
>>>>
>>>> On 2/26/19 11:12 AM, Jim Mattson wrote:
>>>>> On Tue, Feb 26, 2019 at 9:02 AM Singh, Brijesh <brijesh.singh@xxxxxxx> 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>
>>>>>> ---
>>>>>>
>>>>>> Change since v2:
>>>>>> * rename the callback emulate_instruction_possible->need_emulation_on_page_fault
>>>>> This function still seems poorly named. You already know that you
>>>>> *need* emulation by the time you call it, don't you?
>>>>>
>>>>
>>>> We know that we are going to require emulation to handle this #PF.
>>>>
>>>> How about can_emulate_on_page_fault(..) ? Any other suggestions ?
>>> Isn't "can_emulate_on_page_fault()" exactly the same as
>>> "!sev_guest()"?
>>
>> It may seem like that, but in common handlers whenever possible I am
>> trying to avoid if(sev_guest() /* do this */ else /* do that */.
>>
>> If insn_len is zero then retry the instruction whether SEV is enabled or
>> not.
>>
>>
>> The function in question also returns false when CPL
>>> != 3 or CR4.SMAP is clear. I think it's difficult to name this
>>> function because the function is essentially answering two unrelated
>>> questions:
>>>
>>> 1) Did we encounter erratum 1096?
>>> 2) Can we emulate an instruction to make forward progress?
>>>
>>> I would suggest that this be broken up into two separate functions:
>>> one which determines whether or not we've encountered erratum 1096 and
>>> another which determines whether or not SEV is enabled. I suspect that
>>> a function to answer the SEV question on its own is going to prove
>>> itself quite useful over time.
>>>
>>
>> Well, I have been trying not to expose SVM specific functions to the
>> common layer. If we go with this approach then a generic page fault
>> handler will look like this:
>>
>> kvm_mmu_page_fault(...)
>> {
>> ....
>> ....
>> if (kvm_x86->svm_erratum_1096(..))
> I think the real question here is whether or not we will go into an
> infinite loop if we simply try to re-enter the guest and re-execute
> the instruction. You don't necessarily have to reference the erratum.
>
We will get into infinite loop for both the SEV and non SEV case. In
case of SEV, we will need to log the message and request the SHUTDOWN
whereas in non SEV we will need to workaround it.
The check should be something like
if (!insn_len) {
if (erratum_1096()) {
if (sev_guest())
/* log the message, request SHUTDOWN and re-enter the guest */
else
/* goto read_the_instruction_bytes */
}
/* re-enter the guest and re-execute the instruction */
}
The need_emulate_on_page_fault() was taking care of the above checks.
IMHO, a one callback in svm.c is better than modifying the generic
page_fault handler to handle the erratum.
