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

From: Cornelia Huck <cohuck@redhat.com>
To: Pierre Morel <pmorel@linux.ibm.com>
Cc: kvm@vger.kernel.org, linux-s390@vger.kernel.org,
	frankja@linux.ibm.com, david@redhat.com, thuth@redhat.com,
	imbrenda@linux.ibm.com
Subject: Re: [kvm-unit-tests PATCH v2 3/5] s390x: css: implementing Set CHannel Monitor
Date: Fri, 12 Feb 2021 11:53:07 +0100	[thread overview]
Message-ID: <20210212115307.627abe8a.cohuck@redhat.com> (raw)
In-Reply-To: <1612963214-30397-4-git-send-email-pmorel@linux.ibm.com>

On Wed, 10 Feb 2021 14:20:12 +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.
> 
> An initial test reports the presence of the extended measurement
> block feature.
> 
> Several tests on SCHM verify the error reporting of the hypervisor.

Combine these two into one sentence?

"Initial tests report the presence of the extended measurement block
feature, and verify the error reporting of the hypervisor for SCHM."

Also, you add the infrastructure for enabling measurements at the
subchannel -- either mention this in the patch description or move it
to a separate patch or the first user?

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     | 19 +++++++++++-
>  lib/s390x/css_lib.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/css.c         | 36 ++++++++++++++++++++++
>  3 files changed, 128 insertions(+), 1 deletion(-)
> 

(...)

> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 5426a6b..355881d 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -267,6 +267,80 @@ retry:
>  	return -1;
>  }
>  
> +static bool schib_update(int schid, uint64_t mb, uint16_t mbi, uint16_t flags,
> +		  bool format1)

Maybe schib_update_mb()?

> +{
> +	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 */
> +	pmcw->flags |= flags;

Do we also want to be able to disable it again?

> +
> +	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 false;
> +	}
> +
> +	/*
> +	 * 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 false;
> +	}

Hm, you only do the stsch, but do not check the result (that is done by
the caller) -- remove the misleading comment or replace it with "Read
the SCHIB again"?

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

(...)

Otherwise, LGTM.