[v4,06/10] KVM: selftests: Add support for vcpu_args_set to aarch64 and s390x
diff mbox series

Message ID 20200123180436.99487-7-bgardon@google.com
State New
Headers show
Series
  • Create a userfaultfd demand paging test
Related show

Commit Message

Ben Gardon Jan. 23, 2020, 6:04 p.m. UTC
Currently vcpu_args_set is only implemented for x86. This makes writing
tests with multiple vCPUs difficult as each guest vCPU must either a.)
do the same thing or b.) derive some kind of unique token from it's
registers or the architecture. To simplify the process of writing tests
with multiple vCPUs for s390 and aarch64, add set args functions for
those architectures.

Signed-off-by: Ben Gardon <bgardon@google.com>
---
 .../selftests/kvm/lib/aarch64/processor.c     | 33 +++++++++++++++++
 .../selftests/kvm/lib/s390x/processor.c       | 35 +++++++++++++++++++
 2 files changed, 68 insertions(+)

Comments

Paolo Bonzini Jan. 24, 2020, 9:03 a.m. UTC | #1
CCing Marc, Conny and Christian (plus Thomas and Drew who were already
in the list) for review.

Thanks,

Paolo

On 23/01/20 19:04, Ben Gardon wrote:
> Currently vcpu_args_set is only implemented for x86. This makes writing
> tests with multiple vCPUs difficult as each guest vCPU must either a.)
> do the same thing or b.) derive some kind of unique token from it's
> registers or the architecture. To simplify the process of writing tests
> with multiple vCPUs for s390 and aarch64, add set args functions for
> those architectures.
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  .../selftests/kvm/lib/aarch64/processor.c     | 33 +++++++++++++++++
>  .../selftests/kvm/lib/s390x/processor.c       | 35 +++++++++++++++++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 86036a59a668e..a2ff90a75f326 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -333,3 +333,36 @@ 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;
> +
> +	TEST_ASSERT(num >= 1 && num <= 8, "Unsupported number of args,\n"
> +		    "  num: %u\n",
> +		    num);
> +
> +	va_start(ap, num);
> +
> +	for (i = 0; i < num; i++)
> +		set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[num]),
> +			va_arg(ap, uint64_t));
> +
> +	va_end(ap);
> +}
> diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> index 32a02360b1eb0..680f37be9dbc9 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> @@ -269,6 +269,41 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
>  	run->psw_addr = (uintptr_t)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 r2-r6.
> + */
> +void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
> +{
> +	va_list ap;
> +	struct kvm_regs regs;
> +
> +	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"
> +		    "  num: %u\n",
> +		    num);
> +
> +	va_start(ap, num);
> +	vcpu_regs_get(vm, vcpuid, &regs);
> +
> +	for (i = 0; i < num; i++)
> +		regs.gprs[i + 2] = va_arg(ap, uint64_t);
> +
> +	vcpu_regs_set(vm, vcpuid, &regs);
> +	va_end(ap);
> +}
> +
>  void vcpu_dump(FILE *stream, struct kvm_vm *vm, uint32_t vcpuid, uint8_t indent)
>  {
>  	struct vcpu *vcpu = vm->vcpu_head;
>
Andrew Jones Jan. 24, 2020, 9:35 a.m. UTC | #2
On Fri, Jan 24, 2020 at 10:03:08AM +0100, Paolo Bonzini wrote:
> CCing Marc, Conny and Christian (plus Thomas and Drew who were already
> in the list) for review.
> 
> Thanks,
> 
> Paolo
> 
> On 23/01/20 19:04, Ben Gardon wrote:
> > Currently vcpu_args_set is only implemented for x86. This makes writing
> > tests with multiple vCPUs difficult as each guest vCPU must either a.)
> > do the same thing or b.) derive some kind of unique token from it's
> > registers or the architecture. To simplify the process of writing tests
> > with multiple vCPUs for s390 and aarch64, add set args functions for
> > those architectures.

It'd be nice to keep the separate architecture changes in separate
patches. Otherwise I can't really give an r-b.

> > 
> > Signed-off-by: Ben Gardon <bgardon@google.com>
> > ---
> >  .../selftests/kvm/lib/aarch64/processor.c     | 33 +++++++++++++++++
> >  .../selftests/kvm/lib/s390x/processor.c       | 35 +++++++++++++++++++
> >  2 files changed, 68 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > index 86036a59a668e..a2ff90a75f326 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > @@ -333,3 +333,36 @@ 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.
> > + */

