[PATCH v1 0/4] s390x: Testing the Subchannel I/O

[PATCH v1 0/4] s390x: Testing the Subchannel I/O

[Pierre Morel]

Goal of the series is to have a framwork to test Channel-Subsystem I/O with
QEMU/KVM.

To be able to support interrupt for CSS I/O and for SCLP we need to modify
the interrupt framework to allow re-entrant interruptions.

Making the interrupt handler weak allows the test programm to define its own
interrupt handler. We need to do special work under interrupt like acknoledging
the interrupt.

Being working on PSW bits to allow I/O interrupt, we define all PSW bits in a
dedicated file.

The simple test tests the I/O reading by the SUB Channel. It needs QEMU to
be patched to have the pong device defined.
The pong device answers, for now, with a Hello World to the read request.


Pierre Morel (4):
  s390x: saving regs for interrupts
  s390x: Define the PSW bits
  s390x:irq: make IRQ handler weak
  s390x: Testing the Subchannel I/O read

 lib/s390x/asm/arch_bits.h |  32 ++++++
 lib/s390x/asm/arch_def.h  |   6 +-
 lib/s390x/asm/interrupt.h |  15 ++-
 lib/s390x/css.h           | 244 ++++++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c      | 141 +++++++++++++++++++++++++++
 lib/s390x/interrupt.c     |  16 +--
 s390x/Makefile            |   2 +
 s390x/css.c               | 222 +++++++++++++++++++++++++++++++++++++++++
 s390x/cstart64.S          |  30 ++++--
 s390x/unittests.cfg       |   4 +
 10 files changed, 686 insertions(+), 26 deletions(-)
 create mode 100644 lib/s390x/asm/arch_bits.h
 create mode 100644 lib/s390x/css.h
 create mode 100644 lib/s390x/css_dump.c
 create mode 100644 s390x/css.c

-- 
2.7.4

[PATCH v1 1/4] s390x: saving regs for interrupts

[Pierre Morel]

If we use multiple source of interrupts, for exemple, using SCLP console
to print information while using I/O interrupts or during exceptions, we
need to have a re-entrant register saving interruption handling.

Instead of saving at a static place, let's save the base registers on
the stack.

Note that we keep the static register saving that we need for the RESET
tests.

We also care to give the handlers a pointer to the save registers in
case the handler needs it (fixup_pgm_int needs the old psw address).

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/asm/interrupt.h | 15 ++++++++++-----
 lib/s390x/interrupt.c     | 16 ++++++++--------
 s390x/cstart64.S          | 17 ++++++++++++++---
 3 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
index 4cfade9..a39a3a3 100644
--- a/lib/s390x/asm/interrupt.h
+++ b/lib/s390x/asm/interrupt.h
@@ -15,11 +15,16 @@
 #define EXT_IRQ_EXTERNAL_CALL	0x1202
 #define EXT_IRQ_SERVICE_SIG	0x2401
 
-void handle_pgm_int(void);
-void handle_ext_int(void);
-void handle_mcck_int(void);
-void handle_io_int(void);
-void handle_svc_int(void);
+typedef struct saved_registers {
+        unsigned long regs[15];
+} sregs_t;
+
+void handle_pgm_int(sregs_t *regs);
+void handle_ext_int(sregs_t *regs);
+void handle_mcck_int(sregs_t *regs);
+void handle_io_int(sregs_t *regs);
+void handle_svc_int(sregs_t *regs);
+
 void expect_pgm_int(void);
 void expect_ext_int(void);
 uint16_t clear_pgm_int(void);
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 5cade23..7aecfc5 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -50,7 +50,7 @@ void check_pgm_int_code(uint16_t code)
 	       code == lc->pgm_int_code, code, lc->pgm_int_code);
 }
 
-static void fixup_pgm_int(void)
+static void fixup_pgm_int(sregs_t *regs)
 {
 	switch (lc->pgm_int_code) {
 	case PGM_INT_CODE_PRIVILEGED_OPERATION:
@@ -64,7 +64,7 @@ static void fixup_pgm_int(void)
 		/* Handling for iep.c test case. */
 		if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id & 0x04UL &&
 		    !(lc->trans_exc_id & 0x08UL))
-			lc->pgm_old_psw.addr = lc->sw_int_grs[14];
+			lc->pgm_old_psw.addr = regs->regs[14];
 		break;
 	case PGM_INT_CODE_SEGMENT_TRANSLATION:
 	case PGM_INT_CODE_PAGE_TRANSLATION:
@@ -103,7 +103,7 @@ static void fixup_pgm_int(void)
 	/* suppressed/terminated/completed point already at the next address */
 }
 
-void handle_pgm_int(void)
+void handle_pgm_int(sregs_t *regs)
 {
 	if (!pgm_int_expected)
 		report_abort("Unexpected program interrupt: %d at %#lx, ilen %d\n",
@@ -111,10 +111,10 @@ void handle_pgm_int(void)
 			     lc->pgm_int_id);
 
 	pgm_int_expected = false;
-	fixup_pgm_int();
+	fixup_pgm_int(regs);
 }
 
-void handle_ext_int(void)
+void handle_ext_int(sregs_t *regs)
 {
 	if (!ext_int_expected &&
 	    lc->ext_int_code != EXT_IRQ_SERVICE_SIG) {
@@ -134,19 +134,19 @@ void handle_ext_int(void)
 		lc->ext_old_psw.mask &= ~PSW_MASK_EXT;
 }
 
-void handle_mcck_int(void)
+void handle_mcck_int(sregs_t *regs)
 {
 	report_abort("Unexpected machine check interrupt: at %#lx",
 		     lc->mcck_old_psw.addr);
 }
 
-void handle_io_int(void)
+void handle_io_int(sregs_t *regs)
 {
 	report_abort("Unexpected io interrupt: at %#lx",
 		     lc->io_old_psw.addr);
 }
 
-void handle_svc_int(void)
+void handle_svc_int(sregs_t *regs)
 {
 	report_abort("Unexpected supervisor call interrupt: at %#lx",
 		     lc->svc_old_psw.addr);
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 8e2b21e..eaff481 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -90,6 +90,17 @@ memsetxc:
 	xc 0(1,%r1),0(%r1)
 
 	.macro SAVE_REGS
+	slgfi	%r15, 15 * 8
+	stmg	%r0, %r14, 0(%r15)
+	lgr	%r2, %r15
+	.endm
+
+	.macro RESTORE_REGS
+	lmg     %r0, %r14, 0(%r15)
+	algfi   %r15, 15 * 8
+	.endm
+
+	.macro SAVE_REGS_RESET
 	/* save grs 0-15 */
 	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
 	/* save cr0 */
@@ -105,7 +116,7 @@ memsetxc:
 	stfpc	GEN_LC_SW_INT_FPC
 	.endm
 
-	.macro RESTORE_REGS
+	.macro RESTORE_REGS_RESET
 	/* restore fprs 0-15 + fpc */
 	la	%r1, GEN_LC_SW_INT_FPRS
 	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
@@ -125,7 +136,7 @@ memsetxc:
  */
 .globl diag308_load_reset
 diag308_load_reset:
-	SAVE_REGS
+	SAVE_REGS_RESET
 	/* Save the first PSW word to the IPL PSW */
 	epsw	%r0, %r1
 	st	%r0, 0
@@ -142,7 +153,7 @@ diag308_load_reset:
 	/* We lost cr0 due to the reset */
 0:	larl	%r1, initial_cr0
 	lctlg	%c0, %c0, 0(%r1)
-	RESTORE_REGS
+	RESTORE_REGS_RESET
 	lhi	%r2, 1
 	br	%r14
 
-- 
2.7.4

Re: [PATCH v1 1/4] s390x: saving regs for interrupts

[Janosch Frank]

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

On 11/13/19 1:23 PM, Pierre Morel wrote:
> If we use multiple source of interrupts, for exemple, using SCLP console
> to print information while using I/O interrupts or during exceptions, we
> need to have a re-entrant register saving interruption handling.
> 
> Instead of saving at a static place, let's save the base registers on
> the stack.
> 
> Note that we keep the static register saving that we need for the RESET
> tests.
> 
> We also care to give the handlers a pointer to the save registers in
> case the handler needs it (fixup_pgm_int needs the old psw address).

So you're still ignoring the FPRs...
I disassembled a test and looked at all stds and it looks like printf
and related functions use them. Wouldn't we overwrite test FPRs if
printing in a handler?

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/interrupt.h | 15 ++++++++++-----
>  lib/s390x/interrupt.c     | 16 ++++++++--------
>  s390x/cstart64.S          | 17 ++++++++++++++---
>  3 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 4cfade9..a39a3a3 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -15,11 +15,16 @@
>  #define EXT_IRQ_EXTERNAL_CALL	0x1202
>  #define EXT_IRQ_SERVICE_SIG	0x2401
>  
> -void handle_pgm_int(void);
> -void handle_ext_int(void);
> -void handle_mcck_int(void);
> -void handle_io_int(void);
> -void handle_svc_int(void);
> +typedef struct saved_registers {
> +        unsigned long regs[15];
> +} sregs_t;
> +
> +void handle_pgm_int(sregs_t *regs);
> +void handle_ext_int(sregs_t *regs);
> +void handle_mcck_int(sregs_t *regs);
> +void handle_io_int(sregs_t *regs);
> +void handle_svc_int(sregs_t *regs);
> +
>  void expect_pgm_int(void);
>  void expect_ext_int(void);
>  uint16_t clear_pgm_int(void);
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 5cade23..7aecfc5 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -50,7 +50,7 @@ void check_pgm_int_code(uint16_t code)
>  	       code == lc->pgm_int_code, code, lc->pgm_int_code);
>  }
>  
> -static void fixup_pgm_int(void)
> +static void fixup_pgm_int(sregs_t *regs)
>  {
>  	switch (lc->pgm_int_code) {
>  	case PGM_INT_CODE_PRIVILEGED_OPERATION:
> @@ -64,7 +64,7 @@ static void fixup_pgm_int(void)
>  		/* Handling for iep.c test case. */
>  		if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id & 0x04UL &&
>  		    !(lc->trans_exc_id & 0x08UL))
> -			lc->pgm_old_psw.addr = lc->sw_int_grs[14];
> +			lc->pgm_old_psw.addr = regs->regs[14];
>  		break;
>  	case PGM_INT_CODE_SEGMENT_TRANSLATION:
>  	case PGM_INT_CODE_PAGE_TRANSLATION:
> @@ -103,7 +103,7 @@ static void fixup_pgm_int(void)
>  	/* suppressed/terminated/completed point already at the next address */
>  }
>  
> -void handle_pgm_int(void)
> +void handle_pgm_int(sregs_t *regs)
>  {
>  	if (!pgm_int_expected)
>  		report_abort("Unexpected program interrupt: %d at %#lx, ilen %d\n",
> @@ -111,10 +111,10 @@ void handle_pgm_int(void)
>  			     lc->pgm_int_id);
>  
>  	pgm_int_expected = false;
> -	fixup_pgm_int();
> +	fixup_pgm_int(regs);
>  }
>  
> -void handle_ext_int(void)
> +void handle_ext_int(sregs_t *regs)
>  {
>  	if (!ext_int_expected &&
>  	    lc->ext_int_code != EXT_IRQ_SERVICE_SIG) {
> @@ -134,19 +134,19 @@ void handle_ext_int(void)
>  		lc->ext_old_psw.mask &= ~PSW_MASK_EXT;
>  }
>  
> -void handle_mcck_int(void)
> +void handle_mcck_int(sregs_t *regs)
>  {
>  	report_abort("Unexpected machine check interrupt: at %#lx",
>  		     lc->mcck_old_psw.addr);
>  }
>  
> -void handle_io_int(void)
> +void handle_io_int(sregs_t *regs)
>  {
>  	report_abort("Unexpected io interrupt: at %#lx",
>  		     lc->io_old_psw.addr);
>  }
>  
> -void handle_svc_int(void)
> +void handle_svc_int(sregs_t *regs)
>  {
>  	report_abort("Unexpected supervisor call interrupt: at %#lx",
>  		     lc->svc_old_psw.addr);
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 8e2b21e..eaff481 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -90,6 +90,17 @@ memsetxc:
>  	xc 0(1,%r1),0(%r1)
>  
>  	.macro SAVE_REGS
> +	slgfi	%r15, 15 * 8
> +	stmg	%r0, %r14, 0(%r15)
> +	lgr	%r2, %r15
> +	.endm
> +
> +	.macro RESTORE_REGS
> +	lmg     %r0, %r14, 0(%r15)
> +	algfi   %r15, 15 * 8
> +	.endm
> +
> +	.macro SAVE_REGS_RESET
>  	/* save grs 0-15 */
>  	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
>  	/* save cr0 */
> @@ -105,7 +116,7 @@ memsetxc:
>  	stfpc	GEN_LC_SW_INT_FPC
>  	.endm
>  
> -	.macro RESTORE_REGS
> +	.macro RESTORE_REGS_RESET
>  	/* restore fprs 0-15 + fpc */
>  	la	%r1, GEN_LC_SW_INT_FPRS
>  	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> @@ -125,7 +136,7 @@ memsetxc:
>   */
>  .globl diag308_load_reset
>  diag308_load_reset:
> -	SAVE_REGS
> +	SAVE_REGS_RESET
>  	/* Save the first PSW word to the IPL PSW */
>  	epsw	%r0, %r1
>  	st	%r0, 0
> @@ -142,7 +153,7 @@ diag308_load_reset:
>  	/* We lost cr0 due to the reset */
>  0:	larl	%r1, initial_cr0
>  	lctlg	%c0, %c0, 0(%r1)
> -	RESTORE_REGS
> +	RESTORE_REGS_RESET
>  	lhi	%r2, 1
>  	br	%r14
>  
> 



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

Re: [PATCH v1 1/4] s390x: saving regs for interrupts

[Pierre Morel]

On 2019-11-13 17:12, Janosch Frank wrote:
> On 11/13/19 1:23 PM, Pierre Morel wrote:
>> If we use multiple source of interrupts, for exemple, using SCLP console
>> to print information while using I/O interrupts or during exceptions, we
>> need to have a re-entrant register saving interruption handling.
>>
>> Instead of saving at a static place, let's save the base registers on
>> the stack.
>>
>> Note that we keep the static register saving that we need for the RESET
>> tests.
>>
>> We also care to give the handlers a pointer to the save registers in
>> case the handler needs it (fixup_pgm_int needs the old psw address).
> So you're still ignoring the FPRs...
> I disassembled a test and looked at all stds and it looks like printf
> and related functions use them. Wouldn't we overwrite test FPRs if
> printing in a handler?

If printf uses the FPRs in my opinion we should modify the compilation 
options for the library.

What is the reason for printf and related functions to use floating point?

I will have a deeper look at this.


Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 1/4] s390x: saving regs for interrupts

[David Hildenbrand]

> Am 14.11.2019 um 11:11 schrieb Pierre Morel <pmorel@linux.ibm.com>:
> 
> 
>> On 2019-11-13 17:12, Janosch Frank wrote:
>>> On 11/13/19 1:23 PM, Pierre Morel wrote:
>>> If we use multiple source of interrupts, for exemple, using SCLP console
>>> to print information while using I/O interrupts or during exceptions, we
>>> need to have a re-entrant register saving interruption handling.
>>> 
>>> Instead of saving at a static place, let's save the base registers on
>>> the stack.
>>> 
>>> Note that we keep the static register saving that we need for the RESET
>>> tests.
>>> 
>>> We also care to give the handlers a pointer to the save registers in
>>> case the handler needs it (fixup_pgm_int needs the old psw address).
>> So you're still ignoring the FPRs...
>> I disassembled a test and looked at all stds and it looks like printf
>> and related functions use them. Wouldn't we overwrite test FPRs if
>> printing in a handler?
> 
> If printf uses the FPRs in my opinion we should modify the compilation options for the library.
> 
> What is the reason for printf and related functions to use floating point?
> 

Register spilling. This can and will be done.

Cheers.

> I will have a deeper look at this.
> 
> 
> Regards,
> 
> Pierre
> 
> 
> -- 
> Pierre Morel
> IBM Lab Boeblingen
> 

Re: [PATCH v1 1/4] s390x: saving regs for interrupts

[Pierre Morel]

On 2019-11-14 11:28, David Hildenbrand wrote:
>
>> Am 14.11.2019 um 11:11 schrieb Pierre Morel <pmorel@linux.ibm.com>:
>>
>> 
>>> On 2019-11-13 17:12, Janosch Frank wrote:
>>>> On 11/13/19 1:23 PM, Pierre Morel wrote:
>>>> If we use multiple source of interrupts, for exemple, using SCLP console
>>>> to print information while using I/O interrupts or during exceptions, we
>>>> need to have a re-entrant register saving interruption handling.
>>>>
>>>> Instead of saving at a static place, let's save the base registers on
>>>> the stack.
>>>>
>>>> Note that we keep the static register saving that we need for the RESET
>>>> tests.
>>>>
>>>> We also care to give the handlers a pointer to the save registers in
>>>> case the handler needs it (fixup_pgm_int needs the old psw address).
>>> So you're still ignoring the FPRs...
>>> I disassembled a test and looked at all stds and it looks like printf
>>> and related functions use them. Wouldn't we overwrite test FPRs if
>>> printing in a handler?
>> If printf uses the FPRs in my opinion we should modify the compilation options for the library.
>>
>> What is the reason for printf and related functions to use floating point?
>>
> Register spilling. This can and will be done.


Hum, can you please clarify?

AFAIK register spilling is for a compiler, to use memory if it has not 
enough registers.

So your answer is for the my first sentence, meaning yes register 
spilling will be done
or
do you mean register spilling is the reason why the compiler use FPRs 
and it must be done so?

Thanks,

Pierre


>
> Cheers.
>
>> I will have a deeper look at this.
>>
>>
>> Regards,
>>
>> Pierre
>>
>>
>> -- 
>> Pierre Morel
>> IBM Lab Boeblingen
>>
-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 1/4] s390x: saving regs for interrupts

