| Message ID | 20230412213510.1220557-8-amoorthy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve scalability of KVM + userfaultfd live migration via annotated memory faults. | expand |
On Wed, Apr 12, 2023 at 09:34:55PM +0000, Anish Moorthy wrote: > Implement KVM_CAP_MEMORY_FAULT_INFO for efaults from > kvm_vcpu_write_guest_page() > > Signed-off-by: Anish Moorthy <amoorthy@google.com> > --- > virt/kvm/kvm_main.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 63b4285d858d1..b29a38af543f0 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -3119,8 +3119,11 @@ int kvm_vcpu_write_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn, > const void *data, int offset, int len) > { > struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn); > + int ret = __kvm_write_guest_page(vcpu->kvm, slot, gfn, data, offset, len); > > - return __kvm_write_guest_page(vcpu->kvm, slot, gfn, data, offset, len); > + if (ret == -EFAULT) > + kvm_populate_efault_info(vcpu, gfn * PAGE_SIZE + offset, len); > + return ret; > } > EXPORT_SYMBOL_GPL(kvm_vcpu_write_guest_page); Why need to trap this? Is this -EFAULT part of the "scalable userfault" plan or not? My previous memory was one can still leave things like copy_to_user() to go via the userfaults channels which should work in parallel with the new vcpu MEMORY_FAULT exit. But maybe the plan changed? Thanks,
On Thu, Apr 20, 2023 at 1:52 PM Peter Xu <peterx@redhat.com> wrote: > > On Wed, Apr 12, 2023 at 09:34:55PM +0000, Anish Moorthy wrote: > > Implement KVM_CAP_MEMORY_FAULT_INFO for efaults from > > kvm_vcpu_write_guest_page() > > > > Signed-off-by: Anish Moorthy <amoorthy@google.com> > > --- > > virt/kvm/kvm_main.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 63b4285d858d1..b29a38af543f0 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -3119,8 +3119,11 @@ int kvm_vcpu_write_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn, > > const void *data, int offset, int len) > > { > > struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn); > > + int ret = __kvm_write_guest_page(vcpu->kvm, slot, gfn, data, offset, len); > > > > - return __kvm_write_guest_page(vcpu->kvm, slot, gfn, data, offset, len); > > + if (ret == -EFAULT) > > + kvm_populate_efault_info(vcpu, gfn * PAGE_SIZE + offset, len); > > + return ret; > > } > > EXPORT_SYMBOL_GPL(kvm_vcpu_write_guest_page); > > Why need to trap this? Is this -EFAULT part of the "scalable userfault" > plan or not? > > My previous memory was one can still leave things like copy_to_user() to go > via the userfaults channels which should work in parallel with the new vcpu > MEMORY_FAULT exit. But maybe the plan changed? This commit isn't really part of the "scalable uffd" changes, which basically correspond to KVM_CAP_ABSENT_MAPPING_FAULT. There should be more details in the cover letter, but basically my v1 just included KVM_CAP_ABSENT_MAPPING_FAULT: Sean argued that the API there ("return to userspace whenever KVM fails a guest memory access to a page fault") was problematic, and so I reworked the series to include a general capability for reporting extra information for failed guest memory accesses (KVM_CAP_MEMORY_FAULT_INFO) and KVM_CAP_ABSENT_MAPPING_FAULT (which is meant to be used in combination with the other cap) for the "scalable userfaultfd" changes. As such most of the commits in this series are unrelated to KVM_CAP_ABSENT_MAPPING_FAULT, and this is one of those commits. It doesn't affect page faults generated by copy_to_user (which should still be delivered via uffd).
On Thu, Apr 20, 2023 at 04:29:38PM -0700, Anish Moorthy wrote: > On Thu, Apr 20, 2023 at 1:52 PM Peter Xu <peterx@redhat.com> wrote: > > > > On Wed, Apr 12, 2023 at 09:34:55PM +0000, Anish Moorthy wrote: > > > Implement KVM_CAP_MEMORY_FAULT_INFO for efaults from > > > kvm_vcpu_write_guest_page() > > > > > > Signed-off-by: Anish Moorthy <amoorthy@google.com> > > > --- > > > virt/kvm/kvm_main.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > > index 63b4285d858d1..b29a38af543f0 100644 > > > --- a/virt/kvm/kvm_main.c > > > +++ b/virt/kvm/kvm_main.c > > > @@ -3119,8 +3119,11 @@ int kvm_vcpu_write_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn, > > > const void *data, int offset, int len) > > > { > > > struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn); > > > + int ret = __kvm_write_guest_page(vcpu->kvm, slot, gfn, data, offset, len); > > > > > > - return __kvm_write_guest_page(vcpu->kvm, slot, gfn, data, offset, len); > > > + if (ret == -EFAULT) > > > + kvm_populate_efault_info(vcpu, gfn * PAGE_SIZE + offset, len); > > > + return ret; > > > } > > > EXPORT_SYMBOL_GPL(kvm_vcpu_write_guest_page); > > > > Why need to trap this? Is this -EFAULT part of the "scalable userfault" > > plan or not? > > > > My previous memory was one can still leave things like copy_to_user() to go > > via the userfaults channels which should work in parallel with the new vcpu > > MEMORY_FAULT exit. But maybe the plan changed? > > This commit isn't really part of the "scalable uffd" changes, which > basically correspond to KVM_CAP_ABSENT_MAPPING_FAULT. There should be > more details in the cover letter, but basically my v1 just included > KVM_CAP_ABSENT_MAPPING_FAULT: Sean argued that the API there ("return > to userspace whenever KVM fails a guest memory access to a page > fault") was problematic, and so I reworked the series to include a > general capability for reporting extra information for failed guest > memory accesses (KVM_CAP_MEMORY_FAULT_INFO) and > KVM_CAP_ABSENT_MAPPING_FAULT (which is meant to be used in combination > with the other cap) for the "scalable userfaultfd" changes. > > As such most of the commits in this series are unrelated to > KVM_CAP_ABSENT_MAPPING_FAULT, and this is one of those commits. It > doesn't affect page faults generated by copy_to_user (which should > still be delivered via uffd). Indeed it'll be an improvement itself to report more details for such an error already. Makes sense to me, thanks,
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 63b4285d858d1..b29a38af543f0 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -3119,8 +3119,11 @@ int kvm_vcpu_write_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn, const void *data, int offset, int len) { struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn); + int ret = __kvm_write_guest_page(vcpu->kvm, slot, gfn, data, offset, len); - return __kvm_write_guest_page(vcpu->kvm, slot, gfn, data, offset, len); + if (ret == -EFAULT) + kvm_populate_efault_info(vcpu, gfn * PAGE_SIZE + offset, len); + return ret; } EXPORT_SYMBOL_GPL(kvm_vcpu_write_guest_page);
Implement KVM_CAP_MEMORY_FAULT_INFO for efaults from kvm_vcpu_write_guest_page() Signed-off-by: Anish Moorthy <amoorthy@google.com> --- virt/kvm/kvm_main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)