Re: [kvm-unit-tests PATCH v1 3/5] s390x: css: implementing Set CHannel Monitor

Re: [kvm-unit-tests PATCH v1 3/5] s390x: css: implementing Set CHannel Monitor

[Cornelia Huck]

On Fri, 29 Jan 2021 15:34:27 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We implement the call of the Set CHannel Monitor instruction,
> starting the monitoring of the all Channel Sub System, and

"initializing channel subsystem monitoring" ?

> the initialization of the monitoring on a Sub Channel.

"enabling monitoring for a subchannel" ?

> 
> An initial test reports the presence of the extended measurement
> block feature.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     | 17 +++++++++-
>  lib/s390x/css_lib.c | 77 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/css.c         |  7 +++++
>  3 files changed, 100 insertions(+), 1 deletion(-)
> 

(...)

> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index f300969..9e0f568 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -205,6 +205,83 @@ retry:
>  	return -1;
>  }
>  
> +/*
> + * css_enable_mb: enable the subchannel Mesurement Block
> + * @schid: Subchannel Identifier
> + * @mb   : 64bit address of the measurement block
> + * @format1: set if format 1 is to be used
> + * @mbi : the measurement block offset
> + * @flags : PMCW_MBUE to enable measurement block update
> + *	    PMCW_DCTME to enable device connect time
> + * Return value:
> + *   On success: 0
> + *   On error the CC of the faulty instruction
> + *      or -1 if the retry count is exceeded.
> + */
> +int css_enable_mb(int schid, uint64_t mb, int format1, uint16_t mbi,
> +		  uint16_t flags)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int retry_count = 0;
> +	int cc;
> +
> +	/* Read the SCHIB for this subchannel */
> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch: sch %08x failed with cc=%d", schid, cc);
> +		return cc;
> +	}
> +
> +retry:
> +	/* Update the SCHIB to enable the measurement block */
> +	pmcw->flags |= flags;
> +
> +	if (format1)
> +		pmcw->flags2 |= PMCW_MBF1;
> +	else
> +		pmcw->flags2 &= ~PMCW_MBF1;
> +
> +	pmcw->mbi = mbi;
> +	schib.mbo = mb;
> +
> +	/* Tell the CSS we want to modify the subchannel */
> +	cc = msch(schid, &schib);

Setting some invalid flags for measurements in the schib could lead to
an operand exception. Do we want to rely on the caller always getting
it right, or should we add handling for those invalid flags? (Might
also make a nice test case.)

> +	if (cc) {
> +		/*
> +		 * If the subchannel is status pending or
> +		 * if a function is in progress,
> +		 * we consider both cases as errors.
> +		 */
> +		report_info("msch: sch %08x failed with cc=%d", schid, cc);
> +		return cc;
> +	}
> +
> +	/*
> +	 * Read the SCHIB again to verify the measurement block origin
> +	 */
> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch: updating sch %08x failed with cc=%d",
> +			    schid, cc);
> +		return cc;
> +	}
> +
> +	if (schib.mbo == mb) {
> +		report_info("stsch: sch %08x successfully modified after %d retries",
> +			    schid, retry_count);
> +		return 0;
> +	}
> +
> +	if (retry_count++ < MAX_ENABLE_RETRIES) {
> +		mdelay(10); /* the hardware was not ready, give it some time */
> +		goto retry;
> +	}
> +
> +	report_info("msch: modifying sch %08x failed after %d retries. pmcw flags: %04x",
> +		    schid, retry_count, pmcw->flags);
> +	return -1;
> +}
> +
>  static struct irb irb;
>  
>  void css_irq_io(void)

(...)

Re: [kvm-unit-tests PATCH v1 3/5] s390x: css: implementing Set CHannel Monitor

[Pierre Morel]

On 2/2/21 12:48 PM, Cornelia Huck wrote:
> On Fri, 29 Jan 2021 15:34:27 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We implement the call of the Set CHannel Monitor instruction,
>> starting the monitoring of the all Channel Sub System, and
> 
> "initializing channel subsystem monitoring" ?

Yes, better I take this.

> 
>> the initialization of the monitoring on a Sub Channel.
> 
> "enabling monitoring for a subchannel" ?
> 
>>
>> An initial test reports the presence of the extended measurement
>> block feature.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     | 17 +++++++++-
>>   lib/s390x/css_lib.c | 77 +++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/css.c         |  7 +++++
>>   3 files changed, 100 insertions(+), 1 deletion(-)
>>
> 
> (...)
> 
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index f300969..9e0f568 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -205,6 +205,83 @@ retry:
>>   	return -1;
>>   }
>>   
>> +/*
>> + * css_enable_mb: enable the subchannel Mesurement Block
>> + * @schid: Subchannel Identifier
>> + * @mb   : 64bit address of the measurement block
>> + * @format1: set if format 1 is to be used
>> + * @mbi : the measurement block offset
>> + * @flags : PMCW_MBUE to enable measurement block update
>> + *	    PMCW_DCTME to enable device connect time
>> + * Return value:
>> + *   On success: 0
>> + *   On error the CC of the faulty instruction
>> + *      or -1 if the retry count is exceeded.
>> + */
>> +int css_enable_mb(int schid, uint64_t mb, int format1, uint16_t mbi,
>> +		  uint16_t flags)
>> +{
>> +	struct pmcw *pmcw = &schib.pmcw;
>> +	int retry_count = 0;
>> +	int cc;
>> +
>> +	/* Read the SCHIB for this subchannel */
>> +	cc = stsch(schid, &schib);
>> +	if (cc) {
>> +		report_info("stsch: sch %08x failed with cc=%d", schid, cc);
>> +		return cc;
>> +	}
>> +
>> +retry:
>> +	/* Update the SCHIB to enable the measurement block */
>> +	pmcw->flags |= flags;
>> +
>> +	if (format1)
>> +		pmcw->flags2 |= PMCW_MBF1;
>> +	else
>> +		pmcw->flags2 &= ~PMCW_MBF1;
>> +
>> +	pmcw->mbi = mbi;
>> +	schib.mbo = mb;
>> +
>> +	/* Tell the CSS we want to modify the subchannel */
>> +	cc = msch(schid, &schib);
> 
> Setting some invalid flags for measurements in the schib could lead to
> an operand exception. Do we want to rely on the caller always getting
> it right, or should we add handling for those invalid flags? (Might
> also make a nice test case.)

Yes it does.
I add new test cases to test if we get the right error.


Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 3/5] s390x: css: implementing Set CHannel Monitor

[Thomas Huth]

On 29/01/2021 15.34, Pierre Morel wrote:
> We implement the call of the Set CHannel Monitor instruction,
> starting the monitoring of the all Channel Sub System, and
> the initialization of the monitoring on a Sub Channel.
> 
> An initial test reports the presence of the extended measurement
> block feature.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/css.h     | 17 +++++++++-
>   lib/s390x/css_lib.c | 77 +++++++++++++++++++++++++++++++++++++++++++++
>   s390x/css.c         |  7 +++++
>   3 files changed, 100 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index f8bfa37..938f0ab 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -82,6 +82,7 @@ struct pmcw {
>   	uint32_t intparm;
>   #define PMCW_DNV	0x0001
>   #define PMCW_ENABLE	0x0080
> +#define PMCW_MBUE	0x0010
>   #define PMCW_ISC_MASK	0x3800
>   #define PMCW_ISC_SHIFT	11
>   	uint16_t flags;
> @@ -94,6 +95,7 @@ struct pmcw {
>   	uint8_t  pom;
>   	uint8_t  pam;
>   	uint8_t  chpid[8];
> +#define PMCW_MBF1	0x0004
>   	uint32_t flags2;
>   };
>   #define PMCW_CHANNEL_TYPE(pmcw) (pmcw->flags2 >> 21)
> @@ -101,7 +103,8 @@ struct pmcw {
>   struct schib {
>   	struct pmcw pmcw;
>   	struct scsw scsw;
> -	uint8_t  md[12];
> +	uint64_t mbo;
> +	uint8_t  md[4];
>   } __attribute__ ((aligned(4)));
>   
>   struct irb {
> @@ -305,6 +308,7 @@ struct chsc_scsc {
>   	u32 reserved3;
>   	struct chsc_header res;
>   	u32 format;
> +#define CSSC_EXTENDED_MEASUREMENT_BLOCK 48
>   	u64 general_char[255];
>   	u64 chsc_char[254];
>   };
> @@ -346,4 +350,15 @@ static inline int chsc(void *p, uint16_t code, uint16_t len)
>   #define css_general_feature(bit) test_bit_inv(bit, chsc_scsc->general_char)
>   #define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
>   
> +static inline void schm(void *mbo, unsigned int flags)
> +{
> +	register void *__gpr2 asm("2") = mbo;
> +	register long __gpr1 asm("1") = flags;
> +
> +	asm("schm" : : "d" (__gpr2), "d" (__gpr1));
> +}
> +
> +int css_enable_mb(int schid, uint64_t mb, int format1, uint16_t mbi,
> +		  uint16_t flags);
> +
>   #endif
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index f300969..9e0f568 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -205,6 +205,83 @@ retry:
>   	return -1;
>   }
>   
> +/*
> + * css_enable_mb: enable the subchannel Mesurement Block
> + * @schid: Subchannel Identifier
> + * @mb   : 64bit address of the measurement block
> + * @format1: set if format 1 is to be used
> + * @mbi : the measurement block offset
> + * @flags : PMCW_MBUE to enable measurement block update
> + *	    PMCW_DCTME to enable device connect time
> + * Return value:
> + *   On success: 0
> + *   On error the CC of the faulty instruction
> + *      or -1 if the retry count is exceeded.
> + */
> +int css_enable_mb(int schid, uint64_t mb, int format1, uint16_t mbi,
> +		  uint16_t flags)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int retry_count = 0;
> +	int cc;
> +
> +	/* Read the SCHIB for this subchannel */
> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch: sch %08x failed with cc=%d", schid, cc);
> +		return cc;
> +	}
> +
> +retry:
> +	/* Update the SCHIB to enable the measurement block */
> +	pmcw->flags |= flags;
> +
> +	if (format1)
> +		pmcw->flags2 |= PMCW_MBF1;
> +	else
> +		pmcw->flags2 &= ~PMCW_MBF1;
> +
> +	pmcw->mbi = mbi;
> +	schib.mbo = mb;
> +
> +	/* Tell the CSS we want to modify the subchannel */
> +	cc = msch(schid, &schib);
> +	if (cc) {
> +		/*
> +		 * If the subchannel is status pending or
> +		 * if a function is in progress,
> +		 * we consider both cases as errors.
> +		 */
> +		report_info("msch: sch %08x failed with cc=%d", schid, cc);
> +		return cc;
> +	}
> +
> +	/*
> +	 * Read the SCHIB again to verify the measurement block origin
> +	 */
> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch: updating sch %08x failed with cc=%d",
> +			    schid, cc);
> +		return cc;
> +	}
> +
> +	if (schib.mbo == mb) {
> +		report_info("stsch: sch %08x successfully modified after %d retries",
> +			    schid, retry_count);
> +		return 0;
> +	}
> +
> +	if (retry_count++ < MAX_ENABLE_RETRIES) {
> +		mdelay(10); /* the hardware was not ready, give it some time */
> +		goto retry;
> +	}