[David Hildenbrand]

On 14.11.19 12:57, Pierre Morel wrote:
> 
> On 2019-11-14 11:28, David Hildenbrand wrote:
>>
>>> Am 14.11.2019 um 11:11 schrieb Pierre Morel <pmorel@linux.ibm.com>:
>>>
>>> 
>>>> On 2019-11-13 17:12, Janosch Frank wrote:
>>>>> On 11/13/19 1:23 PM, Pierre Morel wrote:
>>>>> If we use multiple source of interrupts, for exemple, using SCLP console
>>>>> to print information while using I/O interrupts or during exceptions, we
>>>>> need to have a re-entrant register saving interruption handling.
>>>>>
>>>>> Instead of saving at a static place, let's save the base registers on
>>>>> the stack.
>>>>>
>>>>> Note that we keep the static register saving that we need for the RESET
>>>>> tests.
>>>>>
>>>>> We also care to give the handlers a pointer to the save registers in
>>>>> case the handler needs it (fixup_pgm_int needs the old psw address).
>>>> So you're still ignoring the FPRs...
>>>> I disassembled a test and looked at all stds and it looks like printf
>>>> and related functions use them. Wouldn't we overwrite test FPRs if
>>>> printing in a handler?
>>> If printf uses the FPRs in my opinion we should modify the compilation options for the library.
>>>
>>> What is the reason for printf and related functions to use floating point?
>>>
>> Register spilling. This can and will be done.
> 
> 
> Hum, can you please clarify?
> 
> AFAIK register spilling is for a compiler, to use memory if it has not
> enough registers.

Not strictly memory. If the compiler needs more GPRS, it can 
save/restore GPRS to FPRS.

Any function the compiler generates is free to use the FPRS..

> 
> So your answer is for the my first sentence, meaning yes register
> spilling will be done
> or
> do you mean register spilling is the reason why the compiler use FPRs
> and it must be done so?

Confused by both options :D The compiler might generate code that uses 
the FPRS although no floating point instructions are in use. That's why 
we have to enable the AFP control and properly take care of FPRS being used.


-- 

Thanks,

David / dhildenb

Re: [PATCH v1 1/4] s390x: saving regs for interrupts

[Pierre Morel]

On 2019-11-14 13:11, David Hildenbrand wrote:
> On 14.11.19 12:57, Pierre Morel wrote:
>>
>> On 2019-11-14 11:28, David Hildenbrand wrote:
>>>
>>>> Am 14.11.2019 um 11:11 schrieb Pierre Morel <pmorel@linux.ibm.com>:
>>>>
>>>> 
>>>>> On 2019-11-13 17:12, Janosch Frank wrote:
>>>>>> On 11/13/19 1:23 PM, Pierre Morel wrote:
>>>>>> If we use multiple source of interrupts, for exemple, using SCLP 
>>>>>> console
>>>>>> to print information while using I/O interrupts or during 
>>>>>> exceptions, we
>>>>>> need to have a re-entrant register saving interruption handling.
>>>>>>
>>>>>> Instead of saving at a static place, let's save the base 
>>>>>> registers on
>>>>>> the stack.
>>>>>>
>>>>>> Note that we keep the static register saving that we need for the 
>>>>>> RESET
>>>>>> tests.
>>>>>>
>>>>>> We also care to give the handlers a pointer to the save registers in
>>>>>> case the handler needs it (fixup_pgm_int needs the old psw address).
>>>>> So you're still ignoring the FPRs...
>>>>> I disassembled a test and looked at all stds and it looks like printf
>>>>> and related functions use them. Wouldn't we overwrite test FPRs if
>>>>> printing in a handler?
>>>> If printf uses the FPRs in my opinion we should modify the 
>>>> compilation options for the library.
>>>>
>>>> What is the reason for printf and related functions to use floating 
>>>> point?
>>>>
>>> Register spilling. This can and will be done.
>>
>>
>> Hum, can you please clarify?
>>
>> AFAIK register spilling is for a compiler, to use memory if it has not
>> enough registers.
>
> Not strictly memory. If the compiler needs more GPRS, it can 
> save/restore GPRS to FPRS.
>
> Any function the compiler generates is free to use the FPRS..
>
>>
>> So your answer is for the my first sentence, meaning yes register
>> spilling will be done
>> or
>> do you mean register spilling is the reason why the compiler use FPRs
>> and it must be done so?
>
> Confused by both options :D The compiler might generate code that uses 
> the FPRS although no floating point instructions are in use. That's 
> why we have to enable the AFP control and properly take care of FPRS 
> being used.
>
>
The compiler has the -msoft-float switch to avoid using the floating 
point instructions and registers, so it is our decision.

Saving the FP registers on exceptions is not very efficient, we loose 
time on each interrupt, not sure that we win it back by using FPregs to 
as Regs backup.

Usually a system at low level uses some enter_fpu, leave_fpu routine to 
enter critical sections using FPU instead of losing time on each 
interruptions.

We can think about this, in between I do as you recomand and save the 
FPregs too.

Best regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 1/4] s390x: saving regs for interrupts

[David Hildenbrand]

> Am 14.11.2019 um 16:21 schrieb Pierre Morel <pmorel@linux.ibm.com>:
> 
> 
>> On 2019-11-14 13:11, David Hildenbrand wrote:
>>> On 14.11.19 12:57, Pierre Morel wrote:
>>> 
>>> On 2019-11-14 11:28, David Hildenbrand wrote:
>>>> 
>>>>> Am 14.11.2019 um 11:11 schrieb Pierre Morel <pmorel@linux.ibm.com>:
>>>>> 
>>>>> 
>>>>>> On 2019-11-13 17:12, Janosch Frank wrote:
>>>>>>> On 11/13/19 1:23 PM, Pierre Morel wrote:
>>>>>>> If we use multiple source of interrupts, for exemple, using SCLP console
>>>>>>> to print information while using I/O interrupts or during exceptions, we
>>>>>>> need to have a re-entrant register saving interruption handling.
>>>>>>> 
>>>>>>> Instead of saving at a static place, let's save the base registers on
>>>>>>> the stack.
>>>>>>> 
>>>>>>> Note that we keep the static register saving that we need for the RESET
>>>>>>> tests.
>>>>>>> 
>>>>>>> We also care to give the handlers a pointer to the save registers in
>>>>>>> case the handler needs it (fixup_pgm_int needs the old psw address).
>>>>>> So you're still ignoring the FPRs...
>>>>>> I disassembled a test and looked at all stds and it looks like printf
>>>>>> and related functions use them. Wouldn't we overwrite test FPRs if
>>>>>> printing in a handler?
>>>>> If printf uses the FPRs in my opinion we should modify the compilation options for the library.
>>>>> 
>>>>> What is the reason for printf and related functions to use floating point?
>>>>> 
>>>> Register spilling. This can and will be done.
>>> 
>>> 
>>> Hum, can you please clarify?
>>> 
>>> AFAIK register spilling is for a compiler, to use memory if it has not
>>> enough registers.
>> 
>> Not strictly memory. If the compiler needs more GPRS, it can save/restore GPRS to FPRS.
>> 
>> Any function the compiler generates is free to use the FPRS..
>> 
>>> 
>>> So your answer is for the my first sentence, meaning yes register
>>> spilling will be done
>>> or
>>> do you mean register spilling is the reason why the compiler use FPRs
>>> and it must be done so?
>> 
>> Confused by both options :D The compiler might generate code that uses the FPRS although no floating point instructions are in use. That's why we have to enable the AFP control and properly take care of FPRS being used.
>> 
>> 
> The compiler has the -msoft-float switch to avoid using the floating point instructions and registers, so it is our decision.

No, not registers AFAIK.

> 
> Saving the FP registers on exceptions is not very efficient, we loose time on each interrupt, not sure that we win it back by using FPregs to as Regs backup.

Who on earth cares about performance here?

> 
> Usually a system at low level uses some enter_fpu, leave_fpu routine to enter critical sections using FPU instead of losing time on each interruptions.
> 
> We can think about this, in between I do as you recomand and save the FPregs too.
> 
> Best regards,
> 
> Pierre
> 
> 
> -- 
> Pierre Morel
> IBM Lab Boeblingen
> 

Re: [PATCH v1 1/4] s390x: saving regs for interrupts

[Pierre Morel]

On 2019-11-14 16:25, David Hildenbrand wrote:
>
>> Am 14.11.2019 um 16:21 schrieb Pierre Morel <pmorel@linux.ibm.com>:
>>
>> 
>>> On 2019-11-14 13:11, David Hildenbrand wrote:
>>>> On 14.11.19 12:57, Pierre Morel wrote:
>>>>
>>>> On 2019-11-14 11:28, David Hildenbrand wrote:
>>>>>> Am 14.11.2019 um 11:11 schrieb Pierre Morel <pmorel@linux.ibm.com>:
>>>>>>
>>>>>> 
>>>>>>> On 2019-11-13 17:12, Janosch Frank wrote:
>>>>>>>> On 11/13/19 1:23 PM, Pierre Morel wrote:
>>>>>>>> If we use multiple source of interrupts, for exemple, using SCLP console
>>>>>>>> to print information while using I/O interrupts or during exceptions, we
>>>>>>>> need to have a re-entrant register saving interruption handling.
>>>>>>>>
>>>>>>>> Instead of saving at a static place, let's save the base registers on
>>>>>>>> the stack.
>>>>>>>>
>>>>>>>> Note that we keep the static register saving that we need for the RESET
>>>>>>>> tests.
>>>>>>>>
>>>>>>>> We also care to give the handlers a pointer to the save registers in
>>>>>>>> case the handler needs it (fixup_pgm_int needs the old psw address).
>>>>>>> So you're still ignoring the FPRs...
>>>>>>> I disassembled a test and looked at all stds and it looks like printf
>>>>>>> and related functions use them. Wouldn't we overwrite test FPRs if
>>>>>>> printing in a handler?
>>>>>> If printf uses the FPRs in my opinion we should modify the compilation options for the library.
>>>>>>
>>>>>> What is the reason for printf and related functions to use floating point?
>>>>>>
>>>>> Register spilling. This can and will be done.
>>>>
>>>> Hum, can you please clarify?
>>>>
>>>> AFAIK register spilling is for a compiler, to use memory if it has not
>>>> enough registers.
>>> Not strictly memory. If the compiler needs more GPRS, it can save/restore GPRS to FPRS.
>>>
>>> Any function the compiler generates is free to use the FPRS..
>>>
>>>> So your answer is for the my first sentence, meaning yes register
>>>> spilling will be done
>>>> or
>>>> do you mean register spilling is the reason why the compiler use FPRs
>>>> and it must be done so?
>>> Confused by both options :D The compiler might generate code that uses the FPRS although no floating point instructions are in use. That's why we have to enable the AFP control and properly take care of FPRS being used.
>>>
>>>
>> The compiler has the -msoft-float switch to avoid using the floating point instructions and registers, so it is our decision.
> No, not registers AFAIK.


sorry, exact, it must be used in conjunction with -ffloat-store


>
>> Saving the FP registers on exceptions is not very efficient, we loose time on each interrupt, not sure that we win it back by using FPregs to as Regs backup.
> Who on earth cares about performance here?

I only care about simplicity and compiler optimizations are not my friends.


>> Usually a system at low level uses some enter_fpu, leave_fpu routine to enter critical sections using FPU instead of losing time on each interruptions.
>>
>> We can think about this, in between I do as you recomand and save the FPregs too.
>>
>> Best regards,
>>
>> Pierre
>>
>>
>> -- 
>> Pierre Morel
>> IBM Lab Boeblingen
>>
-- 
Pierre Morel
IBM Lab Boeblingen

[PATCH v1 2/4] s390x: Define the PSW bits

[Pierre Morel]

Instead of assigning obfuscated masks to the PSW dedicated to the
exceptions, let's define the masks explicitely, it will clarify the
usage.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/asm/arch_bits.h | 32 ++++++++++++++++++++++++++++++++
 lib/s390x/asm/arch_def.h  |  6 ++----
 s390x/cstart64.S          | 13 +++++++------
 3 files changed, 41 insertions(+), 10 deletions(-)
 create mode 100644 lib/s390x/asm/arch_bits.h

diff --git a/lib/s390x/asm/arch_bits.h b/lib/s390x/asm/arch_bits.h
new file mode 100644
index 0000000..0521125
--- /dev/null
+++ b/lib/s390x/asm/arch_bits.h
@@ -0,0 +1,32 @@
+
+/*
+ * Copyright (c) 2019 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+#ifndef _ASM_S390X_ARCH_BITS_H_
+#define _ASM_S390X_ARCH_BITS_H_
+
+#define PSW_MASK_PER		0x4000000000000000
+#define PSW_MASK_DAT		0x0400000000000000
+#define PSW_MASK_IO		0x0200000000000000
+#define PSW_MASK_EXT		0x0100000000000000
+#define PSW_MASK_BASE		0x0000000000000000
+#define PSW_MASK_KEY		0x00F0000000000000
+#define PSW_MASK_MCHECK		0x0004000000000000
+#define PSW_MASK_WAIT		0x0002000000000000
+#define PSW_MASK_PSTATE		0x0001000000000000
+#define PSW_MASK_ASC		0x0000C00000000000
+#define PSW_MASK_CC		0x0000300000000000
+#define PSW_MASK_PM		0x00000F0000000000
+#define PSW_MASK_RI		0x0000008000000000
+#define PSW_MASK_EA		0x0000000100000000
+#define PSW_MASK_BA		0x0000000080000000
+
+#define PSW_EXCEPTION_MASK (PSW_MASK_EA|PSW_MASK_BA)
+
+#endif
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 96cca2e..34c1188 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -10,15 +10,13 @@
 #ifndef _ASM_S390X_ARCH_DEF_H_
 #define _ASM_S390X_ARCH_DEF_H_
 
+#include <asm/arch_bits.h>
+
 struct psw {
 	uint64_t	mask;
 	uint64_t	addr;
 };
 
-#define PSW_MASK_EXT			0x0100000000000000UL
-#define PSW_MASK_DAT			0x0400000000000000UL
-#define PSW_MASK_PSTATE			0x0001000000000000UL
-
 #define CR0_EXTM_SCLP			0X0000000000000200UL
 #define CR0_EXTM_EXTC			0X0000000000002000UL
 #define CR0_EXTM_EMGC			0X0000000000004000UL
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index eaff481..7475f32 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -11,6 +11,7 @@
  * under the terms of the GNU Library General Public License version 2.
  */
 #include <asm/asm-offsets.h>
+#include <asm/arch_bits.h>
 #include <asm/sigp.h>
 
 .section .init
@@ -196,17 +197,17 @@ svc_int:
 
 	.align	8
 initial_psw:
-	.quad	0x0000000180000000, clear_bss_start
+	.quad	PSW_EXCEPTION_MASK, clear_bss_start
 pgm_int_psw:
-	.quad	0x0000000180000000, pgm_int
+	.quad	PSW_EXCEPTION_MASK, pgm_int
 ext_int_psw:
-	.quad	0x0000000180000000, ext_int
+	.quad	PSW_EXCEPTION_MASK, ext_int
 mcck_int_psw:
-	.quad	0x0000000180000000, mcck_int
+	.quad	PSW_EXCEPTION_MASK, mcck_int
 io_int_psw:
-	.quad	0x0000000180000000, io_int
+	.quad	PSW_EXCEPTION_MASK, io_int
 svc_int_psw:
-	.quad	0x0000000180000000, svc_int
+	.quad	PSW_EXCEPTION_MASK, svc_int
 initial_cr0:
 	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
 	.quad	0x0000000000040000
-- 
2.7.4

Re: [PATCH v1 2/4] s390x: Define the PSW bits

[Janosch Frank]

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

On 11/13/19 1:23 PM, Pierre Morel wrote:
> Instead of assigning obfuscated masks to the PSW dedicated to the
> exceptions, let's define the masks explicitely, it will clarify the

s/explicitely/explicitly/
Try to break that up into sentences.

