[kvm-unit-tests PATCH 0/3] s390x: Improve console handling

[kvm-unit-tests PATCH 0/3] s390x: Improve console handling

[Janosch Frank]

Console IO is and has been in a state of "works for me". I don't think
that will change soon since there's no need for a proper console
driver when all we want is the ability to print or read a line at a
time.

However since input is only supported on the ASCII console I was
forced to use it on the HMC. The HMC generally does not add a \r on a
\n so each line doesn't start at column 0. It's time to finally fix
that.

Also, since there are environments that only provide the line-mode
console it's time to add line-mode input to properly support them.

v1:
	* Fenced ASCII compat handling so it's only use in LPAR
	* Squashed compat handling into one patch
	* Added an early detect_host since SCLP might be used in setup
	* Fixed up a few formatting issues in input patch
	* Fixed up copyright stuff

Janosch Frank (3):
  lib: s390x: hw: Provide early detect host
  lib: s390x: sclp: Add compat handling for HMC ASCII consoles
  lib: s390x: sclp: Add line mode input handling

 lib/s390x/hardware.c     |   8 ++
 lib/s390x/hardware.h     |   1 +
 lib/s390x/sclp-console.c | 203 +++++++++++++++++++++++++++++++++++----
 lib/s390x/sclp.h         |  26 ++++-
 4 files changed, 216 insertions(+), 22 deletions(-)

-- 
2.34.1

[kvm-unit-tests PATCH 1/3] lib: s390x: hw: Provide early detect host

[Janosch Frank]

For early sclp printing it's necessary to know if we're under LPAR or
not so we can apply compat SCLP ASCII transformations.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/hardware.c | 8 ++++++++
 lib/s390x/hardware.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/lib/s390x/hardware.c b/lib/s390x/hardware.c
index 2bcf9c4c..d5a752c0 100644
--- a/lib/s390x/hardware.c
+++ b/lib/s390x/hardware.c
@@ -52,6 +52,14 @@ static enum s390_host do_detect_host(void *buf)
 	return HOST_IS_UNKNOWN;
 }
 
+enum s390_host detect_host_early(void)
+{
+	if (stsi_get_fc() == 2)
+		return HOST_IS_LPAR;
+
+	return HOST_IS_UNKNOWN;
+}
+
 enum s390_host detect_host(void)
 {
 	static enum s390_host host = HOST_IS_UNKNOWN;
diff --git a/lib/s390x/hardware.h b/lib/s390x/hardware.h
index 86fe873c..5e5a9d90 100644
--- a/lib/s390x/hardware.h
+++ b/lib/s390x/hardware.h
@@ -24,6 +24,7 @@ enum s390_host {
 };
 
 enum s390_host detect_host(void);
+enum s390_host detect_host_early(void);
 
 static inline uint16_t get_machine_id(void)
 {
-- 
2.34.1

[kvm-unit-tests PATCH 2/3] lib: s390x: sclp: Add compat handling for HMC ASCII consoles

[Janosch Frank]

Without the \r the output of the HMC ASCII console takes a lot of
additional effort to read in comparison to the line mode console.

Additionally we add a console clear for the HMC ASCII console so that
old messages from a previously running operating system are not
polluting the console.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/sclp-console.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
index 19c74e46..313be1e4 100644
--- a/lib/s390x/sclp-console.c
+++ b/lib/s390x/sclp-console.c
@@ -11,6 +11,7 @@
 #include <asm/arch_def.h>
 #include <asm/io.h>
 #include <asm/spinlock.h>
+#include "hardware.h"
 #include "sclp.h"
 
 /*
@@ -85,6 +86,8 @@ static uint8_t _ascebc[256] = {
      0x90, 0x3F, 0x3F, 0x3F, 0x3F, 0xEA, 0x3F, 0xFF
 };
 
+static bool lpar_ascii_compat;
+
 static char lm_buff[120];
 static unsigned char lm_buff_off;
 static struct spinlock lm_buff_lock;
@@ -97,14 +100,27 @@ static void sclp_print_ascii(const char *str)
 {
 	int len = strlen(str);
 	WriteEventData *sccb = (void *)_sccb;
+	char *str_dest = (char *)&sccb->msg;
+	int i = 0;
 
 	sclp_mark_busy();
 	memset(sccb, 0, sizeof(*sccb));
+
+	for (; i < len; i++) {
+		*str_dest = str[i];
+		str_dest++;
+		/* Add a \r to the \n for HMC ASCII console */
+		if (str[i] == '\n' && lpar_ascii_compat) {
+			*str_dest = '\r';
+			str_dest++;
+		}
+	}
+
+	len = (uintptr_t)str_dest - (uintptr_t)&sccb->msg;
 	sccb->h.length = offsetof(WriteEventData, msg) + len;
 	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
 	sccb->ebh.length = sizeof(EventBufferHeader) + len;
 	sccb->ebh.type = SCLP_EVENT_ASCII_CONSOLE_DATA;
-	memcpy(&sccb->msg, str, len);
 
 	sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb);
 }
@@ -218,8 +234,13 @@ static void sclp_console_disable_read(void)
 
 void sclp_console_setup(void)
 {
+	lpar_ascii_compat = detect_host_early() == HOST_IS_LPAR;
+
 	/* We send ASCII and line mode. */
 	sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
+	/* Hard terminal reset to clear screen for HMC ASCII console */
+	if (lpar_ascii_compat)
+		sclp_print_ascii("\ec");
 }
 
 void sclp_print(const char *str)
-- 
2.34.1

[kvm-unit-tests PATCH 3/3] lib: s390x: sclp: Add line mode input handling

[Janosch Frank]

Time to add line-mode input so we can use input handling under LPAR if
there's no access to a ASCII console.

Line-mode IO is pretty wild and the documentation could be improved a
lot. Hence I've copied the input parsing functions from Linux.

For some reason output is a type 2 event but input is a type 1
event. This also means that the input and output structures are
different from each other.

The input can consist of multiple structures which don't contain text
data before the input text data is reached. Hence we need a bunch of
search functions to retrieve a pointer to the text data.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/sclp-console.c | 180 ++++++++++++++++++++++++++++++++++-----
 lib/s390x/sclp.h         |  26 +++++-
 2 files changed, 185 insertions(+), 21 deletions(-)

diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
index 313be1e4..23c09b70 100644
--- a/lib/s390x/sclp-console.c
+++ b/lib/s390x/sclp-console.c
@@ -1,8 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */
 /*
- * SCLP ASCII access driver
+ * SCLP line mode and ASCII console driver
  *
  * Copyright (c) 2013 Alexander Graf <agraf@suse.de>
+ *
+ * Copyright IBM Corp. 1999
+ * Author(s): Martin Peschke <mpeschke@de.ibm.com>
+ *	      Martin Schwidefsky <schwidefsky@de.ibm.com>
  */
 
 #include <libcflat.h>