"goto retries" are always a good indication that you likely should use a 
proper loop instead. And maybe put the code in the loop body into a separate 
function?

  Thomas

Re: [kvm-unit-tests PATCH v1 3/5] s390x: css: implementing Set CHannel Monitor

[Pierre Morel]

On 2/2/21 6:32 PM, Thomas Huth wrote:
> On 29/01/2021 15.34, Pierre Morel wrote:
>> We implement the call of the Set CHannel Monitor instruction,
>> starting the monitoring of the all Channel Sub System, and
>> the initialization of the monitoring on a Sub Channel.
>>
>> An initial test reports the presence of the extended measurement
>> block feature.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     | 17 +++++++++-
>>   lib/s390x/css_lib.c | 77 +++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/css.c         |  7 +++++
>>   3 files changed, 100 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index f8bfa37..938f0ab 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -82,6 +82,7 @@ struct pmcw {
>>       uint32_t intparm;
>>   #define PMCW_DNV    0x0001
>>   #define PMCW_ENABLE    0x0080
>> +#define PMCW_MBUE    0x0010
>>   #define PMCW_ISC_MASK    0x3800
>>   #define PMCW_ISC_SHIFT    11
>>       uint16_t flags;
>> @@ -94,6 +95,7 @@ struct pmcw {
>>       uint8_t  pom;
>>       uint8_t  pam;
>>       uint8_t  chpid[8];
>> +#define PMCW_MBF1    0x0004
>>       uint32_t flags2;
>>   };
>>   #define PMCW_CHANNEL_TYPE(pmcw) (pmcw->flags2 >> 21)
>> @@ -101,7 +103,8 @@ struct pmcw {
>>   struct schib {
>>       struct pmcw pmcw;
>>       struct scsw scsw;
>> -    uint8_t  md[12];
>> +    uint64_t mbo;
>> +    uint8_t  md[4];
>>   } __attribute__ ((aligned(4)));
>>   struct irb {
>> @@ -305,6 +308,7 @@ struct chsc_scsc {
>>       u32 reserved3;
>>       struct chsc_header res;
>>       u32 format;
>> +#define CSSC_EXTENDED_MEASUREMENT_BLOCK 48
>>       u64 general_char[255];
>>       u64 chsc_char[254];
>>   };
>> @@ -346,4 +350,15 @@ static inline int chsc(void *p, uint16_t code, 
>> uint16_t len)
>>   #define css_general_feature(bit) test_bit_inv(bit, 
>> chsc_scsc->general_char)
>>   #define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
>> +static inline void schm(void *mbo, unsigned int flags)
>> +{
>> +    register void *__gpr2 asm("2") = mbo;
>> +    register long __gpr1 asm("1") = flags;
>> +
>> +    asm("schm" : : "d" (__gpr2), "d" (__gpr1));
>> +}
>> +
>> +int css_enable_mb(int schid, uint64_t mb, int format1, uint16_t mbi,
>> +          uint16_t flags);
>> +
>>   #endif
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index f300969..9e0f568 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -205,6 +205,83 @@ retry:
>>       return -1;
>>   }
>> +/*
>> + * css_enable_mb: enable the subchannel Mesurement Block
>> + * @schid: Subchannel Identifier
>> + * @mb   : 64bit address of the measurement block
>> + * @format1: set if format 1 is to be used
>> + * @mbi : the measurement block offset
>> + * @flags : PMCW_MBUE to enable measurement block update
>> + *        PMCW_DCTME to enable device connect time
>> + * Return value:
>> + *   On success: 0
>> + *   On error the CC of the faulty instruction
>> + *      or -1 if the retry count is exceeded.
>> + */
>> +int css_enable_mb(int schid, uint64_t mb, int format1, uint16_t mbi,
>> +          uint16_t flags)
>> +{
>> +    struct pmcw *pmcw = &schib.pmcw;
>> +    int retry_count = 0;
>> +    int cc;
>> +
>> +    /* Read the SCHIB for this subchannel */
>> +    cc = stsch(schid, &schib);
>> +    if (cc) {
>> +        report_info("stsch: sch %08x failed with cc=%d", schid, cc);
>> +        return cc;
>> +    }
>> +
>> +retry:
>> +    /* Update the SCHIB to enable the measurement block */
>> +    pmcw->flags |= flags;
>> +
>> +    if (format1)
>> +        pmcw->flags2 |= PMCW_MBF1;
>> +    else
>> +        pmcw->flags2 &= ~PMCW_MBF1;
>> +
>> +    pmcw->mbi = mbi;
>> +    schib.mbo = mb;
>> +
>> +    /* Tell the CSS we want to modify the subchannel */
>> +    cc = msch(schid, &schib);
>> +    if (cc) {
>> +        /*
>> +         * If the subchannel is status pending or
>> +         * if a function is in progress,
>> +         * we consider both cases as errors.
>> +         */
>> +        report_info("msch: sch %08x failed with cc=%d", schid, cc);
>> +        return cc;
>> +    }
>> +
>> +    /*
>> +     * Read the SCHIB again to verify the measurement block origin
>> +     */
>> +    cc = stsch(schid, &schib);
>> +    if (cc) {
>> +        report_info("stsch: updating sch %08x failed with cc=%d",
>> +                schid, cc);
>> +        return cc;
>> +    }
>> +
>> +    if (schib.mbo == mb) {
>> +        report_info("stsch: sch %08x successfully modified after %d 
>> retries",
>> +                schid, retry_count);
>> +        return 0;
>> +    }
>> +
>> +    if (retry_count++ < MAX_ENABLE_RETRIES) {
>> +        mdelay(10); /* the hardware was not ready, give it some time */
>> +        goto retry;
>> +    }
> 
> "goto retries" are always a good indication that you likely should use a 
> proper loop instead. And maybe put the code in the loop body into a 
> separate function?

Yes, thanks, I do so.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 5/5] s390x: css: testing measurement block format 1

[Cornelia Huck]

On Fri, 29 Jan 2021 15:34:29 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Measurement block format 1 is made available by the extended
> mesurement block facility and is indicated in the SCHIB by
> the bit in the PMCW.
> 
> The MBO is specified in the SCHIB of each channel and the MBO
> defined by the SCHM instruction is ignored.
> 
> The test of the MB format 1 is just skip if the feature is

s/skip/skipped/

> not available.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/css.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 

(...)

> @@ -208,6 +251,14 @@ static void test_schm(void)
>  	schm(mb0, SCHM_MBU);
>  	test_schm_fmt0(mb0);
>  
> +	mb1 = alloc_io_mem(sizeof(struct measurement_block_format1), 0);
> +	if (!mb1) {

I'm wondering whether you should move this into the format 1
invocation, so you won't try to allocate memory if format 1 is not even
supported.

> +		report(0, "mesurement_block_format0 allocation");

In any case, s/mesurement_block_format0/measurement_block_format1/ :)

> +		goto end_free;
> +	}
> +	test_schm_fmt1(mb1);
> +
> +	free_io_mem(mb1, sizeof(struct measurement_block_format1));
>  end_free:
>  	free_io_mem(mb0, sizeof(struct measurement_block_format0));
>  }

