| Message ID | 20200214145920.30792-3-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: Various fixes and cleanups | expand |
On 14.02.20 15:59, Andrew Jones wrote: > [Fixed array index (num => i) and made some style changes.] > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > .../selftests/kvm/lib/aarch64/processor.c | 24 ++++--------------- subject says s390, the patch not.
On Fri, Feb 14, 2020 at 09:35:06PM +0100, Christian Borntraeger wrote: > > > On 14.02.20 15:59, Andrew Jones wrote: > > [Fixed array index (num => i) and made some style changes.] > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > .../selftests/kvm/lib/aarch64/processor.c | 24 ++++--------------- > > subject says s390, the patch not. > It's a "fixup!" patch. The original subject has s390, so this one must as well. Thanks, drew
On Fri, Feb 14, 2020 at 6:59 AM Andrew Jones <drjones@redhat.com> wrote: > > [Fixed array index (num => i) and made some style changes.] > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > .../selftests/kvm/lib/aarch64/processor.c | 24 ++++--------------- > 1 file changed, 4 insertions(+), 20 deletions(-) > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c > index 839a76c96f01..f7dffccea12c 100644 > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c > @@ -334,36 +334,20 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code) > aarch64_vcpu_add_default(vm, vcpuid, NULL, guest_code); > } > > -/* VM VCPU Args Set > - * > - * Input Args: > - * vm - Virtual Machine > - * vcpuid - VCPU ID > - * num - number of arguments > - * ... - arguments, each of type uint64_t > - * > - * Output Args: None > - * > - * Return: None > - * > - * Sets the first num function input arguments to the values > - * given as variable args. Each of the variable args is expected to > - * be of type uint64_t. The registers set by this function are r0-r7. > - */ I'm sad to see this comment go. I realize it might be more verbose than necessary, but calling out that the args will all be interpreted as uint_64s and which registers are set feels like useful context to have here. > void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...) > { > va_list ap; > int i; > > TEST_ASSERT(num >= 1 && num <= 8, "Unsupported number of args,\n" > - " num: %u\n", > - num); > + " num: %u\n", num); > > va_start(ap, num); > > - for (i = 0; i < num; i++) > - set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[num]), > + for (i = 0; i < num; i++) { > + set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[i]), > va_arg(ap, uint64_t)); > + } Woops, I should have caught this in the original demand paging test series, but didn't notice because this function was only ever called with one argument. Thank you for fixing this. > > va_end(ap); > } > -- > 2.21.1 >
On Tue, Feb 18, 2020 at 09:30:25AM -0800, Ben Gardon wrote: > On Fri, Feb 14, 2020 at 6:59 AM Andrew Jones <drjones@redhat.com> wrote: > > > > [Fixed array index (num => i) and made some style changes.] > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > .../selftests/kvm/lib/aarch64/processor.c | 24 ++++--------------- > > 1 file changed, 4 insertions(+), 20 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c > > index 839a76c96f01..f7dffccea12c 100644 > > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c > > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c > > @@ -334,36 +334,20 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code) > > aarch64_vcpu_add_default(vm, vcpuid, NULL, guest_code); > > } > > > > -/* VM VCPU Args Set > > - * > > - * Input Args: > > - * vm - Virtual Machine > > - * vcpuid - VCPU ID > > - * num - number of arguments > > - * ... - arguments, each of type uint64_t > > - * > > - * Output Args: None > > - * > > - * Return: None > > - * > > - * Sets the first num function input arguments to the values > > - * given as variable args. Each of the variable args is expected to > > - * be of type uint64_t. The registers set by this function are r0-r7. > > - */ > I'm sad to see this comment go. I realize it might be more verbose > than necessary, but calling out that the args will all be interpreted > as uint_64s and which registers are set feels like useful context to > have here. For me the code makes that super obvious, and I prefer not to describe what code does. Also, I'd put these type of comment blocks, written more generally, in the header files if they're functions that are implemented by all architectures, rather than duplicating them in each source file. > > > void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...) > > { > > va_list ap; > > int i; > > > > TEST_ASSERT(num >= 1 && num <= 8, "Unsupported number of args,\n" > > - " num: %u\n", > > - num); > > + " num: %u\n", num); > > > > va_start(ap, num); > > > > - for (i = 0; i < num; i++) > > - set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[num]), > > + for (i = 0; i < num; i++) { > > + set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[i]), > > va_arg(ap, uint64_t)); > > + } > Woops, I should have caught this in the original demand paging test > series, but didn't notice because this function was only ever called > with one argument. > Thank you for fixing this. > > > > > va_end(ap); > > } > > -- > > 2.21.1 > > > Thanks, drew
On 18/02/20 18:38, Andrew Jones wrote: > For me the code makes that super obvious, and I prefer not to describe what > code does. Also, I'd put these type of comment blocks, written more > generally, in the header files if they're functions that are implemented > by all architectures, rather than duplicating them in each source file. This makes sense. For now I've restored the comment, but moving them to the .h files is a good idea. Paolo
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c index 839a76c96f01..f7dffccea12c 100644 --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c @@ -334,36 +334,20 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code) aarch64_vcpu_add_default(vm, vcpuid, NULL, guest_code); } -/* VM VCPU Args Set - * - * Input Args: - * vm - Virtual Machine - * vcpuid - VCPU ID - * num - number of arguments - * ... - arguments, each of type uint64_t - * - * Output Args: None - * - * Return: None - * - * Sets the first num function input arguments to the values - * given as variable args. Each of the variable args is expected to - * be of type uint64_t. The registers set by this function are r0-r7. - */ void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...) { va_list ap; int i; TEST_ASSERT(num >= 1 && num <= 8, "Unsupported number of args,\n" - " num: %u\n", - num); + " num: %u\n", num); va_start(ap, num); - for (i = 0; i < num; i++) - set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[num]), + for (i = 0; i < num; i++) { + set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[i]), va_arg(ap, uint64_t)); + } va_end(ap); }
[Fixed array index (num => i) and made some style changes.] Signed-off-by: Andrew Jones <drjones@redhat.com> --- .../selftests/kvm/lib/aarch64/processor.c | 24 ++++--------------- 1 file changed, 4 insertions(+), 20 deletions(-)