lib/aarch64/processor.c so far doesn't have big function headers like
this. Also, since this function is common for all architectures [now],
I feel like the documentation should be in common code - so in the header
file.

> > +void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
> > +{
> > +	va_list ap;
> > +
> > +	TEST_ASSERT(num >= 1 && num <= 8, "Unsupported number of args,\n"
> > +		    "  num: %u\n",
> > +		    num);

Weird line breaking. I see it came from the x86 implementation, but it's
weird there too... Personally I'd just put it all on one line, because
my vt100 died two decades ago.

> > +
> > +	va_start(ap, num);
> > +
> > +	for (i = 0; i < num; i++)
> > +		set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[num]),
                                                             ^^ should be 'i'

> > +			va_arg(ap, uint64_t));

nit: I'd use {} because of the line break. Or just not break the line and
bust the 80 char "limit" (RIP vt100).

Thanks,
drew

> > +
> > +	va_end(ap);
> > +}
> > diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> > index 32a02360b1eb0..680f37be9dbc9 100644
> > --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> > @@ -269,6 +269,41 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
> >  	run->psw_addr = (uintptr_t)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 r2-r6.
> > + */
> > +void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
> > +{
> > +	va_list ap;
> > +	struct kvm_regs regs;
> > +
> > +	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"
> > +		    "  num: %u\n",
> > +		    num);
> > +
> > +	va_start(ap, num);
> > +	vcpu_regs_get(vm, vcpuid, &regs);
> > +
> > +	for (i = 0; i < num; i++)
> > +		regs.gprs[i + 2] = va_arg(ap, uint64_t);
> > +
> > +	vcpu_regs_set(vm, vcpuid, &regs);
> > +	va_end(ap);
> > +}
> > +
> >  void vcpu_dump(FILE *stream, struct kvm_vm *vm, uint32_t vcpuid, uint8_t indent)
> >  {
> >  	struct vcpu *vcpu = vm->vcpu_head;
> > 
>
Paolo Bonzini Jan. 24, 2020, 9:44 a.m. UTC | #3
On 24/01/20 10:35, Andrew Jones wrote:
> On Fri, Jan 24, 2020 at 10:03:08AM +0100, Paolo Bonzini wrote:
>> CCing Marc, Conny and Christian (plus Thomas and Drew who were already
>> in the list) for review.
>>
>> Thanks,
>>
>> Paolo
>>
>> On 23/01/20 19:04, Ben Gardon wrote:
>>> Currently vcpu_args_set is only implemented for x86. This makes writing
>>> tests with multiple vCPUs difficult as each guest vCPU must either a.)
>>> do the same thing or b.) derive some kind of unique token from it's
>>> registers or the architecture. To simplify the process of writing tests
>>> with multiple vCPUs for s390 and aarch64, add set args functions for
>>> those architectures.
> 
> It'd be nice to keep the separate architecture changes in separate
> patches. Otherwise I can't really give an r-b.

You can say:

For ARM parts:

Reviewed-by: ...

Paolo