> usage.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_bits.h | 32 ++++++++++++++++++++++++++++++++
>  lib/s390x/asm/arch_def.h  |  6 ++----
>  s390x/cstart64.S          | 13 +++++++------
>  3 files changed, 41 insertions(+), 10 deletions(-)
>  create mode 100644 lib/s390x/asm/arch_bits.h
> 
> diff --git a/lib/s390x/asm/arch_bits.h b/lib/s390x/asm/arch_bits.h
> new file mode 100644
> index 0000000..0521125
> --- /dev/null
> +++ b/lib/s390x/asm/arch_bits.h
> @@ -0,0 +1,32 @@
> +
> +/*
> + * Copyright (c) 2019 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Library General Public License version 2.
> + */
> +#ifndef _ASM_S390X_ARCH_BITS_H_
> +#define _ASM_S390X_ARCH_BITS_H_
> +
> +#define PSW_MASK_PER		0x4000000000000000
> +#define PSW_MASK_DAT		0x0400000000000000
> +#define PSW_MASK_IO		0x0200000000000000
> +#define PSW_MASK_EXT		0x0100000000000000
> +#define PSW_MASK_BASE		0x0000000000000000
> +#define PSW_MASK_KEY		0x00F0000000000000
> +#define PSW_MASK_MCHECK		0x0004000000000000
> +#define PSW_MASK_WAIT		0x0002000000000000
> +#define PSW_MASK_PSTATE		0x0001000000000000
> +#define PSW_MASK_ASC		0x0000C00000000000
> +#define PSW_MASK_CC		0x0000300000000000
> +#define PSW_MASK_PM		0x00000F0000000000
> +#define PSW_MASK_RI		0x0000008000000000
> +#define PSW_MASK_EA		0x0000000100000000
> +#define PSW_MASK_BA		0x0000000080000000

a-f should be lower case in hex values.
Also, do we need all of them?
I'd like to keep it as small as possible.

> +
> +#define PSW_EXCEPTION_MASK (PSW_MASK_EA|PSW_MASK_BA)

That's not a bit anymore, shouldn't that be in arch_def.h?
Also please add a comment, that this is 64 bit addressing.

> +
> +#endif
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 96cca2e..34c1188 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -10,15 +10,13 @@
>  #ifndef _ASM_S390X_ARCH_DEF_H_
>  #define _ASM_S390X_ARCH_DEF_H_
>  
> +#include <asm/arch_bits.h>
> +
>  struct psw {
>  	uint64_t	mask;
>  	uint64_t	addr;
>  };
>  
> -#define PSW_MASK_EXT			0x0100000000000000UL
> -#define PSW_MASK_DAT			0x0400000000000000UL
> -#define PSW_MASK_PSTATE			0x0001000000000000UL
> -
>  #define CR0_EXTM_SCLP			0X0000000000000200UL
>  #define CR0_EXTM_EXTC			0X0000000000002000UL
>  #define CR0_EXTM_EMGC			0X0000000000004000UL
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index eaff481..7475f32 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -11,6 +11,7 @@
>   * under the terms of the GNU Library General Public License version 2.
>   */
>  #include <asm/asm-offsets.h>
> +#include <asm/arch_bits.h>
>  #include <asm/sigp.h>
>  
>  .section .init
> @@ -196,17 +197,17 @@ svc_int:
>  
>  	.align	8
>  initial_psw:
> -	.quad	0x0000000180000000, clear_bss_start
> +	.quad	PSW_EXCEPTION_MASK, clear_bss_start
>  pgm_int_psw:
> -	.quad	0x0000000180000000, pgm_int
> +	.quad	PSW_EXCEPTION_MASK, pgm_int
>  ext_int_psw:
> -	.quad	0x0000000180000000, ext_int
> +	.quad	PSW_EXCEPTION_MASK, ext_int
>  mcck_int_psw:
> -	.quad	0x0000000180000000, mcck_int
> +	.quad	PSW_EXCEPTION_MASK, mcck_int
>  io_int_psw:
> -	.quad	0x0000000180000000, io_int
> +	.quad	PSW_EXCEPTION_MASK, io_int
>  svc_int_psw:
> -	.quad	0x0000000180000000, svc_int
> +	.quad	PSW_EXCEPTION_MASK, svc_int
>  initial_cr0:
>  	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
>  	.quad	0x0000000000040000
> 



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

Re: [PATCH v1 2/4] s390x: Define the PSW bits

[Pierre Morel]

On 2019-11-13 17:05, Janosch Frank wrote:
> On 11/13/19 1:23 PM, Pierre Morel wrote:
>> Instead of assigning obfuscated masks to the PSW dedicated to the
>> exceptions, let's define the masks explicitely, it will clarify the
> s/explicitely/explicitly/
> Try to break that up into sentences.
OK thx
>
>> usage.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/asm/arch_bits.h | 32 ++++++++++++++++++++++++++++++++
>>   lib/s390x/asm/arch_def.h  |  6 ++----
>>   s390x/cstart64.S          | 13 +++++++------
>>   3 files changed, 41 insertions(+), 10 deletions(-)
>>   create mode 100644 lib/s390x/asm/arch_bits.h
>>
>> diff --git a/lib/s390x/asm/arch_bits.h b/lib/s390x/asm/arch_bits.h
>> new file mode 100644
>> index 0000000..0521125
>> --- /dev/null
>> +++ b/lib/s390x/asm/arch_bits.h
>> @@ -0,0 +1,32 @@
>> +
>> +/*
>> + * Copyright (c) 2019 IBM Corp
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU Library General Public License version 2.
>> + */
>> +#ifndef _ASM_S390X_ARCH_BITS_H_
>> +#define _ASM_S390X_ARCH_BITS_H_
>> +
>> +#define PSW_MASK_PER		0x4000000000000000
>> +#define PSW_MASK_DAT		0x0400000000000000
>> +#define PSW_MASK_IO		0x0200000000000000
>> +#define PSW_MASK_EXT		0x0100000000000000
>> +#define PSW_MASK_BASE		0x0000000000000000
>> +#define PSW_MASK_KEY		0x00F0000000000000
>> +#define PSW_MASK_MCHECK		0x0004000000000000
>> +#define PSW_MASK_WAIT		0x0002000000000000
>> +#define PSW_MASK_PSTATE		0x0001000000000000
>> +#define PSW_MASK_ASC		0x0000C00000000000
>> +#define PSW_MASK_CC		0x0000300000000000
>> +#define PSW_MASK_PM		0x00000F0000000000
>> +#define PSW_MASK_RI		0x0000008000000000
>> +#define PSW_MASK_EA		0x0000000100000000
>> +#define PSW_MASK_BA		0x0000000080000000
> a-f should be lower case in hex values.
> Also, do we need all of them?
> I'd like to keep it as small as poss
>
>
>>> +
>>> +#endif
>>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>>> index 96cca2e..34c1188 100644
>>> --- a/lib/s390x/asm/arch_def.h
>>> +++ b/lib/s390x/asm/arch_def.h
>>> @@ -10,15 +10,13 @@
>>>   #ifndef _ASM_S390X_ARCH_DEF_H_
>>>   #define _ASM_S390X_ARCH_DEF_H_
>>>   
>>> +#include <asm/arch_bits.h>
>>> +
>>>   struct psw {
>>>   	uint64_t	mask;
>>>   	uint64_t	addr;
>>>   };
>>>   
>>> -#define PSW_MASK_EXT			0x0100000000000000UL
>>> -#define PSW_MASK_DAT			0x0400000000000000UL
>>> -#define PSW_MASK_PSTATE			0x0001000000000000UL
>>> -
>>>   #define CR0_EXTM_SCLP			0X0000000000000200UL
>>>   #define CR0_EXTM_EXTC			0X0000000000002000UL
>>>   #define CR0_EXTM_EMGC			0X0000000000004000UL
>>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>>> index eaff481..7475f32 100644
>>> --- a/s390x/cstart64.S
>>> +++ b/s390x/cstart64.S
>>> @@ -11,6 +11,7 @@
>>>    * under the terms of the GNU Library General Public License version 2.
>>>    */
>>>   #include <asm/asm-offsets.h>
>>> +#include <asm/arch_bits.h>
>>>   #include <asm/sigp.h>
>>>   
>>>   .section .init
>>> @@ -196,17 +197,17 @@ svc_int:
>>>   
>>>   	.align	8
>>>   initial_psw:
>>> -	.quad	0x0000000180000000, clear_bss_start
>>> +	.quad	PSW_EXCEPTION_MASK, clear_bss_start
>>>   pgm_int_psw:
>>> -	.quad	0x0000000180000000, pgm_int
>>> +	.quad	PSW_EXCEPTION_MASK, pgm_int
>>>   ext_int_psw:
>>> -	.quad	0x0000000180000000, ext_int
>>> +	.quad	PSW_EXCEPTION_MASK, ext_int
>>>   mcck_int_psw:
>>> -	.quad	0x0000000180000000, mcck_int
>>> +	.quad	PSW_EXCEPTION_MASK, mcck_int
>>>   io_int_psw:
>>> -	.quad	0x0000000180000000, io_int
>>> +	.quad	PSW_EXCEPTION_MASK, io_int
>>>   svc_int_psw:
>>> -	.quad	0x0000000180000000, svc_int
>>> +	.quad	PSW_EXCEPTION_MASK, svc_int
>>>   initial_cr0:
>>>   	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
>>>   	.quad	0x0000000000040000
>>>
> ible.

OK


>
>> +
>> +#define PSW_EXCEPTION_MASK (PSW_MASK_EA|PSW_MASK_BA)
> That's not a bit anymore, shouldn't that be in arch_def.h?
> Also please add a comment, that this is 64 bit addressing.


Don't we use the 64bit architecture only?

Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 2/4] s390x: Define the PSW bits

[Janosch Frank]

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

On 11/14/19 9:40 AM, Pierre Morel wrote:
> 
> On 2019-11-13 17:05, Janosch Frank wrote:
>> On 11/13/19 1:23 PM, Pierre Morel wrote:
>>> Instead of assigning obfuscated masks to the PSW dedicated to the
>>> exceptions, let's define the masks explicitely, it will clarify the
>> s/explicitely/explicitly/
>> Try to break that up into sentences.
> OK thx
>>
>>> usage.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   lib/s390x/asm/arch_bits.h | 32 ++++++++++++++++++++++++++++++++
>>>   lib/s390x/asm/arch_def.h  |  6 ++----
>>>   s390x/cstart64.S          | 13 +++++++------
>>>   3 files changed, 41 insertions(+), 10 deletions(-)
>>>   create mode 100644 lib/s390x/asm/arch_bits.h
>>>
>>> diff --git a/lib/s390x/asm/arch_bits.h b/lib/s390x/asm/arch_bits.h
>>> new file mode 100644
>>> index 0000000..0521125
>>> --- /dev/null
>>> +++ b/lib/s390x/asm/arch_bits.h
>>> @@ -0,0 +1,32 @@
>>> +
>>> +/*
>>> + * Copyright (c) 2019 IBM Corp
>>> + *
>>> + * Authors:
>>> + *  Pierre Morel <pmorel@linux.ibm.com>
>>> + *
>>> + * This code is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU Library General Public License version 2.
>>> + */
>>> +#ifndef _ASM_S390X_ARCH_BITS_H_
>>> +#define _ASM_S390X_ARCH_BITS_H_
>>> +
>>> +#define PSW_MASK_PER		0x4000000000000000
>>> +#define PSW_MASK_DAT		0x0400000000000000
>>> +#define PSW_MASK_IO		0x0200000000000000
>>> +#define PSW_MASK_EXT		0x0100000000000000
>>> +#define PSW_MASK_BASE		0x0000000000000000
>>> +#define PSW_MASK_KEY		0x00F0000000000000
>>> +#define PSW_MASK_MCHECK		0x0004000000000000
>>> +#define PSW_MASK_WAIT		0x0002000000000000
>>> +#define PSW_MASK_PSTATE		0x0001000000000000
>>> +#define PSW_MASK_ASC		0x0000C00000000000
>>> +#define PSW_MASK_CC		0x0000300000000000
>>> +#define PSW_MASK_PM		0x00000F0000000000
>>> +#define PSW_MASK_RI		0x0000008000000000
>>> +#define PSW_MASK_EA		0x0000000100000000
>>> +#define PSW_MASK_BA		0x0000000080000000
>> a-f should be lower case in hex values.
>> Also, do we need all of them?
>> I'd like to keep it as small as poss
>>
>>
>>>> +
>>>> +#endif
>>>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>>>> index 96cca2e..34c1188 100644
>>>> --- a/lib/s390x/asm/arch_def.h
>>>> +++ b/lib/s390x/asm/arch_def.h
>>>> @@ -10,15 +10,13 @@
>>>>   #ifndef _ASM_S390X_ARCH_DEF_H_
>>>>   #define _ASM_S390X_ARCH_DEF_H_
>>>>   
>>>> +#include <asm/arch_bits.h>
>>>> +
>>>>   struct psw {
>>>>   	uint64_t	mask;
>>>>   	uint64_t	addr;
>>>>   };
>>>>   
>>>> -#define PSW_MASK_EXT			0x0100000000000000UL
>>>> -#define PSW_MASK_DAT			0x0400000000000000UL
>>>> -#define PSW_MASK_PSTATE			0x0001000000000000UL
>>>> -
>>>>   #define CR0_EXTM_SCLP			0X0000000000000200UL
>>>>   #define CR0_EXTM_EXTC			0X0000000000002000UL
>>>>   #define CR0_EXTM_EMGC			0X0000000000004000UL
>>>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>>>> index eaff481..7475f32 100644
>>>> --- a/s390x/cstart64.S
>>>> +++ b/s390x/cstart64.S
>>>> @@ -11,6 +11,7 @@
>>>>    * under the terms of the GNU Library General Public License version 2.
>>>>    */
>>>>   #include <asm/asm-offsets.h>
>>>> +#include <asm/arch_bits.h>
>>>>   #include <asm/sigp.h>
>>>>   
>>>>   .section .init
>>>> @@ -196,17 +197,17 @@ svc_int:
>>>>   
>>>>   	.align	8
>>>>   initial_psw:
>>>> -	.quad	0x0000000180000000, clear_bss_start
>>>> +	.quad	PSW_EXCEPTION_MASK, clear_bss_start
>>>>   pgm_int_psw:
>>>> -	.quad	0x0000000180000000, pgm_int
>>>> +	.quad	PSW_EXCEPTION_MASK, pgm_int
>>>>   ext_int_psw:
>>>> -	.quad	0x0000000180000000, ext_int
>>>> +	.quad	PSW_EXCEPTION_MASK, ext_int
>>>>   mcck_int_psw:
>>>> -	.quad	0x0000000180000000, mcck_int
>>>> +	.quad	PSW_EXCEPTION_MASK, mcck_int
>>>>   io_int_psw:
>>>> -	.quad	0x0000000180000000, io_int
>>>> +	.quad	PSW_EXCEPTION_MASK, io_int
>>>>   svc_int_psw:
>>>> -	.quad	0x0000000180000000, svc_int
>>>> +	.quad	PSW_EXCEPTION_MASK, svc_int
>>>>   initial_cr0:
>>>>   	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
>>>>   	.quad	0x0000000000040000
>>>>
>> ible.
> 
> OK
> 
> 
>>
>>> +
>>> +#define PSW_EXCEPTION_MASK (PSW_MASK_EA|PSW_MASK_BA)
>> That's not a bit anymore, shouldn't that be in arch_def.h?
>> Also please add a comment, that this is 64 bit addressing.
> 
> 
> Don't we use the 64bit architecture only?

architecture != addressing
We can do 24 bit addressing on zArch...
We mostly use ESAME (zArch), but old machines start up in the old mode
and then we transition to zArch via a SIGP.

> 
> Regards,
> 
> Pierre
> 
> 



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

Re: [PATCH v1 2/4] s390x: Define the PSW bits

[Pierre Morel]

On 2019-11-14 09:53, Janosch Frank wrote:
> On 11/14/19 9:40 AM, Pierre Morel wrote:
>> On 2019-11-13 17:05, Janosch Frank wrote:
>>> On 11/13/19 1:23 PM, Pierre Morel wrote:
>>>> Instead of assigning obfuscated masks to the PSW dedicated to the
>>>> exceptions, let's define the masks explicitely, it will clarify the
>>> s/explicitely/explicitly/
>>> Try to break that up into sentences.
>> OK thx
...snip...
>>>> +
>>>> +#define PSW_EXCEPTION_MASK (PSW_MASK_EA|PSW_MASK_BA)
>>> That's not a bit anymore, shouldn't that be in arch_def.h?
>>> Also please add a comment, that this is 64 bit addressing.
>>
>> Don't we use the 64bit architecture only?
> architecture != addressing
> We can do 24 bit addressing on zArch...
> We mostly use ESAME (zArch), but old machines start up in the old mode
> and then we transition to zArch via a SIGP.


Yes, this is done during the first instructions of cstart.S:

- Setting the architecture to 64bit / ESAME using SIGP

- Set addressing mode to 64bit

After that AFAIK we never change the addressing mode.

The definitions in the file are intended for PSW flags which AFAIK are 
always 64bits.

I created arch_bits.h to avoid using arch_def.h which contains C 
structures and functions, so preventing to include it in assembler files.


Regards,

Pierre


>
>> Regards,
>>
>> Pierre
>>
>>
>
-- 
Pierre Morel
IBM Lab Boeblingen

[PATCH v1 3/4] s390x:irq: make IRQ handler weak

[Pierre Morel]

