Re: [kvm-unit-tests PATCH v2 3/3] s390x: Ultravisor guest API test

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

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

On 7/30/20 5:58 PM, Thomas Huth wrote:
> On 30/07/2020 13.16, Cornelia Huck wrote:
>> On Mon, 27 Jul 2020 05:54:15 -0400
>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>>> Test the error conditions of guest 2 Ultravisor calls, namely:
>>>      * Query Ultravisor information
>>>      * Set shared access
>>>      * Remove shared access
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>> ---
>>>  lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
>>>  s390x/Makefile      |   1 +
>>>  s390x/unittests.cfg |   3 +
>>>  s390x/uv-guest.c    | 159 ++++++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 231 insertions(+)
>>>  create mode 100644 lib/s390x/asm/uv.h
>>>  create mode 100644 s390x/uv-guest.c
>>>
>>
>> (...)
>>
>>> +static inline int uv_call(unsigned long r1, unsigned long r2)
>>> +{
>>> +	int cc;
>>> +
>>> +	asm volatile(
>>> +		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
>>> +		"		brc	3,0b\n"
>>> +		"		ipm	%[cc]\n"
>>> +		"		srl	%[cc],28\n"
>>> +		: [cc] "=d" (cc)
>>> +		: [r1] "a" (r1), [r2] "a" (r2)
>>> +		: "memory", "cc");
>>> +	return cc;
>>> +}
>>
>> This returns the condition code, but no caller seems to check it
>> (instead, they look at header.rc, which is presumably only set if the
>> instruction executed successfully in some way?)
>>
>> Looking at the kernel, it retries for cc > 1 (presumably busy
>> conditions), and cc != 0 seems to be considered a failure. Do we want
>> to look at the cc here as well?
> 
> It's there - but here it's in the assembly code, the "brc 3,0b".

Yes, we needed to factor that out in KVM because we sometimes need to
schedule and then it looks nicer handling that in C code. The branch on
condition will jump back for cc 2 and 3. cc 0 and 1 are success and
error respectively and only then the rc and rrc in the UV header are set.

> 
> Patch looks ok to me (but I didn't do a full review):
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 



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