Re: [kvm-unit-tests PATCH v3 4/5] s390x: css: testing measurement block format 0

[Pierre Morel <pmorel@linux.ibm.com>]

On 2/23/21 2:27 PM, Cornelia Huck wrote:
> On Thu, 18 Feb 2021 18:26:43 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We test the update of the measurement block format 0, the
>> measurement 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 | 12 +++++++++
>>   s390x/css.c     | 67 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 79 insertions(+)
>>
> 
> (...)
> 
>> diff --git a/s390x/css.c b/s390x/css.c
>> index fc693f3..b65aa89 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -191,6 +191,72 @@ static void test_schm(void)
>>   	report_prefix_pop();
>>   }
>>   
>> +#define SCHM_UPDATE_CNT 10
>> +static bool start_measure(uint64_t mbo, uint16_t mbi, bool fmt1)
> 
> Maybe "start_measuring"? Or "start_measurements"?

OK

> 
>> +{
>> +	int i;
>> +
>> +	if (!css_enable_mb(test_device_sid, mbo, mbi, PMCW_MBUE, fmt1)) {
>> +		report(0, "Enabling measurement_block_format");
>> +		return false;
>> +	}
>> +
>> +	for (i = 0; i < SCHM_UPDATE_CNT; i++) {
>> +		if (!do_test_sense()) {
>> +			report(0, "Error during sense");
>> +			return false;
>> +		}
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +/*
>> + * test_schm_fmt0:
>> + * With measurement block format 0 a memory space is shared
>> + * by all subchannels, each subchannel can provide an index
>> + * for the measurement block facility to store the measures.
> 
> s/measures/measurements/

yes

> 
>> + */
>> +static void test_schm_fmt0(void)
>> +{
>> +	struct measurement_block_format0 *mb0;
>> +	int shared_mb_size = 2 * sizeof(struct measurement_block_format0);
>> +
>> +	report_prefix_push("Format 0");
>> +
>> +	/* Allocate zeroed Measurement block */
>> +	mb0 = alloc_io_mem(shared_mb_size, 0);
>> +	if (!mb0) {
>> +		report_abort("measurement_block_format0 allocation failed");
>> +		goto end;
>> +	}
>> +
>> +	schm(NULL, 0); /* Stop any previous measurement */
> 
> Probably not strictly needed, but cannot hurt.
yes

> 
>> +	schm(mb0, SCHM_MBU);
>> +
>> +	/* Expect success */
>> +	report_prefix_push("Valid MB address and index 0");
>> +	report(start_measure(0, 0, false) &&
>> +	       mb0->ssch_rsch_count == SCHM_UPDATE_CNT,
>> +	       "SSCH measured %d", mb0->ssch_rsch_count);
>> +	report_prefix_pop();
>> +
>> +	/* Clear the measurement block for the next test */
>> +	memset(mb0, 0, shared_mb_size);
>> +
>> +	/* Expect success */
>> +	report_prefix_push("Valid MB address and index 1");
>> +	report(start_measure(0, 1, false) &&
>> +	       mb0[1].ssch_rsch_count == SCHM_UPDATE_CNT,
>> +	       "SSCH measured %d", mb0[1].ssch_rsch_count);
>> +	report_prefix_pop();
>> +
>> +	schm(NULL, 0); /* Stop the measurement */
> 
> Shouldn't you call css_disable_mb() here as well?

I do not think it is obligatory, measurements are stopped but it may be 
indeed better so we get a clean SCHIB.
So yes,

     css_disable_mb();
     schm(NULL, 0);

seems the right thing to do.


Thanks,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen