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

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 18 Feb 2021 18:26:42 +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.
> 
> Initial tests report the presence of the extended measurement block
> feature, and verify the error reporting of the hypervisor for SCHM.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  21 +++++++++-
>  lib/s390x/css_lib.c | 100 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/css.c         |  35 ++++++++++++++++
>  3 files changed, 155 insertions(+), 1 deletion(-)

(...)

> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 65b58ff..4c8a6ae 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -265,6 +265,106 @@ retry:
>  	return -1;
>  }
>  
> +/*
> + * schib_update_mb: update the subchannel Mesurement Block

s/Mesurement/Measurement/

I guess that one is hard to get out of one's fingers :)

> + * @schid: Subchannel Identifier
> + * @mb   : 64bit address of the measurement block
> + * @mbi : the measurement block offset
> + * @flags : PMCW_MBUE to enable measurement block update
> + *	    PMCW_DCTME to enable device connect time
> + *	    0 to disable measurement
> + * @format1: set if format 1 is to be used
> + */
> +static bool schib_update_mb(int schid, uint64_t mb, uint16_t mbi,
> +			    uint16_t flags, bool format1)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	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 false;
> +	}
> +
> +	/* Update the SCHIB to enable the measurement block */
> +	if (flags) {
> +		pmcw->flags |= flags;
> +
> +		if (format1)
> +			pmcw->flags2 |= PMCW_MBF1;
> +		else
> +			pmcw->flags2 &= ~PMCW_MBF1;
> +
> +		pmcw->mbi = mbi;
> +		schib.mbo = mb;
> +	} else {
> +		pmcw->flags &= ~(PMCW_MBUE | PMCW_DCTME);
> +	}
> +
> +	/* 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 false;
> +	}
> +
> +	/*
> +	 * Read the SCHIB again
> +	 */
> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch: updating sch %08x failed with cc=%d",
> +			    schid, cc);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +/*
> + * css_enable_mb: enable the subchannel Mesurement Block

s/Mesurement/Measurement/

> + * @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
> + */
> +bool css_enable_mb(int schid, uint64_t mb, uint16_t mbi, uint16_t flags,
> +		   bool format1)
> +{
> +	int retry_count = MAX_ENABLE_RETRIES;
> +	struct pmcw *pmcw = &schib.pmcw;
> +
> +	while (retry_count-- &&
> +	       !schib_update_mb(schid, mb, mbi, flags, format1))
> +		mdelay(10); /* the hardware was not ready, give it some time */
> +
> +	return schib.mbo == mb && pmcw->mbi == mbi;
> +}
> +
> +/*
> + * css_disable_mb: enable the subchannel Mesurement Block

s/enable/disable/
s/Mesurement/Measurement/

> + * @schid: Subchannel Identifier
> + */
> +bool css_disable_mb(int schid)
> +{
> +	int retry_count = MAX_ENABLE_RETRIES;
> +
> +	while (retry_count-- &&
> +	       !schib_update_mb(schid, 0, 0, 0, 0))
> +		mdelay(10); /* the hardware was not ready, give it some time */
> +
> +	return retry_count > 0;
> +}
> +
>  static struct irb irb;
>  
>  void css_irq_io(void)

(...)

I'd still have split out the subchannel-modifying functions into a
separate patch, but no strong opinion.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>