Re: [kvm-unit-tests PATCH v2 5/9] s390x: Library resources for CSS tests

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

 





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

These are the include and library utilities for the css tests patch
series.

Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
  lib/s390x/css.h      | 269 +++++++++++++++++++++++++++++++++++++++++++
  lib/s390x/css_dump.c | 147 +++++++++++++++++++++++
  2 files changed, 416 insertions(+)
  create mode 100644 lib/s390x/css.h
  create mode 100644 lib/s390x/css_dump.c

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
new file mode 100644
index 0000000..95dec72
--- /dev/null
+++ b/lib/s390x/css.h
@@ -0,0 +1,269 @@
+/*
+ * CSS definitions
+ *
+ * Copyright IBM, Corp. 2019
+ * Author: Pierre Morel <pmorel@xxxxxxxxxxxxx>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef CSS_H
+#define CSS_H
+
+#define CCW_F_CD	0x80
+#define CCW_F_CC	0x40
+#define CCW_F_SLI	0x20
+#define CCW_F_SKP	0x10
+#define CCW_F_PCI	0x08
+#define CCW_F_IDA	0x04
+#define CCW_F_S		0x02
+#define CCW_F_MIDA	0x01
+
+#define CCW_C_NOP	0x03
+#define CCW_C_TIC	0x08
+
+struct ccw {

ccw1, to make it clear that this is a format-1 ccw?

(Do you have any plans to test this with format-0 ccws as well?)

not in a near future


+	unsigned char code;
+	unsigned char flags;
+	unsigned short count;
+	unsigned int data;

data_address?

OK


+} __attribute__ ((aligned(4)));
+
+#define ORB_M_KEY	0xf0000000
+#define ORB_F_SUSPEND	0x08000000
+#define ORB_F_STREAMING	0x04000000
+#define ORB_F_MODIFCTRL	0x02000000
+#define ORB_F_SYNC	0x01000000
+#define ORB_F_FORMAT	0x00800000
+#define ORB_F_PREFETCH	0x00400000
+#define ORB_F_INIT_IRQ	0x00200000
+#define ORB_F_ADDRLIMIT	0x00100000
+#define ORB_F_SUSP_IRQ	0x00080000
+#define ORB_F_TRANSPORT	0x00040000
+#define ORB_F_IDAW2	0x00020000
+#define ORB_F_IDAW_2K	0x00010000
+#define ORB_M_LPM	0x0000ff00
+#define ORB_F_LPM_DFLT	0x00008000
+#define ORB_F_ILSM	0x00000080
+#define ORB_F_CCW_IND	0x00000040
+#define ORB_F_ORB_EXT	0x00000001
+
+struct orb {
+	unsigned int	intparm;
+	unsigned int	ctrl;
+	unsigned int	cpa;
+	unsigned int	prio;
+	unsigned int	reserved[4];
+} __attribute__ ((aligned(4)));
+
+struct scsw {
+	uint32_t ctrl;
+	uint32_t addr;

ccw_addr?

OK


+	uint8_t  devs;
+	uint8_t  schs;

Maybe dev_stat/sch_stat?

OK


+	uint16_t count;
+};

Out of curiousity (I'm not familiar with the conventions in
kvm-unit-tests): You use the explicit uint32_t et al. types here, while
you used unsigned int et al. in the other definitions... maybe it would
be better to use one or the other?

OK, I suppose that if we want to be working with TCG uintxxx_t is better.
So I will take care to be coherent.


Also, you probably always want to test against a QEMU-provided device
anyway, so you can probably ignore transport mode and stick with
command mode only, right?

Yes


+
+struct pmcw {
+	uint32_t intparm;
+#define PMCW_DNV        0x0001
+#define PMCW_ENABLE     0x0080
+	uint16_t flags;
+	uint16_t devnum;
+	uint8_t  lpm;
+	uint8_t  pnom;
+	uint8_t  lpum;
+	uint8_t  pim;
+	uint16_t mbi;
+	uint8_t  pom;
+	uint8_t  pam;
+	uint8_t  chpid[8];
+	struct {
+		uint8_t res0;
+		uint8_t st:3;
+		uint8_t :5;
+		uint16_t :13;
+		uint16_t f:1;
+		uint16_t x:1;
+		uint16_t s:1;
+	};

Hm... you used masks for the other fields, any reason you didn't here?

Here too , I must be coherent.
Reason was that I use the flags differently, the test of the bit fields is easier to read than shift & mask but ... anyway I prefer masks for hardware close programing.


+};
+
+struct schib {
+	struct pmcw pmcw;
+	struct scsw scsw;
+	uint32_t  md0;
+	uint32_t  md1;
+	uint32_t  md2;

Hm, both Linux and QEMU express the fields you called md<n> as a 64 bit
measurement-block address and a four bytes model-dependent area...
would it make sense to do so here as well? If the extended measurement
block facility is not installed, we'd get a 12 bytes model-dependent
area, which IMHO would also look better here.

OK, IMHO too


+} __attribute__ ((aligned(4)));
+
+struct irb {
+	struct scsw scsw;
+	uint32_t esw[5];
+	uint32_t ecw[8];
+	uint32_t emw[8];

If I read the PoP correctly, esw, ecw, and emw are defined bytewise,
not u32-wise.

Hum, I found them referenced u32-wise.
I intended to define them as a structure at the moment they are used.
If you really prefer I can use uint8_t esw[40] uint8_t ecw[64] etc.
but I like better word aligned structures.


+} __attribute__ ((aligned(4)));
+
+/* CSS low level access functions */
+
+static inline int ssch(unsigned long schid, struct orb *addr)
+{
+	register long long reg1 asm("1") = schid;
+	int cc = -1;
+
+	asm volatile(
+		"	   ssch	0(%2)\n"
+		"0:	 ipm	 %0\n"
+		"	   srl	 %0,28\n"
+		"1:\n"
+		: "+d" (cc)
+		: "d" (reg1), "a" (addr), "m" (*addr)
+		: "cc", "memory");
+	return cc;
+}