@@ -86,6 +90,41 @@ static uint8_t _ascebc[256] = {
      0x90, 0x3F, 0x3F, 0x3F, 0x3F, 0xEA, 0x3F, 0xFF
 };
 
+static const uint8_t _ebcasc[] = {
+	0x00, 0x01, 0x02, 0x03, 0x07, 0x09, 0x07, 0x7F,
+	0x07, 0x07, 0x07, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
+	0x10, 0x11, 0x12, 0x13, 0x07, 0x0A, 0x08, 0x07,
+	0x18, 0x19, 0x07, 0x07, 0x07, 0x07, 0x07, 0x07,
+	0x07, 0x07, 0x1C, 0x07, 0x07, 0x0A, 0x17, 0x1B,
+	0x07, 0x07, 0x07, 0x07, 0x07, 0x05, 0x06, 0x07,
+	0x07, 0x07, 0x16, 0x07, 0x07, 0x07, 0x07, 0x04,
+	0x07, 0x07, 0x07, 0x07, 0x14, 0x15, 0x07, 0x1A,
+	0x20, 0xFF, 0x83, 0x84, 0x85, 0xA0, 0x07, 0x86,
+	0x87, 0xA4, 0x5B, 0x2E, 0x3C, 0x28, 0x2B, 0x21,
+	0x26, 0x82, 0x88, 0x89, 0x8A, 0xA1, 0x8C, 0x07,
+	0x8D, 0xE1, 0x5D, 0x24, 0x2A, 0x29, 0x3B, 0x5E,
+	0x2D, 0x2F, 0x07, 0x8E, 0x07, 0x07, 0x07, 0x8F,
+	0x80, 0xA5, 0x07, 0x2C, 0x25, 0x5F, 0x3E, 0x3F,
+	0x07, 0x90, 0x07, 0x07, 0x07, 0x07, 0x07, 0x07,
+	0x70, 0x60, 0x3A, 0x23, 0x40, 0x27, 0x3D, 0x22,
+	0x07, 0x61, 0x62, 0x63, 0x64, 0x65, 0x66, 0x67,
+	0x68, 0x69, 0xAE, 0xAF, 0x07, 0x07, 0x07, 0xF1,
+	0xF8, 0x6A, 0x6B, 0x6C, 0x6D, 0x6E, 0x6F, 0x70,
+	0x71, 0x72, 0xA6, 0xA7, 0x91, 0x07, 0x92, 0x07,
+	0xE6, 0x7E, 0x73, 0x74, 0x75, 0x76, 0x77, 0x78,
+	0x79, 0x7A, 0xAD, 0xAB, 0x07, 0x07, 0x07, 0x07,
+	0x9B, 0x9C, 0x9D, 0xFA, 0x07, 0x07, 0x07, 0xAC,
+	0xAB, 0x07, 0xAA, 0x7C, 0x07, 0x07, 0x07, 0x07,
+	0x7B, 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47,
+	0x48, 0x49, 0x07, 0x93, 0x94, 0x95, 0xA2, 0x07,
+	0x7D, 0x4A, 0x4B, 0x4C, 0x4D, 0x4E, 0x4F, 0x50,
+	0x51, 0x52, 0x07, 0x96, 0x81, 0x97, 0xA3, 0x98,
+	0x5C, 0xF6, 0x53, 0x54, 0x55, 0x56, 0x57, 0x58,
+	0x59, 0x5A, 0xFD, 0x07, 0x99, 0x07, 0x07, 0x07,
+	0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37,
+	0x38, 0x39, 0x07, 0x07, 0x9A, 0x07, 0x07, 0x07,
+};
+
 static bool lpar_ascii_compat;
 
 static char lm_buff[120];
@@ -224,7 +263,8 @@ static void sclp_write_event_mask(int receive_mask, int send_mask)
 
 static void sclp_console_enable_read(void)
 {
-	sclp_write_event_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
+	sclp_write_event_mask(SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_OPCMD,
+			      SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
 }
 
 static void sclp_console_disable_read(void)
@@ -262,37 +302,137 @@ void sclp_print(const char *str)
 	sclp_print_lm(str);
 }
 