Re: [kvm-unit-tests PATCH v1 1/5] s390x: css: Store CSS Characteristics

[Janosch Frank]

On 1/29/21 3:34 PM, Pierre Morel wrote:
> CSS characteristics exposes the features of the Channel SubSystem.
> Let's use Store Channel Subsystem Characteristics to retrieve
> the features of the CSS.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     | 57 +++++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/css_lib.c | 50 ++++++++++++++++++++++++++++++++++++++-
>  s390x/css.c         | 12 ++++++++++
>  3 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 3e57445..bc0530d 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -288,4 +288,61 @@ int css_residual_count(unsigned int schid);
>  void enable_io_isc(uint8_t isc);
>  int wait_and_check_io_completion(int schid);
>  
> +/*
> + * CHSC definitions
> + */
> +struct chsc_header {
> +	u16 len;
> +	u16 code;
> +};
> +
> +/* Store Channel Subsystem Characteristics */
> +struct chsc_scsc {
> +	struct chsc_header req;
> +	u32 reserved1;
> +	u32 reserved2;
> +	u32 reserved3;

Array?

> +	struct chsc_header res;
> +	u32 format;
> +	u64 general_char[255];
> +	u64 chsc_char[254];
> +};
> +extern struct chsc_scsc *chsc_scsc;
> +
> +#define CSS_GENERAL_FEAT_BITLEN	(255 * 64)
> +#define CSS_CHSC_FEAT_BITLEN	(254 * 64)
> +
> +int get_chsc_scsc(void);
> +
> +static inline int _chsc(void *p)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"	.insn   rre,0xb25f0000,%2,0\n"
> +		"	ipm     %0\n"
> +		"	srl     %0,28\n"
> +		: "=d" (cc), "=m" (p)
> +		: "d" (p), "m" (p)
> +		: "cc");
> +
> +	return cc;
> +}
> +
> +#define CHSC_SCSC	0x0010
> +#define CHSC_SCSC_LEN	0x0010
> +
> +static inline int chsc(void *p, uint16_t code, uint16_t len)
> +{
> +	struct chsc_header *h = p;
> +
> +	h->code = code;
> +	h->len = len;
> +	return _chsc(p);
> +}
> +
> +#include <bitops.h>
> +#define css_general_feature(bit) test_bit_inv(bit, chsc_scsc->general_char)
> +#define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
> +
>  #endif
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 3c24480..fe05021 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -15,11 +15,59 @@
>  #include <asm/arch_def.h>
>  #include <asm/time.h>
>  #include <asm/arch_def.h>
> -
> +#include <alloc_page.h>
>  #include <malloc_io.h>
>  #include <css.h>
>  
>  static struct schib schib;
> +struct chsc_scsc *chsc_scsc;
> +
> +int get_chsc_scsc(void)
> +{
> +	int i, n;
> +	int ret = 0;
> +	char buffer[510];
> +	char *p;
> +
> +	report_prefix_push("Channel Subsystem Call");
> +
> +	if (chsc_scsc) {
> +		report_info("chsc_scsc already initialized");
> +		goto end;
> +	}
> +
> +	chsc_scsc = alloc_pages(0);

alloc_page() ?

> +	report_info("scsc_scsc at: %016lx", (u64)chsc_scsc);
> +	if (!chsc_scsc) {
> +		ret = -1;
> +		report(0, "could not allocate chsc_scsc page!");
> +		goto end;
> +	}
> +
> +	ret = chsc(chsc_scsc, CHSC_SCSC, CHSC_SCSC_LEN);
> +	if (ret) {
> +		report(0, "chsc: CC %d", ret);
> +		goto end;
> +	}
> +
> +	for (i = 0, p = buffer; i < CSS_GENERAL_FEAT_BITLEN; i++)
> +		if (css_general_feature(i)) {
> +			n = snprintf(p, sizeof(buffer) - ret, "%d,", i);
> +			p += n;
> +		}
> +	report_info("General features: %s", buffer);
> +
> +	for (i = 0, p = buffer, ret = 0; i < CSS_CHSC_FEAT_BITLEN; i++)
> +		if (css_chsc_feature(i)) {
> +			n = snprintf(p, sizeof(buffer) - ret, "%d,", i);
> +			p += n;
> +		}
> +	report_info("CHSC features: %s", buffer);
> +
> +end:
> +	report_prefix_pop();
> +	return ret;
> +}
>  
>  /*
>   * css_enumerate:
> diff --git a/s390x/css.c b/s390x/css.c
> index 1a61a5c..18dbf01 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -14,6 +14,7 @@
>  #include <string.h>
>  #include <interrupt.h>
>  #include <asm/arch_def.h>
> +#include <alloc_page.h>
>  
>  #include <malloc_io.h>
>  #include <css.h>
> @@ -140,10 +141,21 @@ error_senseid:
>  	unregister_io_int_func(css_irq_io);
>  }
>  
> +static void css_init(void)
> +{
> +	int ret;
> +
> +	ret = get_chsc_scsc();
> +	if (!ret)
> +		report(1, " ");
> +}
> +
>  static struct {
>  	const char *name;
>  	void (*func)(void);
>  } tests[] = {
> +	/* The css_init test is needed to initialize the CSS Characteristics */
> +	{ "initialize CSS (chsc)", css_init },
>  	{ "enumerate (stsch)", test_enumerate },
>  	{ "enable (msch)", test_enable },
>  	{ "sense (ssch/tsch)", test_sense },
> 

Re: [kvm-unit-tests PATCH v1 1/5] s390x: css: Store CSS Characteristics

[Pierre Morel]

On 2/1/21 11:01 AM, Janosch Frank wrote:
> On 1/29/21 3:34 PM, Pierre Morel wrote:
>> CSS characteristics exposes the features of the Channel SubSystem.

...

>> +/* Store Channel Subsystem Characteristics */
>> +struct chsc_scsc {
>> +	struct chsc_header req;
>> +	u32 reserved1;
>> +	u32 reserved2;
>> +	u32 reserved3;
> 
> Array?

OK

...snip...

>> +int get_chsc_scsc(void)
>> +{
>> +	int i, n;
>> +	int ret = 0;
>> +	char buffer[510];
>> +	char *p;
>> +
>> +	report_prefix_push("Channel Subsystem Call");
>> +
>> +	if (chsc_scsc) {
>> +		report_info("chsc_scsc already initialized");
>> +		goto end;
>> +	}
>> +
>> +	chsc_scsc = alloc_pages(0);
> 
> alloc_page() ?

OK too


Thanks for the comments,
Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 1/5] s390x: css: Store CSS Characteristics

[Cornelia Huck]

On Fri, 29 Jan 2021 15:34:25 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> CSS characteristics exposes the features of the Channel SubSystem.
> Let's use Store Channel Subsystem Characteristics to retrieve
> the features of the CSS.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     | 57 +++++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/css_lib.c | 50 ++++++++++++++++++++++++++++++++++++++-
>  s390x/css.c         | 12 ++++++++++
>  3 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 3e57445..bc0530d 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -288,4 +288,61 @@ int css_residual_count(unsigned int schid);
>  void enable_io_isc(uint8_t isc);
>  int wait_and_check_io_completion(int schid);
>  
> +/*
> + * CHSC definitions
> + */
> +struct chsc_header {
> +	u16 len;
> +	u16 code;
> +};
> +
> +/* Store Channel Subsystem Characteristics */
> +struct chsc_scsc {
> +	struct chsc_header req;
> +	u32 reserved1;
> +	u32 reserved2;
> +	u32 reserved3;
> +	struct chsc_header res;
> +	u32 format;
> +	u64 general_char[255];
> +	u64 chsc_char[254];

Both the kernel and QEMU use arrays of 32 bit values to model that. Not
a problem, just a stumbling block when comparing code :)

> +};
> +extern struct chsc_scsc *chsc_scsc;
> +
> +#define CSS_GENERAL_FEAT_BITLEN	(255 * 64)
> +#define CSS_CHSC_FEAT_BITLEN	(254 * 64)
> +
> +int get_chsc_scsc(void);
> +
> +static inline int _chsc(void *p)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"	.insn   rre,0xb25f0000,%2,0\n"
> +		"	ipm     %0\n"
> +		"	srl     %0,28\n"
> +		: "=d" (cc), "=m" (p)
> +		: "d" (p), "m" (p)
> +		: "cc");
> +
> +	return cc;
> +}
> +
> +#define CHSC_SCSC	0x0010
> +#define CHSC_SCSC_LEN	0x0010
> +
> +static inline int chsc(void *p, uint16_t code, uint16_t len)
> +{
> +	struct chsc_header *h = p;
> +
> +	h->code = code;
> +	h->len = len;
> +	return _chsc(p);
> +}

