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

From: Pierre Morel <pmorel@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.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 16:51:53 +0100	[thread overview]
Message-ID: <1b3b3b05-6961-d217-b398-d7a41ef4910a@linux.ibm.com> (raw)
In-Reply-To: <20210212115307.627abe8a.cohuck@redhat.com>

On 2/12/21 11:53 AM, Cornelia Huck wrote:
> 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?

yes, I change for one of these solutions, thanks.

> 
>>
>> 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()?

yes, it is dedicated.

> 
>> +{
>> +	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?

Yes, I can add the disabling of the measurement on a channel.
In the test I disable it for the system but we may need this later.

> 
>> +
>> +	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"?

right, "Read the SCHIB again"

> 
>> +
>> +	return true;
>> +}
>> +
> 
> (...)
> 
> Otherwise, LGTM.
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen