Re: [PATCH] KVM: s390: get rid of register asm usage

[Thomas Huth <thuth@redhat.com>]

On 22/06/2021 09.50, Christian Borntraeger wrote:
> 
> 
> On 22.06.21 09:46, Thomas Huth wrote:
>> On 22/06/2021 09.43, Christian Borntraeger wrote:
>>>
>>>
>>> On 22.06.21 09:36, Thomas Huth wrote:
>>>> On 21/06/2021 16.03, Heiko Carstens wrote:
>>>>> Using register asm statements has been proven to be very error prone,
>>>>> especially when using code instrumentation where gcc may add function
>>>>> calls, which clobbers register contents in an unexpected way.
>>>>>
>>>>> Therefore get rid of register asm statements in kvm code, even though
>>>>> there is currently nothing wrong with them. This way we know for sure
>>>>> that this bug class won't be introduced here.
>>>>>
>>>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
>>>>> ---
>>>>>   arch/s390/kvm/kvm-s390.c | 18 +++++++++---------
>>>>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>>> index 1296fc10f80c..4b7b24f07790 100644
>>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>>> @@ -329,31 +329,31 @@ static void allow_cpu_feat(unsigned long nr)
>>>>>   static inline int plo_test_bit(unsigned char nr)
>>>>>   {
>>>>> -    register unsigned long r0 asm("0") = (unsigned long) nr | 0x100;
>>>>> +    unsigned long function = (unsigned long) nr | 0x100;
>>>>>       int cc;
>>>>>       asm volatile(
>>>>> +        "    lgr    0,%[function]\n"
>>>>>           /* Parameter registers are ignored for "test bit" */
>>>>>           "    plo    0,0,0,0(0)\n"
>>>>>           "    ipm    %0\n"
>>>>>           "    srl    %0,28\n"
>>>>>           : "=d" (cc)
>>>>> -        : "d" (r0)
>>>>> -        : "cc");
>>>>> +        : [function] "d" (function)
>>>>> +        : "cc", "0");
>>>>>       return cc == 0;
>>>>>   }
>>>>>   static __always_inline void __insn32_query(unsigned int opcode, u8 
>>>>> *query)
>>>>>   {
>>>>> -    register unsigned long r0 asm("0") = 0;    /* query function */
>>>>> -    register unsigned long r1 asm("1") = (unsigned long) query;
>>>>> -
>>>>>       asm volatile(
>>>>> -        /* Parameter regs are ignored */
>>>>> +        "    lghi    0,0\n"
>>>>> +        "    lgr    1,%[query]\n"
>>>>> +        /* Parameter registers are ignored */
>>>>>           "    .insn    rrf,%[opc] << 16,2,4,6,0\n"
>>>>>           :
>>>>> -        : "d" (r0), "a" (r1), [opc] "i" (opcode)
>>>>> -        : "cc", "memory");
>>>>> +        : [query] "d" ((unsigned long)query), [opc] "i" (opcode)
>>>>
>>>> Wouldn't it be better to keep the "a" constraint instead of "d" to avoid 
>>>> that the compiler ever passes the "query" value in r0 ?
>>>> Otherwise the query value might get trashed if it is passed in r0...
>>>
>>> I first thought the same, but if you look closely the value is only used 
>>> by the lgr, to load
>>> the value finally into r1. So d is correct as lgr can take all registers.
>>
>> But what about the "lghi    0,0" right in front of it? ... I've got the 
>> feeling that I'm missing something here...
> 
> It does load an immediate value of 0 into register 0. Are you afraid of an 
> early clobber if
> gcc decides to use r0 for query?

Right, that was my concern. It's a "static __always_inline" function, so can 
we be sure that query is still always located in a register that is reserved 
for parameters (i.e. >= r2) ?

  Thomas