| Message ID | 20200511164752.2158645-3-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Interrupt-based mechanism for async_pf 'page present' notifications | expand |
On Mon, May 11, 2020 at 06:47:46PM +0200, Vitaly Kuznetsov wrote: > Currently, APF mechanism relies on the #PF abuse where the token is being > passed through CR2. If we switch to using interrupts to deliver page-ready > notifications we need a different way to pass the data. Extent the existing > 'struct kvm_vcpu_pv_apf_data' with token information for page-ready > notifications. > > The newly introduced apf_put_user_ready() temporary puts both reason > and token information, this will be changed to put token only when we > switch to interrupt based notifications. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/include/uapi/asm/kvm_para.h | 3 ++- > arch/x86/kvm/x86.c | 17 +++++++++++++---- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h > index 2a8e0b6b9805..e3602a1de136 100644 > --- a/arch/x86/include/uapi/asm/kvm_para.h > +++ b/arch/x86/include/uapi/asm/kvm_para.h > @@ -113,7 +113,8 @@ struct kvm_mmu_op_release_pt { > > struct kvm_vcpu_pv_apf_data { > __u32 reason; > - __u8 pad[60]; > + __u32 pageready_token; How about naming this just "token". That will allow me to deliver error as well. pageready_token name seems to imply that this will always be successful with page being ready. And reason will tell whether page could successfully be ready or it was an error. And token will help us identify the task which is waiting for the event. Thanks Vivek
Vivek Goyal <vgoyal@redhat.com> writes: > On Mon, May 11, 2020 at 06:47:46PM +0200, Vitaly Kuznetsov wrote: >> Currently, APF mechanism relies on the #PF abuse where the token is being >> passed through CR2. If we switch to using interrupts to deliver page-ready >> notifications we need a different way to pass the data. Extent the existing >> 'struct kvm_vcpu_pv_apf_data' with token information for page-ready >> notifications. >> >> The newly introduced apf_put_user_ready() temporary puts both reason >> and token information, this will be changed to put token only when we >> switch to interrupt based notifications. >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- >> arch/x86/include/uapi/asm/kvm_para.h | 3 ++- >> arch/x86/kvm/x86.c | 17 +++++++++++++---- >> 2 files changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h >> index 2a8e0b6b9805..e3602a1de136 100644 >> --- a/arch/x86/include/uapi/asm/kvm_para.h >> +++ b/arch/x86/include/uapi/asm/kvm_para.h >> @@ -113,7 +113,8 @@ struct kvm_mmu_op_release_pt { >> >> struct kvm_vcpu_pv_apf_data { >> __u32 reason; >> - __u8 pad[60]; >> + __u32 pageready_token; > > How about naming this just "token". That will allow me to deliver error > as well. pageready_token name seems to imply that this will always be > successful with page being ready. > > And reason will tell whether page could successfully be ready or > it was an error. And token will help us identify the task which > is waiting for the event. I added 'pageready_' prefix to make it clear this is not used for 'page not present' notifications where we pass token through CR2. (BTW 'reason' also becomes a misnomer because we can only see 'KVM_PV_REASON_PAGE_NOT_PRESENT' there.) I have no strong opinion, can definitely rename this to 'token' and add a line to the documentation to re-state that this is not used for type 1 events.
On Tue, May 12, 2020 at 05:40:10PM +0200, Vitaly Kuznetsov wrote: > Vivek Goyal <vgoyal@redhat.com> writes: > > > On Mon, May 11, 2020 at 06:47:46PM +0200, Vitaly Kuznetsov wrote: > >> Currently, APF mechanism relies on the #PF abuse where the token is being > >> passed through CR2. If we switch to using interrupts to deliver page-ready > >> notifications we need a different way to pass the data. Extent the existing > >> 'struct kvm_vcpu_pv_apf_data' with token information for page-ready > >> notifications. > >> > >> The newly introduced apf_put_user_ready() temporary puts both reason > >> and token information, this will be changed to put token only when we > >> switch to interrupt based notifications. > >> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > >> --- > >> arch/x86/include/uapi/asm/kvm_para.h | 3 ++- > >> arch/x86/kvm/x86.c | 17 +++++++++++++---- > >> 2 files changed, 15 insertions(+), 5 deletions(-) > >> > >> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h > >> index 2a8e0b6b9805..e3602a1de136 100644 > >> --- a/arch/x86/include/uapi/asm/kvm_para.h > >> +++ b/arch/x86/include/uapi/asm/kvm_para.h > >> @@ -113,7 +113,8 @@ struct kvm_mmu_op_release_pt { > >> > >> struct kvm_vcpu_pv_apf_data { > >> __u32 reason; > >> - __u8 pad[60]; > >> + __u32 pageready_token; > > > > How about naming this just "token". That will allow me to deliver error > > as well. pageready_token name seems to imply that this will always be > > successful with page being ready. > > > > And reason will tell whether page could successfully be ready or > > it was an error. And token will help us identify the task which > > is waiting for the event. > > I added 'pageready_' prefix to make it clear this is not used for 'page > not present' notifications where we pass token through CR2. (BTW > 'reason' also becomes a misnomer because we can only see > 'KVM_PV_REASON_PAGE_NOT_PRESENT' there.) Sure. I am just trying to keep names in such a way so that we could deliver more events and not keep it too tightly coupled with only two events (page not present, page ready). > > I have no strong opinion, can definitely rename this to 'token' and add > a line to the documentation to re-state that this is not used for type 1 > events. I don't even know why are we calling "type 1" and "type 2" event. Calling it KVM_PV_REASON_PAGE_NOT_PRESENT and KVM_PV_REASON_PAGE_READY event is much more intuitive. If somebody is confused about how event will be delivered, that could be part of documentation. And "type1" and "type2" does not say anything about delivery method anyway. 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). Thanks Vivek
On Tue, May 12, 2020 at 11:53:39AM -0400, Vivek Goyal wrote: > On Tue, May 12, 2020 at 05:40:10PM +0200, Vitaly Kuznetsov wrote: > > Vivek Goyal <vgoyal@redhat.com> writes: > > > > > On Mon, May 11, 2020 at 06:47:46PM +0200, Vitaly Kuznetsov wrote: > > >> Currently, APF mechanism relies on the #PF abuse where the token is being > > >> passed through CR2. If we switch to using interrupts to deliver page-ready > > >> notifications we need a different way to pass the data. Extent the existing > > >> 'struct kvm_vcpu_pv_apf_data' with token information for page-ready > > >> notifications. > > >> > > >> The newly introduced apf_put_user_ready() temporary puts both reason > > >> and token information, this will be changed to put token only when we > > >> switch to interrupt based notifications. > > >> > > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > >> --- > > >> arch/x86/include/uapi/asm/kvm_para.h | 3 ++- > > >> arch/x86/kvm/x86.c | 17 +++++++++++++---- > > >> 2 files changed, 15 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h > > >> index 2a8e0b6b9805..e3602a1de136 100644 > > >> --- a/arch/x86/include/uapi/asm/kvm_para.h > > >> +++ b/arch/x86/include/uapi/asm/kvm_para.h > > >> @@ -113,7 +113,8 @@ struct kvm_mmu_op_release_pt { > > >> > > >> struct kvm_vcpu_pv_apf_data { > > >> __u32 reason; > > >> - __u8 pad[60]; > > >> + __u32 pageready_token; > > > > > > How about naming this just "token". That will allow me to deliver error > > > as well. pageready_token name seems to imply that this will always be > > > successful with page being ready. > > > > > > And reason will tell whether page could successfully be ready or > > > it was an error. And token will help us identify the task which > > > is waiting for the event. > > > > I added 'pageready_' prefix to make it clear this is not used for 'page > > not present' notifications where we pass token through CR2. (BTW > > 'reason' also becomes a misnomer because we can only see > > 'KVM_PV_REASON_PAGE_NOT_PRESENT' there.) > > Sure. I am just trying to keep names in such a way so that we could > deliver more events and not keep it too tightly coupled with only > two events (page not present, page ready). > > > > > I have no strong opinion, can definitely rename this to 'token' and add > > a line to the documentation to re-state that this is not used for type 1 > > events. > > I don't even know why are we calling "type 1" and "type 2" event. Calling > it KVM_PV_REASON_PAGE_NOT_PRESENT and KVM_PV_REASON_PAGE_READY event > is much more intuitive. If somebody is confused about how event will > be delivered, that could be part of documentation. And "type1" and "type2" > does not say anything about delivery method anyway. > > 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? AIUI, both KVM and guest will support async #PF iff interrupt delivery is enabled. Why not make the interrupt delivery approach KVM_ASYNC_PF_V2 and completely redefine the ABI? E.g. to make it compatible with reflecting !PRESENT faults without a VM-Exit via Intel's EPT Violation #VE?
On Tue, May 12, 2020 at 05:40:10PM +0200, Vitaly Kuznetsov wrote: > Vivek Goyal <vgoyal@redhat.com> writes: > > > On Mon, May 11, 2020 at 06:47:46PM +0200, Vitaly Kuznetsov wrote: > >> Currently, APF mechanism relies on the #PF abuse where the token is being > >> passed through CR2. If we switch to using interrupts to deliver page-ready > >> notifications we need a different way to pass the data. Extent the existing > >> 'struct kvm_vcpu_pv_apf_data' with token information for page-ready > >> notifications. > >> > >> The newly introduced apf_put_user_ready() temporary puts both reason > >> and token information, this will be changed to put token only when we > >> switch to interrupt based notifications. > >> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > >> --- > >> arch/x86/include/uapi/asm/kvm_para.h | 3 ++- > >> arch/x86/kvm/x86.c | 17 +++++++++++++---- > >> 2 files changed, 15 insertions(+), 5 deletions(-) > >> > >> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h > >> index 2a8e0b6b9805..e3602a1de136 100644 > >> --- a/arch/x86/include/uapi/asm/kvm_para.h > >> +++ b/arch/x86/include/uapi/asm/kvm_para.h > >> @@ -113,7 +113,8 @@ struct kvm_mmu_op_release_pt { > >> > >> struct kvm_vcpu_pv_apf_data { > >> __u32 reason; > >> - __u8 pad[60]; > >> + __u32 pageready_token; > > > > How about naming this just "token". That will allow me to deliver error > > as well. pageready_token name seems to imply that this will always be > > successful with page being ready. > > > > And reason will tell whether page could successfully be ready or > > it was an error. And token will help us identify the task which > > is waiting for the event. > > I added 'pageready_' prefix to make it clear this is not used for 'page > not present' notifications where we pass token through CR2. (BTW > 'reason' also becomes a misnomer because we can only see > 'KVM_PV_REASON_PAGE_NOT_PRESENT' there.) "kvm_vcpu_pv_apf_data" being shared between two events at the same time is little concerning. At least there should be clear demarkation that which events will use which fields. I guess I could extend "reason" to also report KVM_PV_REASON_ERROR as long as I make error reporting opt in. That way new code is able to handle more values and old code will not receive it. For reporting errors with page ready events, I probably will have to use more padding bytes to report errors as I can't use reason field anymore. In your previous posting in one of the emails Paolo mentioned that data structures for #VE will be separate. If that's the case, then we will end up changing this protocol one more time. To me it feels that both #VE changes and these changes should go in together as part of async page fault redesign V2. > > I have no strong opinion, can definitely rename this to 'token' and add > a line to the documentation to re-state that this is not used for type 1 > events. Now I understand that both events could use this shared data at the same time. So prefixing toke with pageready makes it clear that it has to be used only with pageready event. So sounds better that way. Thanks Vivek
Sean Christopherson <sean.j.christopherson@intel.com> writes: > > Why bother preserving backwards compatibility? AIUI, both KVM and guest > will support async #PF iff interrupt delivery is enabled. Why not make > the interrupt delivery approach KVM_ASYNC_PF_V2 and completely redefine the > ABI? E.g. to make it compatible with reflecting !PRESENT faults without a > VM-Exit via Intel's EPT Violation #VE? > That's the plan, actually. 'page not present' becomes #VE and 'page present' becomes an interrupt (we don't need to inject them synchronously). These two parts are fairly independent but can be merged to reuse the same new capability/CPUID.
On Tue, May 12, 2020 at 10:50:17AM -0700, Sean Christopherson wrote: > On Tue, May 12, 2020 at 11:53:39AM -0400, Vivek Goyal wrote: > > On Tue, May 12, 2020 at 05:40:10PM +0200, Vitaly Kuznetsov wrote: > > > Vivek Goyal <vgoyal@redhat.com> writes: > > > > > > > On Mon, May 11, 2020 at 06:47:46PM +0200, Vitaly Kuznetsov wrote: > > > >> Currently, APF mechanism relies on the #PF abuse where the token is being > > > >> passed through CR2. If we switch to using interrupts to deliver page-ready > > > >> notifications we need a different way to pass the data. Extent the existing > > > >> 'struct kvm_vcpu_pv_apf_data' with token information for page-ready > > > >> notifications. > > > >> > > > >> The newly introduced apf_put_user_ready() temporary puts both reason > > > >> and token information, this will be changed to put token only when we > > > >> switch to interrupt based notifications. > > > >> > > > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > > >> --- > > > >> arch/x86/include/uapi/asm/kvm_para.h | 3 ++- > > > >> arch/x86/kvm/x86.c | 17 +++++++++++++---- > > > >> 2 files changed, 15 insertions(+), 5 deletions(-) > > > >> > > > >> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h > > > >> index 2a8e0b6b9805..e3602a1de136 100644 > > > >> --- a/arch/x86/include/uapi/asm/kvm_para.h > > > >> +++ b/arch/x86/include/uapi/asm/kvm_para.h > > > >> @@ -113,7 +113,8 @@ struct kvm_mmu_op_release_pt { > > > >> > > > >> struct kvm_vcpu_pv_apf_data { > > > >> __u32 reason; > > > >> - __u8 pad[60]; > > > >> + __u32 pageready_token; > > > > > > > > How about naming this just "token". That will allow me to deliver error > > > > as well. pageready_token name seems to imply that this will always be > > > > successful with page being ready. > > > > > > > > And reason will tell whether page could successfully be ready or > > > > it was an error. And token will help us identify the task which > > > > is waiting for the event. > > > > > > I added 'pageready_' prefix to make it clear this is not used for 'page > > > not present' notifications where we pass token through CR2. (BTW > > > 'reason' also becomes a misnomer because we can only see > > > 'KVM_PV_REASON_PAGE_NOT_PRESENT' there.) > > > > Sure. I am just trying to keep names in such a way so that we could > > deliver more events and not keep it too tightly coupled with only > > two events (page not present, page ready). > > > > > > > > I have no strong opinion, can definitely rename this to 'token' and add > > > a line to the documentation to re-state that this is not used for type 1 > > > events. > > > > I don't even know why are we calling "type 1" and "type 2" event. Calling > > it KVM_PV_REASON_PAGE_NOT_PRESENT and KVM_PV_REASON_PAGE_READY event > > is much more intuitive. If somebody is confused about how event will > > be delivered, that could be part of documentation. And "type1" and "type2" > > does not say anything about delivery method anyway. > > > > 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. > AIUI, both KVM and guest > will support async #PF iff interrupt delivery is enabled. Why not make > the interrupt delivery approach KVM_ASYNC_PF_V2 and completely redefine the > ABI? That makes sense to me. Probably leave existing ABI untouched and deprecate it over a period of time and define V2 of ABI and new guests use it. > E.g. to make it compatible with reflecting !PRESENT faults without a > VM-Exit via Intel's EPT Violation #VE? IIUC, that's what paolo is planning, that is use #VE to inform guest of page not present. It probably will be good if both #VE notification and interrupt based page ready notifications happen at the same time under V2 of ABI, IMHO. Thanks Vivek
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. That said, type1/type2 is quite bad. :) Let's change that to page not present / page ready. Thanks, Paolo
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.
On 15/05/20 20:46, Sean Christopherson wrote: >> 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. Yes, but it's a fairly simple hypercall to implement. >> 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: Possibly, but it's water under the bridge now. And the #PF mechanism also has the problem with NMIs that happen before the error code is read and page faults happening in the handler (you may connect some dots now). For #VE, the virtualization exception data area is enough to hold all the data that is needed. Paolo
On Fri, May 15, 2020 at 09:18:07PM +0200, Paolo Bonzini wrote: > On 15/05/20 20:46, Sean Christopherson wrote: > >> 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. > > Yes, but it's a fairly simple hypercall to implement. > > >> 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: > > Possibly, but it's water under the bridge now. > And the #PF mechanism also has the problem with NMIs that happen before > the error code is read > and page faults happening in the handler (you may connect some dots now). I understood that following was racy. do_async_page_fault <--- kvm injected async page fault NMI happens (Before kvm_read_and_reset_pf_reason() is done) ->do_async_page_fault() (This is regular page fault but it will read reason from shared area and will treat itself as async page fault) So this is racy. But if we get rid of the notion of reading from shared region in page fault handler, will we not get rid of this race. I am assuming that error_code is not racy as it is pushed on stack. What am I missing. Thanks Vivek
On Fri, May 15, 2020 at 09:18:07PM +0200, Paolo Bonzini wrote: > On 15/05/20 20:46, Sean Christopherson wrote: > > 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: > > Possibly, but it's water under the bridge now. Why is that? I thought we were redoing the entire thing because the current ABI is unfixably broken? In other words, since the guest needs to change, why are we keeping any of the current async #PF pieces? E.g. why keep using #PF instead of usurping something like #NP? > And the #PF mechanism also has the problem with NMIs that happen before the > error code is read and page faults happening in the handler. Hrm, I think there's no unfixable problem except for a pathological #PF->NMI->#DB->#PF scenario. But it is a problem :-( FWIW, the error code isn't a problem, CR2 is the killer. The issue Andy originally pointed out is #PF: async page fault (KVM_PV_REASON_PAGE_NOT_PRESENT) NMI: before CR2 or KVM_PF_REASON_PAGE_NOT_PRESENT #PF: normal page fault (NMI handler accesses user memory, e.g. perf) With current async #PF, the problem is that CR2 and apf_reason are out of sync, not that CR2 or the error code are lost. E.g. the above could also happen with a regular #PF on both ends, and obviously that works just fine. In other words, the error code doesn't suffer the same problem because it's pushed on the stack, not shoved into a static memory location. CR2 is the real problem, even though it's saved by the NMI handler. The simple case where the NMI handler hits an async #PF before it can save CR2 is avoidable by KVM not injecting #PF if NMIs are blocked. The pathological case would be if there's a #DB at the beginning of the NMI handler; the IRET from the #DB would unblock NMIs and then open up the guest to hitting an async #PF on the NMI handler before CR2 is saved by the guest. :-(
On Fri, May 15, 2020 at 04:33:52PM -0400, Vivek Goyal wrote: > On Fri, May 15, 2020 at 09:18:07PM +0200, Paolo Bonzini wrote: > > On 15/05/20 20:46, Sean Christopherson wrote: > > >> 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. > > > > Yes, but it's a fairly simple hypercall to implement. > > > > >> 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: > > > > Possibly, but it's water under the bridge now. > > And the #PF mechanism also has the problem with NMIs that happen before > > the error code is read > > and page faults happening in the handler (you may connect some dots now). > > I understood that following was racy. > > do_async_page_fault <--- kvm injected async page fault > NMI happens (Before kvm_read_and_reset_pf_reason() is done) > ->do_async_page_fault() (This is regular page fault but it will read > reason from shared area and will treat itself > as async page fault) > > So this is racy. > > But if we get rid of the notion of reading from shared region in page > fault handler, will we not get rid of this race. > > I am assuming that error_code is not racy as it is pushed on stack. > What am I missing. Nothing, AFAICT. As I mentioned in a different mail, CR2 can be squished, but I don't see how error code can be lost. But, because CR2 can be squished, there still needs to be an in-memory busy flag even if error code is used as the host #PF indicator, otherwise the guest could lose one of the tokens.
On 15/05/20 22:43, Sean Christopherson wrote: > On Fri, May 15, 2020 at 09:18:07PM +0200, Paolo Bonzini wrote: >> On 15/05/20 20:46, Sean Christopherson wrote: >>> 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: >> >> Possibly, but it's water under the bridge now. > > Why is that? I thought we were redoing the entire thing because the current > ABI is unfixably broken? In other words, since the guest needs to change, > why are we keeping any of the current async #PF pieces? E.g. why keep using > #PF instead of usurping something like #NP? Because that would be 3 ABIs to support instead of 2. The #PF solution is only broken as long as you allow async PF from ring 0 (which wasn't even true except for preemptable kernels) _and_ have NMIs that can generate page faults. We also have the #PF vmexit part for nested virtualization. This adds up and makes a quick fix for 'page not ready' notifications not that quick. However, interrupts for 'page ready' do have a bunch of advantages (more control on what can be preempted by the notification, a saner check for new page faults which is effectively a bug fix) so it makes sense to get them in more quickly (probably 5.9 at this point due to the massive cleanups that are being done around interrupt vectors). >> And the #PF mechanism also has the problem with NMIs that happen before the >> error code is read and page faults happening in the handler. > > Hrm, I think there's no unfixable problem except for a pathological > #PF->NMI->#DB->#PF scenario. But it is a problem :-( Yeah, that made no sense. But still I'm not sure the x86 maintainers would like it. The only possible isue with #VE is the re-entrancy at the end. Andy proposed re-enabling it from an interrupt, but here is one solution that can be done almost entirely from C. The idea is to split the IST in two halves, and flip between them in the TSS with an XOR operation on entry to the interrupt handler. This is possible because there won't ever be more than two handlers active at the same time. Unlike if you used SUB/ADD, with XOR you don't have to restore the IST on exit: the two halves will take turns as the current IST and there's no problematic window between the ADD and the IRET. The pseudocode would be: - on #VE entry xor 512 with the IST address in the TSS check if saved RSP comes from the IST if so: overwrite the saved flags/CS/SS in the "other" IST half with the current value of the registers overwrite the saved RSP in the "other" IST half with the address of the top of the IST itself overwrite the saved RIP in the "other" IST half with the address of a trampoline (see below) else: save the top 5 words of the IST somewhere do normal stuff - the trampoline restores the 5 words at the top of the IST with five push instructions, and jumps back to the first instruction of the handler Everything except the first step can even be done in C. Here is an example. Assuming that on entry to the outer #VE the IST is the "even" half, the outer #VE moves the IST to the "odd" half and the return flags/CS/SS/RSP/RIP are saved. After the reentrancy flag has been cleared, a nested #VE arrives and runs within the "odd" half of the IST. The IST is moved back to the "even" half and the return flags/CS/SS/RSP/RIP in the "even" half are patched to point to the trampoline. When we get back to the outer handler the reentrancy flag not zero, so even though the IST points to the current stack, reentrancy is impossible and we go just fine through the few final instructions of the handler. On outer #VE exit, the IRET instruction jumps to the trampoline, with RSP pointing at the top of the "even" half. The return flags/CS/SS/RSP/RIP are restored, and everything restarts from the beginning: the outer #VE moves the IST to the "odd" half, the return flags/CS/SS/RSP/RIP are saved, the data for the nested #VE is fished out of the virtualization exception area and so on. Paolo
On Sat, May 16, 2020 at 12:23:31AM +0200, Paolo Bonzini wrote: > On 15/05/20 22:43, Sean Christopherson wrote: > > On Fri, May 15, 2020 at 09:18:07PM +0200, Paolo Bonzini wrote: > >> On 15/05/20 20:46, Sean Christopherson wrote: > >>> 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: > >> > >> Possibly, but it's water under the bridge now. > > > > Why is that? I thought we were redoing the entire thing because the current > > ABI is unfixably broken? In other words, since the guest needs to change, > > why are we keeping any of the current async #PF pieces? E.g. why keep using > > #PF instead of usurping something like #NP? > > Because that would be 3 ABIs to support instead of 2. The #PF solution > is only broken as long as you allow async PF from ring 0 (which wasn't > even true except for preemptable kernels) _and_ have NMIs that can > generate page faults. We also have the #PF vmexit part for nested > virtualization. This adds up and makes a quick fix for 'page not ready' > notifications not that quick. > > However, interrupts for 'page ready' do have a bunch of advantages (more > control on what can be preempted by the notification, a saner check for > new page faults which is effectively a bug fix) so it makes sense to get > them in more quickly (probably 5.9 at this point due to the massive > cleanups that are being done around interrupt vectors). Ah, so the plan is to fix 'page ready' for the current ABI, but keep the existing 'page not ready' part because it's not thaaaat broken. Correct? In that case, is Andy's patch to kill KVM_ASYNC_PF_SEND_ALWAYS in the guest being picked up? I'll read through your #VE stuff on Monday :-).
Paolo Bonzini <pbonzini@redhat.com> writes: > However, interrupts for 'page ready' do have a bunch of advantages (more > control on what can be preempted by the notification, a saner check for > new page faults which is effectively a bug fix) so it makes sense to get > them in more quickly (probably 5.9 at this point due to the massive > cleanups that are being done around interrupt vectors). > Actually, I have almost no feedback to address in v2 :-) Almost all discussion are happening around #VE. Don't mean to rush or anything but if the 'cleanups' are finalized I can hopefully rebase and retest very quickly as it's only the KVM guest part which intersects with them, the rest should be KVM-only. But 5.9 is good too)
On Mon, May 11, 2020 at 06:47:46PM +0200, Vitaly Kuznetsov wrote: > Currently, APF mechanism relies on the #PF abuse where the token is being > passed through CR2. If we switch to using interrupts to deliver page-ready > notifications we need a different way to pass the data. Extent the existing > 'struct kvm_vcpu_pv_apf_data' with token information for page-ready > notifications. > > The newly introduced apf_put_user_ready() temporary puts both reason > and token information, this will be changed to put token only when we > switch to interrupt based notifications. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/include/uapi/asm/kvm_para.h | 3 ++- > arch/x86/kvm/x86.c | 17 +++++++++++++---- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h > index 2a8e0b6b9805..e3602a1de136 100644 > --- a/arch/x86/include/uapi/asm/kvm_para.h > +++ b/arch/x86/include/uapi/asm/kvm_para.h > @@ -113,7 +113,8 @@ struct kvm_mmu_op_release_pt { > > struct kvm_vcpu_pv_apf_data { > __u32 reason; Hi Vitaly, Given we are redoing it, can we convert "reason" into a flag instead and use bit 0 for signalling "page not present" Then rest of the 31 bits can be used for other purposes. I potentially want to use one bit to signal error (if it is known at the time of injecting #PF). > - __u8 pad[60]; > + __u32 pageready_token; > + __u8 pad[56]; Given token is 32 bit, for returning error in "page ready" type messages, I will probably use padding bytes and create pagready_flag and use one of the bits to signal error. Thanks Vivek
On 21/05/20 16:59, Vitaly Kuznetsov wrote: >> However, interrupts for 'page ready' do have a bunch of advantages (more >> control on what can be preempted by the notification, a saner check for >> new page faults which is effectively a bug fix) so it makes sense to get >> them in more quickly (probably 5.9 at this point due to the massive >> cleanups that are being done around interrupt vectors). >> > Actually, I have almost no feedback to address in v2 :-) Almost all > discussion are happening around #VE. Don't mean to rush or anything but > if the 'cleanups' are finalized I can hopefully rebase and retest very > quickly as it's only the KVM guest part which intersects with them, the > rest should be KVM-only. But 5.9 is good too) Yeah, going for 5.9 would only be due to the conflicts. Do send v2 anyway now, so that we can use a merge commit to convert the interrupt vector to the 5.8 style. Paolo
Vivek Goyal <vgoyal@redhat.com> writes: > On Mon, May 11, 2020 at 06:47:46PM +0200, Vitaly Kuznetsov wrote: >> Currently, APF mechanism relies on the #PF abuse where the token is being >> passed through CR2. If we switch to using interrupts to deliver page-ready >> notifications we need a different way to pass the data. Extent the existing >> 'struct kvm_vcpu_pv_apf_data' with token information for page-ready >> notifications. >> >> The newly introduced apf_put_user_ready() temporary puts both reason >> and token information, this will be changed to put token only when we >> switch to interrupt based notifications. >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- >> arch/x86/include/uapi/asm/kvm_para.h | 3 ++- >> arch/x86/kvm/x86.c | 17 +++++++++++++---- >> 2 files changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h >> index 2a8e0b6b9805..e3602a1de136 100644 >> --- a/arch/x86/include/uapi/asm/kvm_para.h >> +++ b/arch/x86/include/uapi/asm/kvm_para.h >> @@ -113,7 +113,8 @@ struct kvm_mmu_op_release_pt { >> >> struct kvm_vcpu_pv_apf_data { >> __u32 reason; > > Hi Vitaly, > > Given we are redoing it, can we convert "reason" into a flag instead > and use bit 0 for signalling "page not present" Then rest of the 31 > bits can be used for other purposes. I potentially want to use one bit to > signal error (if it is known at the time of injecting #PF). Yes, I think we can do that. The existing KVM_PV_REASON_PAGE_READY and KVM_PV_REASON_PAGE_NOT_PRESENT are mutually exclusive and can be converted to flags (we'll only have KVM_PV_REASON_PAGE_NOT_PRESENT in use when this series is merged). > >> - __u8 pad[60]; >> + __u32 pageready_token; >> + __u8 pad[56]; > > Given token is 32 bit, for returning error in "page ready" type messages, > I will probably use padding bytes and create pagready_flag and use one > of the bits to signal error. In case we're intended to pass more data in synchronous notifications, shall we leave some blank space after 'flags' ('reason' previously) and before 'token'?
On Sat, May 23, 2020 at 06:34:17PM +0200, Vitaly Kuznetsov wrote: > Vivek Goyal <vgoyal@redhat.com> writes: > > > On Mon, May 11, 2020 at 06:47:46PM +0200, Vitaly Kuznetsov wrote: > >> Currently, APF mechanism relies on the #PF abuse where the token is being > >> passed through CR2. If we switch to using interrupts to deliver page-ready > >> notifications we need a different way to pass the data. Extent the existing > >> 'struct kvm_vcpu_pv_apf_data' with token information for page-ready > >> notifications. > >> > >> The newly introduced apf_put_user_ready() temporary puts both reason > >> and token information, this will be changed to put token only when we > >> switch to interrupt based notifications. > >> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > >> --- > >> arch/x86/include/uapi/asm/kvm_para.h | 3 ++- > >> arch/x86/kvm/x86.c | 17 +++++++++++++---- > >> 2 files changed, 15 insertions(+), 5 deletions(-) > >> > >> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h > >> index 2a8e0b6b9805..e3602a1de136 100644 > >> --- a/arch/x86/include/uapi/asm/kvm_para.h > >> +++ b/arch/x86/include/uapi/asm/kvm_para.h > >> @@ -113,7 +113,8 @@ struct kvm_mmu_op_release_pt { > >> > >> struct kvm_vcpu_pv_apf_data { > >> __u32 reason; > > > > Hi Vitaly, > > > > Given we are redoing it, can we convert "reason" into a flag instead > > and use bit 0 for signalling "page not present" Then rest of the 31 > > bits can be used for other purposes. I potentially want to use one bit to > > signal error (if it is known at the time of injecting #PF). > > Yes, I think we can do that. The existing KVM_PV_REASON_PAGE_READY and > KVM_PV_REASON_PAGE_NOT_PRESENT are mutually exclusive and can be > converted to flags (we'll only have KVM_PV_REASON_PAGE_NOT_PRESENT in > use when this series is merged). > > > > >> - __u8 pad[60]; > >> + __u32 pageready_token; > >> + __u8 pad[56]; > > > > Given token is 32 bit, for returning error in "page ready" type messages, > > I will probably use padding bytes and create pagready_flag and use one > > of the bits to signal error. > > In case we're intended to pass more data in synchronous notifications, > shall we leave some blank space after 'flags' ('reason' previously) and > before 'token'? Given you are planning to move away from using kvm_vcpu_pv_apf_data for synchronous notifications, I will not be too concerned about it. Thanks Vivek
diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h index 2a8e0b6b9805..e3602a1de136 100644 --- a/arch/x86/include/uapi/asm/kvm_para.h +++ b/arch/x86/include/uapi/asm/kvm_para.h @@ -113,7 +113,8 @@ struct kvm_mmu_op_release_pt { struct kvm_vcpu_pv_apf_data { __u32 reason; - __u8 pad[60]; + __u32 pageready_token; + __u8 pad[56]; __u32 enabled; }; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index edd4a6415b92..28868cc16e4d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -2662,7 +2662,7 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data) } if (kvm_gfn_to_hva_cache_init(vcpu->kvm, &vcpu->arch.apf.data, gpa, - sizeof(u32))) + sizeof(u64))) return 1; vcpu->arch.apf.send_user_only = !(data & KVM_ASYNC_PF_SEND_ALWAYS); @@ -10352,8 +10352,17 @@ static void kvm_del_async_pf_gfn(struct kvm_vcpu *vcpu, gfn_t gfn) } } -static int apf_put_user(struct kvm_vcpu *vcpu, u32 val) +static inline int apf_put_user_notpresent(struct kvm_vcpu *vcpu) { + u32 reason = KVM_PV_REASON_PAGE_NOT_PRESENT; + + return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.apf.data, &reason, + sizeof(reason)); +} + +static inline int apf_put_user_ready(struct kvm_vcpu *vcpu, u32 token) +{ + u64 val = (u64)token << 32 | KVM_PV_REASON_PAGE_READY; return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.apf.data, &val, sizeof(val)); @@ -10398,7 +10407,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, kvm_add_async_pf_gfn(vcpu, work->arch.gfn); if (kvm_can_deliver_async_pf(vcpu) && - !apf_put_user(vcpu, KVM_PV_REASON_PAGE_NOT_PRESENT)) { + !apf_put_user_notpresent(vcpu)) { fault.vector = PF_VECTOR; fault.error_code_valid = true; fault.error_code = 0; @@ -10431,7 +10440,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, trace_kvm_async_pf_ready(work->arch.token, work->cr2_or_gpa); if (vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED && - !apf_put_user(vcpu, KVM_PV_REASON_PAGE_READY)) { + !apf_put_user_ready(vcpu, work->arch.token)) { fault.vector = PF_VECTOR; fault.error_code_valid = true; fault.error_code = 0;
Currently, APF mechanism relies on the #PF abuse where the token is being passed through CR2. If we switch to using interrupts to deliver page-ready notifications we need a different way to pass the data. Extent the existing 'struct kvm_vcpu_pv_apf_data' with token information for page-ready notifications. The newly introduced apf_put_user_ready() temporary puts both reason and token information, this will be changed to put token only when we switch to interrupt based notifications. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/include/uapi/asm/kvm_para.h | 3 ++- arch/x86/kvm/x86.c | 17 +++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-)