I'm wondering how useful this function is. For store channel subsystem
characteristics, you indeed only need to fill in code and len, but
other commands may need more fields filled out in the header, and
filling in code and len is not really extra work. I guess it depends
whether you plan to add more commands in the future.

Also maybe move the definitions to the actual invocation of scsc?

> +
> +#include <bitops.h>
> +#define css_general_feature(bit) test_bit_inv(bit, chsc_scsc->general_char)
> +#define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
> +
>  #endif
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 3c24480..fe05021 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -15,11 +15,59 @@
>  #include <asm/arch_def.h>
>  #include <asm/time.h>
>  #include <asm/arch_def.h>
> -
> +#include <alloc_page.h>
>  #include <malloc_io.h>
>  #include <css.h>
>  
>  static struct schib schib;
> +struct chsc_scsc *chsc_scsc;
> +
> +int get_chsc_scsc(void)
> +{
> +	int i, n;
> +	int ret = 0;
> +	char buffer[510];
> +	char *p;
> +
> +	report_prefix_push("Channel Subsystem Call");
> +
> +	if (chsc_scsc) {
> +		report_info("chsc_scsc already initialized");
> +		goto end;
> +	}
> +
> +	chsc_scsc = alloc_pages(0);
> +	report_info("scsc_scsc at: %016lx", (u64)chsc_scsc);
> +	if (!chsc_scsc) {
> +		ret = -1;
> +		report(0, "could not allocate chsc_scsc page!");
> +		goto end;
> +	}
> +
> +	ret = chsc(chsc_scsc, CHSC_SCSC, CHSC_SCSC_LEN);
> +	if (ret) {
> +		report(0, "chsc: CC %d", ret);
> +		goto end;
> +	}

Shouldn't you check the response code in the chsc area as well?

> +
> +	for (i = 0, p = buffer; i < CSS_GENERAL_FEAT_BITLEN; i++)
> +		if (css_general_feature(i)) {
> +			n = snprintf(p, sizeof(buffer) - ret, "%d,", i);
> +			p += n;
> +		}
> +	report_info("General features: %s", buffer);
> +
> +	for (i = 0, p = buffer, ret = 0; i < CSS_CHSC_FEAT_BITLEN; i++)
> +		if (css_chsc_feature(i)) {
> +			n = snprintf(p, sizeof(buffer) - ret, "%d,", i);
> +			p += n;
> +		}
> +	report_info("CHSC features: %s", buffer);
> +
> +end:
> +	report_prefix_pop();
> +	return ret;
> +}
>  
>  /*
>   * css_enumerate:

(...)

Re: [kvm-unit-tests PATCH v1 1/5] s390x: css: Store CSS Characteristics

[Pierre Morel]

On 2/2/21 12:11 PM, Cornelia Huck wrote:
> On Fri, 29 Jan 2021 15:34:25 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> CSS characteristics exposes the features of the Channel SubSystem.
>> Let's use Store Channel Subsystem Characteristics to retrieve
>> the features of the CSS.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     | 57 +++++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/css_lib.c | 50 ++++++++++++++++++++++++++++++++++++++-
>>   s390x/css.c         | 12 ++++++++++
>>   3 files changed, 118 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 3e57445..bc0530d 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -288,4 +288,61 @@ int css_residual_count(unsigned int schid);
>>   void enable_io_isc(uint8_t isc);
>>   int wait_and_check_io_completion(int schid);
>>   
>> +/*
>> + * CHSC definitions
>> + */
>> +struct chsc_header {
>> +	u16 len;
>> +	u16 code;
>> +};
>> +
>> +/* Store Channel Subsystem Characteristics */
>> +struct chsc_scsc {
>> +	struct chsc_header req;
>> +	u32 reserved1;
>> +	u32 reserved2;
>> +	u32 reserved3;
>> +	struct chsc_header res;
>> +	u32 format;
>> +	u64 general_char[255];
>> +	u64 chsc_char[254];
> 
> Both the kernel and QEMU use arrays of 32 bit values to model that. Not
> a problem, just a stumbling block when comparing code :)

This spares a devilish cast when using test_bit_inv, so I prefer to keep 
it like this since you seem OK with it.

> 
>> +};
>> +extern struct chsc_scsc *chsc_scsc;
>> +
>> +#define CSS_GENERAL_FEAT_BITLEN	(255 * 64)
>> +#define CSS_CHSC_FEAT_BITLEN	(254 * 64)
>> +
>> +int get_chsc_scsc(void);
>> +
>> +static inline int _chsc(void *p)
>> +{
>> +	int cc;
>> +
>> +	asm volatile(
>> +		"	.insn   rre,0xb25f0000,%2,0\n"
>> +		"	ipm     %0\n"
>> +		"	srl     %0,28\n"
>> +		: "=d" (cc), "=m" (p)
>> +		: "d" (p), "m" (p)
>> +		: "cc");
>> +
>> +	return cc;
>> +}
>> +
>> +#define CHSC_SCSC	0x0010
>> +#define CHSC_SCSC_LEN	0x0010
>> +
>> +static inline int chsc(void *p, uint16_t code, uint16_t len)
>> +{
>> +	struct chsc_header *h = p;
>> +
>> +	h->code = code;
>> +	h->len = len;
>> +	return _chsc(p);
>> +}
> 
> I'm wondering how useful this function is. For store channel subsystem
> characteristics, you indeed only need to fill in code and len, but
> other commands may need more fields filled out in the header, and
> filling in code and len is not really extra work. I guess it depends
> whether you plan to add more commands in the future.

It is different levels, CHSC instruction is the support for the 
different commands.
That is why I prefer to separate CHSC from the command's handling.

After your comment on the check of the response code, I will expand this 
function with response code check and report.


> 
> Also maybe move the definitions to the actual invocation of scsc?

Yes.

> 
>> +
>> +#include <bitops.h>
>> +#define css_general_feature(bit) test_bit_inv(bit, chsc_scsc->general_char)
>> +#define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
>> +
>>   #endif
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index 3c24480..fe05021 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -15,11 +15,59 @@
>>   #include <asm/arch_def.h>
>>   #include <asm/time.h>
>>   #include <asm/arch_def.h>
>> -
>> +#include <alloc_page.h>
>>   #include <malloc_io.h>
>>   #include <css.h>
>>   
>>   static struct schib schib;
>> +struct chsc_scsc *chsc_scsc;
>> +
>> +int get_chsc_scsc(void)
>> +{
>> +	int i, n;
>> +	int ret = 0;
>> +	char buffer[510];
>> +	char *p;
>> +
>> +	report_prefix_push("Channel Subsystem Call");
>> +
>> +	if (chsc_scsc) {
>> +		report_info("chsc_scsc already initialized");
>> +		goto end;
>> +	}
>> +
>> +	chsc_scsc = alloc_pages(0);
>> +	report_info("scsc_scsc at: %016lx", (u64)chsc_scsc);
>> +	if (!chsc_scsc) {
>> +		ret = -1;
>> +		report(0, "could not allocate chsc_scsc page!");
>> +		goto end;
>> +	}
>> +
>> +	ret = chsc(chsc_scsc, CHSC_SCSC, CHSC_SCSC_LEN);
>> +	if (ret) {
>> +		report(0, "chsc: CC %d", ret);
>> +		goto end;
>> +	}
> 
> Shouldn't you check the response code in the chsc area as well?

yes, I should... I will.

I will rework the chsc() function to add the response code check.

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 1/5] s390x: css: Store CSS Characteristics

[Thomas Huth]