>>>
>>> Signed-off-by: Ben Gardon <bgardon@google.com>
>>> ---
>>>  .../selftests/kvm/lib/aarch64/processor.c     | 33 +++++++++++++++++
>>>  .../selftests/kvm/lib/s390x/processor.c       | 35 +++++++++++++++++++
>>>  2 files changed, 68 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
>>> index 86036a59a668e..a2ff90a75f326 100644
>>> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
>>> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
>>> @@ -333,3 +333,36 @@ 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.
>>> + */
> 
> lib/aarch64/processor.c so far doesn't have big function headers like
> this. Also, since this function is common for all architectures [now],
> I feel like the documentation should be in common code - so in the header
> file.
> 
>>> +void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
>>> +{
>>> +	va_list ap;
>>> +
>>> +	TEST_ASSERT(num >= 1 && num <= 8, "Unsupported number of args,\n"
>>> +		    "  num: %u\n",
>>> +		    num);
> 
> Weird line breaking. I see it came from the x86 implementation, but it's
> weird there too... Personally I'd just put it all on one line, because
> my vt100 died two decades ago.
> 
>>> +
>>> +	va_start(ap, num);
>>> +
>>> +	for (i = 0; i < num; i++)
>>> +		set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[num]),
>                                                              ^^ should be 'i'
> 
>>> +			va_arg(ap, uint64_t));
> 
> nit: I'd use {} because of the line break. Or just not break the line and
> bust the 80 char "limit" (RIP vt100).
> 
> Thanks,
> drew
> 
>>> +
>>> +	va_end(ap);
>>> +}
>>> diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
>>> index 32a02360b1eb0..680f37be9dbc9 100644
>>> --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
>>> +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
>>> @@ -269,6 +269,41 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
>>>  	run->psw_addr = (uintptr_t)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 r2-r6.
>>> + */
>>> +void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
>>> +{
>>> +	va_list ap;
>>> +	struct kvm_regs regs;
>>> +
>>> +	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"
>>> +		    "  num: %u\n",
>>> +		    num);
>>> +
>>> +	va_start(ap, num);
>>> +	vcpu_regs_get(vm, vcpuid, &regs);
>>> +
>>> +	for (i = 0; i < num; i++)
>>> +		regs.gprs[i + 2] = va_arg(ap, uint64_t);
>>> +
>>> +	vcpu_regs_set(vm, vcpuid, &regs);
>>> +	va_end(ap);
>>> +}
>>> +
>>>  void vcpu_dump(FILE *stream, struct kvm_vm *vm, uint32_t vcpuid, uint8_t indent)
>>>  {
>>>  	struct vcpu *vcpu = vm->vcpu_head;
>>>
>>
>
Andrew Jones Jan. 24, 2020, 10:45 a.m. UTC | #4
On Fri, Jan 24, 2020 at 10:35:43AM +0100, Andrew Jones wrote:
> > > +void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
> > > +{
> > > +	va_list ap;
> > > +
> > > +	TEST_ASSERT(num >= 1 && num <= 8, "Unsupported number of args,\n"
> > > +		    "  num: %u\n",
> > > +		    num);
> 
> Weird line breaking. I see it came from the x86 implementation, but it's
> weird there too... Personally I'd just put it all on one line, because
> my vt100 died two decades ago.
> 
> > > +
> > > +	va_start(ap, num);
> > > +
> > > +	for (i = 0; i < num; i++)
> > > +		set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[num]),
>                                                              ^^ should be 'i'

The declaration of 'i' is also missing.

> 
> > > +			va_arg(ap, uint64_t));
> 
> nit: I'd use {} because of the line break. Or just not break the line and
> bust the 80 char "limit" (RIP vt100).
>

Thanks,
drew
Christian Borntraeger Jan. 24, 2020, 6:33 p.m. UTC | #5
On 24.01.20 10:03, Paolo Bonzini wrote:
> CCing Marc, Conny and Christian (plus Thomas and Drew who were already
> in the list) for review.

CC Thomas Huth,

