| Message ID | 20231109210325.3806151-9-amoorthy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve KVM + userfaultfd performance via KVM_MEMORY_FAULT_EXITs on stage-2 faults | expand |
On Thu, Nov 9, 2023 at 1:03 PM Anish Moorthy <amoorthy@google.com> wrote: > > TODO: Changelog -- and possibly just merge into the "god" arm commit? *Facepalm* Well as you can tell, I wasn't sure if there was anything to actually put in the long-form log. Lmk if you have suggestions
On Thu, Nov 09, 2023, Anish Moorthy wrote: > On Thu, Nov 9, 2023 at 1:03 PM Anish Moorthy <amoorthy@google.com> wrote: > > > > TODO: Changelog -- and possibly just merge into the "god" arm commit? > > *Facepalm* > > Well as you can tell, I wasn't sure if there was anything to actually > put in the long-form log. Lmk if you have suggestions I think the right way to organize things is to have this chunk: diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index b1e5e42bdeb4..bc978260d2be 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3309,6 +3309,10 @@ static int kvm_handle_error_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fa return RET_PF_RETRY; } + WARN_ON_ONCE(fault->goal_level != PG_LEVEL_4K); + + kvm_prepare_memory_fault_exit(vcpu, gfn_to_gpa(fault->gfn), PAGE_SIZE, + fault->write, fault->exec, fault->is_private); return -EFAULT; } be part of this patch. Because otherwise, advertising KVM_CAP_MEMORY_FAULT_INFO is a lie. Userspace can't catch KVM in the lie, but that doesn't make it right. That should in turn make it easier to write a useful changelog.
On Wed, Feb 07, 2024 at 07:39:50AM -0800, Sean Christopherson wrote: > On Thu, Nov 09, 2023, Anish Moorthy wrote: > > On Thu, Nov 9, 2023 at 1:03 PM Anish Moorthy <amoorthy@google.com> wrote: > > > > > > TODO: Changelog -- and possibly just merge into the "god" arm commit? > > > > *Facepalm* > > > > Well as you can tell, I wasn't sure if there was anything to actually > > put in the long-form log. Lmk if you have suggestions > > I think the right way to organize things is to have this chunk: > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index b1e5e42bdeb4..bc978260d2be 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -3309,6 +3309,10 @@ static int kvm_handle_error_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fa > return RET_PF_RETRY; > } > > + WARN_ON_ONCE(fault->goal_level != PG_LEVEL_4K); > + > + kvm_prepare_memory_fault_exit(vcpu, gfn_to_gpa(fault->gfn), PAGE_SIZE, > + fault->write, fault->exec, fault->is_private); > return -EFAULT; > } Err.. This is the arm64 patch. x86 already advertises KVM_CAP_MEMORY_FAULT_INFO. The rest of the advertisement happens over in the arch-neutral code when the arch selects CONFIG_HAVE_KVM_EXIT_ON_MISSING. Having said that... > be part of this patch. Because otherwise, advertising KVM_CAP_MEMORY_FAULT_INFO > is a lie. Userspace can't catch KVM in the lie, but that doesn't make it right. > > That should in turn make it easier to write a useful changelog. The feedback still stands. The capability needs to be squashed into the patch that actually introduces the functionality.
On Wed, Feb 7, 2024 at 8:41 AM Oliver Upton <oliver.upton@linux.dev> wrote: > > On Wed, Feb 07, 2024 at 07:39:50AM -0800, Sean Christopherson wrote: > > Having said that... > > > be part of this patch. Because otherwise, advertising KVM_CAP_MEMORY_FAULT_INFO > > is a lie. Userspace can't catch KVM in the lie, but that doesn't make it right. > > > > That should in turn make it easier to write a useful changelog. > > The feedback still stands. The capability needs to be squashed into the > patch that actually introduces the functionality. > > -- > Thanks, > Oliver Hold on, I think there may be confusion here. KVM_CAP_MEMORY_FAULT_INFO is the mechanism for reporting annotated EFAULTs. These are generic in that other things (such as the guest memfd stuff) may also report information to userspace using annotated EFAULTs. KVM_CAP_EXIT_ON_MISSING is the thing that says "do an annotated EFAULT when a stage-2 violation would require faulting in host mapping" On both x86 and arm64, the relevant functionality is added and the cap is advertised in a single patch. I think it makes sense to enable/advertise the two caps separately (as I've done here). The former, after all, just says that userspace "may get annotated EFAULTs for whatever reason" (as opposed to the latter cap, which says that userspace *will* get annotated EFAULTs when the stage-2 handler is failed). So even if arm64 userspaces never get annotated EFAULTs as of this patch, I don't think we're "lying" to them. Consider a related problem: suppose that code is added in core KVM which also generates annotated EFAULTs, and that later the arm64 "Enable KVM_CAP_EXIT_ON_MISSING" patch [1] ends up needing to be reverted for some reason. If the two patches were merged, that revert would silence *all* annotated EFAULTs for arm64, which seems incorrect. James and I had this discussion in [2] (I propose an updated description there too). [1] https://lore.kernel.org/kvm/20231109210325.3806151-10-amoorthy@google.com/ [2] https://lore.kernel.org/kvm/CAF7b7mrALBBWCg+ctU867BjQhtLQNuX=Yo8u9TZEuDTEtCV6qw@mail.gmail.com/
On Wed, Feb 07, 2024, Anish Moorthy wrote: > On Wed, Feb 7, 2024 at 8:41 AM Oliver Upton <oliver.upton@linux.dev> wrote: > > > > On Wed, Feb 07, 2024 at 07:39:50AM -0800, Sean Christopherson wrote: > > > > Having said that... > > > > > be part of this patch. Because otherwise, advertising KVM_CAP_MEMORY_FAULT_INFO > > > is a lie. Userspace can't catch KVM in the lie, but that doesn't make it right. > > > > > > That should in turn make it easier to write a useful changelog. > > > > The feedback still stands. The capability needs to be squashed into the > > patch that actually introduces the functionality. > > > > -- > > Thanks, > > Oliver > > Hold on, I think there may be confusion here. > KVM_CAP_MEMORY_FAULT_INFO is the mechanism for reporting annotated > EFAULTs. These are generic in that other things (such as the guest > memfd stuff) may also report information to userspace using annotated > EFAULTs. > > KVM_CAP_EXIT_ON_MISSING is the thing that says "do an annotated EFAULT > when a stage-2 violation would require faulting in host mapping" On > both x86 and arm64, the relevant functionality is added and the cap is > advertised in a single patch. > > I think it makes sense to enable/advertise the two caps separately (as > I've done here). The former, after all, just says that userspace "may > get annotated EFAULTs for whatever reason" (as opposed to the latter > cap, which says that userspace *will* get annotated EFAULTs when the > stage-2 handler is failed). So even if arm64 userspaces never get > annotated EFAULTs as of this patch, I don't think we're "lying" to > them. Neither Oliver nor I are advocating you smush the two together. We're saying the code that actually fills memory_fault should be either (a) a separate patch (x86) or (b) in the patch that advertises KVM_CAP_MEMORY_FAULT_INFO (arm). I *highly* doubt it will matter in practice, but if there was a problem with filling memory_fault, it would be nice to isolate that to a standalone patch, and not the EXIT_ON_MISSING_CHANGE.
On Wed, Feb 07, 2024 at 01:21:05PM -0800, Anish Moorthy wrote: > On Wed, Feb 7, 2024 at 8:41 AM Oliver Upton <oliver.upton@linux.dev> wrote: > > > > On Wed, Feb 07, 2024 at 07:39:50AM -0800, Sean Christopherson wrote: > > > > Having said that... > > > > > be part of this patch. Because otherwise, advertising KVM_CAP_MEMORY_FAULT_INFO > > > is a lie. Userspace can't catch KVM in the lie, but that doesn't make it right. > > > > > > That should in turn make it easier to write a useful changelog. > > > > The feedback still stands. The capability needs to be squashed into the > > patch that actually introduces the functionality. > > > > -- > > Thanks, > > Oliver > > Hold on, I think there may be confusion here. No, there isn't. > KVM_CAP_MEMORY_FAULT_INFO is the mechanism for reporting annotated > EFAULTs. These are generic in that other things (such as the guest > memfd stuff) may also report information to userspace using annotated > EFAULTs. > > KVM_CAP_EXIT_ON_MISSING is the thing that says "do an annotated EFAULT > when a stage-2 violation would require faulting in host mapping" On > both x86 and arm64, the relevant functionality is added and the cap is > advertised in a single patch. > > I think it makes sense to enable/advertise the two caps separately (as > I've done here). The former, after all, just says that userspace "may > get annotated EFAULTs for whatever reason" (as opposed to the latter > cap, which says that userspace *will* get annotated EFAULTs when the > stage-2 handler is failed). So even if arm64 userspaces never get > annotated EFAULTs as of this patch, I don't think we're "lying" to > them. I don't know about you, but I find describing UAPI in terms of "may" and "whatever reason" quite unsettling. I like to keep my interactions with userspace deterministic. Overall, I find the informational capability to be quite superfluous as it pertains to this feature. Userspace has *explicitly* opted in to a specific behavior, and the side band capability provides no useful information. You can easily document KVM_CAP_MEMORY_FAULT_INFO in such a way that userspace expects to take this sort of exit. Nobody has presented a use case for annotated EFAULTs on arm64 beyond this opt-in and there is zero interest in predefining UAPI for anything else. x86 may've done this a different way, but that's their business. We're not making UAPI out of any of our other EFAULT returns right now. > Consider a related problem: suppose that code is added in core KVM > which also generates annotated EFAULTs, and that later the arm64 > "Enable KVM_CAP_EXIT_ON_MISSING" patch [1] ends up needing to be > reverted for some reason. The single rule we try to uphold in the kernel is to *never break userspace*, so I don't see this being in the realm of possibility. The moment we expose a feature to userspace we're on the hook for it in perpetuity, and if we break that then you're welcome to send a nastygram to Marc or I.
Ah, I finally see what I'm being beaten over the head with- the kvm_prepare_memory_fault_exit() is a feature of KVM_CAP_MEMORY_FAULT_INFO, not EXIT_ON_MISSING. You're right of course Ok, so for this commit in the next version of the series I'm thinking the following description > KVM: arm64: Enable KVM_CAP_MEMORY_FAULT_INFO and annotate fault in the > stage-2 fault handler > > At the moment the only intended use case for KVM_CAP_MEMORY_FAULT_INFO > on arm64 is to annotate EFAULTs from the stage-2 fault handler, so > add that annotation now.
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 317964bad1e1..b5c1d1fb77d0 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -241,6 +241,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) case KVM_CAP_ARM_SYSTEM_SUSPEND: case KVM_CAP_IRQFD_RESAMPLE: case KVM_CAP_COUNTER_OFFSET: + case KVM_CAP_MEMORY_FAULT_INFO: r = 1; break; case KVM_CAP_SET_GUEST_DEBUG2:
TODO: Changelog -- and possibly just merge into the "god" arm commit? Signed-off-by: Anish Moorthy <amoorthy@google.com> --- arch/arm64/kvm/arm.c | 1 + 1 file changed, 1 insertion(+)