On 29/01/2021 15.34, Pierre Morel wrote:
> CSS characteristics exposes the features of the Channel SubSystem.
> Let's use Store Channel Subsystem Characteristics to retrieve
> the features of the CSS.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/css.h     | 57 +++++++++++++++++++++++++++++++++++++++++++++
>   lib/s390x/css_lib.c | 50 ++++++++++++++++++++++++++++++++++++++-
>   s390x/css.c         | 12 ++++++++++
>   3 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 3e57445..bc0530d 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -288,4 +288,61 @@ int css_residual_count(unsigned int schid);
>   void enable_io_isc(uint8_t isc);
>   int wait_and_check_io_completion(int schid);
>   
> +/*
> + * CHSC definitions
> + */
> +struct chsc_header {
> +	u16 len;
> +	u16 code;
> +};
> +
> +/* Store Channel Subsystem Characteristics */
> +struct chsc_scsc {
> +	struct chsc_header req;
> +	u32 reserved1;
> +	u32 reserved2;
> +	u32 reserved3;
> +	struct chsc_header res;
> +	u32 format;
> +	u64 general_char[255];
> +	u64 chsc_char[254];
> +};
> +extern struct chsc_scsc *chsc_scsc;
> +
> +#define CSS_GENERAL_FEAT_BITLEN	(255 * 64)
> +#define CSS_CHSC_FEAT_BITLEN	(254 * 64)
> +
> +int get_chsc_scsc(void);
> +
> +static inline int _chsc(void *p)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"	.insn   rre,0xb25f0000,%2,0\n"
> +		"	ipm     %0\n"
> +		"	srl     %0,28\n"
> +		: "=d" (cc), "=m" (p)
> +		: "d" (p), "m" (p)
> +		: "cc");
> +
> +	return cc;
> +}
> +
> +#define CHSC_SCSC	0x0010
> +#define CHSC_SCSC_LEN	0x0010
> +
> +static inline int chsc(void *p, uint16_t code, uint16_t len)
> +{
> +	struct chsc_header *h = p;
> +
> +	h->code = code;
> +	h->len = len;
> +	return _chsc(p);
> +}
> +
> +#include <bitops.h>
> +#define css_general_feature(bit) test_bit_inv(bit, chsc_scsc->general_char)
> +#define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
> +
>   #endif
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 3c24480..fe05021 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -15,11 +15,59 @@
>   #include <asm/arch_def.h>
>   #include <asm/time.h>
>   #include <asm/arch_def.h>
> -
> +#include <alloc_page.h>
>   #include <malloc_io.h>
>   #include <css.h>
>   
>   static struct schib schib;
> +struct chsc_scsc *chsc_scsc;
> +
> +int get_chsc_scsc(void)
> +{
> +	int i, n;
> +	int ret = 0;
> +	char buffer[510];
> +	char *p;
> +
> +	report_prefix_push("Channel Subsystem Call");
> +
> +	if (chsc_scsc) {
> +		report_info("chsc_scsc already initialized");
> +		goto end;
> +	}
> +
> +	chsc_scsc = alloc_pages(0);
> +	report_info("scsc_scsc at: %016lx", (u64)chsc_scsc);
> +	if (!chsc_scsc) {
> +		ret = -1;
> +		report(0, "could not allocate chsc_scsc page!");
> +		goto end;
> +	}
> +
> +	ret = chsc(chsc_scsc, CHSC_SCSC, CHSC_SCSC_LEN);
> +	if (ret) {
> +		report(0, "chsc: CC %d", ret);
> +		goto end;
> +	}
> +
> +	for (i = 0, p = buffer; i < CSS_GENERAL_FEAT_BITLEN; i++)
> +		if (css_general_feature(i)) {
> +			n = snprintf(p, sizeof(buffer) - ret, "%d,", i);
> +			p += n;
> +		}
> +	report_info("General features: %s", buffer);
> +
> +	for (i = 0, p = buffer, ret = 0; i < CSS_CHSC_FEAT_BITLEN; i++)
> +		if (css_chsc_feature(i)) {
> +			n = snprintf(p, sizeof(buffer) - ret, "%d,", i);
> +			p += n;
> +		}

Please use curly braces for the for-loops here, too. Rationale: Kernel 
coding style:

"Also, use braces when a loop contains more than a single simple statement"

  Thanks,
   Thomas

Re: [kvm-unit-tests PATCH v1 1/5] s390x: css: Store CSS Characteristics

[Pierre Morel]

On 2/2/21 6:25 PM, Thomas Huth wrote:
> On 29/01/2021 15.34, Pierre Morel wrote:
>> CSS characteristics exposes the features of the Channel SubSystem.
>> Let's use Store Channel Subsystem Characteristics to retrieve
>> the features of the CSS.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     | 57 +++++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/css_lib.c | 50 ++++++++++++++++++++++++++++++++++++++-
>>   s390x/css.c         | 12 ++++++++++
>>   3 files changed, 118 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 3e57445..bc0530d 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -288,4 +288,61 @@ int css_residual_count(unsigned int schid);
>>   void enable_io_isc(uint8_t isc);
>>   int wait_and_check_io_completion(int schid);
>> +/*
>> + * CHSC definitions
>> + */
>> +struct chsc_header {
>> +    u16 len;
>> +    u16 code;
>> +};
>> +
>> +/* Store Channel Subsystem Characteristics */
>> +struct chsc_scsc {
>> +    struct chsc_header req;
>> +    u32 reserved1;
>> +    u32 reserved2;
>> +    u32 reserved3;
>> +    struct chsc_header res;
>> +    u32 format;
>> +    u64 general_char[255];
>> +    u64 chsc_char[254];
>> +};
>> +extern struct chsc_scsc *chsc_scsc;
>> +
>> +#define CSS_GENERAL_FEAT_BITLEN    (255 * 64)
>> +#define CSS_CHSC_FEAT_BITLEN    (254 * 64)
>> +
>> +int get_chsc_scsc(void);
>> +
>> +static inline int _chsc(void *p)
>> +{
>> +    int cc;
>> +
>> +    asm volatile(
>> +        "    .insn   rre,0xb25f0000,%2,0\n"
>> +        "    ipm     %0\n"
>> +        "    srl     %0,28\n"
>> +        : "=d" (cc), "=m" (p)
>> +        : "d" (p), "m" (p)
>> +        : "cc");
>> +
>> +    return cc;
>> +}
>> +
>> +#define CHSC_SCSC    0x0010
>> +#define CHSC_SCSC_LEN    0x0010
>> +
>> +static inline int chsc(void *p, uint16_t code, uint16_t len)
>> +{
>> +    struct chsc_header *h = p;
>> +
>> +    h->code = code;
>> +    h->len = len;
>> +    return _chsc(p);
>> +}
>> +
>> +#include <bitops.h>
>> +#define css_general_feature(bit) test_bit_inv(bit, 
>> chsc_scsc->general_char)
>> +#define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
>> +
>>   #endif
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index 3c24480..fe05021 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -15,11 +15,59 @@
>>   #include <asm/arch_def.h>
>>   #include <asm/time.h>
>>   #include <asm/arch_def.h>
>> -
>> +#include <alloc_page.h>
>>   #include <malloc_io.h>
>>   #include <css.h>
>>   static struct schib schib;
>> +struct chsc_scsc *chsc_scsc;
>> +
>> +int get_chsc_scsc(void)
>> +{
>> +    int i, n;
>> +    int ret = 0;
>> +    char buffer[510];
>> +    char *p;
>> +
>> +    report_prefix_push("Channel Subsystem Call");
>> +
>> +    if (chsc_scsc) {
>> +        report_info("chsc_scsc already initialized");
>> +        goto end;
>> +    }
>> +
>> +    chsc_scsc = alloc_pages(0);
>> +    report_info("scsc_scsc at: %016lx", (u64)chsc_scsc);
>> +    if (!chsc_scsc) {
>> +        ret = -1;
>> +        report(0, "could not allocate chsc_scsc page!");
>> +        goto end;
>> +    }
>> +
>> +    ret = chsc(chsc_scsc, CHSC_SCSC, CHSC_SCSC_LEN);
>> +    if (ret) {
>> +        report(0, "chsc: CC %d", ret);
>> +        goto end;
>> +    }
>> +
>> +    for (i = 0, p = buffer; i < CSS_GENERAL_FEAT_BITLEN; i++)
>> +        if (css_general_feature(i)) {
>> +            n = snprintf(p, sizeof(buffer) - ret, "%d,", i);
>> +            p += n;
>> +        }
>> +    report_info("General features: %s", buffer);
>> +
>> +    for (i = 0, p = buffer, ret = 0; i < CSS_CHSC_FEAT_BITLEN; i++)
>> +        if (css_chsc_feature(i)) {
>> +            n = snprintf(p, sizeof(buffer) - ret, "%d,", i);
>> +            p += n;
>> +        }
> 
> Please use curly braces for the for-loops here, too. Rationale: Kernel 
> coding style:
> 
> "Also, use braces when a loop contains more than a single simple statement"

OK, is better to read.

Thanks,
Pierre


> 
>   Thanks,
>    Thomas
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 2/5] s390x: css: simplifications of the tests

[Janosch Frank]

