Re: [kvm-unit-tests PATCH v11 8/9] s390x: css: msch, enable test

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2020-07-09 15:52, Cornelia Huck wrote:
On Thu, 9 Jul 2020 15:41:56 +0200
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:

On 2020-07-09 15:30, Cornelia Huck wrote:
On Thu, 9 Jul 2020 15:12:05 +0200
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:
On 2020-07-09 13:40, Cornelia Huck wrote:
On Thu,  9 Jul 2020 10:07:47 +0200
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:
A second step when testing the channel subsystem is to prepare a channel
for use.
This includes:
- Get the current subchannel Information Block (SCHIB) using STSCH
- Update it in memory to set the ENABLE bit and the specified ISC
- Tell the CSS that the SCHIB has been modified using MSCH
- Get the SCHIB from the CSS again to verify that the subchannel is
     enabled and uses the specified ISC.
- If the command succeeds but subchannel is not enabled or the ISC
     field is not as expected, retry a predefined retries count.
- If the command fails, report the failure and do not retry, even
     if cc indicates a busy/status pending as we do not expect this.

This tests the MSCH instruction to enable a channel successfully.
Retries are done and in case of error, and if the retries count
is exceeded, a report is made.

Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
Acked-by: Thomas Huth <thuth@xxxxxxxxxx>
Reviewed-by: Cornelia Huck <cohuck@xxxxxxxxxx>
---
    lib/s390x/css.h     |  8 +++--
    lib/s390x/css_lib.c | 72 +++++++++++++++++++++++++++++++++++++++++++++
    s390x/css.c         | 15 ++++++++++
    3 files changed, 92 insertions(+), 3 deletions(-)

(...)
+/*
+ * css_msch: enable subchannel and set with specified ISC

"css_enable: enable the subchannel with the specified ISC"

?
+ * @schid: Subchannel Identifier
+ * @isc  : number of the interruption subclass to use
+ * Return value:
+ *   On success: 0
+ *   On error the CC of the faulty instruction
+ *      or -1 if the retry count is exceeded.
+ */
+int css_enable(int schid, int isc)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int retry_count = 0;
+	uint16_t flags;
+	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 cc;
+	}
+
+	flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT);
+	if ((pmcw->flags & flags) == flags) {

I think you want (pmcw->flags & PMCW_ENABLE) == PMCW_ENABLE -- this
catches the case of "subchannel has been enabled before, but with a
different isc".

If with a different ISC, we need to modify the ISC.
Don't we ?

I think that's a policy decision (I would probably fail and require a
disable before setting another isc, but that's a matter of taste).

Regardless, I think the current check doesn't even catch the 'different
isc' case?

hum, right.
If it is OK I remove this one.
And I must rework the same test I do later
   in this patch.

So, you mean checking for PMCW_ENABLE? Or not at all?

(I'd check for PMCW_ENABLE.)


-       if ((pmcw->flags & flags) == flags) {
+       if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) {
                report_info("stsch: sch %08x already enabled", schid);
                return 0;
        }

I keep both, otherwise I return 0 without setting the ISC.
 then I have another error:

 retry:
        /* Update the SCHIB to enable the channel and set the ISC */
+       pmcw->flags &= ~(PMCW_ISC_MASK | PMCW_ENABLE);
        pmcw->flags |= flags;

and finaly the same as the first later...

-       if ((pmcw->flags & flags) == flags) {
+       if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) {
report_info("stsch: sch %08x successfully modified after %d retries",
                            schid, retry_count);


is better I think.
What do you think?

--
Pierre Morel
IBM Lab Boeblingen



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux