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

Thomas Huth
2021-06-22T08:04:29Z

On 22/06/2021 10.00, Christian Borntraeger wrote:
>
>
> On 22.06.21 09:57, Christian Borntraeger wrote:
>>
>>
>> On 22.06.21 09:53, Thomas Huth wrote:
>>> 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)
>>          : "cc", "memory", "0", "1");
>>>>>>>
>>>>>>> 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) ?
>>
>> Adding Andreas. I know that gcc provides the early clobber "&" if an
>> output is written before all imputs are used.
>> but in this case it is not an output but a clobbered register.
>> The clobber list contains 0 and 1, so I guess gcc will never use those as
>> input register.
>> Andreas, is that correct?
>>
>>
>> PS: Of course switching both instructions does not hurt
>
> Answering myself:
>
> https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Clobbers-and-Scratch-Registers
>
> When the compiler selects which registers to use to represent input and
> output operands, it does not use any of the clobbered registers. As a
> result, clobbered registers are available for any use in the assembler code.

Ah, ok, TIL, thanks, then it is fine as it is. Feel free to add my
Reviewed-by without any further modifications.

Thomas