On 1/29/21 3:34 PM, Pierre Morel wrote:
> In order to ease the writing of tests based on:
> - interrupt
> - enabling a subchannel
> - using multiple I/O on a channel without disabling it
> 
> We do the following simplifications:
> - the I/O interrupt handler is registered on CSS initialization
> - We do not enable again a subchannel in senseid if it is already
>   enabled
> - we add a css_enabled() function to test if a subchannel is enabled
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  1 +
>  lib/s390x/css_lib.c | 37 ++++++++++++++++++++++++----------
>  s390x/css.c         | 49 +++++++++++++++++++++++++--------------------
>  3 files changed, 54 insertions(+), 33 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index bc0530d..f8bfa37 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -278,6 +278,7 @@ int css_enumerate(void);
>  
>  #define IO_SCH_ISC      3
>  int css_enable(int schid, int isc);
> +bool css_enabled(int schid);
>  
>  /* Library functions */
>  int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw);
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index fe05021..f300969 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -118,6 +118,31 @@ out:
>  	return schid;
>  }
>  
> +/*
> + * css_enable: enable the subchannel with the specified ISC

enabled or enable?

I.e. do you test if it is enabled or do you want to enable it.

> + * @schid: Subchannel Identifier
> + * Return value:
> + *   true if the subchannel is enabled
> + *   false otherwise
> + */
> +bool css_enabled(int schid)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int cc;
> +
> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch: updating sch %08x failed with cc=%d",
> +			    schid, cc);
> +		return false;
> +	}
> +
> +	if (!(pmcw->flags & PMCW_ENABLE)) {
> +		report_info("stsch: sch %08x not enabled", schid);
> +		return 0;

Please stay with true/false or change the return type to int and use ints.

> +	}
> +	return true;
> +}
>  /*
>   * css_enable: enable the subchannel with the specified ISC
>   * @schid: Subchannel Identifier
> @@ -167,18 +192,8 @@ retry:
>  	/*
>  	 * Read the SCHIB again to verify the enablement
>  	 */
> -	cc = stsch(schid, &schib);
> -	if (cc) {
> -		report_info("stsch: updating sch %08x failed with cc=%d",
> -			    schid, cc);
> -		return cc;
> -	}
> -
> -	if ((pmcw->flags & flags) == flags) {
> -		report_info("stsch: sch %08x successfully modified after %d retries",
> -			    schid, retry_count);
> +	if (css_enabled(schid))
>  		return 0;
> -	}
>  
>  	if (retry_count++ < MAX_ENABLE_RETRIES) {
>  		mdelay(10); /* the hardware was not ready, give it some time */
> diff --git a/s390x/css.c b/s390x/css.c
> index 18dbf01..230f819 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -56,36 +56,27 @@ static void test_enable(void)
>   * - We need the test device as the first recognized
>   *   device by the enumeration.
>   */
> -static void test_sense(void)
> +static bool do_test_sense(void)
>  {
>  	struct ccw1 *ccw;
> +	bool retval = false;
>  	int ret;
>  	int len;
>  
>  	if (!test_device_sid) {
>  		report_skip("No device");
> -		return;
> +		return retval;
>  	}
>  
> -	ret = css_enable(test_device_sid, IO_SCH_ISC);
> -	if (ret) {
> -		report(0, "Could not enable the subchannel: %08x",
> -		       test_device_sid);
> -		return;
> +	if (!css_enabled(test_device_sid)) {
> +		report(0, "enabled subchannel: %08x", test_device_sid);
> +		return retval;
>  	}
>  
> -	ret = register_io_int_func(css_irq_io);
> -	if (ret) {
> -		report(0, "Could not register IRQ handler");
> -		return;
> -	}
> -
> -	lowcore_ptr->io_int_param = 0;
> -
>  	senseid = alloc_io_mem(sizeof(*senseid), 0);
>  	if (!senseid) {
>  		report(0, "Allocation of senseid");
> -		goto error_senseid;
> +		return retval;
>  	}
>  
>  	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
> @@ -129,16 +120,21 @@ static void test_sense(void)
>  	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
>  		    senseid->reserved, senseid->cu_type, senseid->cu_model,
>  		    senseid->dev_type, senseid->dev_model);
> +	report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type,
> +		    senseid->cu_type);
>  
> -	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
> -	       (uint16_t)cu_type, senseid->cu_type);
> +	retval = senseid->cu_type == cu_type;
>  
>  error:
>  	free_io_mem(ccw, sizeof(*ccw));
>  error_ccw:
>  	free_io_mem(senseid, sizeof(*senseid));
> -error_senseid:
> -	unregister_io_int_func(css_irq_io);
> +	return retval;

Could you return senseid->cu_type == cu_type here?

> +}
> +
> +static void test_sense(void)
> +{
> +	report(do_test_sense(), "Got CU type expected");
>  }
>  
>  static void css_init(void)
> @@ -146,8 +142,17 @@ static void css_init(void)
>  	int ret;
>  
>  	ret = get_chsc_scsc();
> -	if (!ret)
> -		report(1, " ");
> +	if (ret)
> +		return;
> +
> +	ret = register_io_int_func(css_irq_io);
> +	if (ret) {
> +		report(0, "Could not register IRQ handler");
> +		return;
> +	}
> +	lowcore_ptr->io_int_param = 0;
> +
> +	report(1, "CSS initialized");
>  }
>  
>  static struct {
> 

Re: [kvm-unit-tests PATCH v1 2/5] s390x: css: simplifications of the tests

[Pierre Morel]

On 2/1/21 11:11 AM, Janosch Frank wrote:
> On 1/29/21 3:34 PM, Pierre Morel wrote:

...snip...

>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index fe05021..f300969 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -118,6 +118,31 @@ out:
>>   	return schid;
>>   }
>>   
>> +/*
>> + * css_enable: enable the subchannel with the specified ISC
> 
> enabled or enable?

Forgot to change the cut and paste :(

/*
  * css_enabled: report if the sub channel is enabled

> 
> I.e. do you test if it is enabled or do you want to enable it.
> 
>> + * @schid: Subchannel Identifier
>> + * Return value:
>> + *   true if the subchannel is enabled
>> + *   false otherwise
>> + */
>> +bool css_enabled(int schid)
>> +{
>> +	struct pmcw *pmcw = &schib.pmcw;
>> +	int cc;
>> +
>> +	cc = stsch(schid, &schib);
>> +	if (cc) {
>> +		report_info("stsch: updating sch %08x failed with cc=%d",
>> +			    schid, cc);
>> +		return false;
>> +	}
>> +
>> +	if (!(pmcw->flags & PMCW_ENABLE)) {
>> +		report_info("stsch: sch %08x not enabled", schid);
>> +		return 0;
> 
> Please stay with true/false or change the return type to int and use ints.

yes

> 
>> +	}
>> +	return true;
>> +}

...snip...

>> @@ -129,16 +120,21 @@ static void test_sense(void)
>>   	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
>>   		    senseid->reserved, senseid->cu_type, senseid->cu_model,
>>   		    senseid->dev_type, senseid->dev_model);
>> +	report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type,
>> +		    senseid->cu_type);
>>   
>> -	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
>> -	       (uint16_t)cu_type, senseid->cu_type);
>> +	retval = senseid->cu_type == cu_type;
>>   
>>   error:
>>   	free_io_mem(ccw, sizeof(*ccw));
>>   error_ccw:
>>   	free_io_mem(senseid, sizeof(*senseid));
>> -error_senseid:
>> -	unregister_io_int_func(css_irq_io);
>> +	return retval;
> 
> Could you return senseid->cu_type == cu_type here?

It would work for the current code and DEFAULT_CU_TYPE.
But I do not think it is a good idea due to the goto we have in error 
case before I find that it makes the code less understandable.

Other opinion?

Thanks for the comments,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 2/5] s390x: css: simplifications of the tests

[Janosch Frank]

On 2/1/21 1:15 PM, Pierre Morel wrote:
> 
> 
> On 2/1/21 11:11 AM, Janosch Frank wrote:
>> On 1/29/21 3:34 PM, Pierre Morel wrote:
> 
> ...snip...
> 
>>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>>> index fe05021..f300969 100644
>>> --- a/lib/s390x/css_lib.c
>>> +++ b/lib/s390x/css_lib.c
>>> @@ -118,6 +118,31 @@ out:
>>>   	return schid;
>>>   }
>>>   
>>> +/*
>>> + * css_enable: enable the subchannel with the specified ISC
>>
>> enabled or enable?
> 
> Forgot to change the cut and paste :(
> 
> /*
>   * css_enabled: report if the sub channel is enabled
> 
>>
>> I.e. do you test if it is enabled or do you want to enable it.
>>
>>> + * @schid: Subchannel Identifier
>>> + * Return value:
>>> + *   true if the subchannel is enabled
>>> + *   false otherwise
>>> + */
>>> +bool css_enabled(int schid)
>>> +{
>>> +	struct pmcw *pmcw = &schib.pmcw;
>>> +	int cc;
>>> +
>>> +	cc = stsch(schid, &schib);
>>> +	if (cc) {
>>> +		report_info("stsch: updating sch %08x failed with cc=%d",
>>> +			    schid, cc);
>>> +		return false;
>>> +	}
>>> +
>>> +	if (!(pmcw->flags & PMCW_ENABLE)) {
>>> +		report_info("stsch: sch %08x not enabled", schid);
>>> +		return 0;
>>
>> Please stay with true/false or change the return type to int and use ints.
> 
> yes
> 
>>
>>> +	}
>>> +	return true;
>>> +}
> 
> ...snip...
> 
>>> @@ -129,16 +120,21 @@ static void test_sense(void)
>>>   	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
>>>   		    senseid->reserved, senseid->cu_type, senseid->cu_model,
>>>   		    senseid->dev_type, senseid->dev_model);
>>> +	report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type,
>>> +		    senseid->cu_type);
>>>   
>>> -	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
>>> -	       (uint16_t)cu_type, senseid->cu_type);
>>> +	retval = senseid->cu_type == cu_type;
>>>   
>>>   error:
>>>   	free_io_mem(ccw, sizeof(*ccw));
>>>   error_ccw:
>>>   	free_io_mem(senseid, sizeof(*senseid));
>>> -error_senseid:
>>> -	unregister_io_int_func(css_irq_io);
>>> +	return retval;
>>
>> Could you return senseid->cu_type == cu_type here?
> 
> It would work for the current code and DEFAULT_CU_TYPE.
> But I do not think it is a good idea due to the goto we have in error 
> case before I find that it makes the code less understandable.
> 
> Other opinion?