Looking at the Linux code, stsch, msch, and ssch all set up an
exception handler. IIRC, I had introduced that for stsch for multiple
subchannels sets, not sure about the others. Are we sure we never need
to catch exceptions here?

This is the next and important step.
I want to surround each call with expect_pgm_int() / check_pgm_int_code()

Also testing each error case.
So we do not need exception handler here, we will use the kvm-test ones.


+
+static inline int stsch(unsigned long schid, struct schib *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	   stsch	0(%3)\n"
+		"	   ipm	 %0\n"
+		"	   srl	 %0,28"
+		: "=d" (cc), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");
+	return cc;
+}
+
+static inline int msch(unsigned long schid, struct schib *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	   msch	0(%3)\n"
+		"	   ipm	 %0\n"
+		"	   srl	 %0,28"
+		: "=d" (cc), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");
+	return cc;
+}

(...)

+static inline int rchp(unsigned long chpid)
+{
+	register unsigned long reg1 asm("1") = chpid;
+	int cc;
+
+	asm volatile(
+		"	rchp\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}

Does rchp actually do anything useful on QEMU? Or is this mainly for
completeness' sake?

It is for completness for this first series.
However I would like to do something with it in a following one.
to be ready for QEMU implementation or to test real hardware when the kvm-unit-test run on LPAR.


+
+/* Debug functions */
+char *dump_pmcw_flags(uint16_t f);
+char *dump_scsw_flags(uint32_t f);
+#undef DEBUG
+#ifdef DEBUG
+void dump_scsw(struct scsw *);
+void dump_irb(struct irb *irbp);
+void dump_schib(struct schib *sch);
+struct ccw *dump_ccw(struct ccw *cp);
+#else
+static inline void dump_scsw(struct scsw *scsw){}
+static inline void dump_irb(struct irb *irbp){}
+static inline void dump_pmcw(struct pmcw *p){}
+static inline void dump_schib(struct schib *sch){}
+static inline void dump_orb(struct orb *op){}
+static inline struct ccw *dump_ccw(struct ccw *cp)
+{
+	return NULL;
+}
+#endif
+
+extern unsigned long stacktop;
+#endif
diff --git a/lib/s390x/css_dump.c b/lib/s390x/css_dump.c
new file mode 100644
index 0000000..5356df2
--- /dev/null
+++ b/lib/s390x/css_dump.c
@@ -0,0 +1,147 @@
+/*
+ * Channel Sub-System structures dumping

I think "subsystem" is the more usual spelling.

OK, I found it much fun so :)


+ *
+ * Copyright (c) 2019 IBM Corp.
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@xxxxxxxxxxxxx>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ *
+ * Description:
+ * Provides the dumping functions for various structures used by subchannels:
+ * - ORB  : Operation request block, describe the I/O operation and point to

s/describe/describes/
s/point/points/

thanks


+ *          a CCW chain
+ * - CCW  : Channel Command Word, describe the data and flow control

s/describe/describes/

but maybe better:

"contains the command, parameters, and a pointer to data"

?

yes, thanks


Also this is format-1 only, isn't it?

yes, I add this


+ * - IRB  : Interuption response Block, describe the result of an operation

s/describe/describes/

again!
thanks :)


+ *          hold a SCSW and several channel type dependent fields.

s/hold/holds/
hum...

s/several channel type dependent fields/model-dependent data/ ?

yes


+ * - SCHIB: SubChannel Information Block composed of:

s/SubChannel/Subchannel/

+ *   - SCSW: SubChannel Status Word, status of the channel as a result of an

s/SubChannel/Subchannel/
OK

+ *           operation when in IRB.

I think that description is a bit confusing: An SCSW always contains
the subchannel status; it's just when it is contained in an IRB that
the status is associated to the last event on that subchannel (as the
result of an operation, or as an unsolicited status.)

yes, the "when in IRB has nothing to do there" IRB defintion is above.
I will make it clearer


+ *   - PMCW: Path Management Control Word
+ * You need the QEMU ccw-pong device in QEMU to answer the I/O transfers.
+ */
+
+#include <unistd.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <string.h>
+
+#include <css.h>
+
+static const char *scsw_str = "kkkkslccfpixuzen";
+static const char *scsw_str2 = "1SHCrshcsdsAIPSs";

Nice, random strings? :)

these are the bit definitions associated with the scsw flags.
Easier to read than a bit field ... as long as you have the documentation (Pop)


Thanks a lot for the review.
Seems I have some work for the v3

Regards,
Pierre


--
Pierre Morel
IBM Lab Boeblingen




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux