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@xxxxxxxxxx>
>>>> Suggested-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>>> ---
>>>> .../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.
