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

Pierre Morel
2021-02-12T15:52:45Z

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