Re: [PATCH 02/13] fixup! KVM: selftests: Add support for vcpu_args_set to aarch64 and s390x

[Ben Gardon <bgardon@google.com>]

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
>