+static char *console_read_ascii(struct EventBufferHeader *ebh, int *len)
+{
+	struct ReadEventDataAsciiConsole *evdata = (void *)ebh;
+	const int max_event_buffer_len = SCCB_SIZE - offsetof(ReadEventDataAsciiConsole, ebh);
+	const int event_buffer_ascii_recv_header_len = offsetof(ReadEventDataAsciiConsole, data);
+
+	assert(ebh->length <= max_event_buffer_len);
+	assert(ebh->length > event_buffer_ascii_recv_header_len);
+
+	*len = ebh->length - event_buffer_ascii_recv_header_len;
+	return evdata->data;
+}
+
+
+static struct gds_vector *sclp_find_gds_vector(void *start, void *end, uint16_t id)
+{
+	struct gds_vector *v;
+
+	for (v = start; (void *)v < end; v = (void *)v + v->length)
+		if (v->gds_id == id)
+			return v;
+	return NULL;
+}
+
+static struct gds_subvector *sclp_eval_selfdeftextmsg(struct gds_subvector *sv)
+{
+	void *end;
+
+	end = (void *)sv + sv->length;
+	for (sv = sv + 1; (void *)sv < end; sv = (void *)sv + sv->length)
+		if (sv->key == 0x30)
+			return sv;
+	return NULL;
+}
+
+static struct gds_subvector *sclp_eval_textcmd(struct gds_vector *v)
+{
+	struct gds_subvector *sv;
+	void *end;
+
+	end = (void *)v + v->length;
+	for (sv = (struct gds_subvector *)(v + 1); (void *)sv < end;
+	     sv = (void *)sv + sv->length)
+		if (sv->key == GDS_KEY_SELFDEFTEXTMSG)
+			return sclp_eval_selfdeftextmsg(sv);
+	return NULL;
+}
+
+static struct gds_subvector *sclp_eval_cpmsu(struct gds_vector *v)
+{
+	void *end;
+
+	end = (void *)v + v->length;
+	for (v = v + 1; (void *)v < end; v = (void *)v + v->length)
+		if (v->gds_id == GDS_ID_TEXTCMD)
+			return sclp_eval_textcmd(v);
+	return NULL;
+}
+
+static struct gds_subvector *sclp_eval_mdsmu(struct gds_vector *v)
+{
+	v = sclp_find_gds_vector(v + 1, (void *)v + v->length, GDS_ID_CPMSU);
+	if (v)
+		return sclp_eval_cpmsu(v);
+	return NULL;
+}
+
+static char *console_read_lm(struct EventBufferHeader *ebh, int *len)
+{
+	struct gds_vector *v = (void *)ebh + sizeof(*ebh);
+	struct gds_subvector *sv;
+
+	v = sclp_find_gds_vector(v, (void *)ebh + ebh->length,
+				 GDS_ID_MDSMU);
+	if (!v)
+		return NULL;
+
+	sv = sclp_eval_mdsmu(v);
+	if (!sv)
+		return NULL;
+
+	*len = sv->length - (sizeof(*sv));
+	return (char *)(sv + 1);
+}
+
+static void ebc_to_asc(char *data, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		data[i] = _ebcasc[(uint8_t)data[i]];
+}
+
 static int console_refill_read_buffer(void)
 {
-	const int max_event_buffer_len = SCCB_SIZE - offsetof(ReadEventDataAsciiConsole, ebh);
-	ReadEventDataAsciiConsole *sccb = (void *)_sccb;
-	const int event_buffer_ascii_recv_header_len = sizeof(sccb->ebh) + sizeof(sccb->type);
-	int ret = -1;
+	struct SCCBHeader *sccb = (struct SCCBHeader *)_sccb;
+	struct EventBufferHeader *ebh = (void *)_sccb + sizeof(struct SCCBHeader);
+	char *data;
+	int ret = -1, len;
 
 	sclp_console_enable_read();
 
 	sclp_mark_busy();
-	memset(sccb, 0, SCCB_SIZE);
-	sccb->h.length = PAGE_SIZE;
-	sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
-	sccb->h.control_mask[2] = SCLP_CM2_VARIABLE_LENGTH_RESPONSE;
+	memset(_sccb, 0, SCCB_SIZE);
+	sccb->length = PAGE_SIZE;
+	sccb->function_code = SCLP_UNCONDITIONAL_READ;
+	sccb->control_mask[2] = SCLP_CM2_VARIABLE_LENGTH_RESPONSE;
 
 	sclp_service_call(SCLP_CMD_READ_EVENT_DATA, sccb);
 
-	if (sccb->h.response_code == SCLP_RC_NO_EVENT_BUFFERS_STORED ||
-	    sccb->ebh.type != SCLP_EVENT_ASCII_CONSOLE_DATA ||
-	    sccb->type != SCLP_EVENT_ASCII_TYPE_DATA_STREAM_FOLLOWS) {
-		ret = -1;
+	if (sccb->response_code == SCLP_RC_NO_EVENT_BUFFERS_STORED)
+		goto out;
+
+	switch (ebh->type) {
+	case SCLP_EVENT_OP_CMD:
+		data = console_read_lm(ebh, &len);
+		if (data)
+			ebc_to_asc(data, len);
+		break;
+	case SCLP_EVENT_ASCII_CONSOLE_DATA:
+		data = console_read_ascii(ebh, &len);
+		break;
+	default:
 		goto out;
 	}
 
-	assert(sccb->ebh.length <= max_event_buffer_len);
-	assert(sccb->ebh.length > event_buffer_ascii_recv_header_len);
+	if (!data)
+		goto out;
 
-	read_buf_length = sccb->ebh.length - event_buffer_ascii_recv_header_len;
-
-	assert(read_buf_length <= sizeof(read_buf));
-	memcpy(read_buf, sccb->data, read_buf_length);
+	assert(len <= sizeof(read_buf));
+	memcpy(read_buf, data, len);
 
 	read_index = 0;
 	ret = 0;
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 6a611bc3..22f120d1 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -226,6 +226,7 @@ typedef struct SCCB {
 } __attribute__((packed)) SCCB;
 
 /* SCLP event types */
+#define SCLP_EVENT_OP_CMD			0x01
 #define SCLP_EVENT_ASCII_CONSOLE_DATA           0x1a
 #define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
 
@@ -233,6 +234,7 @@ typedef struct SCCB {
 #define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
 #define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
 #define SCLP_EVENT_MASK_MSG          		0x40000000
+#define SCLP_EVENT_MASK_OPCMD			0x80000000
 
 #define SCLP_UNCONDITIONAL_READ                 0x00
 #define SCLP_SELECTIVE_READ                     0x01
@@ -296,6 +298,23 @@ struct mdb {
 	struct mto mto;
 } __attribute__((packed));
 
+/* vector keys and ids */
+#define GDS_ID_MDSMU		0x1310
+#define GDS_ID_CPMSU		0x1212
+#define GDS_ID_TEXTCMD		0x1320
+#define GDS_KEY_SELFDEFTEXTMSG	0x31
+#define EBC_MDB                 0xd4c4c240
+
+struct gds_vector {
+	uint16_t     length;
+	uint16_t     gds_id;
+} __attribute__((packed));
+
+struct gds_subvector {
+	uint8_t      length;
+	uint8_t      key;
+} __attribute__((packed));
+
 typedef struct EventBufferHeader {
 	uint16_t length;
 	uint8_t  type;
@@ -320,12 +339,17 @@ typedef struct ReadEventData {
 
 #define SCLP_EVENT_ASCII_TYPE_DATA_STREAM_FOLLOWS 0
 typedef struct ReadEventDataAsciiConsole {
-	SCCBHeader h;
 	EventBufferHeader ebh;
 	uint8_t type;
 	char data[];
 } __attribute__((packed)) ReadEventDataAsciiConsole;
 
+struct ReadEventDataLMConsole {
+	SCCBHeader h;
+	EventBufferHeader ebh;
+	struct gds_vector v[];
+};
+
 extern char _sccb[];
 void sclp_setup_int(void);
 void sclp_handle_ext(void);
-- 
2.34.1

Re: [kvm-unit-tests PATCH 2/3] lib: s390x: sclp: Add compat handling for HMC ASCII consoles

[Nico Boehr]

Quoting Janosch Frank (2023-10-10 09:38:54)
[...]
> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
> index 19c74e46..313be1e4 100644
> --- a/lib/s390x/sclp-console.c
> +++ b/lib/s390x/sclp-console.c
[...]
> +static bool lpar_ascii_compat;

This only toggles adding \r. So why not name it accordingly?
Something like:
  ascii_line_end_dos
or
  ascii_add_cr_line_end

>  static char lm_buff[120];
>  static unsigned char lm_buff_off;
>  static struct spinlock lm_buff_lock;
> @@ -97,14 +100,27 @@ static void sclp_print_ascii(const char *str)
>  {
>         int len = strlen(str);
>         WriteEventData *sccb = (void *)_sccb;
> +       char *str_dest = (char *)&sccb->msg;
> +       int i = 0;
>  
>         sclp_mark_busy();
>         memset(sccb, 0, sizeof(*sccb));
> +
> +       for (; i < len; i++) {
> +               *str_dest = str[i];
> +               str_dest++;
> +               /* Add a \r to the \n for HMC ASCII console */
> +               if (str[i] == '\n' && lpar_ascii_compat) {
> +                       *str_dest = '\r';
> +                       str_dest++;
> +               }
> +       }

Please don't hide the check inside the loop.
Do:
if (lpar_ascii_compat)
  // your loop
else
  memcpy()

Also, please add protection against overflowing sccb->msg (max 4088 bytes
if I looked it up right).

> +       len = (uintptr_t)str_dest - (uintptr_t)&sccb->msg;

And when you do the above, it should be easy to get rid of pointer
subtraction.

[...]
>  void sclp_console_setup(void)
>  {
> +       lpar_ascii_compat = detect_host_early() == HOST_IS_LPAR;
> +
>         /* We send ASCII and line mode. */
>         sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
> +       /* Hard terminal reset to clear screen for HMC ASCII console */
> +       if (lpar_ascii_compat)
> +               sclp_print_ascii("\ec");

I have in the past cursed programs which clear the screen, but I can see
the advantage here. How do others feel about this?

Re: [kvm-unit-tests PATCH 2/3] lib: s390x: sclp: Add compat handling for HMC ASCII consoles

[Janosch Frank]

On 10/10/23 10:35, Nico Boehr wrote:
> Quoting Janosch Frank (2023-10-10 09:38:54)
> [...]
>> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
>> index 19c74e46..313be1e4 100644
>> --- a/lib/s390x/sclp-console.c
>> +++ b/lib/s390x/sclp-console.c
> [...]
>> +static bool lpar_ascii_compat;
> 
> This only toggles adding \r. So why not name it accordingly?

Because it also toggles clearing the screen

> Something like:
>    ascii_line_end_dos
> or
>    ascii_add_cr_line_end
> 
>>   static char lm_buff[120];
>>   static unsigned char lm_buff_off;
>>   static struct spinlock lm_buff_lock;
>> @@ -97,14 +100,27 @@ static void sclp_print_ascii(const char *str)
>>   {
>>          int len = strlen(str);
>>          WriteEventData *sccb = (void *)_sccb;
>> +       char *str_dest = (char *)&sccb->msg;
>> +       int i = 0;
>>   
>>          sclp_mark_busy();
>>          memset(sccb, 0, sizeof(*sccb));
>> +
>> +       for (; i < len; i++) {
>> +               *str_dest = str[i];
>> +               str_dest++;
>> +               /* Add a \r to the \n for HMC ASCII console */
>> +               if (str[i] == '\n' && lpar_ascii_compat) {
>> +                       *str_dest = '\r';
>> +                       str_dest++;
>> +               }
>> +       }
> 
> Please don't hide the check inside the loop.
> Do:
> if (lpar_ascii_compat)
>    // your loop
> else
>    memcpy()


I'd rather have a loop than to nest it inside an if.

> 
> Also, please add protection against overflowing sccb->msg (max 4088 bytes
> if I looked it up right).

I considered this but we already have a 2k length check before that and 
I'd like to see someone print ~2k in a single call.

The question is if we want the complexity for something that we'll very 
likely never hit.

> 
>> +       len = (uintptr_t)str_dest - (uintptr_t)&sccb->msg;
> 
> And when you do the above, it should be easy to get rid of pointer
> subtraction.
> 
> [...]
>>   void sclp_console_setup(void)
>>   {
>> +       lpar_ascii_compat = detect_host_early() == HOST_IS_LPAR;
>> +
>>          /* We send ASCII and line mode. */
>>          sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
>> +       /* Hard terminal reset to clear screen for HMC ASCII console */
>> +       if (lpar_ascii_compat)
>> +               sclp_print_ascii("\ec");
> 
> I have in the past cursed programs which clear the screen, but I can see
> the advantage here. How do others feel about this?

Re: [kvm-unit-tests PATCH 3/3] lib: s390x: sclp: Add line mode input handling

[Claudio Imbrenda]

On Tue, 10 Oct 2023 07:38:55 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Time to add line-mode input so we can use input handling under LPAR if
> there's no access to a ASCII console.
> 
> Line-mode IO is pretty wild and the documentation could be improved a
> lot. Hence I've copied the input parsing functions from Linux.
> 
> For some reason output is a type 2 event but input is a type 1
> event. This also means that the input and output structures are
> different from each other.
> 
> The input can consist of multiple structures which don't contain text
> data before the input text data is reached. Hence we need a bunch of
> search functions to retrieve a pointer to the text data.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/sclp-console.c | 180 ++++++++++++++++++++++++++++++++++-----
>  lib/s390x/sclp.h         |  26 +++++-
>  2 files changed, 185 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
> index 313be1e4..23c09b70 100644
> --- a/lib/s390x/sclp-console.c
> +++ b/lib/s390x/sclp-console.c
> @@ -1,8 +1,12 @@
>  /* SPDX-License-Identifier: GPL-2.0-or-later */
>  /*
> - * SCLP ASCII access driver
> + * SCLP line mode and ASCII console driver
>   *
>   * Copyright (c) 2013 Alexander Graf <agraf@suse.de>
> + *
> + * Copyright IBM Corp. 1999
> + * Author(s): Martin Peschke <mpeschke@de.ibm.com>
> + *	      Martin Schwidefsky <schwidefsky@de.ibm.com>

from the weird copyright notices that you are adding I deduce that you
copied those functions from the kernel. maybe add a line in the patch
description to say so? or at least explain better in the comment itself.

>   */
>  
>  #include <libcflat.h>
> @@ -86,6 +90,41 @@ static uint8_t _ascebc[256] = {
>       0x90, 0x3F, 0x3F, 0x3F, 0x3F, 0xEA, 0x3F, 0xFF
>  };
>  
> +static const uint8_t _ebcasc[] = {
> +	0x00, 0x01, 0x02, 0x03, 0x07, 0x09, 0x07, 0x7F,
> +	0x07, 0x07, 0x07, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
> +	0x10, 0x11, 0x12, 0x13, 0x07, 0x0A, 0x08, 0x07,
> +	0x18, 0x19, 0x07, 0x07, 0x07, 0x07, 0x07, 0x07,
> +	0x07, 0x07, 0x1C, 0x07, 0x07, 0x0A, 0x17, 0x1B,
> +	0x07, 0x07, 0x07, 0x07, 0x07, 0x05, 0x06, 0x07,
> +	0x07, 0x07, 0x16, 0x07, 0x07, 0x07, 0x07, 0x04,
> +	0x07, 0x07, 0x07, 0x07, 0x14, 0x15, 0x07, 0x1A,
> +	0x20, 0xFF, 0x83, 0x84, 0x85, 0xA0, 0x07, 0x86,
> +	0x87, 0xA4, 0x5B, 0x2E, 0x3C, 0x28, 0x2B, 0x21,
> +	0x26, 0x82, 0x88, 0x89, 0x8A, 0xA1, 0x8C, 0x07,
> +	0x8D, 0xE1, 0x5D, 0x24, 0x2A, 0x29, 0x3B, 0x5E,
> +	0x2D, 0x2F, 0x07, 0x8E, 0x07, 0x07, 0x07, 0x8F,
> +	0x80, 0xA5, 0x07, 0x2C, 0x25, 0x5F, 0x3E, 0x3F,
> +	0x07, 0x90, 0x07, 0x07, 0x07, 0x07, 0x07, 0x07,
> +	0x70, 0x60, 0x3A, 0x23, 0x40, 0x27, 0x3D, 0x22,
> +	0x07, 0x61, 0x62, 0x63, 0x64, 0x65, 0x66, 0x67,
> +	0x68, 0x69, 0xAE, 0xAF, 0x07, 0x07, 0x07, 0xF1,
> +	0xF8, 0x6A, 0x6B, 0x6C, 0x6D, 0x6E, 0x6F, 0x70,
> +	0x71, 0x72, 0xA6, 0xA7, 0x91, 0x07, 0x92, 0x07,
> +	0xE6, 0x7E, 0x73, 0x74, 0x75, 0x76, 0x77, 0x78,
> +	0x79, 0x7A, 0xAD, 0xAB, 0x07, 0x07, 0x07, 0x07,
> +	0x9B, 0x9C, 0x9D, 0xFA, 0x07, 0x07, 0x07, 0xAC,
> +	0xAB, 0x07, 0xAA, 0x7C, 0x07, 0x07, 0x07, 0x07,
> +	0x7B, 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47,
> +	0x48, 0x49, 0x07, 0x93, 0x94, 0x95, 0xA2, 0x07,
> +	0x7D, 0x4A, 0x4B, 0x4C, 0x4D, 0x4E, 0x4F, 0x50,
> +	0x51, 0x52, 0x07, 0x96, 0x81, 0x97, 0xA3, 0x98,
> +	0x5C, 0xF6, 0x53, 0x54, 0x55, 0x56, 0x57, 0x58,
> +	0x59, 0x5A, 0xFD, 0x07, 0x99, 0x07, 0x07, 0x07,
> +	0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37,
> +	0x38, 0x39, 0x07, 0x07, 0x9A, 0x07, 0x07, 0x07,
> +};
> +
>  static bool lpar_ascii_compat;
>  
>  static char lm_buff[120];
> @@ -224,7 +263,8 @@ static void sclp_write_event_mask(int receive_mask, int send_mask)
>  
>  static void sclp_console_enable_read(void)
>  {
> -	sclp_write_event_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
> +	sclp_write_event_mask(SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_OPCMD,
> +			      SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
>  }
>  
>  static void sclp_console_disable_read(void)
> @@ -262,37 +302,137 @@ void sclp_print(const char *str)
>  	sclp_print_lm(str);
>  }
>  
> +static char *console_read_ascii(struct EventBufferHeader *ebh, int *len)
> +{
> +	struct ReadEventDataAsciiConsole *evdata = (void *)ebh;
> +	const int max_event_buffer_len = SCCB_SIZE - offsetof(ReadEventDataAsciiConsole, ebh);
> +	const int event_buffer_ascii_recv_header_len = offsetof(ReadEventDataAsciiConsole, data);
> +
> +	assert(ebh->length <= max_event_buffer_len);
> +	assert(ebh->length > event_buffer_ascii_recv_header_len);
> +
> +	*len = ebh->length - event_buffer_ascii_recv_header_len;
> +	return evdata->data;
> +}
> +
> +
> +static struct gds_vector *sclp_find_gds_vector(void *start, void *end, uint16_t id)
> +{
> +	struct gds_vector *v;
> +
> +	for (v = start; (void *)v < end; v = (void *)v + v->length)
> +		if (v->gds_id == id)
> +			return v;
> +	return NULL;
> +}
> +
> +static struct gds_subvector *sclp_eval_selfdeftextmsg(struct gds_subvector *sv)
> +{
> +	void *end;
> +
> +	end = (void *)sv + sv->length;
> +	for (sv = sv + 1; (void *)sv < end; sv = (void *)sv + sv->length)
> +		if (sv->key == 0x30)
> +			return sv;
> +	return NULL;
> +}
> +
> +static struct gds_subvector *sclp_eval_textcmd(struct gds_vector *v)
> +{
> +	struct gds_subvector *sv;
> +	void *end;
> +
> +	end = (void *)v + v->length;
> +	for (sv = (struct gds_subvector *)(v + 1); (void *)sv < end;
> +	     sv = (void *)sv + sv->length)
> +		if (sv->key == GDS_KEY_SELFDEFTEXTMSG)
> +			return sclp_eval_selfdeftextmsg(sv);
> +	return NULL;
> +}
> +
> +static struct gds_subvector *sclp_eval_cpmsu(struct gds_vector *v)
> +{
> +	void *end;
> +
> +	end = (void *)v + v->length;
> +	for (v = v + 1; (void *)v < end; v = (void *)v + v->length)
> +		if (v->gds_id == GDS_ID_TEXTCMD)
> +			return sclp_eval_textcmd(v);
> +	return NULL;
> +}
> +
> +static struct gds_subvector *sclp_eval_mdsmu(struct gds_vector *v)
> +{
> +	v = sclp_find_gds_vector(v + 1, (void *)v + v->length, GDS_ID_CPMSU);
> +	if (v)
> +		return sclp_eval_cpmsu(v);
> +	return NULL;
> +}
> +
> +static char *console_read_lm(struct EventBufferHeader *ebh, int *len)
> +{
> +	struct gds_vector *v = (void *)ebh + sizeof(*ebh);
> +	struct gds_subvector *sv;
> +
> +	v = sclp_find_gds_vector(v, (void *)ebh + ebh->length,
> +				 GDS_ID_MDSMU);
> +	if (!v)
> +		return NULL;
> +
> +	sv = sclp_eval_mdsmu(v);
> +	if (!sv)
> +		return NULL;
> +
> +	*len = sv->length - (sizeof(*sv));
> +	return (char *)(sv + 1);
> +}
> +
> +static void ebc_to_asc(char *data, int len)
> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		data[i] = _ebcasc[(uint8_t)data[i]];
> +}
> +
>  static int console_refill_read_buffer(void)
>  {
> -	const int max_event_buffer_len = SCCB_SIZE - offsetof(ReadEventDataAsciiConsole, ebh);
> -	ReadEventDataAsciiConsole *sccb = (void *)_sccb;
> -	const int event_buffer_ascii_recv_header_len = sizeof(sccb->ebh) + sizeof(sccb->type);
> -	int ret = -1;
> +	struct SCCBHeader *sccb = (struct SCCBHeader *)_sccb;
> +	struct EventBufferHeader *ebh = (void *)_sccb + sizeof(struct SCCBHeader);
> +	char *data;
> +	int ret = -1, len;
>  
>  	sclp_console_enable_read();
>  
>  	sclp_mark_busy();
> -	memset(sccb, 0, SCCB_SIZE);
> -	sccb->h.length = PAGE_SIZE;
> -	sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
> -	sccb->h.control_mask[2] = SCLP_CM2_VARIABLE_LENGTH_RESPONSE;
> +	memset(_sccb, 0, SCCB_SIZE);
> +	sccb->length = PAGE_SIZE;
> +	sccb->function_code = SCLP_UNCONDITIONAL_READ;
> +	sccb->control_mask[2] = SCLP_CM2_VARIABLE_LENGTH_RESPONSE;
>  
>  	sclp_service_call(SCLP_CMD_READ_EVENT_DATA, sccb);
>  
> -	if (sccb->h.response_code == SCLP_RC_NO_EVENT_BUFFERS_STORED ||
> -	    sccb->ebh.type != SCLP_EVENT_ASCII_CONSOLE_DATA ||
> -	    sccb->type != SCLP_EVENT_ASCII_TYPE_DATA_STREAM_FOLLOWS) {
> -		ret = -1;
> +	if (sccb->response_code == SCLP_RC_NO_EVENT_BUFFERS_STORED)
> +		goto out;
> +
> +	switch (ebh->type) {
> +	case SCLP_EVENT_OP_CMD:
> +		data = console_read_lm(ebh, &len);
> +		if (data)
> +			ebc_to_asc(data, len);
> +		break;
> +	case SCLP_EVENT_ASCII_CONSOLE_DATA:
> +		data = console_read_ascii(ebh, &len);
> +		break;
> +	default:
>  		goto out;
>  	}
>  
> -	assert(sccb->ebh.length <= max_event_buffer_len);
> -	assert(sccb->ebh.length > event_buffer_ascii_recv_header_len);
> +	if (!data)
> +		goto out;
>  
> -	read_buf_length = sccb->ebh.length - event_buffer_ascii_recv_header_len;
> -
> -	assert(read_buf_length <= sizeof(read_buf));
> -	memcpy(read_buf, sccb->data, read_buf_length);
> +	assert(len <= sizeof(read_buf));
> +	memcpy(read_buf, data, len);
>  
>  	read_index = 0;
>  	ret = 0;
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index 6a611bc3..22f120d1 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -226,6 +226,7 @@ typedef struct SCCB {
>  } __attribute__((packed)) SCCB;
>  
>  /* SCLP event types */
> +#define SCLP_EVENT_OP_CMD			0x01
>  #define SCLP_EVENT_ASCII_CONSOLE_DATA           0x1a
>  #define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
>  
> @@ -233,6 +234,7 @@ typedef struct SCCB {
>  #define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
>  #define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
>  #define SCLP_EVENT_MASK_MSG          		0x40000000
> +#define SCLP_EVENT_MASK_OPCMD			0x80000000
>  
>  #define SCLP_UNCONDITIONAL_READ                 0x00
>  #define SCLP_SELECTIVE_READ                     0x01
> @@ -296,6 +298,23 @@ struct mdb {
>  	struct mto mto;
>  } __attribute__((packed));
>  
> +/* vector keys and ids */
> +#define GDS_ID_MDSMU		0x1310
> +#define GDS_ID_CPMSU		0x1212
> +#define GDS_ID_TEXTCMD		0x1320
> +#define GDS_KEY_SELFDEFTEXTMSG	0x31
> +#define EBC_MDB                 0xd4c4c240
> +
> +struct gds_vector {
> +	uint16_t     length;
> +	uint16_t     gds_id;
> +} __attribute__((packed));
> +
> +struct gds_subvector {
> +	uint8_t      length;
> +	uint8_t      key;
> +} __attribute__((packed));
> +
>  typedef struct EventBufferHeader {
>  	uint16_t length;
>  	uint8_t  type;
> @@ -320,12 +339,17 @@ typedef struct ReadEventData {
>  
>  #define SCLP_EVENT_ASCII_TYPE_DATA_STREAM_FOLLOWS 0
>  typedef struct ReadEventDataAsciiConsole {
> -	SCCBHeader h;
>  	EventBufferHeader ebh;
>  	uint8_t type;
>  	char data[];
>  } __attribute__((packed)) ReadEventDataAsciiConsole;
>  
> +struct ReadEventDataLMConsole {
> +	SCCBHeader h;
> +	EventBufferHeader ebh;
> +	struct gds_vector v[];
> +};
> +
>  extern char _sccb[];
>  void sclp_setup_int(void);
>  void sclp_handle_ext(void);

Re: [kvm-unit-tests PATCH 1/3] lib: s390x: hw: Provide early detect host

[Claudio Imbrenda]

On Tue, 10 Oct 2023 07:38:53 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> For early sclp printing it's necessary to know if we're under LPAR or
> not so we can apply compat SCLP ASCII transformations.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/hardware.c | 8 ++++++++
>  lib/s390x/hardware.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/lib/s390x/hardware.c b/lib/s390x/hardware.c
> index 2bcf9c4c..d5a752c0 100644
> --- a/lib/s390x/hardware.c
> +++ b/lib/s390x/hardware.c
> @@ -52,6 +52,14 @@ static enum s390_host do_detect_host(void *buf)
>  	return HOST_IS_UNKNOWN;
>  }
>  
> +enum s390_host detect_host_early(void)
> +{
> +	if (stsi_get_fc() == 2)
> +		return HOST_IS_LPAR;
> +
> +	return HOST_IS_UNKNOWN;
> +}
> +
>  enum s390_host detect_host(void)
>  {
>  	static enum s390_host host = HOST_IS_UNKNOWN;
> diff --git a/lib/s390x/hardware.h b/lib/s390x/hardware.h
> index 86fe873c..5e5a9d90 100644
> --- a/lib/s390x/hardware.h
> +++ b/lib/s390x/hardware.h
> @@ -24,6 +24,7 @@ enum s390_host {
>  };
>  
>  enum s390_host detect_host(void);
> +enum s390_host detect_host_early(void);

I wonder if it weren't easier to fix detect_host so it can be used
early.... 

>  
>  static inline uint16_t get_machine_id(void)
>  {

Re: [kvm-unit-tests PATCH 1/3] lib: s390x: hw: Provide early detect host

[Janosch Frank]

On 10/10/23 12:40, Claudio Imbrenda wrote:
> On Tue, 10 Oct 2023 07:38:53 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> For early sclp printing it's necessary to know if we're under LPAR or
>> not so we can apply compat SCLP ASCII transformations.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/hardware.c | 8 ++++++++
>>   lib/s390x/hardware.h | 1 +
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/lib/s390x/hardware.c b/lib/s390x/hardware.c
>> index 2bcf9c4c..d5a752c0 100644
>> --- a/lib/s390x/hardware.c
>> +++ b/lib/s390x/hardware.c
>> @@ -52,6 +52,14 @@ static enum s390_host do_detect_host(void *buf)
>>   	return HOST_IS_UNKNOWN;
>>   }
>>   
>> +enum s390_host detect_host_early(void)
>> +{
>> +	if (stsi_get_fc() == 2)
>> +		return HOST_IS_LPAR;
>> +
>> +	return HOST_IS_UNKNOWN;
>> +}
>> +
>>   enum s390_host detect_host(void)
>>   {
>>   	static enum s390_host host = HOST_IS_UNKNOWN;
>> diff --git a/lib/s390x/hardware.h b/lib/s390x/hardware.h
>> index 86fe873c..5e5a9d90 100644
>> --- a/lib/s390x/hardware.h
>> +++ b/lib/s390x/hardware.h
>> @@ -24,6 +24,7 @@ enum s390_host {
>>   };
>>   
>>   enum s390_host detect_host(void);
>> +enum s390_host detect_host_early(void);
> 
> I wonder if it weren't easier to fix detect_host so it can be used
> early....
> 

Problem is, where do you start and where do you end?

We could statically allocate a page but why did we add the current 
version then? (The old version did that if I remember correctly).

Re: [kvm-unit-tests PATCH 3/3] lib: s390x: sclp: Add line mode input handling

[Janosch Frank]

On 10/10/23 12:33, Claudio Imbrenda wrote:
> On Tue, 10 Oct 2023 07:38:55 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Time to add line-mode input so we can use input handling under LPAR if
>> there's no access to a ASCII console.
>>
>> Line-mode IO is pretty wild and the documentation could be improved a
>> lot. Hence I've copied the input parsing functions from Linux.
>>
>> For some reason output is a type 2 event but input is a type 1
>> event. This also means that the input and output structures are
>> different from each other.
>>
>> The input can consist of multiple structures which don't contain text
>> data before the input text data is reached. Hence we need a bunch of
>> search functions to retrieve a pointer to the text data.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/sclp-console.c | 180 ++++++++++++++++++++++++++++++++++-----
>>   lib/s390x/sclp.h         |  26 +++++-
>>   2 files changed, 185 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
>> index 313be1e4..23c09b70 100644
>> --- a/lib/s390x/sclp-console.c
>> +++ b/lib/s390x/sclp-console.c
>> @@ -1,8 +1,12 @@
>>   /* SPDX-License-Identifier: GPL-2.0-or-later */
>>   /*
>> - * SCLP ASCII access driver
>> + * SCLP line mode and ASCII console driver
>>    *
>>    * Copyright (c) 2013 Alexander Graf <agraf@suse.de>
>> + *
>> + * Copyright IBM Corp. 1999
>> + * Author(s): Martin Peschke <mpeschke@de.ibm.com>
>> + *	      Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> from the weird copyright notices that you are adding I deduce that you
> copied those functions from the kernel. maybe add a line in the patch
> description to say so? or at least explain better in the comment itself.

You mean this line which is in the patch description?
"Hence I've copied the input parsing functions from Linux."

But sure, I could add that after "SCLP line mode and ASCII console driver"

Re: [kvm-unit-tests PATCH 1/3] lib: s390x: hw: Provide early detect host

[Claudio Imbrenda]

On Tue, 10 Oct 2023 13:03:08 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 10/10/23 12:40, Claudio Imbrenda wrote:
> > On Tue, 10 Oct 2023 07:38:53 +0000
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> For early sclp printing it's necessary to know if we're under LPAR or
> >> not so we can apply compat SCLP ASCII transformations.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>   lib/s390x/hardware.c | 8 ++++++++
> >>   lib/s390x/hardware.h | 1 +
> >>   2 files changed, 9 insertions(+)
> >>
> >> diff --git a/lib/s390x/hardware.c b/lib/s390x/hardware.c
> >> index 2bcf9c4c..d5a752c0 100644
> >> --- a/lib/s390x/hardware.c
> >> +++ b/lib/s390x/hardware.c
> >> @@ -52,6 +52,14 @@ static enum s390_host do_detect_host(void *buf)
> >>   	return HOST_IS_UNKNOWN;
> >>   }
> >>   
> >> +enum s390_host detect_host_early(void)
> >> +{
> >> +	if (stsi_get_fc() == 2)
> >> +		return HOST_IS_LPAR;
> >> +
> >> +	return HOST_IS_UNKNOWN;
> >> +}
> >> +
> >>   enum s390_host detect_host(void)
> >>   {
> >>   	static enum s390_host host = HOST_IS_UNKNOWN;
> >> diff --git a/lib/s390x/hardware.h b/lib/s390x/hardware.h
> >> index 86fe873c..5e5a9d90 100644
> >> --- a/lib/s390x/hardware.h
> >> +++ b/lib/s390x/hardware.h
> >> @@ -24,6 +24,7 @@ enum s390_host {
> >>   };
> >>   
> >>   enum s390_host detect_host(void);
> >> +enum s390_host detect_host_early(void);  
> > 
> > I wonder if it weren't easier to fix detect_host so it can be used
> > early....
> >   
> 
> Problem is, where do you start and where do you end?
> 
> We could statically allocate a page but why did we add the current 
> version then? (The old version did that if I remember correctly).

the old version also allocated pages, and was a hodgepodge of various
functions replicating the same behaviour, many calling the others. the
main goal of the current version was to make it more readable and
maintainable. 

a possible fix could also involve allocating the buffer on the stack of
do_detect_host(), so it's not taking up memory permanently.

that said, I don't have a strong opinion about this, but maybe it's a
good idea to not replicate the same behaviour again :)

if you don't want to fix detect_host(), then maybe something like this
instead?

diff --git a/lib/s390x/hardware.h b/lib/s390x/hardware.h
index 86fe873c..5e5a9d90 100644
--- a/lib/s390x/hardware.h
+++ b/lib/s390x/hardware.h
@@ -24,6 +24,7 @@ enum s390_host {
 };
 
 enum s390_host detect_host(void);
+enum s390_host detect_host_early(void);
 
 static inline uint16_t get_machine_id(void)
 {
diff --git a/lib/s390x/hardware.c b/lib/s390x/hardware.c
index 2bcf9c4c..b281cf10 100644
--- a/lib/s390x/hardware.c
+++ b/lib/s390x/hardware.c
@@ -28,7 +28,7 @@ static enum s390_host do_detect_host(void *buf)
        if (stsi_get_fc() == 2)
                return HOST_IS_LPAR;
 
-       if (stsi_get_fc() != 3)
+       if (!buf || stsi_get_fc() != 3)
                return HOST_IS_UNKNOWN;
 
        if (!stsi(buf, 1, 1, 1)) {
@@ -67,3 +67,8 @@ enum s390_host detect_host(void)
        initialized = true;
        return host;
 }
+
+enum s390_host detect_host_early(void)
+{
+       return do_detect_host(NULL);
+}

Re: [kvm-unit-tests PATCH 3/3] lib: s390x: sclp: Add line mode input handling

[Claudio Imbrenda]

On Tue, 10 Oct 2023 13:05:59 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 10/10/23 12:33, Claudio Imbrenda wrote:
> > On Tue, 10 Oct 2023 07:38:55 +0000
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> Time to add line-mode input so we can use input handling under LPAR if
> >> there's no access to a ASCII console.
> >>
> >> Line-mode IO is pretty wild and the documentation could be improved a
> >> lot. Hence I've copied the input parsing functions from Linux.
> >>
> >> For some reason output is a type 2 event but input is a type 1
> >> event. This also means that the input and output structures are
> >> different from each other.
> >>
> >> The input can consist of multiple structures which don't contain text
> >> data before the input text data is reached. Hence we need a bunch of
> >> search functions to retrieve a pointer to the text data.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>   lib/s390x/sclp-console.c | 180 ++++++++++++++++++++++++++++++++++-----
> >>   lib/s390x/sclp.h         |  26 +++++-
> >>   2 files changed, 185 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
> >> index 313be1e4..23c09b70 100644
> >> --- a/lib/s390x/sclp-console.c
> >> +++ b/lib/s390x/sclp-console.c
> >> @@ -1,8 +1,12 @@
> >>   /* SPDX-License-Identifier: GPL-2.0-or-later */
> >>   /*
> >> - * SCLP ASCII access driver
> >> + * SCLP line mode and ASCII console driver
> >>    *
> >>    * Copyright (c) 2013 Alexander Graf <agraf@suse.de>
> >> + *
> >> + * Copyright IBM Corp. 1999
> >> + * Author(s): Martin Peschke <mpeschke@de.ibm.com>
> >> + *	      Martin Schwidefsky <schwidefsky@de.ibm.com>  
> > 
> > from the weird copyright notices that you are adding I deduce that you
> > copied those functions from the kernel. maybe add a line in the patch
> > description to say so? or at least explain better in the comment itself.  
> 
> You mean this line which is in the patch description?
> "Hence I've copied the input parsing functions from Linux."

oufff, I'm blind sorry

> 
> But sure, I could add that after "SCLP line mode and ASCII console driver"

yeah that would bring it in line with the rest of the other files which
have been (partially) copied from the kernel

Re: [kvm-unit-tests PATCH 2/3] lib: s390x: sclp: Add compat handling for HMC ASCII consoles

[Nico Boehr]

Quoting Janosch Frank (2023-10-10 10:57:24)
> On 10/10/23 10:35, Nico Boehr wrote:
> > Quoting Janosch Frank (2023-10-10 09:38:54)
> > [...]
> >> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
> >> index 19c74e46..313be1e4 100644
> >> --- a/lib/s390x/sclp-console.c
> >> +++ b/lib/s390x/sclp-console.c
> > [...]
> >> +static bool lpar_ascii_compat;
> > 
> > This only toggles adding \r. So why not name it accordingly?
> 
> Because it also toggles clearing the screen

OK

[...]
> >> @@ -97,14 +100,27 @@ static void sclp_print_ascii(const char *str)
> >>   {
> >>          int len = strlen(str);
> >>          WriteEventData *sccb = (void *)_sccb;
> >> +       char *str_dest = (char *)&sccb->msg;
> >> +       int i = 0;
> >>   
> >>          sclp_mark_busy();
> >>          memset(sccb, 0, sizeof(*sccb));
> >> +
> >> +       for (; i < len; i++) {
> >> +               *str_dest = str[i];
> >> +               str_dest++;
> >> +               /* Add a \r to the \n for HMC ASCII console */
> >> +               if (str[i] == '\n' && lpar_ascii_compat) {
> >> +                       *str_dest = '\r';
> >> +                       str_dest++;
> >> +               }
> >> +       }
> > 
> > Please don't hide the check inside the loop.
> > Do:
> > if (lpar_ascii_compat)
> >    // your loop
> > else
> >    memcpy()
> 
> 
> I'd rather have a loop than to nest it inside an if.

I disagree, but it's not worth discussing too much over this.

> > Also, please add protection against overflowing sccb->msg (max 4088 bytes
> > if I looked it up right).
> 
> I considered this but we already have a 2k length check before that

...which is not sufficient...

> and I'd like to see someone print ~2k in a single call.
> 
> The question is if we want the complexity for something that we'll very 
> likely never hit.

IMO we want it since it can lead to random memory corruption which can be
very hard to debug.

And I don't think it's complex since in the simplest case you could just go
with a strnlen() here. Better just truncate the string than to corrupt
random memory.

Re: [kvm-unit-tests PATCH 1/3] lib: s390x: hw: Provide early detect host

[Janosch Frank]

On 10/10/23 13:42, Claudio Imbrenda wrote:
> On Tue, 10 Oct 2023 13:03:08 +0200
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 10/10/23 12:40, Claudio Imbrenda wrote:
>>> On Tue, 10 Oct 2023 07:38:53 +0000
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>    
>>>> For early sclp printing it's necessary to know if we're under LPAR or
>>>> not so we can apply compat SCLP ASCII transformations.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>    lib/s390x/hardware.c | 8 ++++++++
>>>>    lib/s390x/hardware.h | 1 +
>>>>    2 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/lib/s390x/hardware.c b/lib/s390x/hardware.c
>>>> index 2bcf9c4c..d5a752c0 100644
>>>> --- a/lib/s390x/hardware.c
>>>> +++ b/lib/s390x/hardware.c
>>>> @@ -52,6 +52,14 @@ static enum s390_host do_detect_host(void *buf)
>>>>    	return HOST_IS_UNKNOWN;
>>>>    }
>>>>    
>>>> +enum s390_host detect_host_early(void)
>>>> +{
>>>> +	if (stsi_get_fc() == 2)
>>>> +		return HOST_IS_LPAR;
>>>> +
>>>> +	return HOST_IS_UNKNOWN;
>>>> +}
>>>> +
>>>>    enum s390_host detect_host(void)
>>>>    {
>>>>    	static enum s390_host host = HOST_IS_UNKNOWN;
>>>> diff --git a/lib/s390x/hardware.h b/lib/s390x/hardware.h
>>>> index 86fe873c..5e5a9d90 100644
>>>> --- a/lib/s390x/hardware.h
>>>> +++ b/lib/s390x/hardware.h
>>>> @@ -24,6 +24,7 @@ enum s390_host {
>>>>    };
>>>>    
>>>>    enum s390_host detect_host(void);
>>>> +enum s390_host detect_host_early(void);
>>>
>>> I wonder if it weren't easier to fix detect_host so it can be used
>>> early....
>>>    
>>
>> Problem is, where do you start and where do you end?
>>
>> We could statically allocate a page but why did we add the current
>> version then? (The old version did that if I remember correctly).
> 
> the old version also allocated pages, and was a hodgepodge of various
> functions replicating the same behaviour, many calling the others. the
> main goal of the current version was to make it more readable and
> maintainable.
> 
> a possible fix could also involve allocating the buffer on the stack of
> do_detect_host(), so it's not taking up memory permanently.
> 

We have 64k of stack and since we're calling this from the setup code a 
huge part should be free. I'll have a look at this.