Having a weak function allows the tests programm to declare its own IRQ
handler.
This is helpfull when developping I/O tests.
---
 lib/s390x/interrupt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 7aecfc5..0049194 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -140,7 +140,7 @@ void handle_mcck_int(sregs_t *regs)
 		     lc->mcck_old_psw.addr);
 }
 
-void handle_io_int(sregs_t *regs)
+__attribute__((weak)) void handle_io_int(sregs_t *regs)
 {
 	report_abort("Unexpected io interrupt: at %#lx",
 		     lc->io_old_psw.addr);
-- 
2.7.4

Re: [PATCH v1 3/4] s390x:irq: make IRQ handler weak

[Thomas Huth]

On 13/11/2019 13.23, Pierre Morel wrote:
> Having a weak function allows the tests programm to declare its own IRQ
> handler.
> This is helpfull when developping I/O tests.
> ---
>  lib/s390x/interrupt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 7aecfc5..0049194 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -140,7 +140,7 @@ void handle_mcck_int(sregs_t *regs)
>  		     lc->mcck_old_psw.addr);
>  }
>  
> -void handle_io_int(sregs_t *regs)
> +__attribute__((weak)) void handle_io_int(sregs_t *regs)
>  {
>  	report_abort("Unexpected io interrupt: at %#lx",
>  		     lc->io_old_psw.addr);
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v1 3/4] s390x:irq: make IRQ handler weak

[Pierre Morel]

On 2019-11-15 08:12, Thomas Huth wrote:
> On 13/11/2019 13.23, Pierre Morel wrote:
>> Having a weak function allows the tests programm to declare its own IRQ
>> handler.
>> This is helpfull when developping I/O tests.
>> ---
>>   lib/s390x/interrupt.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
>> index 7aecfc5..0049194 100644
>> --- a/lib/s390x/interrupt.c
>> +++ b/lib/s390x/interrupt.c
>> @@ -140,7 +140,7 @@ void handle_mcck_int(sregs_t *regs)
>>   		     lc->mcck_old_psw.addr);
>>   }
>>   
>> -void handle_io_int(sregs_t *regs)
>> +__attribute__((weak)) void handle_io_int(sregs_t *regs)
>>   {
>>   	report_abort("Unexpected io interrupt: at %#lx",
>>   		     lc->io_old_psw.addr);
>>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
Thanks for the review,

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

[PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Pierre Morel]

This simple test test the I/O reading by the SUB Channel by:
- initializing the Channel SubSystem with predefined CSSID:
  0xfe000000 CSSID for a Virtual CCW
  0x00090000 SSID for CCW-PONG
- initializing the ORB pointing to a single READ CCW
- starts the STSH command with the ORB
- Expect an interrupt
- writes the read data to output

The test implements lots of traces when DEBUG is on and
tests if memory above the stack is corrupted.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h      | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c | 141 +++++++++++++++++++++++++++++
 s390x/Makefile       |   2 +
 s390x/css.c          | 222 ++++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg  |   4 +
 5 files changed, 613 insertions(+)
 create mode 100644 lib/s390x/css.h
 create mode 100644 lib/s390x/css_dump.c
 create mode 100644 s390x/css.c

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
new file mode 100644
index 0000000..a7c42fd
--- /dev/null
+++ b/lib/s390x/css.h
@@ -0,0 +1,244 @@
+/*
+ * CSS definitions
+ *
+ * Copyright IBM, Corp. 2019
+ * Author: Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef CSS_H
+#define CSS_H
+
+#define CCW_F_CD	0x80
+#define CCW_F_CC	0x40
+#define CCW_F_SLI	0x20
+#define CCW_F_SKP	0x10
+#define CCW_F_PCI	0x08
+#define CCW_F_IDA	0x04
+#define CCW_F_S		0x02
+#define CCW_F_MIDA	0x01
+
+#define CCW_C_NOP	0x03
+#define CCW_C_TIC	0x08
+
+struct ccw {
+	unsigned char code;
+	unsigned char flags;
+	unsigned short count;
+	unsigned int data;
+} __attribute__ ((aligned(4)));
+
+#define ORB_M_KEY	0xf0000000
+#define ORB_F_SUSPEND	0x08000000
+#define ORB_F_STREAMING	0x04000000
+#define ORB_F_MODIFCTRL	0x02000000
+#define ORB_F_SYNC	0x01000000
+#define ORB_F_FORMAT	0x00800000
+#define ORB_F_PREFETCH	0x00400000
+#define ORB_F_INIT_IRQ	0x00200000
+#define ORB_F_ADDRLIMIT	0x00100000
+#define ORB_F_SUSP_IRQ	0x00080000
+#define ORB_F_TRANSPORT	0x00040000
+#define ORB_F_IDAW2	0x00020000
+#define ORB_F_IDAW_2K	0x00010000
+#define ORB_M_LPM	0x0000ff00
+#define ORB_F_LPM_DFLT	0x00008000
+#define ORB_F_ILSM	0x00000080
+#define ORB_F_CCW_IND	0x00000040
+#define ORB_F_ORB_EXT	0x00000001
+struct orb {
+	unsigned int	intparm;
+	unsigned int	ctrl;
+	unsigned int	cpa;
+	unsigned int	prio;
+	unsigned int	reserved[4];
+} __attribute__ ((aligned(4)));;
+
+struct scsw {
+	uint32_t ctrl;
+	uint32_t addr;
+	uint8_t  devs;
+	uint8_t  schs;
+	uint16_t count;
+};
+
+struct pmcw {
+	uint32_t intparm;
+	uint16_t flags;
+	uint16_t devnum;
+	uint8_t  lpm;
+	uint8_t  pnom;
+	uint8_t  lpum;
+	uint8_t  pim;
+	uint16_t mbi;
+	uint8_t  pom;
+	uint8_t  pam;
+	uint8_t  chpid[8];
+	uint32_t flag2;
+};
+struct schib {
+	struct pmcw pmcw;
+	struct scsw scsw;
+	uint32_t  md0;
+	uint32_t  md1;
+	uint32_t  md2;
+} __attribute__ ((aligned(4)));
+
+struct irb {
+	struct scsw scsw;
+	uint32_t esw[5];
+	uint32_t ecw[8];
+	uint32_t emw[8];
+} __attribute__ ((aligned(4)));;
+
+/* CSS low level access functions */
+
+static inline int ssch(unsigned long schid, struct orb *addr)
+{
+	register long long reg1 asm("1") = schid;
+	int ccode = -1;
+
+	asm volatile(
+		"	   ssch	0(%2)\n"
+		"0:	 ipm	 %0\n"
+		"	   srl	 %0,28\n"
+		"1:\n"
+		: "+d" (ccode)
+		: "d" (reg1), "a" (addr), "m" (*addr)
+		: "cc", "memory");
+	return ccode;
+}
+
+static inline int stsch(unsigned long schid, struct schib *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int ccode;
+
+	asm volatile(
+		"	   stsch	0(%3)\n"
+		"	   ipm	 %0\n"
+		"	   srl	 %0,28"
+		: "=d" (ccode), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");
+	return ccode;
+}
+
+static inline int msch(unsigned long schid, struct schib *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int ccode;
+
+	asm volatile(
+		"	   msch	0(%3)\n"
+		"	   ipm	 %0\n"
+		"	   srl	 %0,28"
+		: "=d" (ccode), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");
+	return ccode;
+}
+
+static inline int tsch(unsigned long schid, struct irb *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int ccode;
+
+	asm volatile(
+		"	   tsch	0(%3)\n"
+		"	   ipm	 %0\n"
+		"	   srl	 %0,28"
+		: "=d" (ccode), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");
+	return ccode;
+}
+
+static inline int hsch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int ccode;
+
+	asm volatile(
+		"	hsch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (ccode)
+		: "d" (reg1)
+		: "cc");
+	return ccode;
+}
+
+static inline int xsch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int ccode;
+
+	asm volatile(
+		"	xsch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (ccode)
+		: "d" (reg1)
+		: "cc");
+	return ccode;
+}
+
+static inline int csch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int ccode;
+
+	asm volatile(
+		"	csch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (ccode)
+		: "d" (reg1)
+		: "cc");
+	return ccode;
+}
+
+static inline int rsch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int ccode;
+
+	asm volatile(
+		"	rsch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (ccode)
+		: "d" (reg1)
+		: "cc");
+	return ccode;
+}
+
+static inline int rchp(unsigned long chpid)
+{
+	register unsigned long reg1 asm("1") = chpid;
+	int ccode;
+
+	asm volatile(
+		"	rchp\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (ccode)
+		: "d" (reg1)
+		: "cc");
+	return ccode;
+}
+
+void dump_scsw(struct scsw *);
+void dump_irb(struct irb *irbp);
+void dump_pmcw_flags(uint16_t f);
+void dump_pmcw(struct pmcw *p);
+void dump_schib(struct schib *sch);
+struct ccw *dump_ccw(struct ccw *cp);
+void dump_orb(struct orb *op);
+
+extern unsigned long stacktop;
+#endif
diff --git a/lib/s390x/css_dump.c b/lib/s390x/css_dump.c
new file mode 100644
index 0000000..4f6b628
--- /dev/null
+++ b/lib/s390x/css_dump.c
@@ -0,0 +1,141 @@
+/*
+ * Channel Sub-System structures dumping
+ *
+ * Copyright (c) 2019 IBM Corp.
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ *
+ * Description:
+ * Provides the dumping functions for various structures used by subchannels:
+ * - ORB  : Operation request block, describe the I/O operation and point to
+ *          a CCW chain
+ * - CCW  : Channel Command Word, describe the data and flow control
+ * - IRB  : Interuption response Block, describe the result of an operation
+ *          hold a SCSW and several channel type dependent fields.
+ * - SCHIB: SubChannel Information Block composed of:
+ *   - SCSW: SubChannel Status Word, status of the channel as a result of an
+ *           operation when in IRB.
+ *   - PMCW: Path Management Control Word
+ * You need the QEMU ccw-pong device in QEMU to answer the I/O transfers.
+ */
+
+#include <unistd.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <string.h>
+
+#include <css.h>
+
+static const char *scsw_str = "kkkkslccfpixuzen";
+static const char *scsw_str2 = "1SHCrshcsdsAIPSs";
+
+void dump_scsw(struct scsw *s)
+{
+	int i;
+	uint32_t scsw = s->ctrl;
+	char line[64] = {};
+
+	for (i = 0; i < 16; i++) {
+		if ((scsw << i) & 0x80000000)
+			line[i] = scsw_str[i];
+		else
+			line[i] = '_';
+	}
+	line[i] = ' ';
+	for (; i < 32; i++) {
+		if ((scsw << i) & 0x80000000)
+			line[i + 1] = scsw_str2[i - 16];
+		else
+			line[i + 1] = '_';
+	}
+	printf("scsw->flags: %s\n", line);
+	printf("scsw->addr : %08x\n", s->addr);
+	printf("scsw->devs : %02x\n", s->devs);
+	printf("scsw->schs : %02x\n", s->schs);
+	printf("scsw->count: %04x\n", s->count);
+
+}
+
+void dump_irb(struct irb *irbp)
+{
+	int i;
+	uint32_t *p = (uint32_t *)irbp;
+
+	dump_scsw(&irbp->scsw);
+	for (i = 0; i < sizeof(*irbp)/sizeof(*p); i++, p++)
+		printf("irb[%02x] : %08x\n", i, *p);
+}
+
+static const char *pmcw_str = "11iii111ellmmdtv";
+void dump_pmcw_flags(uint16_t f)
+{
+	int i;
+	char line[32] = {};
+
+	for (i = 0; i < 16; i++) {
+		if ((f << i) & 0x8000)
+			line[i] = pmcw_str[i];
+		else
+			line[i] = '_';
+	}
+	printf("pmcw->pmcw flgs: %s\n", line);
+}
+
+void dump_pmcw(struct pmcw *p)
+{
+	int i;
+
+	printf("pmcw->intparm  : %08x\n", p->intparm);
+	printf("pmcw->flags    : %04x\n", p->flags);
+	dump_pmcw_flags(p->flags);
+	printf("pmcw->devnum   : %04x\n", p->devnum);
+	printf("pmcw->lpm      : %02x\n", p->lpm);
+	printf("pmcw->pnom     : %02x\n", p->pnom);
+	printf("pmcw->lpum     : %02x\n", p->lpum);
+	printf("pmcw->pim      : %02x\n", p->pim);
+	printf("pmcw->mbi      : %04x\n", p->mbi);
+	printf("pmcw->pom      : %02x\n", p->pom);
+	printf("pmcw->pam      : %02x\n", p->pam);
+	printf("pmcw->mbi      : %04x\n", p->mbi);
+	for (i = 0; i < 8; i++)
+		printf("pmcw->chpid[%d]: %02x\n", i, p->chpid[i]);
+	printf("pmcw->flags2  : %08x\n", p->flag2);
+}
+
+void dump_schib(struct schib *sch)
+{
+	struct pmcw *p = &sch->pmcw;
+	struct scsw *s = &sch->scsw;
+
+	printf("--SCHIB--\n");
+	dump_pmcw(p);
+	dump_scsw(s);
+}
+
+struct ccw *dump_ccw(struct ccw *cp)
+{
+	printf("CCW: code: %02x flags: %02x count: %04x data: %08x\n", cp->code,
+	    cp->flags, cp->count, cp->data);
+
+	if (cp->code == CCW_C_TIC)
+		return (struct ccw *)(long)cp->data;
+
+	return (cp->flags & CCW_F_CC) ? cp + 1 : NULL;
+}
+
+void dump_orb(struct orb *op)
+{
+	struct ccw *cp;
+
+	printf("ORB: intparm : %08x\n", op->intparm);
+	printf("ORB: ctrl    : %08x\n", op->ctrl);
+	printf("ORB: prio    : %08x\n", op->prio);
+	cp = (struct ccw *)(long) (op->cpa);
+	while (cp)
+		cp = dump_ccw(cp);
+}
+
diff --git a/s390x/Makefile b/s390x/Makefile
index 3744372..9ebbb84 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -16,6 +16,7 @@ tests += $(TEST_DIR)/diag288.elf
 tests += $(TEST_DIR)/stsi.elf
 tests += $(TEST_DIR)/skrf.elf
 tests += $(TEST_DIR)/smp.elf
+tests += $(TEST_DIR)/css.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
 all: directories test_cases test_cases_binary
@@ -50,6 +51,7 @@ cflatobjs += lib/s390x/sclp-console.o
 cflatobjs += lib/s390x/interrupt.o
 cflatobjs += lib/s390x/mmu.o
 cflatobjs += lib/s390x/smp.o
+cflatobjs += lib/s390x/css_dump.o
 
 OBJDIRS += lib/s390x
 
