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

[Andrew Jones <drjones@redhat.com>]

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