Re: [PATCH 2/8] KVM: x86: extend struct kvm_vcpu_pv_apf_data with token info

[Sean Christopherson]

On Fri, May 15, 2020 at 05:59:43PM +0200, Paolo Bonzini wrote:
> On 13/05/20 14:52, Vivek Goyal wrote:
> >>> Also, type of event should not necessarily be tied to delivery method.
> >>> For example if we end up introducing say, "KVM_PV_REASON_PAGE_ERROR", then
> >>> I would think that event can be injected both using exception (#PF or #VE)
> >>> as well as interrupt (depending on state of system).
> >> Why bother preserving backwards compatibility?
> > New machanism does not have to support old guests but old mechanism
> > should probably continue to work and deprecated slowly, IMHO. Otherwise
> > guests which were receiving async page faults will suddenly stop getting
> > it over hypervisor upgrade and possibly see drop in performance.
> 
> Unfortunately, the old mechanism was broken enough, and in enough
> different ways, that it's better to just drop it.
> 
> The new one using #VE is not coming very soon (we need to emulate it for
> <Broadwell and AMD processors, so it's not entirely trivial) so we are
> going to keep "page not ready" delivery using #PF for some time or even
> forever.  However, page ready notification as #PF is going away for good.

And isn't hardware based EPT Violation #VE going to require a completely
different protocol than what is implemented today?  For hardware based #VE,
KVM won't intercept the fault, i.e. the guest will need to make an explicit
hypercall to request the page.  That seems like it'll be as time consuming
to implement as emulating EPT Violation #VE in KVM.

> That said, type1/type2 is quite bad. :)  Let's change that to page not
> present / page ready.

Why even bother using 'struct kvm_vcpu_pv_apf_data' for the #PF case?  VMX
only requires error_code[31:16]==0 and SVM doesn't vet it at all, i.e. we
can (ab)use the error code to indicate an async #PF by setting it to an
impossible value, e.g. 0xaaaa (a is for async!).  That partciular error code
is even enforced by the SDM, which states:

  [SGX] this flag is set only if the P flag (bit 0) is 1 and the RSVD flag
  (bit 3) and the PK flag (bit 5) are both 0.

I.e. we've got bigger problems if hardware generates a !PRESENT, WRITE, RSVD,
PK, SGX page fault :-)

Then the page ready becomes the only guest-side consumer of the in-memory
struct, e.g. it can be renamed to something like kvm_vcpu_pv_apf_ready and
doesn't need a reason field (though it still needs a "busy" bit) as written.
It'd also eliminate the apf_put_user() in kvm_arch_async_page_not_present().

I believe it would also allow implementing (in the future) "async #PF ready"
as a ring buffer, i.e. allow kvm_check_async_pf_completion() to coalesce all
ready pages into a single injected interrupt.