diff --git a/s390x/css.c b/s390x/css.c
new file mode 100644
index 0000000..6cdaf61
--- /dev/null
+++ b/s390x/css.c
@@ -0,0 +1,222 @@
+/*
+ * Channel Sub-System tests
+ *
+ * Copyright (c) 2019 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+
+#include <libcflat.h>
+#include <alloc_phys.h>
+#include <asm/page.h>
+#include <string.h>
+#include <asm/interrupt.h>
+#include <asm/arch_def.h>
+
+#include <css.h>
+
+#define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
+
+#define DEBUG 1
+#ifdef DEBUG
+#define DBG(format, arg...) \
+	printf("KT_%s:%d " format "\n", __func__, __LINE__, ##arg)
+#else
+#define DBG(format, arg...) do {} while (0)
+#endif
+
+#define CSSID_PONG	(0xfe000000 | 0x00090000)
+
+struct lowcore *lowcore = (void *)0x0;
+
+#define NB_CCW  100
+static struct ccw ccw[NB_CCW];
+
+#define NB_ORB  100
+static struct orb orb[NB_ORB];
+
+static struct irb irb;
+static struct schib schib;
+
+static char buffer[4096];
+
+static void delay(int d)
+{
+	int i, j;
+
+	while (d--)
+		for (i = 1000000; i; i--)
+			for (j = 1000000; j; j--)
+				;
+}
+
+static void set_io_irq_subclass_mask(uint64_t const new_mask)
+{
+	asm volatile (
+		"lctlg %%c6, %%c6, %[source]\n"
+		: /* No outputs */
+		: [source] "R" (new_mask));
+}
+
+static void set_system_mask(uint8_t new_mask)
+{
+	asm volatile (
+		"ssm %[source]\n"
+		: /* No outputs */
+		: [source] "R" (new_mask));
+}
+
+static void enable_io_irq(void)
+{
+	set_io_irq_subclass_mask(0x00000000ff000000);
+	set_system_mask(PSW_PRG_MASK >> 56);
+}
+
+void handle_io_int(sregs_t *regs)
+{
+	int ret = 0;
+
+	DBG("IO IRQ: subsys_id_word=%08x", lowcore->subsys_id_word);
+	DBG("......: io_int_parm   =%08x", lowcore->io_int_param);
+	DBG("......: io_int_word   =%08x", lowcore->io_int_word);
+	ret = tsch(lowcore->subsys_id_word, &irb);
+	dump_irb(&irb);
+	if (ret)
+		DBG("......: tsch retval %d", ret);
+	DBG("IO IRQ: END");
+}
+
+static void set_schib(struct schib *sch)
+{
+	struct pmcw *p = &sch->pmcw;
+
+	p->intparm = 0xdeadbeef;
+	p->devnum = 0xc0ca;
+	p->lpm = 0x80;
+	p->flags = 0x3081;
+	p->chpid[7] = 0x22;
+	p->pim = 0x0f;
+	p->pam = 0x0f;
+	p->pom = 0x0f;
+	p->lpm = 0x0f;
+	p->lpum = 0xaa;
+	p->pnom = 0xf0;
+	p->mbi = 0xaa;
+	p->mbi = 0xaaaa;
+}
+
+static void css_enable(void)
+{
+	int ret;
+
+	ret = stsch(CSSID_PONG, &schib);
+	if (ret)
+		DBG("stsch: %x\n", ret);
+	dump_schib(&schib);
+	set_schib(&schib);
+	dump_schib(&schib);
+	ret = msch(CSSID_PONG, &schib);
+	if (ret)
+		DBG("msch : %x\n", ret);
+}
+
+/* These two definitions are part of the QEMU PONG interface */
+#define PONG_WRITE 0x21
+#define PONG_READ  0x22
+
+static int css_run(int fake)
+{
+	struct orb *p = orb;
+	int cc;
+
+	if (fake)
+		return 0;
+	css_enable();
+
+	enable_io_irq();
+
+	ccw[0].code = PONG_READ;
+	ccw[0].flags = CCW_F_PCI;
+	ccw[0].count = 80;
+	ccw[0].data = (unsigned int)(unsigned long) &buffer;
+
+	p->intparm = 0xcafec0ca;
+	p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
+	p->cpa = (unsigned int) (unsigned long)&ccw[0];
+
+	printf("ORB AT %p\n", orb);
+	dump_orb(p);
+	cc = ssch(CSSID_PONG, p);
+	if (cc) {
+		DBG("cc: %x\n", cc);
+		return cc;
+	}
+
+	delay(1);
+
+	stsch(CSSID_PONG, &schib);
+	dump_schib(&schib);
+	DBG("got: %s\n", buffer);
+
+	return 0;
+}
+
+#define MAX_ERRORS 10
+static int checkmem(phys_addr_t start, phys_addr_t end)
+{
+	phys_addr_t curr;
+	int err = 0;
+
+	for (curr = start; curr != end; curr += PAGE_SIZE)
+		if (memcmp((void *)start, (void *)curr, PAGE_SIZE)) {
+			report("memcmp failed %lx", true, curr);
+			if (err++ > MAX_ERRORS)
+				break;
+		}
+	return err;
+}
+
+extern unsigned long bss_end;
+
+int main(int argc, char *argv[])
+{
+	phys_addr_t base, top;
+	int check_mem = 0;
+	int err = 0;
+
+	if (argc == 2 && !strcmp(argv[1], "-i"))
+		check_mem = 1;
+
+	report_prefix_push("css");
+	phys_alloc_get_unused(&base, &top);
+
+	top = 0x08000000; /* 128MB Need to be updated */
+	base = (phys_addr_t)&stacktop;
+
+	if (check_mem)
+		memset((void *)base, 0x00, top - base);
+
+	if (check_mem)
+		err = checkmem(base, top);
+	if (err)
+		goto out;
+
+	err = css_run(0);
+	if (err)
+		goto out;
+
+	if (check_mem)
+		err = checkmem(base, top);
+
+out:
+	if (err)
+		report("Tested", 0);
+	else
+		report("Tested", 1);
+
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index f1b07cd..1755d9e 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -75,3 +75,7 @@ file = stsi.elf
 [smp]
 file = smp.elf
 extra_params =-smp 2
+
+[css]
+file = css.elf
+extra_params =-device ccw-pong
-- 
2.7.4

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Cornelia Huck]

On Wed, 13 Nov 2019 13:23:19 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> This simple test test the I/O reading by the SUB Channel by:
> - initializing the Channel SubSystem with predefined CSSID:
>   0xfe000000 CSSID for a Virtual CCW

0 should be fine with recent QEMU versions as well, I guess?

>   0x00090000 SSID for CCW-PONG

subchannel id, or subchannel set id?

> - initializing the ORB pointing to a single READ CCW

Out of curiosity: Would using a NOP also be an option?

> - starts the STSH command with the ORB

s/STSH/SSCH/ ?

> - Expect an interrupt
> - writes the read data to output
> 
> The test implements lots of traces when DEBUG is on and
> tests if memory above the stack is corrupted.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h      | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/css_dump.c | 141 +++++++++++++++++++++++++++++
>  s390x/Makefile       |   2 +
>  s390x/css.c          | 222 ++++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg  |   4 +
>  5 files changed, 613 insertions(+)
>  create mode 100644 lib/s390x/css.h
>  create mode 100644 lib/s390x/css_dump.c
>  create mode 100644 s390x/css.c
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> new file mode 100644
> index 0000000..a7c42fd
> --- /dev/null
> +++ b/lib/s390x/css.h

(...)

> +static inline int rsch(unsigned long schid)

I don't think anyone has tried rsch with QEMU before; sounds like a
good idea to test this :)

> +{
> +	register unsigned long reg1 asm("1") = schid;
> +	int ccode;
> +
> +	asm volatile(
> +		"	rsch\n"
> +		"	ipm	%0\n"
> +		"	srl	%0,28"
> +		: "=d" (ccode)
> +		: "d" (reg1)
> +		: "cc");
> +	return ccode;
> +}
> +
> +static inline int rchp(unsigned long chpid)

Anything useful we can test here?

> +{
> +	register unsigned long reg1 asm("1") = chpid;
> +	int ccode;
> +
> +	asm volatile(
> +		"	rchp\n"
> +		"	ipm	%0\n"
> +		"	srl	%0,28"
> +		: "=d" (ccode)
> +		: "d" (reg1)
> +		: "cc");
> +	return ccode;
> +}

(...)

> diff --git a/s390x/css.c b/s390x/css.c
> new file mode 100644
> index 0000000..6cdaf61
> --- /dev/null
> +++ b/s390x/css.c

(...)

> +static void set_io_irq_subclass_mask(uint64_t const new_mask)
> +{
> +	asm volatile (
> +		"lctlg %%c6, %%c6, %[source]\n"
> +		: /* No outputs */
> +		: [source] "R" (new_mask));
> +}
> +
> +static void set_system_mask(uint8_t new_mask)
> +{
> +	asm volatile (
> +		"ssm %[source]\n"
> +		: /* No outputs */
> +		: [source] "R" (new_mask));
> +}
> +
> +static void enable_io_irq(void)
> +{
> +	set_io_irq_subclass_mask(0x00000000ff000000);

So, you always enable all iscs? Maybe add a comment?

> +	set_system_mask(PSW_PRG_MASK >> 56);
> +}
> +
> +void handle_io_int(sregs_t *regs)
> +{
> +	int ret = 0;
> +
> +	DBG("IO IRQ: subsys_id_word=%08x", lowcore->subsys_id_word);
> +	DBG("......: io_int_parm   =%08x", lowcore->io_int_param);
> +	DBG("......: io_int_word   =%08x", lowcore->io_int_word);
> +	ret = tsch(lowcore->subsys_id_word, &irb);
> +	dump_irb(&irb);
> +	if (ret)
> +		DBG("......: tsch retval %d", ret);
> +	DBG("IO IRQ: END");
> +}
> +
> +static void set_schib(struct schib *sch)
> +{
> +	struct pmcw *p = &sch->pmcw;
> +
> +	p->intparm = 0xdeadbeef;
> +	p->devnum = 0xc0ca;
> +	p->lpm = 0x80;
> +	p->flags = 0x3081;

Use #defines instead of magic numbers?

> +	p->chpid[7] = 0x22;
> +	p->pim = 0x0f;
> +	p->pam = 0x0f;
> +	p->pom = 0x0f;
> +	p->lpm = 0x0f;
> +	p->lpum = 0xaa;
> +	p->pnom = 0xf0;
> +	p->mbi = 0xaa;
> +	p->mbi = 0xaaaa;

Many of these fields are not supposed to be modifiable by the program
-- do you want to check what you get back after msch?

Also, you set mbi twice ;) (And for it to actually have any effect,
you'd have to execute SET CHANNEL MONITOR, no?)


> +}
> +
> +static void css_enable(void)
> +{
> +	int ret;
> +
> +	ret = stsch(CSSID_PONG, &schib);
> +	if (ret)
> +		DBG("stsch: %x\n", ret);
> +	dump_schib(&schib);
> +	set_schib(&schib);
> +	dump_schib(&schib);
> +	ret = msch(CSSID_PONG, &schib);
> +	if (ret)
> +		DBG("msch : %x\n", ret);
> +}
> +
> +/* These two definitions are part of the QEMU PONG interface */
> +#define PONG_WRITE 0x21
> +#define PONG_READ  0x22

Ah, so it's not a plain read/write, but a specialized one? Mention that
in the patch description?

> +
> +static int css_run(int fake)
> +{
> +	struct orb *p = orb;

I'd maybe call that variable 'orb' instead; at a glance, I was confused
what you did with the pmcw below, until I realized that it's the orb :)

> +	int cc;
> +
> +	if (fake)
> +		return 0;
> +	css_enable();
> +
> +	enable_io_irq();
> +
> +	ccw[0].code = PONG_READ;
> +	ccw[0].flags = CCW_F_PCI;
> +	ccw[0].count = 80;
> +	ccw[0].data = (unsigned int)(unsigned long) &buffer;
> +
> +	p->intparm = 0xcafec0ca;
> +	p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
> +	p->cpa = (unsigned int) (unsigned long)&ccw[0];
> +
> +	printf("ORB AT %p\n", orb);
> +	dump_orb(p);
> +	cc = ssch(CSSID_PONG, p);
> +	if (cc) {
> +		DBG("cc: %x\n", cc);
> +		return cc;
> +	}
> +
> +	delay(1);
> +
> +	stsch(CSSID_PONG, &schib);
> +	dump_schib(&schib);
> +	DBG("got: %s\n", buffer);
> +
> +	return 0;
> +}
(...)

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Pierre Morel]

On 2019-11-13 14:05, Cornelia Huck wrote:
> On Wed, 13 Nov 2019 13:23:19 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> This simple test test the I/O reading by the SUB Channel by:
>> - initializing the Channel SubSystem with predefined CSSID:
>>    0xfe000000 CSSID for a Virtual CCW
> 0 should be fine with recent QEMU versions as well, I guess?
Right


>
>>    0x00090000 SSID for CCW-PONG
> subchannel id, or subchannel set id?

hum, only part of, I had SSID (Subchannel Set ID) 4 (a.k.a m bit) + Bit 
47  =1

But as you said, I can use CSSID 0 and m = 0 which makes:

Subsystem Identification word = 0x00010000


>
>> - initializing the ORB pointing to a single READ CCW
> Out of curiosity: Would using a NOP also be an option?

It will work but will not be handled by this device, css.c intercept it 
in sch_handle_start_func_virtual.

AFAIU If we want to have a really good testing environment, for driver 
testing for exemple, then it would be interesting to add a new 
do_subchannel_work callback like do_subchannel_work_emulation along with 
the _virtual and _paththrough variantes.

Having a dedicated callback for emulation, we can answer to any CSS 
instructions and SSCH commands, including NOP and TIC.

My goal here was to quickly develop a device answering to some basic 
READ/WRITE command to start memory transfers from inside a guest without 
Linux and without implementing VIRTIO in KVM tests.



>
>> - starts the STSH command with the ORB
> s/STSH/SSCH/ ?

:) yes, thanks


>
>> - Expect an interrupt
>> - writes the read data to output
>>
>> The test implements lots of traces when DEBUG is on and
>> tests if memory above the stack is corrupted.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h      | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/css_dump.c | 141 +++++++++++++++++++++++++++++
>>   s390x/Makefile       |   2 +
>>   s390x/css.c          | 222 ++++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg  |   4 +
>>   5 files changed, 613 insertions(+)
>>   create mode 100644 lib/s390x/css.h
>>   create mode 100644 lib/s390x/css_dump.c
>>   create mode 100644 s390x/css.c
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> new file mode 100644
>> index 0000000..a7c42fd
>> --- /dev/null
>> +++ b/lib/s390x/css.h
> (...)
>
>> +static inline int rsch(unsigned long schid)
> I don't think anyone has tried rsch with QEMU before; sounds like a
> good idea to test this :)

With an do_subchannel_work_emulation() callback?


>
>> +{
>> +	register unsigned long reg1 asm("1") = schid;
>> +	int ccode;
>> +
>> +	asm volatile(
>> +		"	rsch\n"
>> +		"	ipm	%0\n"
>> +		"	srl	%0,28"
>> +		: "=d" (ccode)
>> +		: "d" (reg1)
>> +		: "cc");
>> +	return ccode;
>> +}
>> +
>> +static inline int rchp(unsigned long chpid)
> Anything useful we can test here?

Not for now.

I certainly can reduce the size of the file by removing the unused CSS 
instructions calls.

...snip...
> +
>> +static void enable_io_irq(void)
>> +{
>> +	set_io_irq_subclass_mask(0x00000000ff000000);
> So, you always enable all iscs? Maybe add a comment?

OK, is just a lazy option to get IRQ for this test.

Right, I add a comment.


>
>> +	set_system_mask(PSW_PRG_MASK >> 56);
>> +}
>> +
>> +void handle_io_int(sregs_t *regs)
>> +{
>> +	int ret = 0;
>> +
>> +	DBG("IO IRQ: subsys_id_word=%08x", lowcore->subsys_id_word);
>> +	DBG("......: io_int_parm   =%08x", lowcore->io_int_param);
>> +	DBG("......: io_int_word   =%08x", lowcore->io_int_word);
>> +	ret = tsch(lowcore->subsys_id_word, &irb);
>> +	dump_irb(&irb);
>> +	if (ret)
>> +		DBG("......: tsch retval %d", ret);
>> +	DBG("IO IRQ: END");
>> +}
>> +
>> +static void set_schib(struct schib *sch)
>> +{
>> +	struct pmcw *p = &sch->pmcw;
>> +
>> +	p->intparm = 0xdeadbeef;
>> +	p->devnum = 0xc0ca;
>> +	p->lpm = 0x80;
>> +	p->flags = 0x3081;
> Use #defines instead of magic numbers?

OK


>
>> +	p->chpid[7] = 0x22;
>> +	p->pim = 0x0f;
>> +	p->pam = 0x0f;
>> +	p->pom = 0x0f;
>> +	p->lpm = 0x0f;
>> +	p->lpum = 0xaa;
>> +	p->pnom = 0xf0;
>> +	p->mbi = 0xaa;
>> +	p->mbi = 0xaaaa;
> Many of these fields are not supposed to be modifiable by the program
> -- do you want to check what you get back after msch?
>
> Also, you set mbi twice ;) (And for it to actually have any effect,
> you'd have to execute SET CHANNEL MONITOR, no?)

Yes, it was for me to check what happens but I should remove most of 
these field initialization.

As you said they are not modifiable by the program.

Will clean this.


>
>
>> +}
>> +
>> +static void css_enable(void)
>> +{
>> +	int ret;
>> +
>> +	ret = stsch(CSSID_PONG, &schib);
>> +	if (ret)
>> +		DBG("stsch: %x\n", ret);
>> +	dump_schib(&schib);
>> +	set_schib(&schib);
>> +	dump_schib(&schib);
>> +	ret = msch(CSSID_PONG, &schib);
>> +	if (ret)
>> +		DBG("msch : %x\n", ret);
>> +}
>> +
>> +/* These two definitions are part of the QEMU PONG interface */
>> +#define PONG_WRITE 0x21
>> +#define PONG_READ  0x22
> Ah, so it's not a plain read/write, but a specialized one? Mention that
> in the patch description?
>
>> +
>> +static int css_run(int fake)
>> +{
>> +	struct orb *p = orb;
> I'd maybe call that variable 'orb' instead; at a glance, I was confused
> what you did with the pmcw below, until I realized that it's the orb :)

OK, is clearly better to use orb.


Thanks,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Cornelia Huck]

On Thu, 14 Nov 2019 11:11:18 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2019-11-13 14:05, Cornelia Huck wrote:
> > On Wed, 13 Nov 2019 13:23:19 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >  
> >> This simple test test the I/O reading by the SUB Channel by:
> >> - initializing the Channel SubSystem with predefined CSSID:
> >>    0xfe000000 CSSID for a Virtual CCW  
> > 0 should be fine with recent QEMU versions as well, I guess?  
> Right
> 
> 
> >  
> >>    0x00090000 SSID for CCW-PONG  
> > subchannel id, or subchannel set id?  
> 
> hum, only part of, I had SSID (Subchannel Set ID) 4 (a.k.a m bit) + Bit 
> 47  =1
> 
> But as you said, I can use CSSID 0 and m = 0 which makes:
> 
> Subsystem Identification word = 0x00010000

Yeah, I was mainly confused by the name 'SSID'.

