Re: [kvm-unit-tests PATCH v7 4/4] s390x: SCLP unit test

[David Hildenbrand <david@redhat.com>]

On 13.01.20 17:17, Claudio Imbrenda wrote:
> On Mon, 13 Jan 2020 17:06:05 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
> [...]
> 
>>>>> this would be solved by adding special logic to the pgm interrupt
>>>>> handler (as we have discussed in your previous email)
>>>>>     
>>>>
>>>> I see, so the issue should hold for all SCLP checks where we don't
>>>> expect an exception ... hmmm  
>>>  
>>> which is why my wrapper in the unit test is so complicated :)
>>>   
>>
>> so .... if we would implement my suggestion (if we get an exception
>> on a servc instruction, clear sclp_busy) that code would get
>> simplified as well? :)
> 
> sure, as I said, that can be done. The question is if we really want to
> change something in the interrupt handler (shared by all s390x unit
> tests) just for the benefit of this one unit test.
> 
> Also consider that the changes to the interrupt handler would not
> necessarily be trivial. i.e. you need to check that the origin of the
> pgm interrupt is a SERVC instruction, and then act accordingly.
> 

I suggest something like the following:

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 05f30be..d762e83 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -106,10 +106,17 @@ static void fixup_pgm_int(void)
 
 void handle_pgm_int(void)
 {
-       if (!pgm_int_expected)
+       if (!pgm_int_expected) {
+               /*
+                * If we get a PGM interrupt while having sclp_busy=true, we
+                * will loop forever. Just force sclp_busy=false to make
+                * progress here.
+                */
+               sclp_handle_ext();
                report_abort("Unexpected program interrupt: %d at %#lx, ilen %d\n",
                             lc->pgm_int_code, lc->pgm_old_psw.addr,
                             lc->pgm_int_id);
+       }
 
        pgm_int_expected = false;
        fixup_pgm_int();

Then this test could become something like (not sure about cc handling)

diff --git a/s390x/sclp.c b/s390x/sclp.c
index 10f0809..81c5a76 100644
--- a/s390x/sclp.c
+++ b/s390x/sclp.c
@@ -396,25 +396,18 @@ out:
 static void test_instbits(void)
 {
        SCCBHeader *h = (SCCBHeader *)pagebuf;
-       int cc;
 
-       expect_pgm_int();
        sclp_mark_busy();
        h->length = 8;
        sclp_setup_int();
 
        asm volatile(
-               "       .insn   rre,0xb2204200,%1,%2\n"  /* servc %1,%2 */
-               "       ipm     %0\n"
-               "       srl     %0,28"
-               : "=&d" (cc) : "d" (valid_code), "a" (__pa(pagebuf))
+               "       .insn   rre,0xb2204200,%0,%1\n"
+               :: "d" (valid_code), "a" (__pa(pagebuf))
                : "cc", "memory");
-       if (lc->pgm_int_code) {
-               sclp_handle_ext();
-               cc = 1;
-       } else if (!cc)
-               sclp_wait_busy();
-       report(cc == 0, "Instruction format ignored bits");
+       sclp_wait_busy();
+       report(true, "Instruction format ignored bits");
 }


This works correctly. E.g., adding a "*((uint8_t *)-50ul) = 2;"
after the sclp_setup_int(); - to quickly fake a PGM exception - makes the
test abort correctly:

FAIL sclp-1g (0 tests)
FAIL sclp-3g (0 tests)

ABORT: sclp: Unexpected program interrupt: 5 at 0x155e8, ilen 6

-- 
Thanks,

David / dhildenb