selftests: kvm: make syncregs more reliable on s390
diff mbox series

Message ID 20191010075236.100247-1-borntraeger@de.ibm.com
State New
Headers show
Series
  • selftests: kvm: make syncregs more reliable on s390
Related show

Commit Message

Christian Borntraeger Oct. 10, 2019, 7:52 a.m. UTC
similar to commit 2c57da356800 ("selftests: kvm: fix sync_regs_test with
newer gccs") and commit 204c91eff798a ("KVM: selftests: do not blindly
clobber registers in guest asm") we better do not rely on gcc leaving
r11 untouched.  We can write the simple ucall inline and have the guest
code completely as small assembler function.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .../testing/selftests/kvm/s390x/sync_regs_test.c  | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Thomas Huth Oct. 10, 2019, 8:20 a.m. UTC | #1
On 10/10/2019 09.52, Christian Borntraeger wrote:
> similar to commit 2c57da356800 ("selftests: kvm: fix sync_regs_test with
> newer gccs") and commit 204c91eff798a ("KVM: selftests: do not blindly
> clobber registers in guest asm") we better do not rely on gcc leaving
> r11 untouched.  We can write the simple ucall inline and have the guest
> code completely as small assembler function.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  .../testing/selftests/kvm/s390x/sync_regs_test.c  | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> index d5290b4ad636..04d6abe68961 100644
> --- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> +++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> @@ -25,12 +25,15 @@
>  
>  static void guest_code(void)
>  {
> -	register u64 stage asm("11") = 0;
> -
> -	for (;;) {
> -		GUEST_SYNC(0);
> -		asm volatile ("ahi %0,1" : : "r"(stage));
> -	}
> +	/*
> +	 * we embed diag 501 here instead of a ucall as the called function
> +	 * could mess with the content of r11 when doing the hypercall

As far as I understood the issue on x86, it's not the called function
that is messing with the counter register (since r11 is not volatile
between function calls), but the compiler shuffled it around here
between the GUEST_SYNC and the inline assembler code. So I'd prefer a
comment like in the x86 version:

 /*
  * We embed diag 501 here instead of doing a ucall to avoid that
  * the compiler reshuffles registers before calling the function.
  */

?

With such an updated comment:

Reviewed-by: Thomas Huth <thuth@redhat.com>
  > +	 */
> +	asm volatile (
> +		"0:	diag 0,0,0x501\n"
> +		"	ahi 11,1\n"
> +		"	j 0b\n"
> +	);
>  }
Christian Borntraeger Oct. 10, 2019, 9:12 a.m. UTC | #2
On 10.10.19 10:20, Thomas Huth wrote:
> On 10/10/2019 09.52, Christian Borntraeger wrote:
>> similar to commit 2c57da356800 ("selftests: kvm: fix sync_regs_test with
>> newer gccs") and commit 204c91eff798a ("KVM: selftests: do not blindly
>> clobber registers in guest asm") we better do not rely on gcc leaving
>> r11 untouched.  We can write the simple ucall inline and have the guest
>> code completely as small assembler function.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  .../testing/selftests/kvm/s390x/sync_regs_test.c  | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>> index d5290b4ad636..04d6abe68961 100644
>> --- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>> +++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>> @@ -25,12 +25,15 @@
>>  
>>  static void guest_code(void)
>>  {
>> -	register u64 stage asm("11") = 0;
>> -
>> -	for (;;) {
>> -		GUEST_SYNC(0);
>> -		asm volatile ("ahi %0,1" : : "r"(stage));
>> -	}
>> +	/*
>> +	 * we embed diag 501 here instead of a ucall as the called function
>> +	 * could mess with the content of r11 when doing the hypercall
> 
> As far as I understood the issue on x86, it's not the called function
> that is messing with the counter register (since r11 is not volatile
> between function calls), but the compiler shuffled it around here
> between the GUEST_SYNC and the inline assembler code. So I'd prefer a
> comment like in the x86 version:

It is actually both.
The called function is GUEST_SYNC  (which is ucall). 
r11 is call-saved, so when we return from GUEST_SYNC r11 is as it is supposed
to be. The problem is that this function (ucall) is allowed to use r11 for anything
as long as it restores it before returning. As we do the guest exit in ucall
then the test code could see some random value for r11.
> 
>  /*
>   * We embed diag 501 here instead of doing a ucall to avoid that
>   * the compiler reshuffles registers before calling the function.
>   */
> 
> ?

What about
>  /*
>   * We embed diag 501 here instead of doing a ucall to avoid that
>   * the compiler has messed with r11 at the time of the ucall.
>   */


> 
> With such an updated comment:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>   > +	 */
>> +	asm volatile (
>> +		"0:	diag 0,0,0x501\n"
>> +		"	ahi 11,1\n"
>> +		"	j 0b\n"
>> +	);
>>  }
>
Thomas Huth Oct. 10, 2019, 9:15 a.m. UTC | #3
On 10/10/2019 11.12, Christian Borntraeger wrote:
> 
> 
> On 10.10.19 10:20, Thomas Huth wrote:
>> On 10/10/2019 09.52, Christian Borntraeger wrote:
>>> similar to commit 2c57da356800 ("selftests: kvm: fix sync_regs_test with
>>> newer gccs") and commit 204c91eff798a ("KVM: selftests: do not blindly
>>> clobber registers in guest asm") we better do not rely on gcc leaving
>>> r11 untouched.  We can write the simple ucall inline and have the guest
>>> code completely as small assembler function.
>>>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  .../testing/selftests/kvm/s390x/sync_regs_test.c  | 15 +++++++++------
>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>>> index d5290b4ad636..04d6abe68961 100644
>>> --- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>>> +++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>>> @@ -25,12 +25,15 @@
>>>  
>>>  static void guest_code(void)
>>>  {
>>> -	register u64 stage asm("11") = 0;
>>> -
>>> -	for (;;) {
>>> -		GUEST_SYNC(0);
>>> -		asm volatile ("ahi %0,1" : : "r"(stage));
>>> -	}
>>> +	/*
>>> +	 * we embed diag 501 here instead of a ucall as the called function
>>> +	 * could mess with the content of r11 when doing the hypercall
>>
>> As far as I understood the issue on x86, it's not the called function
>> that is messing with the counter register (since r11 is not volatile
>> between function calls), but the compiler shuffled it around here
>> between the GUEST_SYNC and the inline assembler code. So I'd prefer a
>> comment like in the x86 version:
> 
> It is actually both.
> The called function is GUEST_SYNC  (which is ucall). 
> r11 is call-saved, so when we return from GUEST_SYNC r11 is as it is supposed
> to be. The problem is that this function (ucall) is allowed to use r11 for anything
> as long as it restores it before returning. As we do the guest exit in ucall
> then the test code could see some random value for r11.

Oh, right!

>>  /*
>>   * We embed diag 501 here instead of doing a ucall to avoid that
>>   * the compiler reshuffles registers before calling the function.
>>   */
>>
>> ?
> 
> What about
>>  /*
>>   * We embed diag 501 here instead of doing a ucall to avoid that
>>   * the compiler has messed with r11 at the time of the ucall.
>>   */

Sounds good!

 Thanks,
  Thomas
Christian Borntraeger Oct. 10, 2019, 10:04 a.m. UTC | #4
On 10.10.19 11:15, Thomas Huth wrote:
> On 10/10/2019 11.12, Christian Borntraeger wrote:
>>
>>
>> On 10.10.19 10:20, Thomas Huth wrote:
>>> On 10/10/2019 09.52, Christian Borntraeger wrote:
>>>> similar to commit 2c57da356800 ("selftests: kvm: fix sync_regs_test with
>>>> newer gccs") and commit 204c91eff798a ("KVM: selftests: do not blindly
>>>> clobber registers in guest asm") we better do not rely on gcc leaving
>>>> r11 untouched.  We can write the simple ucall inline and have the guest
>>>> code completely as small assembler function.
>>>>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>  .../testing/selftests/kvm/s390x/sync_regs_test.c  | 15 +++++++++------
>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>>>> index d5290b4ad636..04d6abe68961 100644
>>>> --- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>>>> +++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>>>> @@ -25,12 +25,15 @@
>>>>  
>>>>  static void guest_code(void)
>>>>  {
>>>> -	register u64 stage asm("11") = 0;
>>>> -
>>>> -	for (;;) {
>>>> -		GUEST_SYNC(0);
>>>> -		asm volatile ("ahi %0,1" : : "r"(stage));
>>>> -	}
>>>> +	/*
>>>> +	 * we embed diag 501 here instead of a ucall as the called function
>>>> +	 * could mess with the content of r11 when doing the hypercall
>>>
>>> As far as I understood the issue on x86, it's not the called function
>>> that is messing with the counter register (since r11 is not volatile
>>> between function calls), but the compiler shuffled it around here
>>> between the GUEST_SYNC and the inline assembler code. So I'd prefer a
>>> comment like in the x86 version:
>>
>> It is actually both.
>> The called function is GUEST_SYNC  (which is ucall). 
>> r11 is call-saved, so when we return from GUEST_SYNC r11 is as it is supposed
>> to be. The problem is that this function (ucall) is allowed to use r11 for anything
>> as long as it restores it before returning. As we do the guest exit in ucall
>> then the test code could see some random value for r11.
> 
> Oh, right!
> 
>>>  /*
>>>   * We embed diag 501 here instead of doing a ucall to avoid that
>>>   * the compiler reshuffles registers before calling the function.
>>>   */
>>>
>>> ?
>>
>> What about
>>>  /*
>>>   * We embed diag 501 here instead of doing a ucall to avoid that
>>>   * the compiler has messed with r11 at the time of the ucall.
>>>   */
> 
> Sounds good!

applied.

Patch
diff mbox series

diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
index d5290b4ad636..04d6abe68961 100644
--- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
@@ -25,12 +25,15 @@ 
 
 static void guest_code(void)
 {
-	register u64 stage asm("11") = 0;
-
-	for (;;) {
-		GUEST_SYNC(0);
-		asm volatile ("ahi %0,1" : : "r"(stage));
-	}
+	/*
+	 * we embed diag 501 here instead of a ucall as the called function
+	 * could mess with the content of r11 when doing the hypercall
+	 */
+	asm volatile (
+		"0:	diag 0,0,0x501\n"
+		"	ahi 11,1\n"
+		"	j 0b\n"
+	);
 }
 
 #define REG_COMPARE(reg) \