> 
> Thanks,
> 
> Paolo
> 
> On 23/01/20 19:04, Ben Gardon wrote:
>> Currently vcpu_args_set is only implemented for x86. This makes writing
>> tests with multiple vCPUs difficult as each guest vCPU must either a.)
>> do the same thing or b.) derive some kind of unique token from it's
>> registers or the architecture. To simplify the process of writing tests
>> with multiple vCPUs for s390 and aarch64, add set args functions for
>> those architectures.
[...]
>>  .../selftests/kvm/lib/s390x/processor.c       | 35 +++++++++++++++++++
[...]
>> --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
>> +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
>> @@ -269,6 +269,41 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
>>  	run->psw_addr = (uintptr_t)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 r2-r6.
>> + */
>> +void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
>> +{
>> +	va_list ap;
>> +	struct kvm_regs regs;
>> +
>> +	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"
>> +		    "  num: %u\n",
>> +		    num);
>> +
>> +	va_start(ap, num);
>> +	vcpu_regs_get(vm, vcpuid, &regs);
>> +
>> +	for (i = 0; i < num; i++)
>> +		regs.gprs[i + 2] = va_arg(ap, uint64_t);
>> +
>> +	vcpu_regs_set(vm, vcpuid, &regs);
>> +	va_end(ap);
>> +}
>> +
>>  void vcpu_dump(FILE *stream, struct kvm_vm *vm, uint32_t vcpuid, uint8_t indent)
>>  {
>>  	struct vcpu *vcpu = vm->vcpu_head;
>>
> 

Untested but the logic looks sane according to the ELF ABI.
Paolo Bonzini Jan. 25, 2020, 9:34 a.m. UTC | #6
On 23/01/20 19:04, Ben Gardon wrote:
> Currently vcpu_args_set is only implemented for x86. This makes writing
> tests with multiple vCPUs difficult as each guest vCPU must either a.)
> do the same thing or b.) derive some kind of unique token from it's
> registers or the architecture. To simplify the process of writing tests
> with multiple vCPUs for s390 and aarch64, add set args functions for
> those architectures.
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  .../selftests/kvm/lib/aarch64/processor.c     | 33 +++++++++++++++++
>  .../selftests/kvm/lib/s390x/processor.c       | 35 +++++++++++++++++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 86036a59a668e..a2ff90a75f326 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -333,3 +333,36 @@ 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;
> +
> +	TEST_ASSERT(num >= 1 && num <= 8, "Unsupported number of args,\n"
> +		    "  num: %u\n",
> +		    num);
> +
> +	va_start(ap, num);
> +
> +	for (i = 0; i < num; i++)
> +		set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[num]),
> +			va_arg(ap, uint64_t));
> +
> +	va_end(ap);
> +}
> diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> index 32a02360b1eb0..680f37be9dbc9 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> @@ -269,6 +269,41 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
>  	run->psw_addr = (uintptr_t)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 r2-r6.
> + */
> +void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
> +{
> +	va_list ap;
> +	struct kvm_regs regs;
> +
> +	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"
> +		    "  num: %u\n",
> +		    num);
> +
> +	va_start(ap, num);
> +	vcpu_regs_get(vm, vcpuid, &regs);
> +
> +	for (i = 0; i < num; i++)
> +		regs.gprs[i + 2] = va_arg(ap, uint64_t);
> +
> +	vcpu_regs_set(vm, vcpuid, &regs);
> +	va_end(ap);
> +}
> +
>  void vcpu_dump(FILE *stream, struct kvm_vm *vm, uint32_t vcpuid, uint8_t indent)
>  {
>  	struct vcpu *vcpu = vm->vcpu_head;
> 

Squashing this:

diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index a2ff90a75f32..839a76c96f01 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -353,6 +353,7 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
 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",
diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
index 680f37be9dbc..a0b84235c848 100644
--- a/tools/testing/selftests/kvm/lib/s390x/processor.c
+++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
@@ -289,6 +289,7 @@ void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
 {
 	va_list ap;
 	struct kvm_regs regs;
+	int i;
 
 	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"
 		    "  num: %u\n",

Paolo
Andrew Jones Jan. 27, 2020, 8:38 a.m. UTC | #7
On Sat, Jan 25, 2020 at 10:34:16AM +0100, Paolo Bonzini wrote:
> On 23/01/20 19:04, Ben Gardon wrote:
> > Currently vcpu_args_set is only implemented for x86. This makes writing
> > tests with multiple vCPUs difficult as each guest vCPU must either a.)
> > do the same thing or b.) derive some kind of unique token from it's
> > registers or the architecture. To simplify the process of writing tests
> > with multiple vCPUs for s390 and aarch64, add set args functions for
> > those architectures.
> > 
> > Signed-off-by: Ben Gardon <bgardon@google.com>
> > ---
> >  .../selftests/kvm/lib/aarch64/processor.c     | 33 +++++++++++++++++
> >  .../selftests/kvm/lib/s390x/processor.c       | 35 +++++++++++++++++++
> >  2 files changed, 68 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > index 86036a59a668e..a2ff90a75f326 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > @@ -333,3 +333,36 @@ 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;
> > +
> > +	TEST_ASSERT(num >= 1 && num <= 8, "Unsupported number of args,\n"
> > +		    "  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 AArch64 you also need to squash s/num/i/ for this line.

(Plus I'm still not that pleased with adding this big header to this
file for this function, or the weird line breaks in the assert.)

> > +			va_arg(ap, uint64_t));
> > +
> > +	va_end(ap);
> > +}
> > diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> > index 32a02360b1eb0..680f37be9dbc9 100644
> > --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> > @@ -269,6 +269,41 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
> >  	run->psw_addr = (uintptr_t)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 r2-r6.
> > + */
> > +void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
> > +{
> > +	va_list ap;
> > +	struct kvm_regs regs;
> > +
> > +	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"
> > +		    "  num: %u\n",
> > +		    num);
> > +
> > +	va_start(ap, num);
> > +	vcpu_regs_get(vm, vcpuid, &regs);
> > +
> > +	for (i = 0; i < num; i++)
> > +		regs.gprs[i + 2] = va_arg(ap, uint64_t);
> > +
> > +	vcpu_regs_set(vm, vcpuid, &regs);
> > +	va_end(ap);
> > +}
> > +
> >  void vcpu_dump(FILE *stream, struct kvm_vm *vm, uint32_t vcpuid, uint8_t indent)
> >  {
> >  	struct vcpu *vcpu = vm->vcpu_head;
> > 
> 
> Squashing this:
> 
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index a2ff90a75f32..839a76c96f01 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -353,6 +353,7 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
>  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",
> diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> index 680f37be9dbc..a0b84235c848 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> @@ -289,6 +289,7 @@ void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
>  {
>  	va_list ap;
>  	struct kvm_regs regs;
> +	int i;
>  
>  	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"
>  		    "  num: %u\n",
> 
> Paolo
>
Thomas Huth Jan. 27, 2020, 9:01 a.m. UTC | #8
On 23/01/2020 19.04, Ben Gardon wrote:
> Currently vcpu_args_set is only implemented for x86. This makes writing
> tests with multiple vCPUs difficult as each guest vCPU must either a.)
> do the same thing or b.) derive some kind of unique token from it's
> registers or the architecture. To simplify the process of writing tests
> with multiple vCPUs for s390 and aarch64, add set args functions for
> those architectures.
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  .../selftests/kvm/lib/aarch64/processor.c     | 33 +++++++++++++++++
>  .../selftests/kvm/lib/s390x/processor.c       | 35 +++++++++++++++++++
>  2 files changed, 68 insertions(+)
[...]
> diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> index 32a02360b1eb0..680f37be9dbc9 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> @@ -269,6 +269,41 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
>  	run->psw_addr = (uintptr_t)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 r2-r6.
> + */
> +void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
> +{
> +	va_list ap;
> +	struct kvm_regs regs;

You need an "int i;" here to make it compile.

> +	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"

why the "\n" right in the middle of the string? Could you please make it
one-line only?

> +		    "  num: %u\n",
> +		    num);
> +
> +	va_start(ap, num);
> +	vcpu_regs_get(vm, vcpuid, &regs);
> +
> +	for (i = 0; i < num; i++)
> +		regs.gprs[i + 2] = va_arg(ap, uint64_t);
> +
> +	vcpu_regs_set(vm, vcpuid, &regs);
> +	va_end(ap);
> +}

