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!
Thanks,
Thomas