That's why I asked, it wasn't a direct suggestion.
I'm not a big fan of the ret/retval naming but the function is short so
it should be ok.

> 
> Thanks for the comments,
> Pierre
> 

Re: [kvm-unit-tests PATCH v1 2/5] s390x: css: simplifications of the tests

[Cornelia Huck]

On Fri, 29 Jan 2021 15:34:26 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> In order to ease the writing of tests based on:
> - interrupt
> - enabling a subchannel
> - using multiple I/O on a channel without disabling it
> 
> We do the following simplifications:
> - the I/O interrupt handler is registered on CSS initialization
> - We do not enable again a subchannel in senseid if it is already
>   enabled
> - we add a css_enabled() function to test if a subchannel is enabled
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  1 +
>  lib/s390x/css_lib.c | 37 ++++++++++++++++++++++++----------
>  s390x/css.c         | 49 +++++++++++++++++++++++++--------------------
>  3 files changed, 54 insertions(+), 33 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index bc0530d..f8bfa37 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -278,6 +278,7 @@ int css_enumerate(void);
>  
>  #define IO_SCH_ISC      3
>  int css_enable(int schid, int isc);
> +bool css_enabled(int schid);
>  
>  /* Library functions */
>  int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw);
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index fe05021..f300969 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -118,6 +118,31 @@ out:
>  	return schid;
>  }
>  
> +/*
> + * css_enable: enable the subchannel with the specified ISC
> + * @schid: Subchannel Identifier
> + * Return value:
> + *   true if the subchannel is enabled
> + *   false otherwise
> + */
> +bool css_enabled(int schid)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int cc;
> +
> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch: updating sch %08x failed with cc=%d",
> +			    schid, cc);
> +		return false;
> +	}
> +
> +	if (!(pmcw->flags & PMCW_ENABLE)) {
> +		report_info("stsch: sch %08x not enabled", schid);
> +		return 0;
> +	}
> +	return true;
> +}
>  /*
>   * css_enable: enable the subchannel with the specified ISC
>   * @schid: Subchannel Identifier
> @@ -167,18 +192,8 @@ retry:
>  	/*
>  	 * Read the SCHIB again to verify the enablement
>  	 */
> -	cc = stsch(schid, &schib);
> -	if (cc) {
> -		report_info("stsch: updating sch %08x failed with cc=%d",
> -			    schid, cc);
> -		return cc;
> -	}
> -
> -	if ((pmcw->flags & flags) == flags) {
> -		report_info("stsch: sch %08x successfully modified after %d retries",
> -			    schid, retry_count);
> +	if (css_enabled(schid))

This is a slightly different test now. Previously, you also checked
whether the ISC matched the requested one. Not sure how valuable that
test was.

>  		return 0;
> -	}
>  
>  	if (retry_count++ < MAX_ENABLE_RETRIES) {
>  		mdelay(10); /* the hardware was not ready, give it some time */
> diff --git a/s390x/css.c b/s390x/css.c
> index 18dbf01..230f819 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -56,36 +56,27 @@ static void test_enable(void)
>   * - We need the test device as the first recognized
>   *   device by the enumeration.
>   */
> -static void test_sense(void)
> +static bool do_test_sense(void)
>  {
>  	struct ccw1 *ccw;
> +	bool retval = false;
>  	int ret;
>  	int len;
>  
>  	if (!test_device_sid) {
>  		report_skip("No device");
> -		return;
> +		return retval;
>  	}
>  
> -	ret = css_enable(test_device_sid, IO_SCH_ISC);
> -	if (ret) {
> -		report(0, "Could not enable the subchannel: %08x",
> -		       test_device_sid);
> -		return;
> +	if (!css_enabled(test_device_sid)) {
> +		report(0, "enabled subchannel: %08x", test_device_sid);

Isn't that a _not_ enabled subchannel?

> +		return retval;
>  	}
>  
> -	ret = register_io_int_func(css_irq_io);
> -	if (ret) {
> -		report(0, "Could not register IRQ handler");
> -		return;
> -	}
> -
> -	lowcore_ptr->io_int_param = 0;
> -
>  	senseid = alloc_io_mem(sizeof(*senseid), 0);
>  	if (!senseid) {
>  		report(0, "Allocation of senseid");
> -		goto error_senseid;
> +		return retval;
>  	}
>  
>  	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
> @@ -129,16 +120,21 @@ static void test_sense(void)
>  	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
>  		    senseid->reserved, senseid->cu_type, senseid->cu_model,
>  		    senseid->dev_type, senseid->dev_model);
> +	report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type,
> +		    senseid->cu_type);
>  
> -	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
> -	       (uint16_t)cu_type, senseid->cu_type);
> +	retval = senseid->cu_type == cu_type;
>  
>  error:
>  	free_io_mem(ccw, sizeof(*ccw));
>  error_ccw:
>  	free_io_mem(senseid, sizeof(*senseid));
> -error_senseid:
> -	unregister_io_int_func(css_irq_io);
> +	return retval;
> +}
> +
> +static void test_sense(void)
> +{
> +	report(do_test_sense(), "Got CU type expected");
>  }
>  
>  static void css_init(void)
> @@ -146,8 +142,17 @@ static void css_init(void)
>  	int ret;
>  
>  	ret = get_chsc_scsc();
> -	if (!ret)
> -		report(1, " ");
> +	if (ret)
> +		return;

Shouldn't you report a failure here?

> +
> +	ret = register_io_int_func(css_irq_io);
> +	if (ret) {
> +		report(0, "Could not register IRQ handler");
> +		return;
> +	}
> +	lowcore_ptr->io_int_param = 0;
> +
> +	report(1, "CSS initialized");
>  }
>  
>  static struct {

Re: [kvm-unit-tests PATCH v1 2/5] s390x: css: simplifications of the tests

[Pierre Morel]

On 2/2/21 12:23 PM, Cornelia Huck wrote:
> On Fri, 29 Jan 2021 15:34:26 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
...snip...

>> +bool css_enabled(int schid)
>> +{
>> +	struct pmcw *pmcw = &schib.pmcw;
>> +	int cc;
>> +
>> +	cc = stsch(schid, &schib);
>> +	if (cc) {
>> +		report_info("stsch: updating sch %08x failed with cc=%d",
>> +			    schid, cc);
>> +		return false;
>> +	}
>> +
>> +	if (!(pmcw->flags & PMCW_ENABLE)) {
>> +		report_info("stsch: sch %08x not enabled", schid);
>> +		return 0;
>> +	}
>> +	return true;
>> +}
>>   /*
>>    * css_enable: enable the subchannel with the specified ISC
>>    * @schid: Subchannel Identifier
>> @@ -167,18 +192,8 @@ retry:
>>   	/*
>>   	 * Read the SCHIB again to verify the enablement
>>   	 */
>> -	cc = stsch(schid, &schib);
>> -	if (cc) {
>> -		report_info("stsch: updating sch %08x failed with cc=%d",
>> -			    schid, cc);
>> -		return cc;
>> -	}
>> -
>> -	if ((pmcw->flags & flags) == flags) {
>> -		report_info("stsch: sch %08x successfully modified after %d retries",
>> -			    schid, retry_count);
>> +	if (css_enabled(schid))
> 
> This is a slightly different test now. Previously, you also checked
> whether the ISC matched the requested one. Not sure how valuable that
> test was.

Yes, I do not think it can be anything else when the CSS accept to 
enable the subchannel.

...
>>   
>> -	ret = css_enable(test_device_sid, IO_SCH_ISC);
>> -	if (ret) {
>> -		report(0, "Could not enable the subchannel: %08x",
>> -		       test_device_sid);
>> -		return;
>> +	if (!css_enabled(test_device_sid)) {
>> +		report(0, "enabled subchannel: %08x", test_device_sid);
> 
> Isn't that a _not_ enabled subchannel?

:) yes, may be:

s/enabled/enabling/


...snip...

>>   
>>   static void css_init(void)
>> @@ -146,8 +142,17 @@ static void css_init(void)
>>   	int ret;
>>   
>>   	ret = get_chsc_scsc();
>> -	if (!ret)
>> -		report(1, " ");
>> +	if (ret)
>> +		return;
> 
> Shouldn't you report a failure here?

Clearly yes.


Thanks for the comments,
regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 2/5] s390x: css: simplifications of the tests

[Thomas Huth]

On 29/01/2021 15.34, Pierre Morel wrote:
> In order to ease the writing of tests based on:
> - interrupt
> - enabling a subchannel
> - using multiple I/O on a channel without disabling it
> 
> We do the following simplifications:
> - the I/O interrupt handler is registered on CSS initialization
> - We do not enable again a subchannel in senseid if it is already
>    enabled
> - we add a css_enabled() function to test if a subchannel is enabled
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
[...]
> diff --git a/s390x/css.c b/s390x/css.c
> index 18dbf01..230f819 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -56,36 +56,27 @@ static void test_enable(void)
>    * - We need the test device as the first recognized
>    *   device by the enumeration.
>    */
> -static void test_sense(void)
> +static bool do_test_sense(void)
>   {
>   	struct ccw1 *ccw;
> +	bool retval = false;
>   	int ret;
>   	int len;
>   
>   	if (!test_device_sid) {
>   		report_skip("No device");
> -		return;
> +		return retval;
>   	}
>   
> -	ret = css_enable(test_device_sid, IO_SCH_ISC);
> -	if (ret) {
> -		report(0, "Could not enable the subchannel: %08x",
> -		       test_device_sid);
> -		return;
> +	if (!css_enabled(test_device_sid)) {
> +		report(0, "enabled subchannel: %08x", test_device_sid);
> +		return retval;
>   	}
>   
> -	ret = register_io_int_func(css_irq_io);
> -	if (ret) {
> -		report(0, "Could not register IRQ handler");
> -		return;
> -	}
> -
> -	lowcore_ptr->io_int_param = 0;
> -
>   	senseid = alloc_io_mem(sizeof(*senseid), 0);
>   	if (!senseid) {
>   		report(0, "Allocation of senseid");
> -		goto error_senseid;
> +		return retval;
>   	}
>   
>   	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
> @@ -129,16 +120,21 @@ static void test_sense(void)
>   	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
>   		    senseid->reserved, senseid->cu_type, senseid->cu_model,
>   		    senseid->dev_type, senseid->dev_model);
> +	report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type,
> +		    senseid->cu_type);
>   
> -	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
> -	       (uint16_t)cu_type, senseid->cu_type);
> +	retval = senseid->cu_type == cu_type;
>   
>   error:
>   	free_io_mem(ccw, sizeof(*ccw));
>   error_ccw:
>   	free_io_mem(senseid, sizeof(*senseid));
> -error_senseid:
> -	unregister_io_int_func(css_irq_io);
> +	return retval;
> +}

Maybe use "success" as a name for the variable instead of "retval"? ... 
since it's a boolean value...

  Thomas

Re: [kvm-unit-tests PATCH v1 2/5] s390x: css: simplifications of the tests

[Pierre Morel]

On 2/2/21 6:29 PM, Thomas Huth wrote:
> On 29/01/2021 15.34, Pierre Morel wrote:
>> In order to ease the writing of tests based on:
...snip...
>>   error_ccw:
>>       free_io_mem(senseid, sizeof(*senseid));
>> -error_senseid:
>> -    unregister_io_int_func(css_irq_io);
>> +    return retval;
>> +}
> 
> Maybe use "success" as a name for the variable instead of "retval"? ... 
> since it's a boolean value...
yes, I do, thanks.
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 4/5] s390x: css: SCHM tests format 0

[Thomas Huth]

On 29/01/2021 15.34, Pierre Morel wrote:
> We tests the update of the mesurement block format 0, the
> mesurement block origin is calculated from the mbo argument
> used by the SCHM instruction and the offset calculated using
> the measurement block index of the SCHIB.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/css.h | 29 ++++++++++++++++++++++++++++
>   s390x/css.c     | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 80 insertions(+)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 938f0ab..ba0bc67 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -358,7 +358,36 @@ static inline void schm(void *mbo, unsigned int flags)
>   	asm("schm" : : "d" (__gpr2), "d" (__gpr1));
>   }
>   
> +#define SCHM_DCTM	1 /* activate Device Connection TiMe */
> +#define SCHM_MBU	2 /* activate Measurement Block Update */
> +
>   int css_enable_mb(int schid, uint64_t mb, int format1, uint16_t mbi,
>   		  uint16_t flags);
>   
> +struct measurement_block_format0 {
> +	uint16_t ssch_rsch_count;
> +	uint16_t sample_count;
> +	uint32_t device_connect_time;
> +	uint32_t function_pending_time;
> +	uint32_t device_disconnect_time;
> +	uint32_t cu_queuing_time;
> +	uint32_t device_active_only_time;
> +	uint32_t device_busy_time;
> +	uint32_t initial_cmd_resp_time;
> +};
> +
> +struct measurement_block_format1 {
> +	uint32_t ssch_rsch_count;
> +	uint32_t sample_count;
> +	uint32_t device_connect_time;
> +	uint32_t function_pending_time;
> +	uint32_t device_disconnect_time;
> +	uint32_t cu_queuing_time;
> +	uint32_t device_active_only_time;
> +	uint32_t device_busy_time;
> +	uint32_t initial_cmd_resp_time;
> +	uint32_t irq_delay_time;
> +	uint32_t irq_prio_delay_time;
> +};
> +
>   #endif
> diff --git a/s390x/css.c b/s390x/css.c
> index 000ce04..2e9ea47 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -155,10 +155,61 @@ static void css_init(void)
>   	report(1, "CSS initialized");
>   }
>   
> +#define SCHM_UPDATE_CNT 10
> +static void test_schm_fmt0(struct measurement_block_format0 *mb0)
> +{
> +	int ret;
> +	int i;
> +
> +	report_prefix_push("Format 0");
> +	ret = css_enable_mb(test_device_sid, 0, 0, 0, PMCW_MBUE);
> +	if (ret) {
> +		report(0, "Enabling measurement_block_format0");
> +		goto end;
> +	}
> +
> +	for (i = 0; i < SCHM_UPDATE_CNT; i++) {
> +		if (!do_test_sense()) {
> +			report(0, "Error during sense");
> +			break;
> +		}
> +	}
> +
> +	report_info("ssch_rsch_count: %d", mb0->ssch_rsch_count);
> +	report_info("sample_count: %d", mb0->sample_count);
> +	report_info("device_connect_time: %d", mb0->device_connect_time);
> +	report_info("function_pending_time: %d", mb0->function_pending_time);
> +	report_info("device_disconnect_time: %d", mb0->device_disconnect_time);
> +	report_info("cu_queuing_time: %d", mb0->cu_queuing_time);
> +	report_info("device_active_only_time: %d", mb0->device_active_only_time);
> +	report_info("device_busy_time: %d", mb0->device_busy_time);
> +	report_info("initial_cmd_resp_time: %d", mb0->initial_cmd_resp_time);
> +
> +	report(i == mb0->ssch_rsch_count,
> +	       "SSCH expected %d measured %d", i, mb0->ssch_rsch_count);
> +
> +end:
> +	report_prefix_pop();
> +}
> +
>   static void test_schm(void)
>   {
> +	struct measurement_block_format0 *mb0;
> +
>   	if (css_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK))
>   		report_info("Extended measurement block available");
> +
> +	mb0 = alloc_io_mem(sizeof(struct measurement_block_format0), 0);
> +	if (!mb0) {
> +		report(0, "measurement_block_format0 allocation");
> +		goto end_free;

If allocation failed, there is certainly no need to try to free it, so you 
can get rid of the goto and the label here and return directly instead. Or 
maybe
Maybe also simply use report_abort() in this case?

  Thomas


> +	}
> +
> +	schm(mb0, SCHM_MBU);
> +	test_schm_fmt0(mb0);
> +
> +end_free:
> +	free_io_mem(mb0, sizeof(struct measurement_block_format0));
>   }
>   
>   static struct {
> 

Re: [kvm-unit-tests PATCH v1 4/5] s390x: css: SCHM tests format 0

[Pierre Morel]

On 2/2/21 6:35 PM, Thomas Huth wrote:
> On 29/01/2021 15.34, Pierre Morel wrote:
...snip...
>>   static void test_schm(void)
>>   {
>> +    struct measurement_block_format0 *mb0;
>> +
>>       if (css_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK))
>>           report_info("Extended measurement block available");
>> +
>> +    mb0 = alloc_io_mem(sizeof(struct measurement_block_format0), 0);
>> +    if (!mb0) {
>> +        report(0, "measurement_block_format0 allocation");
>> +        goto end_free;
> 
> If allocation failed, there is certainly no need to try to free it, so 

:) yes

> you can get rid of the goto and the label here and return directly 
> instead. Or maybe
> Maybe also simply use report_abort() in this case?

OK, report_abort when an allocation failed seems right.

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen