* Re: [PATCH] KVM: s390: get rid of register asm usage
2021-06-21 14:03 [PATCH] KVM: s390: get rid of register asm usage Heiko Carstens
@ 2021-06-21 14:34 ` Cornelia Huck
2021-06-21 14:39 ` Claudio Imbrenda
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Cornelia Huck @ 2021-06-21 14:34 UTC (permalink / raw)
To: Heiko Carstens, Christian Borntraeger, Janosch Frank
Cc: David Hildenbrand, Claudio Imbrenda, kvm, linux-s390
%On Mon, Jun 21 2021, Heiko Carstens <hca@linux.ibm.com> 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(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] KVM: s390: get rid of register asm usage
2021-06-21 14:03 [PATCH] KVM: s390: get rid of register asm usage Heiko Carstens
2021-06-21 14:34 ` Cornelia Huck
@ 2021-06-21 14:39 ` Claudio Imbrenda
2021-06-22 7:36 ` Thomas Huth
2021-06-22 14:56 ` Christian Borntraeger
3 siblings, 0 replies; 12+ messages in thread
From: Claudio Imbrenda @ 2021-06-21 14:39 UTC (permalink / raw)
To: Heiko Carstens
Cc: Christian Borntraeger, Janosch Frank, David Hildenbrand,
Cornelia Huck, kvm, linux-s390
On Mon, 21 Jun 2021 16:03:56 +0200
Heiko Carstens <hca@linux.ibm.com> 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>
Reviewed-by: Claudio Imbrenda <imbrenda@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");
> }
>
> #define INSN_SORTL 0xb938
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] KVM: s390: get rid of register asm usage
2021-06-21 14:03 [PATCH] KVM: s390: get rid of register asm usage Heiko Carstens
2021-06-21 14:34 ` Cornelia Huck
2021-06-21 14:39 ` Claudio Imbrenda
@ 2021-06-22 7:36 ` Thomas Huth
2021-06-22 7:43 ` Christian Borntraeger
2021-06-22 14:56 ` Christian Borntraeger
3 siblings, 1 reply; 12+ messages in thread
From: Thomas Huth @ 2021-06-22 7:36 UTC (permalink / raw)
To: Heiko Carstens, Christian Borntraeger, Janosch Frank
Cc: David Hildenbrand, Cornelia Huck, Claudio Imbrenda, kvm, linux-s390
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...
> + : "cc", "memory", "0", "1");
> }
With "a" instead of "d":
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] KVM: s390: get rid of register asm usage
2021-06-22 7:36 ` Thomas Huth
@ 2021-06-22 7:43 ` Christian Borntraeger
2021-06-22 7:46 ` Thomas Huth
0 siblings, 1 reply; 12+ messages in thread
From: Christian Borntraeger @ 2021-06-22 7:43 UTC (permalink / raw)
To: Thomas Huth, Heiko Carstens, Janosch Frank
Cc: David Hildenbrand, Cornelia Huck, Claudio Imbrenda, kvm, linux-s390
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] KVM: s390: get rid of register asm usage
2021-06-22 7:43 ` Christian Borntraeger
@ 2021-06-22 7:46 ` Thomas Huth
2021-06-22 7:50 ` Christian Borntraeger
0 siblings, 1 reply; 12+ messages in thread
From: Thomas Huth @ 2021-06-22 7:46 UTC (permalink / raw)
To: Christian Borntraeger, Heiko Carstens, Janosch Frank
Cc: David Hildenbrand, Cornelia Huck, Claudio Imbrenda, kvm, linux-s390
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...
Heiko, maybe you could at least swap the initialization of r0 and r1, then
I'd feel a little bit better...
Thomas
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] KVM: s390: get rid of register asm usage
2021-06-22 7:46 ` Thomas Huth
@ 2021-06-22 7:50 ` Christian Borntraeger
2021-06-22 7:53 ` Thomas Huth
0 siblings, 1 reply; 12+ messages in thread
From: Christian Borntraeger @ 2021-06-22 7:50 UTC (permalink / raw)
To: Thomas Huth, Heiko Carstens, Janosch Frank
Cc: David Hildenbrand, Cornelia Huck, Claudio Imbrenda, kvm, linux-s390
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?
> Heiko, maybe you could at least swap the initialization of r0 and r1, then I'd feel a little bit better...
>
> Thomas
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] KVM: s390: get rid of register asm usage
2021-06-22 7:50 ` Christian Borntraeger
@ 2021-06-22 7:53 ` Thomas Huth
2021-06-22 7:57 ` Christian Borntraeger
0 siblings, 1 reply; 12+ messages in thread
From: Thomas Huth @ 2021-06-22 7:53 UTC (permalink / raw)
To: Christian Borntraeger, Heiko Carstens, Janosch Frank
Cc: David Hildenbrand, Cornelia Huck, Claudio Imbrenda, kvm, linux-s390
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] KVM: s390: get rid of register asm usage
2021-06-22 7:53 ` Thomas Huth
@ 2021-06-22 7:57 ` Christian Borntraeger
2021-06-22 8:00 ` Christian Borntraeger
0 siblings, 1 reply; 12+ messages in thread
From: Christian Borntraeger @ 2021-06-22 7:57 UTC (permalink / raw)
To: Thomas Huth, Heiko Carstens, Janosch Frank, Andreas Krebbel
Cc: David Hildenbrand, Cornelia Huck, Claudio Imbrenda, kvm, linux-s390
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] KVM: s390: get rid of register asm usage
2021-06-22 7:57 ` Christian Borntraeger
@ 2021-06-22 8:00 ` Christian Borntraeger
2021-06-22 8:04 ` Thomas Huth
0 siblings, 1 reply; 12+ messages in thread
From: Christian Borntraeger @ 2021-06-22 8:00 UTC (permalink / raw)
To: Thomas Huth, Heiko Carstens, Janosch Frank, Andreas Krebbel
Cc: David Hildenbrand, Cornelia Huck, Claudio Imbrenda, kvm, linux-s390
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] KVM: s390: get rid of register asm usage
2021-06-22 8:00 ` Christian Borntraeger
@ 2021-06-22 8:04 ` Thomas Huth
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Huth @ 2021-06-22 8:04 UTC (permalink / raw)
To: Christian Borntraeger, Heiko Carstens, Janosch Frank, Andreas Krebbel
Cc: David Hildenbrand, Cornelia Huck, Claudio Imbrenda, kvm, linux-s390
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] KVM: s390: get rid of register asm usage
2021-06-21 14:03 [PATCH] KVM: s390: get rid of register asm usage Heiko Carstens
` (2 preceding siblings ...)
2021-06-22 7:36 ` Thomas Huth
@ 2021-06-22 14:56 ` Christian Borntraeger
3 siblings, 0 replies; 12+ messages in thread
From: Christian Borntraeger @ 2021-06-22 14:56 UTC (permalink / raw)
To: Heiko Carstens, Janosch Frank
Cc: David Hildenbrand, Cornelia Huck, Claudio Imbrenda, kvm, linux-s390
On 21.06.21 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>
thanks applied.
> ---
> 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");
> }
>
> #define INSN_SORTL 0xb938
>
^ permalink raw reply [flat|nested] 12+ messages in thread