| Message ID | 20230602161921.208564-5-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 6/3/2023 12:19 AM, Anish Moorthy wrote: > The order of parameters in these function signature is a little strange, > with "offset" actually applying to "gfn" rather than to "data". Add > short comments to make things perfectly clear. > > Signed-off-by: Anish Moorthy <amoorthy@google.com> > --- > virt/kvm/kvm_main.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 09d4d85691e1..d9c0fa7c907f 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2984,6 +2984,9 @@ static int next_segment(unsigned long len, int offset) > return len; > } > > +/* > + * Copy 'len' bytes from guest memory at '(gfn * PAGE_SIZE) + offset' to 'data' > + */ > static int __kvm_read_guest_page(struct kvm_memory_slot *slot, gfn_t gfn, > void *data, int offset, int len) > { > @@ -3085,6 +3088,9 @@ int kvm_vcpu_read_guest_atomic(struct kvm_vcpu *vcpu, gpa_t gpa, > } > EXPORT_SYMBOL_GPL(kvm_vcpu_read_guest_atomic); > > +/* > + * Copy 'len' bytes from 'data' into guest memory at '(gfn * PAGE_SIZE) + offset' > + */ > static int __kvm_write_guest_page(struct kvm *kvm, > struct kvm_memory_slot *memslot, gfn_t gfn, > const void *data, int offset, int len) Agree, and how about one step further, i.e. adjust the param's sequence. diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 2c276d4d0821..db2bc5d3e2c2 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2992,7 +2992,7 @@ static int next_segment(unsigned long len, int offset) */ static int __kvm_read_guest_page(struct kvm_memory_slot *slot, struct kvm_vcpu *vcpu, - gfn_t gfn, void *data, int offset, int len) + gfn_t gfn, int offset, void *data, int len) { int r; unsigned long addr; @@ -3015,7 +3015,7 @@ int kvm_read_guest_page(struct kvm *kvm, gfn_t gfn, void *data, int offset, { struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn); - return __kvm_read_guest_page(slot, NULL, gfn, data, offset, len); + return __kvm_read_guest_page(slot, NULL, gfn, offset, data, len); } EXPORT_SYMBOL_GPL(kvm_read_guest_page); @@ -3024,7 +3024,7 @@ int kvm_vcpu_read_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn, void *data, { struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn); - return __kvm_read_guest_page(slot, vcpu, gfn, data, offset, len); + return __kvm_read_guest_page(slot, vcpu, gfn, offset, data, len); } EXPORT_SYMBOL_GPL(kvm_vcpu_read_guest_page); @@ -3103,7 +3103,7 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_read_guest_atomic); */ static int __kvm_write_guest_page(struct kvm *kvm, struct kvm_vcpu *vcpu, struct kvm_memory_slot *memslot, gfn_t gfn, - const void *data, int offset, int len) + int offset, const void *data, int len) { int r; unsigned long addr; @@ -3128,7 +3128,7 @@ int kvm_write_guest_page(struct kvm *kvm, gfn_t gfn, { struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn); - return __kvm_write_guest_page(kvm, NULL, slot, gfn, data, offset, len); + return __kvm_write_guest_page(kvm, NULL, slot, gfn, offset, data, len); } EXPORT_SYMBOL_GPL(kvm_write_guest_page); @@ -3136,8 +3136,8 @@ 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); - return __kvm_write_guest_page(vcpu->kvm, vcpu, slot, gfn, data, - offset, len); + return __kvm_write_guest_page(vcpu->kvm, vcpu, slot, gfn, offset, + data, len); } EXPORT_SYMBOL_GPL(kvm_vcpu_write_guest_page);
On Wed, Jun 14, 2023 at 7:41 PM Robert Hoo <robert.hoo.linux@gmail.com> wrote: > > Agree, and how about one step further, i.e. adjust the param's sequence. > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 2c276d4d0821..db2bc5d3e2c2 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2992,7 +2992,7 @@ static int next_segment(unsigned long len, int offset) > */ > static int __kvm_read_guest_page(struct kvm_memory_slot *slot, > struct kvm_vcpu *vcpu, > - gfn_t gfn, void *data, int offset, int len) > + gfn_t gfn, int offset, void *data, int len) There are a lot of functions/callsites in kvm_main.c which use the "offset, data, len" convention. I'd prefer to switch them all at the same time for consistency, but I think that's too large of a change to splice in here: so I'll leave it as is for now.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 09d4d85691e1..d9c0fa7c907f 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2984,6 +2984,9 @@ static int next_segment(unsigned long len, int offset) return len; } +/* + * Copy 'len' bytes from guest memory at '(gfn * PAGE_SIZE) + offset' to 'data' + */ static int __kvm_read_guest_page(struct kvm_memory_slot *slot, gfn_t gfn, void *data, int offset, int len) { @@ -3085,6 +3088,9 @@ int kvm_vcpu_read_guest_atomic(struct kvm_vcpu *vcpu, gpa_t gpa, } EXPORT_SYMBOL_GPL(kvm_vcpu_read_guest_atomic); +/* + * Copy 'len' bytes from 'data' into guest memory at '(gfn * PAGE_SIZE) + offset' + */ static int __kvm_write_guest_page(struct kvm *kvm, struct kvm_memory_slot *memslot, gfn_t gfn, const void *data, int offset, int len)
The order of parameters in these function signature is a little strange, with "offset" actually applying to "gfn" rather than to "data". Add short comments to make things perfectly clear. Signed-off-by: Anish Moorthy <amoorthy@google.com> --- virt/kvm/kvm_main.c | 6 ++++++ 1 file changed, 6 insertions(+)