Re: [kvm-unit-tests PATCH v2 8/9] s390x: css: ssch/tsch with sense and interrupt

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

 





On 2019-12-02 15:55, Cornelia Huck wrote:
On Thu, 28 Nov 2019 13:46:06 +0100
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:

When a channel is enabled we can start a SENSE command using the SSCH
instruction to recognize the control unit and device.

This tests the success of SSCH, the I/O interruption and the TSCH
instructions.

The test expect a device with a control unit type of 0xC0CA.

s/expect/expects/

... :(



Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
  lib/s390x/css.h |  13 +++++
  s390x/css.c     | 137 ++++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 150 insertions(+)


(...)

diff --git a/s390x/css.c b/s390x/css.c
index e42dc2f..534864f 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -11,12 +11,28 @@
   */
#include <libcflat.h>
+#include <alloc_phys.h>
+#include <asm/page.h>
+#include <string.h>
+#include <asm/interrupt.h>
+#include <asm/arch_def.h>
+#include <asm/clock.h>
#include <css.h> #define SID_ONE 0x00010000
+#define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
+
+struct lowcore *lowcore = (void *)0x0;
static struct schib schib;
+#define NB_CCW  100

s/NB_CCW/NUM_CCWS/ ?

I was scratching my head a bit when I first saw that define.

French and english.... sorry
of course better, I change it


+static struct ccw ccw[NB_CCW];
+#define NB_ORB  100
+static struct orb orb[NB_ORB];
+static struct irb irb;
+static char buffer[0x1000] __attribute__ ((aligned(8)));
+static struct senseid senseid;
static const char *Channel_type[3] = {
  	"I/O", "CHSC", "MSG"
@@ -24,6 +40,34 @@ static const char *Channel_type[3] = {
static int test_device_sid;

(...)

+void handle_io_int(void)
+{
+	int ret = 0;
+	char *flags;
+
+	report_prefix_push("Interrupt");
+	if (lowcore->io_int_param != 0xcafec0ca) {
+		report("Bad io_int_param: %x", 0, lowcore->io_int_param);
+		report_prefix_pop();
+		return;
+	}

Should you accommodate unsolicited interrupts as well?

Yet, I do not expect unsolicited interrupt.
But should be at least kept in mind.

May be I add this for v3.


+	report("io_int_param: %x", 1, lowcore->io_int_param);
+	report_prefix_pop();
+
+	ret = tsch(lowcore->subsys_id_word, &irb);
+	dump_irb(&irb);
+	flags = dump_scsw_flags(irb.scsw.ctrl);
+
+	if (ret)
+		report("IRB scsw flags: %s", 0, flags);
+	else
+		report("IRB scsw flags: %s", 1, flags);
+	report_prefix_pop();
+}
+
+static int start_subchannel(int code, char *data, int count)
+{
+	int ret;
+	struct pmcw *p = &schib.pmcw;
+	struct orb *orb_p = &orb[0];
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return 0;
+	}
+	ret = stsch(test_device_sid, &schib);

That schib is a global variable, isn't it? Why do you need to re-check?

In the principe the previous tests, storing the SHIB could have been disabled.


+	if (ret) {
+		report("Err %d on stsch on sid %08x", 0, ret, test_device_sid);
+		return 0;
+	}
+	if (!(p->flags & PMCW_ENABLE)) {
+		report_skip("Device (sid %08x) not enabled", test_device_sid);
+		return 0;
+	}
+	ccw[0].code = code ;

Extra ' ' before ';'

yes, thanks


+	ccw[0].flags = CCW_F_PCI;

Huh, what's that PCI for?

Program Control Interruption

I will add a comment :)


+	ccw[0].count = count;
+	ccw[0].data = (int)(unsigned long)data;

Can you be sure that data is always below 2G?

Currently yes, the program is loaded at 0x10000 and is quite small
also doing a test does not hurt for the case the function is used in another test someday.


+	orb_p->intparm = 0xcafec0ca;
+	orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
+	orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
+
+	report_prefix_push("Start Subchannel");
+	ret = ssch(test_device_sid, orb_p);
+	if (ret) {
+		report("ssch cc=%d", 0, ret);
+		report_prefix_pop();
+		return 0;
+	}
+	report_prefix_pop();
+	return 1;
+}
+
+static void test_sense(void)
+{
+	int success;
+
+	enable_io_irq();
+
+	success = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
+	if (!success) {
+		report("start_subchannel failed", 0);
+		return;
+	}
+
+	senseid.cu_type = buffer[2] | (buffer[1] << 8);
+	delay(1000);
+
+	/* Sense ID is non packed cut_type is at offset +1 byte */
+	if (senseid.cu_type == PONG_CU)
+		report("cu_type: expect c0ca, got %04x", 1, senseid.cu_type);
+	else
+		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
+}

I'm not really convinced by that logic here. This will fall apart if
you don't have your pong device exactly in the right place, and it does
not make it easy to extend this for more devices in the future.

Wanted to keep things simple. PONG must be the first valid channel.
also, should be documented at least.


What about the following:
- do a stsch() loop (basically re-use your first patch)
- for each I/O subchannel with dnv=1, do SenseID
- use the first (?) device with a c0ca CU type as your test device

Or maybe I'm overthinking this? It just does not strike me as very
robust and reusable.

I can do it.

Thanks for the comments,

Best regards,
Pierre

--
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