> >> - initializing the ORB pointing to a single READ CCW  
> > Out of curiosity: Would using a NOP also be an option?  
> 
> It will work but will not be handled by this device, css.c intercept it 
> in sch_handle_start_func_virtual.
> 
> AFAIU If we want to have a really good testing environment, for driver 
> testing for exemple, then it would be interesting to add a new 
> do_subchannel_work callback like do_subchannel_work_emulation along with 
> the _virtual and _paththrough variantes.
> 
> Having a dedicated callback for emulation, we can answer to any CSS 
> instructions and SSCH commands, including NOP and TIC.

I guess that depends on what you want to test; if you actually want to
test device emulation as used by virtio etc., you obviously want to go
through the existing _virtual callback :)

The actual motivation behind my question was:
Is it possible to e.g. throw NOP (or TIC, or something else not
device-specific) at a normal, existing virtio device for test purposes?
You'd end up testing the common emulation code without needing any
extra support in QEMU. No idea how useful that would be.

> 
> My goal here was to quickly develop a device answering to some basic 
> READ/WRITE command to start memory transfers from inside a guest without 
> Linux and without implementing VIRTIO in KVM tests.

Yes, if you want to do some simple memory transfers, virtio is probably
not the first choice. Would e.g. doing a SenseID or so actually be
useful in some way already? After all, it does transfer memory (but
only in one direction).

> >> +static inline int rsch(unsigned long schid)  
> > I don't think anyone has tried rsch with QEMU before; sounds like a
> > good idea to test this :)  
> 
> With an do_subchannel_work_emulation() callback?

You probably need to build a simple channel program that suspends
itself and can be resumed later.

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Pierre Morel]

On 2019-11-21 17:02, Cornelia Huck wrote:
> On Thu, 14 Nov 2019 11:11:18 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> On 2019-11-13 14:05, Cornelia Huck wrote:
>>> On Wed, 13 Nov 2019 13:23:19 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>   
>>>> This simple test test the I/O reading by the SUB Channel by:
>>>> - initializing the Channel SubSystem with predefined CSSID:
>>>>     0xfe000000 CSSID for a Virtual CCW
>>> 0 should be fine with recent QEMU versions as well, I guess?
>> Right
>>
>>
>>>   
>>>>     0x00090000 SSID for CCW-PONG
>>> subchannel id, or subchannel set id?
>> hum, only part of, I had SSID (Subchannel Set ID) 4 (a.k.a m bit) + Bit
>> 47  =1
>>
>> But as you said, I can use CSSID 0 and m = 0 which makes:
>>
>> Subsystem Identification word = 0x00010000
> Yeah, I was mainly confused by the name 'SSID'.

Hum, yes sorry, I posted this to give a response to the kvm-test-unit 
css test.

I should have a lot more rework this old device before to post this series.
In between I did, so I will post a v2 which will suppress all these 
approximations.


>
>>>> - initializing the ORB pointing to a single READ CCW
>>> Out of curiosity: Would using a NOP also be an option?
>> It will work but will not be handled by this device, css.c intercept it
>> in sch_handle_start_func_virtual.
>>
>> AFAIU If we want to have a really good testing environment, for driver
>> testing for exemple, then it would be interesting to add a new
>> do_subchannel_work callback like do_subchannel_work_emulation along with
>> the _virtual and _paththrough variantes.
>>
>> Having a dedicated callback for emulation, we can answer to any CSS
>> instructions and SSCH commands, including NOP and TIC.
> I guess that depends on what you want to test; if you actually want to
> test device emulation as used by virtio etc., you obviously want to go
> through the existing _virtual callback :)

The first goal is to test basic I/O from inside the kvm-unit-test, 
producing errors and see how the system respond to errors.

In a standard system errors will be generated by QEMU analysing the I/O 
instruction after interception.

In a secured guest, we expect the same errors, however we want to check 
this.

This PONG device is intended to be low level, no VIRTIO, and to allow 
basic I/O.


>
> The actual motivation behind my question was:
> Is it possible to e.g. throw NOP (or TIC, or something else not
> device-specific) at a normal, existing virtio device for test purposes?
> You'd end up testing the common emulation code without needing any
> extra support in QEMU. No idea how useful that would be.

Writing a VIRTIO driver inside the kvm-unit-test is something we can do 
in the future.

As you said, the common code already handle NOP and TIC, the 
interpretation of the
CCW chain, once the SSCH has been intercepted is done by QEMU.
I do not think it would be different with SE.


>
>> My goal here was to quickly develop a device answering to some basic
>> READ/WRITE command to start memory transfers from inside a guest without
>> Linux and without implementing VIRTIO in KVM tests.
> Yes, if you want to do some simple memory transfers, virtio is probably
> not the first choice. Would e.g. doing a SenseID or so actually be
> useful in some way already? After all, it does transfer memory (but
> only in one direction).

The kvm-unit-test part is in development too.

Doing a SenseID will be implemented to recognize the PONG device.


>
>>>> +static inline int rsch(unsigned long schid)
>>> I don't think anyone has tried rsch with QEMU before; sounds like a
>>> good idea to test this :)
>> With an do_subchannel_work_emulation() callback?
> You probably need to build a simple channel program that suspends
> itself and can be resumed later.

Yes, that is something I plan to do.

To sum-up:

in kvm-unit-test: implement all I/O instructions and force instructions 
errors, like memory error, operand etc. and expect the right reaction of 
the system.

in QEMU, add the necessary infrastructure to test this.


Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Cornelia Huck]

On Fri, 22 Nov 2019 10:03:21 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2019-11-21 17:02, Cornelia Huck wrote:
> > On Thu, 14 Nov 2019 11:11:18 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >  
> >> On 2019-11-13 14:05, Cornelia Huck wrote:  
> >>> On Wed, 13 Nov 2019 13:23:19 +0100
> >>> Pierre Morel <pmorel@linux.ibm.com> wrote:

> >>>> - initializing the ORB pointing to a single READ CCW  
> >>> Out of curiosity: Would using a NOP also be an option?  
> >> It will work but will not be handled by this device, css.c intercept it
> >> in sch_handle_start_func_virtual.
> >>
> >> AFAIU If we want to have a really good testing environment, for driver
> >> testing for exemple, then it would be interesting to add a new
> >> do_subchannel_work callback like do_subchannel_work_emulation along with
> >> the _virtual and _paththrough variantes.
> >>
> >> Having a dedicated callback for emulation, we can answer to any CSS
> >> instructions and SSCH commands, including NOP and TIC.  
> > I guess that depends on what you want to test; if you actually want to
> > test device emulation as used by virtio etc., you obviously want to go
> > through the existing _virtual callback :)  
> 
> The first goal is to test basic I/O from inside the kvm-unit-test, 
> producing errors and see how the system respond to errors.
> 
> In a standard system errors will be generated by QEMU analysing the I/O 
> instruction after interception.
> 
> In a secured guest, we expect the same errors, however we want to check 
> this.

But we still get the intercepts for all I/O instructions, right? We
just get/inject the parameters in a slightly different way, IIUC.

Not that I disagree with wanting to check this :)

> This PONG device is intended to be low level, no VIRTIO, and to allow 
> basic I/O.

Ok, so this is designed to test basic channel I/O handling, not
necessarily if the guest has set up all its control structures
correctly?

> > The actual motivation behind my question was:
> > Is it possible to e.g. throw NOP (or TIC, or something else not
> > device-specific) at a normal, existing virtio device for test purposes?
> > You'd end up testing the common emulation code without needing any
> > extra support in QEMU. No idea how useful that would be.  
> 
> Writing a VIRTIO driver inside the kvm-unit-test is something we can do 
> in the future.
> 
> As you said, the common code already handle NOP and TIC, the 
> interpretation of the
> CCW chain, once the SSCH has been intercepted is done by QEMU.
> I do not think it would be different with SE.

Yes. You don't really need to get the virtio device up on the virtio
side; if recognizing the device correctly via senseID works and you
maybe can do some NOP/TIC commands, you might have a very basic test
without introducing a new device.

Testing virtio-ccw via kvm-unit-tests is probably a good idea for the
future.

> To sum-up:
> 
> in kvm-unit-test: implement all I/O instructions and force instructions 
> errors, like memory error, operand etc. and expect the right reaction of 
> the system.
> 
> in QEMU, add the necessary infrastructure to test this.

Sounds good to me.

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Pierre Morel]

On 2019-11-22 11:54, Cornelia Huck wrote:
> On Fri, 22 Nov 2019 10:03:21 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> On 2019-11-21 17:02, Cornelia Huck wrote:
>>> On Thu, 14 Nov 2019 11:11:18 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>   
>>>> On 2019-11-13 14:05, Cornelia Huck wrote:
>>>>> On Wed, 13 Nov 2019 13:23:19 +0100
>>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>>>> - initializing the ORB pointing to a single READ CCW
>>>>> Out of curiosity: Would using a NOP also be an option?
>>>> It will work but will not be handled by this device, css.c intercept it
>>>> in sch_handle_start_func_virtual.
>>>>
>>>> AFAIU If we want to have a really good testing environment, for driver
>>>> testing for exemple, then it would be interesting to add a new
>>>> do_subchannel_work callback like do_subchannel_work_emulation along with
>>>> the _virtual and _paththrough variantes.
>>>>
>>>> Having a dedicated callback for emulation, we can answer to any CSS
>>>> instructions and SSCH commands, including NOP and TIC.
>>> I guess that depends on what you want to test; if you actually want to
>>> test device emulation as used by virtio etc., you obviously want to go
>>> through the existing _virtual callback :)
>> The first goal is to test basic I/O from inside the kvm-unit-test,
>> producing errors and see how the system respond to errors.
>>
>> In a standard system errors will be generated by QEMU analysing the I/O
>> instruction after interception.
>>
>> In a secured guest, we expect the same errors, however we want to check
>> this.
> But we still get the intercepts for all I/O instructions, right? We
> just get/inject the parameters in a slightly different way, IIUC.
>
> Not that I disagree with wanting to check this :)

AFAIU the SE firmware, the SIE and KVM first handle the instruction 
interception before it comes to the QEMU code.

There are two major changes with secure execution that we want to test, 
SE firmware and SIE modifications.
If the instruction is treated by QEMU, then hopefully we get the same 
answer as without SE.


>
>> This PONG device is intended to be low level, no VIRTIO, and to allow
>> basic I/O.
> Ok, so this is designed to test basic channel I/O handling, not
> necessarily if the guest has set up all its control structures
> correctly?

More than this it is intended, in the next version, to test answers to 
bad configurations and wrong instruction's arguments.


>
>>> The actual motivation behind my question was:
>>> Is it possible to e.g. throw NOP (or TIC, or something else not
>>> device-specific) at a normal, existing virtio device for test purposes?
>>> You'd end up testing the common emulation code without needing any
>>> extra support in QEMU. No idea how useful that would be.
>> Writing a VIRTIO driver inside the kvm-unit-test is something we can do
>> in the future.
>>
>> As you said, the common code already handle NOP and TIC, the
>> interpretation of the
>> CCW chain, once the SSCH has been intercepted is done by QEMU.
>> I do not think it would be different with SE.
> Yes. You don't really need to get the virtio device up on the virtio
> side; if recognizing the device correctly via senseID works and you
> maybe can do some NOP/TIC commands, you might have a very basic test
> without introducing a new device.

Right, but the test is incomplete, as you said before, no write 
operation with this procedure.


>
> Testing virtio-ccw via kvm-unit-tests is probably a good idea for the
> future.
>
>> To sum-up:
>>
>> in kvm-unit-test: implement all I/O instructions and force instructions
>> errors, like memory error, operand etc. and expect the right reaction of
>> the system.
>>
>> in QEMU, add the necessary infrastructure to test this.
> Sounds good to me.

Thanks,

I think the next version will make the purpose of all of it even more 
obvious,
and hopefully answers all your questions better.

Best regards,

Pierre

>
-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Janosch Frank]

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

On 11/13/19 1:23 PM, Pierre Morel wrote:
> This simple test test the I/O reading by the SUB Channel by:
> - initializing the Channel SubSystem with predefined CSSID:
>   0xfe000000 CSSID for a Virtual CCW
>   0x00090000 SSID for CCW-PONG
> - initializing the ORB pointing to a single READ CCW
> - starts the STSH command with the ORB
> - Expect an interrupt
> - writes the read data to output
> 
> The test implements lots of traces when DEBUG is on and
> tests if memory above the stack is corrupted.

What happens if we do not habe the pong device?

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h      | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/css_dump.c | 141 +++++++++++++++++++++++++++++

Hmm, what about splitting the patch into css.h/css_dump.c and the actual
test in s390x/css.c?

