On Fri, 31 Jul 2020 11:06:25 +0200 Janosch Frank wrote: > On 7/31/20 10:42 AM, Cornelia Huck wrote: > > On Fri, 31 Jul 2020 09:34:41 +0200 > > Janosch Frank wrote: > > > >> 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 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 > >>>>> Reviewed-by: Claudio Imbrenda > >>>>> --- > >>>>> 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". > > > > Ah yes, I missed that. > > > >> > >> 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. > > > > Yeah, it's a bit surprising that rc/rrc are also set with cc 1. > > Is it? > The (r)rc *only* contain meaningful information on CC 1. > On CC 0 they will simply say everything is fine which CC 0 states > already anyway. I would consider "things worked" to actually be meaningful :) (I've seen other instructions indicating different kinds of success.) > > > > > (Can you add a comment? Just so that it is clear that callers never > > need to check the cc, as rc/rrc already contain more information than > > that.) > > I'd rather fix my test code and also check the CC. > I did check it for my other UV tests so I've no idea why I didn't do it > here... > > > How about adding a comment for the cc 2/3 case? > "The brc instruction will take care of the cc 2/3 case where we need to > continue the execution because we were interrupted. > The inline assembly will only return on success/error i.e. cc 0/1." Sounds good.