Re: [kvm-unit-tests PATCH v6 3/4] s390x: lib: add SPX and STPX instruction wrapper

[Janosch Frank <frankja@linux.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3329 bytes --]

On 1/9/20 6:13 PM, Claudio Imbrenda wrote:
> On Thu, 9 Jan 2020 18:05:42 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 1/9/20 5:58 PM, Thomas Huth wrote:
>>> On 09/01/2020 17.50, Claudio Imbrenda wrote:  
>>>> On Thu, 9 Jan 2020 17:43:55 +0100
>>>> Thomas Huth <thuth@redhat.com> wrote:
>>>>  
>>>>> On 09/01/2020 17.16, Claudio Imbrenda wrote:  
>>>>>> Add a wrapper for the SET PREFIX and STORE PREFIX instructions,
>>>>>> and use it instead of using inline assembly everywhere.
>>>>>>
>>>>>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>>>>> ---
>>>>>>  lib/s390x/asm/arch_def.h | 10 ++++++++++
>>>>>>  s390x/intercept.c        | 33 +++++++++++++--------------------
>>>>>>  2 files changed, 23 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>>>>>> index 1a5e3c6..465fe0f 100644
>>>>>> --- a/lib/s390x/asm/arch_def.h
>>>>>> +++ b/lib/s390x/asm/arch_def.h
>>>>>> @@ -284,4 +284,14 @@ static inline int servc(uint32_t command,
>>>>>> unsigned long sccb) return cc;
>>>>>>  }
>>>>>>  
>>>>>> +static inline void spx(uint32_t *new_prefix)    
>>>>>
>>>>> Looking at this a second time ... why is new_prefix a pointer? A
>>>>> normal value should be sufficient here, shouldn't it?  
>>>>
>>>> no. if you look at the code in the same patch, intercept.c at some
>>>> points needs to pass "wrong" pointers to spx and stpx in order to
>>>> test them, so this needs to be a pointer
>>>>
>>>> the instructions themselves expect pointers (base register +
>>>> offset)  
>>>
>>> Ah, you're right, that "Q" constraint always confuses me... I guess
>>> you could do it without pointers when using the "r" constraint, but
>>> it's likely better to do it the same way as stpx, so your patch
>>> should be fine.  
>>
>> Honestly, I'd rather have stpx return a u32 than passing a ptr.
> 
> that's what I had done initially, but it doesn't work, see above for
> the reasons why we need a pointer

I prefer having the "normal" usage in the library and the abnormal usage
as inline assembly, that's most often why we use inline assembly anyway
(apart from the lib). The library doesn't need to fit every use-case,
but rather serve the most used things to increase development speed and
readability.

> 
>> That's how the kernel does it and is in-line with epswe/lpswe and
>> sctlg/lctlg which are already in the library.
> 
> the kernel does not need to test wrong addresses.
> 
> I could have spx accept an int and stpx return an int, but then
> intercept.c would still need some inline assembly for SPX and STPX
> 
>> Also, if possible names like set_prefix and store_prefix (or better
>> get_prefix) prefix would make it much more readable.
> 
> this can be done, but that's not how all the other wrappers are

Maybe it's just me, but I always confuse stpx with spx since the
designers did not use lpx/stpx which is much more obvious.

> 
>>>   
>>>>>> +{
>>>>>> +	asm volatile("spx %0" : : "Q" (*new_prefix) : "memory");
>>>>>> +}
>>>>>> +
>>>>>> +static inline void stpx(uint32_t *current_prefix)
>>>>>> +{
>>>>>> +	asm volatile("stpx %0" : "=Q" (*current_prefix));
>>>>>> +}
>>>>>> +    
>>>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>   
>>
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]