>  s390x/Makefile       |   2 +
>  s390x/css.c          | 222 ++++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg  |   4 +
>  5 files changed, 613 insertions(+)
>  create mode 100644 lib/s390x/css.h
>  create mode 100644 lib/s390x/css_dump.c
>  create mode 100644 s390x/css.c
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> new file mode 100644
> index 0000000..a7c42fd
> --- /dev/null
> +++ b/lib/s390x/css.h
> @@ -0,0 +1,244 @@
> +/*
> + * CSS definitions
> + *
> + * Copyright IBM, Corp. 2019
> + * Author: Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#ifndef CSS_H
> +#define CSS_H
> +
> +#define CCW_F_CD	0x80
> +#define CCW_F_CC	0x40
> +#define CCW_F_SLI	0x20
> +#define CCW_F_SKP	0x10
> +#define CCW_F_PCI	0x08
> +#define CCW_F_IDA	0x04
> +#define CCW_F_S		0x02
> +#define CCW_F_MIDA	0x01
> +
> +#define CCW_C_NOP	0x03
> +#define CCW_C_TIC	0x08
> +
> +struct ccw {
> +	unsigned char code;
> +	unsigned char flags;
> +	unsigned short count;
> +	unsigned int data;
> +} __attribute__ ((aligned(4)));
> +
> +#define ORB_M_KEY	0xf0000000
> +#define ORB_F_SUSPEND	0x08000000
> +#define ORB_F_STREAMING	0x04000000
> +#define ORB_F_MODIFCTRL	0x02000000
> +#define ORB_F_SYNC	0x01000000
> +#define ORB_F_FORMAT	0x00800000
> +#define ORB_F_PREFETCH	0x00400000
> +#define ORB_F_INIT_IRQ	0x00200000
> +#define ORB_F_ADDRLIMIT	0x00100000
> +#define ORB_F_SUSP_IRQ	0x00080000
> +#define ORB_F_TRANSPORT	0x00040000
> +#define ORB_F_IDAW2	0x00020000
> +#define ORB_F_IDAW_2K	0x00010000
> +#define ORB_M_LPM	0x0000ff00
> +#define ORB_F_LPM_DFLT	0x00008000
> +#define ORB_F_ILSM	0x00000080
> +#define ORB_F_CCW_IND	0x00000040
> +#define ORB_F_ORB_EXT	0x00000001

\n

> +struct orb {
> +	unsigned int	intparm;
> +	unsigned int	ctrl;
> +	unsigned int	cpa;
> +	unsigned int	prio;
> +	unsigned int	reserved[4];
> +} __attribute__ ((aligned(4)));;

Double ;;

> +
> +struct scsw {
> +	uint32_t ctrl;
> +	uint32_t addr;
> +	uint8_t  devs;
> +	uint8_t  schs;
> +	uint16_t count;
> +};
> +
> +struct pmcw {
> +	uint32_t intparm;
> +	uint16_t flags;
> +	uint16_t devnum;
> +	uint8_t  lpm;
> +	uint8_t  pnom;
> +	uint8_t  lpum;
> +	uint8_t  pim;
> +	uint16_t mbi;
> +	uint8_t  pom;
> +	uint8_t  pam;
> +	uint8_t  chpid[8];
> +	uint32_t flag2;
> +};
> +struct schib {
> +	struct pmcw pmcw;
> +	struct scsw scsw;
> +	uint32_t  md0;
> +	uint32_t  md1;
> +	uint32_t  md2;
> +} __attribute__ ((aligned(4)));
> +
> +struct irb {
> +	struct scsw scsw;
> +	uint32_t esw[5];
> +	uint32_t ecw[8];
> +	uint32_t emw[8];
> +} __attribute__ ((aligned(4)));;

Double ;;

> +
> +/* CSS low level access functions */
> +
> +static inline int ssch(unsigned long schid, struct orb *addr)
> +{
> +	register long long reg1 asm("1") = schid;
> +	int ccode = -1;

s/ccode/cc/

> +
> +	asm volatile(
> +		"	   ssch	0(%2)\n"
> +		"0:	 ipm	 %0\n"
> +		"	   srl	 %0,28\n"
> +		"1:\n"
> +		: "+d" (ccode)
> +		: "d" (reg1), "a" (addr), "m" (*addr)
> +		: "cc", "memory");
> +	return ccode;
> +}
> +
> +static inline int stsch(unsigned long schid, struct schib *addr)
> +{
> +	register unsigned long reg1 asm ("1") = schid;
> +	int ccode;
> +
> +	asm volatile(
> +		"	   stsch	0(%3)\n"
> +		"	   ipm	 %0\n"
> +		"	   srl	 %0,28"
> +		: "=d" (ccode), "=m" (*addr)
> +		: "d" (reg1), "a" (addr)
> +		: "cc");
> +	return ccode;
> +}
> +
> +static inline int msch(unsigned long schid, struct schib *addr)
> +{
> +	register unsigned long reg1 asm ("1") = schid;
> +	int ccode;
> +
> +	asm volatile(
> +		"	   msch	0(%3)\n"
> +		"	   ipm	 %0\n"
> +		"	   srl	 %0,28"
> +		: "=d" (ccode), "=m" (*addr)
> +		: "d" (reg1), "a" (addr)
> +		: "cc");
> +	return ccode;
> +}
> +
> +static inline int tsch(unsigned long schid, struct irb *addr)
> +{
> +	register unsigned long reg1 asm ("1") = schid;
> +	int ccode;
> +
> +	asm volatile(
> +		"	   tsch	0(%3)\n"
> +		"	   ipm	 %0\n"
> +		"	   srl	 %0,28"
> +		: "=d" (ccode), "=m" (*addr)
> +		: "d" (reg1), "a" (addr)
> +		: "cc");
> +	return ccode;
> +}
> +
> +static inline int hsch(unsigned long schid)
> +{
> +	register unsigned long reg1 asm("1") = schid;
> +	int ccode;
> +
> +	asm volatile(
> +		"	hsch\n"
> +		"	ipm	%0\n"
> +		"	srl	%0,28"
> +		: "=d" (ccode)
> +		: "d" (reg1)
> +		: "cc");
> +	return ccode;
> +}
> +
> +static inline int xsch(unsigned long schid)
> +{
> +	register unsigned long reg1 asm("1") = schid;
> +	int ccode;
> +
> +	asm volatile(
> +		"	xsch\n"
> +		"	ipm	%0\n"
> +		"	srl	%0,28"
> +		: "=d" (ccode)
> +		: "d" (reg1)
> +		: "cc");
> +	return ccode;
> +}
> +
> +static inline int csch(unsigned long schid)
> +{
> +	register unsigned long reg1 asm("1") = schid;
> +	int ccode;> +
> +	asm volatile(
> +		"	csch\n"
> +		"	ipm	%0\n"
> +		"	srl	%0,28"
> +		: "=d" (ccode)
> +		: "d" (reg1)
> +		: "cc");
> +	return ccode;
> +}
> +
> +static inline int rsch(unsigned long schid)
> +{
> +	register unsigned long reg1 asm("1") = schid;
> +	int ccode;
> +
> +	asm volatile(
> +		"	rsch\n"
> +		"	ipm	%0\n"
> +		"	srl	%0,28"
> +		: "=d" (ccode)
> +		: "d" (reg1)
> +		: "cc");
> +	return ccode;
> +}
> +
> +static inline int rchp(unsigned long chpid)
> +{
> +	register unsigned long reg1 asm("1") = chpid;
> +	int ccode;
> +
> +	asm volatile(
> +		"	rchp\n"
> +		"	ipm	%0\n"
> +		"	srl	%0,28"
> +		: "=d" (ccode)
> +		: "d" (reg1)
> +		: "cc");
> +	return ccode;
> +}
> +
> +void dump_scsw(struct scsw *);
> +void dump_irb(struct irb *irbp);
> +void dump_pmcw_flags(uint16_t f);
> +void dump_pmcw(struct pmcw *p);
> +void dump_schib(struct schib *sch);
> +struct ccw *dump_ccw(struct ccw *cp);
> +void dump_orb(struct orb *op);
> +
> +extern unsigned long stacktop;
> +#endif
> diff --git a/lib/s390x/css_dump.c b/lib/s390x/css_dump.c
> new file mode 100644
> index 0000000..4f6b628
> --- /dev/null
> +++ b/lib/s390x/css_dump.c
> @@ -0,0 +1,141 @@
> +/*
> + * Channel Sub-System structures dumping
> + *
> + * Copyright (c) 2019 IBM Corp.
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Library General Public License version 2.
> + *
> + * Description:
> + * Provides the dumping functions for various structures used by subchannels:
> + * - ORB  : Operation request block, describe the I/O operation and point to
> + *          a CCW chain
> + * - CCW  : Channel Command Word, describe the data and flow control
> + * - IRB  : Interuption response Block, describe the result of an operation
> + *          hold a SCSW and several channel type dependent fields.
> + * - SCHIB: SubChannel Information Block composed of:
> + *   - SCSW: SubChannel Status Word, status of the channel as a result of an
> + *           operation when in IRB.
> + *   - PMCW: Path Management Control Word
> + * You need the QEMU ccw-pong device in QEMU to answer the I/O transfers.
> + */
> +
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include <css.h>
> +
> +static const char *scsw_str = "kkkkslccfpixuzen";
> +static const char *scsw_str2 = "1SHCrshcsdsAIPSs";
> +
> +void dump_scsw(struct scsw *s)
> +{
> +	int i;
> +	uint32_t scsw = s->ctrl;
> +	char line[64] = {};
> +
> +	for (i = 0; i < 16; i++) {
> +		if ((scsw << i) & 0x80000000)
> +			line[i] = scsw_str[i];
> +		else
> +			line[i] = '_';
> +	}
> +	line[i] = ' ';
> +	for (; i < 32; i++) {
> +		if ((scsw << i) & 0x80000000)
> +			line[i + 1] = scsw_str2[i - 16];
> +		else
> +			line[i + 1] = '_';
> +	}
> +	printf("scsw->flags: %s\n", line);
> +	printf("scsw->addr : %08x\n", s->addr);
> +	printf("scsw->devs : %02x\n", s->devs);
> +	printf("scsw->schs : %02x\n", s->schs);
> +	printf("scsw->count: %04x\n", s->count);
> +

Stray \n

> +}
> +
> +void dump_irb(struct irb *irbp)
> +{
> +	int i;
> +	uint32_t *p = (uint32_t *)irbp;
> +
> +	dump_scsw(&irbp->scsw);
> +	for (i = 0; i < sizeof(*irbp)/sizeof(*p); i++, p++)
> +		printf("irb[%02x] : %08x\n", i, *p);
> +}
> +
> +static const char *pmcw_str = "11iii111ellmmdtv";
> +void dump_pmcw_flags(uint16_t f)
> +{
> +	int i;
> +	char line[32] = {};
> +
> +	for (i = 0; i < 16; i++) {
> +		if ((f << i) & 0x8000)
> +			line[i] = pmcw_str[i];
> +		else
> +			line[i] = '_';
> +	}
> +	printf("pmcw->pmcw flgs: %s\n", line);
> +}
> +
> +void dump_pmcw(struct pmcw *p)
> +{
> +	int i;
> +
> +	printf("pmcw->intparm  : %08x\n", p->intparm);
> +	printf("pmcw->flags    : %04x\n", p->flags);
> +	dump_pmcw_flags(p->flags);
> +	printf("pmcw->devnum   : %04x\n", p->devnum);
> +	printf("pmcw->lpm      : %02x\n", p->lpm);
> +	printf("pmcw->pnom     : %02x\n", p->pnom);
> +	printf("pmcw->lpum     : %02x\n", p->lpum);
> +	printf("pmcw->pim      : %02x\n", p->pim);
> +	printf("pmcw->mbi      : %04x\n", p->mbi);
> +	printf("pmcw->pom      : %02x\n", p->pom);
> +	printf("pmcw->pam      : %02x\n", p->pam);
> +	printf("pmcw->mbi      : %04x\n", p->mbi);
> +	for (i = 0; i < 8; i++)
> +		printf("pmcw->chpid[%d]: %02x\n", i, p->chpid[i]);
> +	printf("pmcw->flags2  : %08x\n", p->flag2);
> +}
> +
> +void dump_schib(struct schib *sch)
> +{
> +	struct pmcw *p = &sch->pmcw;
> +	struct scsw *s = &sch->scsw;
> +
> +	printf("--SCHIB--\n");
> +	dump_pmcw(p);
> +	dump_scsw(s);
> +}
> +
> +struct ccw *dump_ccw(struct ccw *cp)
> +{
> +	printf("CCW: code: %02x flags: %02x count: %04x data: %08x\n", cp->code,
> +	    cp->flags, cp->count, cp->data);
> +
> +	if (cp->code == CCW_C_TIC)
> +		return (struct ccw *)(long)cp->data;
> +
> +	return (cp->flags & CCW_F_CC) ? cp + 1 : NULL;
> +}
> +
> +void dump_orb(struct orb *op)
> +{
> +	struct ccw *cp;
> +
> +	printf("ORB: intparm : %08x\n", op->intparm);
> +	printf("ORB: ctrl    : %08x\n", op->ctrl);
> +	printf("ORB: prio    : %08x\n", op->prio);
> +	cp = (struct ccw *)(long) (op->cpa);
> +	while (cp)
> +		cp = dump_ccw(cp);
> +}
> +
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 3744372..9ebbb84 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -16,6 +16,7 @@ tests += $(TEST_DIR)/diag288.elf
>  tests += $(TEST_DIR)/stsi.elf
>  tests += $(TEST_DIR)/skrf.elf
>  tests += $(TEST_DIR)/smp.elf
> +tests += $(TEST_DIR)/css.elf
>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>  
>  all: directories test_cases test_cases_binary
> @@ -50,6 +51,7 @@ cflatobjs += lib/s390x/sclp-console.o
>  cflatobjs += lib/s390x/interrupt.o
>  cflatobjs += lib/s390x/mmu.o
>  cflatobjs += lib/s390x/smp.o
> +cflatobjs += lib/s390x/css_dump.o
>  
>  OBJDIRS += lib/s390x
>  
> diff --git a/s390x/css.c b/s390x/css.c
> new file mode 100644
> index 0000000..6cdaf61
> --- /dev/null
> +++ b/s390x/css.c
> @@ -0,0 +1,222 @@
> +/*
> + * Channel Sub-System tests
> + *
> + * Copyright (c) 2019 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2.
> + */
> +
> +#include <libcflat.h>
> +#include <alloc_phys.h>
> +#include <asm/page.h>
> +#include <string.h>
> +#include <asm/interrupt.h>
> +#include <asm/arch_def.h>
> +
> +#include <css.h>
> +
> +#define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
> +
> +#define DEBUG 1
> +#ifdef DEBUG
> +#define DBG(format, arg...) \
> +	printf("KT_%s:%d " format "\n", __func__, __LINE__, ##arg)
> +#else
> +#define DBG(format, arg...) do {} while (0)
> +#endif
> +
> +#define CSSID_PONG	(0xfe000000 | 0x00090000)
> +
> +struct lowcore *lowcore = (void *)0x0;
> +
> +#define NB_CCW  100
> +static struct ccw ccw[NB_CCW];
> +
> +#define NB_ORB  100
> +static struct orb orb[NB_ORB];
> +
> +static struct irb irb;
> +static struct schib schib;
> +
> +static char buffer[4096];
> +
> +static void delay(int d)
> +{
> +	int i, j;
> +
> +	while (d--)
> +		for (i = 1000000; i; i--)
> +			for (j = 1000000; j; j--)
> +				;
> +}

You could set a timer.

> +
> +static void set_io_irq_subclass_mask(uint64_t const new_mask)
> +{
> +	asm volatile (
> +		"lctlg %%c6, %%c6, %[source]\n"
> +		: /* No outputs */
> +		: [source] "R" (new_mask));

arch_def.h has lctlg() and ctl_set/clear_bit

> +}
> +
> +static void set_system_mask(uint8_t new_mask)
> +{
> +	asm volatile (
> +		"ssm %[source]\n"
> +		: /* No outputs */
> +		: [source] "R" (new_mask));
> +}
> +
> +static void enable_io_irq(void)
> +{
> +	set_io_irq_subclass_mask(0x00000000ff000000);
> +	set_system_mask(PSW_PRG_MASK >> 56);

load_psw_mask(extract_psw_mask() | PSW_PRG_MASK); no need for another
inline asm function :)

Or add a psw_set/clear_bit function and fixup enter_pstate()

> +}
> +
> +void handle_io_int(sregs_t *regs)
> +{
> +	int ret = 0;
> +
> +	DBG("IO IRQ: subsys_id_word=%08x", lowcore->subsys_id_word);
> +	DBG("......: io_int_parm   =%08x", lowcore->io_int_param);
> +	DBG("......: io_int_word   =%08x", lowcore->io_int_word);
> +	ret = tsch(lowcore->subsys_id_word, &irb);
> +	dump_irb(&irb);
> +	if (ret)
> +		DBG("......: tsch retval %d", ret);
> +	DBG("IO IRQ: END");
> +}
> +
> +static void set_schib(struct schib *sch)
> +{
> +	struct pmcw *p = &sch->pmcw;
> +
> +	p->intparm = 0xdeadbeef;
> +	p->devnum = 0xc0ca;
> +	p->lpm = 0x80;
> +	p->flags = 0x3081;
> +	p->chpid[7] = 0x22;
> +	p->pim = 0x0f;
> +	p->pam = 0x0f;
> +	p->pom = 0x0f;
> +	p->lpm = 0x0f;
> +	p->lpum = 0xaa;
> +	p->pnom = 0xf0;
> +	p->mbi = 0xaa;
> +	p->mbi = 0xaaaa;
> +}
> +
> +static void css_enable(void)
> +{
> +	int ret;
> +
> +	ret = stsch(CSSID_PONG, &schib);
> +	if (ret)
> +		DBG("stsch: %x\n", ret);
> +	dump_schib(&schib);
> +	set_schib(&schib);
> +	dump_schib(&schib);
> +	ret = msch(CSSID_PONG, &schib);
> +	if (ret)
> +		DBG("msch : %x\n", ret);
> +}
> +
> +/* These two definitions are part of the QEMU PONG interface */
> +#define PONG_WRITE 0x21
> +#define PONG_READ  0x22
> +
> +static int css_run(int fake)
> +{
> +	struct orb *p = orb;
> +	int cc;
> +
> +	if (fake)
> +		return 0;
> +	css_enable();
> +
> +	enable_io_irq();
> +
> +	ccw[0].code = PONG_READ;
> +	ccw[0].flags = CCW_F_PCI;
> +	ccw[0].count = 80;
> +	ccw[0].data = (unsigned int)(unsigned long) &buffer;
> +
> +	p->intparm = 0xcafec0ca;
> +	p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
> +	p->cpa = (unsigned int) (unsigned long)&ccw[0];
> +
> +	printf("ORB AT %p\n", orb);
> +	dump_orb(p);
> +	cc = ssch(CSSID_PONG, p);
> +	if (cc) {
> +		DBG("cc: %x\n", cc);
> +		return cc;
> +	}
> +
> +	delay(1);
> +
> +	stsch(CSSID_PONG, &schib);
> +	dump_schib(&schib);

Is all that dumping necessary or just a dev remainder?

> +	DBG("got: %s\n", buffer);
> +
> +	return 0;
> +}
> +
> +#define MAX_ERRORS 10
> +static int checkmem(phys_addr_t start, phys_addr_t end)
> +{
> +	phys_addr_t curr;
> +	int err = 0;
> +
> +	for (curr = start; curr != end; curr += PAGE_SIZE)
> +		if (memcmp((void *)start, (void *)curr, PAGE_SIZE)) {
> +			report("memcmp failed %lx", true, curr);

How many errors do you normally run into (hopefully 0)?

> +			if (err++ > MAX_ERRORS)
> +				break;
> +		}
> +	return err;
> +}
> +
> +extern unsigned long bss_end;
> +
> +int main(int argc, char *argv[])
> +{
> +	phys_addr_t base, top;
> +	int check_mem = 0;
> +	int err = 0;
> +
> +	if (argc == 2 && !strcmp(argv[1], "-i"))
> +		check_mem = 1;
> +
> +	report_prefix_push("css");
> +	phys_alloc_get_unused(&base, &top);
> +
> +	top = 0x08000000; /* 128MB Need to be updated */
> +	base = (phys_addr_t)&stacktop;
> +
> +	if (check_mem)
> +		memset((void *)base, 0x00, top - base);
> +
> +	if (check_mem)
> +		err = checkmem(base, top);
> +	if (err)
> +		goto out;
> +
> +	err = css_run(0);
> +	if (err)
> +		goto out;
> +
> +	if (check_mem)
> +		err = checkmem(base, top);
> +
> +out:
> +	if (err)
> +		report("Tested", 0);
> +	else
> +		report("Tested", 1);

Normally we report the sucsess or failure of single actions and a
summary will tell us if the whole test ran into errors.

> +
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index f1b07cd..1755d9e 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -75,3 +75,7 @@ file = stsi.elf
>  [smp]
>  file = smp.elf
>  extra_params =-smp 2
> +
> +[css]
> +file = css.elf
> +extra_params =-device ccw-pong
> 



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

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Pierre Morel]

On 2019-11-14 10:15, Janosch Frank wrote:
> On 11/13/19 1:23 PM, Pierre Morel wrote:
>> This simple test test the I/O reading by the SUB Channel by:
>> - initializing the Channel SubSystem with predefined CSSID:
>>    0xfe000000 CSSID for a Virtual CCW
>>    0x00090000 SSID for CCW-PONG
>> - initializing the ORB pointing to a single READ CCW
>> - starts the STSH command with the ORB
>> - Expect an interrupt
>> - writes the read data to output
>>
>> The test implements lots of traces when DEBUG is on and
>> tests if memory above the stack is corrupted.
> What happens if we do not habe the pong device?

