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.
> ....
> if (kvm_x86->svm_sev_guest(...))
For this one, you can always use your previous name,
"can_emulate_on_page_fault()," and negate the return value.
> ...
>
> }
>
> Please correct me if I misunderstood you.