... but apart from the nits, this looks basically sane to me.

 Thomas

Patch
diff mbox series

diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 86036a59a668e..a2ff90a75f326 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -333,3 +333,36 @@  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;
+
+	TEST_ASSERT(num >= 1 && num <= 8, "Unsupported number of args,\n"
+		    "  num: %u\n",
+		    num);
+
+	va_start(ap, num);
+
+	for (i = 0; i < num; i++)
+		set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[num]),
+			va_arg(ap, uint64_t));
+
+	va_end(ap);
+}
diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
index 32a02360b1eb0..680f37be9dbc9 100644
--- a/tools/testing/selftests/kvm/lib/s390x/processor.c
+++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
@@ -269,6 +269,41 @@  void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
 	run->psw_addr = (uintptr_t)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 r2-r6.
+ */
+void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
+{
+	va_list ap;
+	struct kvm_regs regs;
+
+	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"
+		    "  num: %u\n",
+		    num);
+
+	va_start(ap, num);
+	vcpu_regs_get(vm, vcpuid, &regs);
+
+	for (i = 0; i < num; i++)
+		regs.gprs[i + 2] = va_arg(ap, uint64_t);
+
+	vcpu_regs_set(vm, vcpuid, &regs);
+	va_end(ap);
+}
+
 void vcpu_dump(FILE *stream, struct kvm_vm *vm, uint32_t vcpuid, uint8_t indent)
 {
 	struct vcpu *vcpu = vm->vcpu_head;