CC error on stsch() which is currently not cached (but will in the next 
version)

CC error on msch() and on ssch() which is cached and makes the test to fail.


>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h      | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/css_dump.c | 141 +++++++++++++++++++++++++++++
> Hmm, what about splitting the patch into css.h/css_dump.c and the actual
> test in s390x/css.c?

OK


>
>>   s390x/Makefile       |   2 +
>>   s390x/css.c          | 222 ++++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg  |   4 +
>>   5 files changed, 613 insertions(+)
>>   create mode 100644 lib/s390x/css.h
>>   create mode 100644 lib/s390x/css_dump.c
>>   create mode 100644 s390x/css.c
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> new file mode 100644

OK to all comments...  (I sniped out for clarity)

...snip...


>> +static char buffer[4096];
>> +
>> +static void delay(int d)
>> +{
>> +	int i, j;
>> +
>> +	while (d--)
>> +		for (i = 1000000; i; i--)
>> +			for (j = 1000000; j; j--)
>> +				;
>> +}
> You could set a timer.


Hum, do we really want to do this?


>
>> +
>> +static void set_io_irq_subclass_mask(uint64_t const new_mask)
>> +{
>> +	asm volatile (
>> +		"lctlg %%c6, %%c6, %[source]\n"
>> +		: /* No outputs */
>> +		: [source] "R" (new_mask));
> arch_def.h has lctlg() and ctl_set/clear_bit


OK, thanks


>
>> +}
>> +
>> +static void set_system_mask(uint8_t new_mask)
>> +{
>> +	asm volatile (
>> +		"ssm %[source]\n"
>> +		: /* No outputs */
>> +		: [source] "R" (new_mask));
>> +}
>> +
>> +static void enable_io_irq(void)
>> +{
>> +	set_io_irq_subclass_mask(0x00000000ff000000);
>> +	set_system_mask(PSW_PRG_MASK >> 56);
> load_psw_mask(extract_psw_mask() | PSW_PRG_MASK); no need for another
> inline asm function :)
>
> Or add a psw_set/clear_bit function and fixup enter_pstate()

I look at this.


>
>> +}
>> +
>> +void handle_io_int(sregs_t *regs)
>> +{
,,,snip...
>> +
>> +	delay(1);
>> +
>> +	stsch(CSSID_PONG, &schib);
>> +	dump_schib(&schib);
> Is all that dumping necessary or just a dev remainder?


it goes in the logs, so I thought it could be interresting to keep it.


>
>> +	DBG("got: %s\n", buffer);
>> +
>> +	return 0;
>> +}
>> +
>> +#define MAX_ERRORS 10
>> +static int checkmem(phys_addr_t start, phys_addr_t end)
>> +{
>> +	phys_addr_t curr;
>> +	int err = 0;
>> +
>> +	for (curr = start; curr != end; curr += PAGE_SIZE)
>> +		if (memcmp((void *)start, (void *)curr, PAGE_SIZE)) {
>> +			report("memcmp failed %lx", true, curr);
> How many errors do you normally run into (hopefully 0)?


hopefully.

However I thought it could be interesting to know how many pages have 
been dirtied.


>
>> +			if (err++ > MAX_ERRORS)
>> +				break;
>> +		}
>> +	return err;
>> +}
>> +
>> +extern unsigned long bss_end;
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +	phys_addr_t base, top;
>> +	int check_mem = 0;
>> +	int err = 0;
>> +
>> +	if (argc == 2 && !strcmp(argv[1], "-i"))
>> +		check_mem = 1;
>> +
>> +	report_prefix_push("css");
>> +	phys_alloc_get_unused(&base, &top);
>> +
>> +	top = 0x08000000; /* 128MB Need to be updated */
>> +	base = (phys_addr_t)&stacktop;
>> +
>> +	if (check_mem)
>> +		memset((void *)base, 0x00, top - base);
>> +
>> +	if (check_mem)
>> +		err = checkmem(base, top);
>> +	if (err)
>> +		goto out;
>> +
>> +	err = css_run(0);
>> +	if (err)
>> +		goto out;
>> +
>> +	if (check_mem)
>> +		err = checkmem(base, top);
>> +
>> +out:
>> +	if (err)
>> +		report("Tested", 0);
>> +	else
>> +		report("Tested", 1);
> Normally we report the sucsess or failure of single actions and a
> summary will tell us if the whole test ran into errors.

Right, will be enhanced.

Thanks for the comments.

Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Thomas Huth]

On 14/11/2019 17.38, Pierre Morel wrote:
[...]
>>> +static char buffer[4096];
>>> +
>>> +static void delay(int d)
>>> +{
>>> +    int i, j;
>>> +
>>> +    while (d--)
>>> +        for (i = 1000000; i; i--)
>>> +            for (j = 1000000; j; j--)
>>> +                ;
>>> +}
>> You could set a timer.
> 
> Hum, do we really want to do this?

I'm pretty sure that the compiler optimizes empty loops away. Maybe have
a look at the disassembly of your delay function...

Anyway, it's likely better to use STCK and friends to get a proper
timing. You could move get_clock_ms() from s390x/intercept.c to the
lib/s390x folder and then use that function here.

 Thomas

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Pierre Morel]

On 2019-11-14 17:51, Thomas Huth wrote:
> On 14/11/2019 17.38, Pierre Morel wrote:
> [...]
>>>> +static char buffer[4096];
>>>> +
>>>> +static void delay(int d)
>>>> +{
>>>> +    int i, j;
>>>> +
>>>> +    while (d--)
>>>> +        for (i = 1000000; i; i--)
>>>> +            for (j = 1000000; j; j--)
>>>> +                ;
>>>> +}
>>> You could set a timer.
>> Hum, do we really want to do this?
> I'm pretty sure that the compiler optimizes empty loops away. Maybe have
> a look at the disassembly of your delay function...
>
> Anyway, it's likely better to use STCK and friends to get a proper
> timing. You could move get_clock_ms() from s390x/intercept.c to the
> lib/s390x folder and then use that function here.


Yes, I can, thanks.

Pierre



>
>   Thomas
>
-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Janosch Frank]

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

On 11/14/19 5:38 PM, Pierre Morel wrote:
> 
> On 2019-11-14 10:15, Janosch Frank wrote:
>> On 11/13/19 1:23 PM, Pierre Morel wrote:
>>> This simple test test the I/O reading by the SUB Channel by:
>>> - initializing the Channel SubSystem with predefined CSSID:
>>>    0xfe000000 CSSID for a Virtual CCW
>>>    0x00090000 SSID for CCW-PONG
>>> - initializing the ORB pointing to a single READ CCW
>>> - starts the STSH command with the ORB
>>> - Expect an interrupt
>>> - writes the read data to output
>>>
>>> The test implements lots of traces when DEBUG is on and
>>> tests if memory above the stack is corrupted.
>> What happens if we do not habe the pong device?
> 
> CC error on stsch() which is currently not cached (but will in the next 
> version)
> 
> CC error on msch() and on ssch() which is cached and makes the test to fail.
> 
> 
>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   lib/s390x/css.h      | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   lib/s390x/css_dump.c | 141 +++++++++++++++++++++++++++++
>> Hmm, what about splitting the patch into css.h/css_dump.c and the actual
>> test in s390x/css.c?
> 
> OK
> 
> 
>>
>>>   s390x/Makefile       |   2 +
>>>   s390x/css.c          | 222 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   s390x/unittests.cfg  |   4 +
>>>   5 files changed, 613 insertions(+)
>>>   create mode 100644 lib/s390x/css.h
>>>   create mode 100644 lib/s390x/css_dump.c
>>>   create mode 100644 s390x/css.c
>>>
>>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>>> new file mode 100644
> 
> OK to all comments...  (I sniped out for clarity)
> 
> ...snip...
> 
> 
>>> +static char buffer[4096];
>>> +
>>> +static void delay(int d)
>>> +{
>>> +	int i, j;
>>> +
>>> +	while (d--)
>>> +		for (i = 1000000; i; i--)
>>> +			for (j = 1000000; j; j--)
>>> +				;
>>> +}
>> You could set a timer.
> 
> 
> Hum, do we really want to do this?

Why exactly do you need it if you can't have an exact time to wait for?

> 
> 
>>
>>> +
>>> +static void set_io_irq_subclass_mask(uint64_t const new_mask)
>>> +{
>>> +	asm volatile (
>>> +		"lctlg %%c6, %%c6, %[source]\n"
>>> +		: /* No outputs */
>>> +		: [source] "R" (new_mask));
>> arch_def.h has lctlg() and ctl_set/clear_bit
> 
> 
> OK, thanks
> 
> 
>>
>>> +}
>>> +
>>> +static void set_system_mask(uint8_t new_mask)
>>> +{
>>> +	asm volatile (
>>> +		"ssm %[source]\n"
>>> +		: /* No outputs */
>>> +		: [source] "R" (new_mask));
>>> +}
>>> +
>>> +static void enable_io_irq(void)
>>> +{
>>> +	set_io_irq_subclass_mask(0x00000000ff000000);
>>> +	set_system_mask(PSW_PRG_MASK >> 56);
>> load_psw_mask(extract_psw_mask() | PSW_PRG_MASK); no need for another
>> inline asm function :)
>>
>> Or add a psw_set/clear_bit function and fixup enter_pstate()
> 
> I look at this.
> 
> 
>>
>>> +}
>>> +
>>> +void handle_io_int(sregs_t *regs)
>>> +{
> ,,,snip...
>>> +
>>> +	delay(1);
>>> +
>>> +	stsch(CSSID_PONG, &schib);
>>> +	dump_schib(&schib);
>> Is all that dumping necessary or just a dev remainder?
> 
> 
> it goes in the logs, so I thought it could be interresting to keep it.

Depends on how much output is produced.
If I have to scroll through your dumps to get to the ouptuts
 of the reports then they are .

See the answer below...

> 
> 
>>
>>> +	DBG("got: %s\n", buffer);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +#define MAX_ERRORS 10
>>> +static int checkmem(phys_addr_t start, phys_addr_t end)
>>> +{
>>> +	phys_addr_t curr;
>>> +	int err = 0;
>>> +
>>> +	for (curr = start; curr != end; curr += PAGE_SIZE)
>>> +		if (memcmp((void *)start, (void *)curr, PAGE_SIZE)) {
>>> +			report("memcmp failed %lx", true, curr);
>> How many errors do you normally run into (hopefully 0)?
> 
> 
> hopefully.
> 
> However I thought it could be interesting to know how many pages have 
> been dirtied.

Honestly, for debugging a failing test we would need to add prints or
attach gdb anyway. So I see no reason to not fail on the first occurrence.

> 
> 
>>
>>> +			if (err++ > MAX_ERRORS)
>>> +				break;
>>> +		}
>>> +	return err;
>>> +}
>>> +
>>> +extern unsigned long bss_end;
>>> +
>>> +int main(int argc, char *argv[])
>>> +{
>>> +	phys_addr_t base, top;
>>> +	int check_mem = 0;
>>> +	int err = 0;
>>> +
>>> +	if (argc == 2 && !strcmp(argv[1], "-i"))
>>> +		check_mem = 1;
>>> +
>>> +	report_prefix_push("css");
>>> +	phys_alloc_get_unused(&base, &top);
>>> +
>>> +	top = 0x08000000; /* 128MB Need to be updated */
>>> +	base = (phys_addr_t)&stacktop;
>>> +
>>> +	if (check_mem)
>>> +		memset((void *)base, 0x00, top - base);
>>> +
>>> +	if (check_mem)
>>> +		err = checkmem(base, top);
>>> +	if (err)
>>> +		goto out;
>>> +
>>> +	err = css_run(0);
>>> +	if (err)
>>> +		goto out;
>>> +
>>> +	if (check_mem)
>>> +		err = checkmem(base, top);
>>> +
>>> +out:
>>> +	if (err)
>>> +		report("Tested", 0);
>>> +	else
>>> +		report("Tested", 1);
>> Normally we report the sucsess or failure of single actions and a
>> summary will tell us if the whole test ran into errors.
> 
> Right, will be enhanced.
> 
> Thanks for the comments.
> 
> Regards,
> 
> Pierre
> 
> 



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

Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Pierre Morel]

On 2019-11-14 18:09, Janosch Frank wrote:
> On 11/14/19 5:38 PM, Pierre Morel wrote:
>> On 2019-11-14 10:15, Janosch Frank wrote:
>>> On 11/13/19 1:23 PM, Pierre Morel wrote:
>>>> This simple test test the I/O reading by the SUB Channel by:
>>>> - initializing the Channel SubSystem with predefined CSSID:
>>>>     0xfe000000 CSSID for a Virtual CCW
>>>>     0x00090000 SSID for CCW-PONG
>>>> - initializing the ORB pointing to a single READ CCW
>>>> - starts the STSH command with the ORB
>>>> - Expect an interrupt
>>>> - writes the read data to output
>>>>
>>>> The test implements lots of traces when DEBUG is on and
>>>> tests if memory above the stack is corrupted.
>>> What happens if we do not habe the pong device?
>> CC error on stsch() which is currently not cached (but will in the next
>> version)
>>
>> CC error on msch() and on ssch() which is cached and makes the test to fail.
>>
>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>    lib/s390x/css.h      | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    lib/s390x/css_dump.c | 141 +++++++++++++++++++++++++++++
>>> Hmm, what about splitting the patch into css.h/css_dump.c and the actual
>>> test in s390x/css.c?
>> OK
>>
>>
>>>>    s390x/Makefile       |   2 +
>>>>    s390x/css.c          | 222 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>    s390x/unittests.cfg  |   4 +
>>>>    5 files changed, 613 insertions(+)
>>>>    create mode 100644 lib/s390x/css.h
>>>>    create mode 100644 lib/s390x/css_dump.c
>>>>    create mode 100644 s390x/css.c
>>>>
>>>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>>>> new file mode 100644
>> OK to all comments...  (I sniped out for clarity)
>>
>> ...snip...
>>
>>
>>>> +static char buffer[4096];
>>>> +
>>>> +static void delay(int d)
>>>> +{
>>>> +	int i, j;
>>>> +
>>>> +	while (d--)
>>>> +		for (i = 1000000; i; i--)
>>>> +			for (j = 1000000; j; j--)
>>>> +				;
>>>> +}
>>> You could set a timer.
>>
>> Hum, do we really want to do this?
> Why exactly do you need it if you can't have an exact time to wait for?

In fact I do not need it since the CCW instructions are treated 
synchronously.

It was for the fact we speak to a real device or if QEMU uses iothreads 
for CCW handling.


...snip


>>>> +
>>>> +	delay(1);
>>>> +
>>>> +	stsch(CSSID_PONG, &schib);
>>>> +	dump_schib(&schib);
>>> Is all that dumping necessary or just a dev remainder?
>>
>> it goes in the logs, so I thought it could be interresting to keep it.
> Depends on how much output is produced.
> If I have to scroll through your dumps to get to the ouptuts
>   of the reports then they are .
>
> See the answer below...


OK


>
>>
>>>> +	DBG("got: %s\n", buffer);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +#define MAX_ERRORS 10
>>>> +static int checkmem(phys_addr_t start, phys_addr_t end)
>>>> +{
>>>> +	phys_addr_t curr;
>>>> +	int err = 0;
>>>> +
>>>> +	for (curr = start; curr != end; curr += PAGE_SIZE)
>>>> +		if (memcmp((void *)start, (void *)curr, PAGE_SIZE)) {
>>>> +			report("memcmp failed %lx", true, curr);
>>> How many errors do you normally run into (hopefully 0)?
>>
>> hopefully.
>>
>> However I thought it could be interesting to know how many pages have
>> been dirtied.
> Honestly, for debugging a failing test we would need to add prints or
> attach gdb anyway. So I see no reason to not fail on the first occurrence.


OK

I will make the second version more quiet and much better.

Thanks for the comments,

Regards,

Pierre



-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 0/4] s390x: Testing the Subchannel I/O

[Thomas Huth]

 Hi Pierre!

Meta-comment: Please use "[kvm-unit-tests PATCH ...]" in the subject for
future kvm-unit-tests patches.

On 13/11/2019 13.23, Pierre Morel wrote:
[...]
> The simple test tests the I/O reading by the SUB Channel. It needs QEMU to
> be patched to have the pong device defined.

Are you going to send QEMU patches for this? I assume that's a
prerequisite for this patch series?

 Thomas

Re: [PATCH v1 0/4] s390x: Testing the Subchannel I/O

[Pierre Morel]

On 2019-11-13 13:35, Thomas Huth wrote:
>   Hi Pierre!
>
> Meta-comment: Please use "[kvm-unit-tests PATCH ...]" in the subject for
> future kvm-unit-tests patches.


Oh sorry, I didn't notice, thanks.

Will take care for the next series.


>
> On 13/11/2019 13.23, Pierre Morel wrote:
> [...]
>> The simple test tests the I/O reading by the SUB Channel. It needs QEMU to
>> be patched to have the pong device defined.
> Are you going to send QEMU patches for this? I assume that's a
> prerequisite for this patch series?
>
>   Thomas
>
Yes I will.

Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen