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

[Thomas Huth <thuth@redhat.com>]

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