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(..))
....
if (kvm_x86->svm_sev_guest(...))
...
}
Please correct me if I misunderstood you.
