[kvm-unit-tests PATCH v4 0/3] s390x: css: pv: css test adaptation for PV

[kvm-unit-tests PATCH v4 0/3] s390x: css: pv: css test adaptation for PV

[Pierre Morel]

Hi all,

To adapt the CSS I/O tests to protected virtualisation we need
utilities to:

1- allocate the I/O buffers in a private page using (patch 2)
   It must be in a dedicated page to avoid exporting code or
   guest private data to the host.
   We accept a size in byte and flags and allocate page integral
   memory to handle the Protected Virtualization.

2- share the I/O buffers with the host (patch 1)
   This patch uses the page allocator reworked by Claudio.

The 2 first patches are the implementation of the tools,
patch 3 is the modification of the css.c test for PV.

The checkpatch always asked me to modify MAINTAINERS,
so I added me as reviewer to be in copy for CSS at least.
May be we could create a finer grain MAINTAINERS in the
future.

regards,
Pierre

Pierre Morel (3):
  s390x: pv: implement routine to share/unshare memory
  s390x: define UV compatible I/O allocation
  s390x: css: pv: css test adaptation for PV

 MAINTAINERS           |  1 +
 lib/s390x/asm/uv.h    | 38 +++++++++++++++++++++++
 lib/s390x/css.h       |  3 +-
 lib/s390x/css_lib.c   | 28 +++++------------
 lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
 s390x/Makefile        |  1 +
 s390x/css.c           | 43 ++++++++++++++++++--------
 8 files changed, 195 insertions(+), 34 deletions(-)
 create mode 100644 lib/s390x/malloc_io.c
 create mode 100644 lib/s390x/malloc_io.h

-- 
2.17.1

changelog:

from v3:
- add failure report to the new allocations in css.c
  (Thomas)

- rework alloc_io_page and free_io_page
  (Thomas, Claudio)

- add SPDX licenses
  (Thomas)

- add comment for the functions declaration.

- add me as reviewer for the CSS

from v2:

- use the page allocator reworked by Claudio

from v1:

- add the kvm-unit-test header

- correct checks for errors on Set/Remove Shared Access

- Add report for uv Set/Remove Shared Access

- zero the allocated I/O page before use

- free the page on error.

[kvm-unit-tests PATCH v4 1/3] s390x: pv: implement routine to share/unshare memory

[Pierre Morel]

When communicating with the host we need to share part of
the memory.

Let's implement the ultravisor calls for this.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Suggested-by: Janosch Frank <frankja@linux.ibm.com>
Acked-by: Cornelia Huck <cohuck@redhat.com>
Acked-by: Thomas Huth <thuth@redhat.com>
---
 lib/s390x/asm/uv.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
index 4c2fc48..8400026 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -71,4 +71,42 @@ static inline int uv_call(unsigned long r1, unsigned long r2)
 	return cc;
 }
 
+static inline int share(unsigned long addr, u16 cmd)
+{
+	struct uv_cb_share uvcb = {
+		.header.cmd = cmd,
+		.header.len = sizeof(uvcb),
+		.paddr = addr
+	};
+	int cc;
+
+	cc = uv_call(0, (u64)&uvcb);
+	if (!cc && uvcb.header.rc == 0x0001)
+		return 0;
+
+	report_info("cc %d response code: %04x", cc, uvcb.header.rc);
+	return -1;
+}
+
+/*
+ * Guest 2 request to the Ultravisor to make a page shared with the
+ * hypervisor for IO.
+ *
+ * @addr: Real or absolute address of the page to be shared
+ */
+static inline int uv_set_shared(unsigned long addr)
+{
+	return share(addr, UVC_CMD_SET_SHARED_ACCESS);
+}
+
+/*
+ * Guest 2 request to the Ultravisor to make a page unshared.
+ *
+ * @addr: Real or absolute address of the page to be unshared
+ */
+static inline int uv_remove_shared(unsigned long addr)
+{
+	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
+}
+
 #endif
-- 
2.17.1

[kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Pierre Morel]

To centralize the memory allocation for I/O we define
the alloc_io_page/free_io_page functions which share the I/O
memory with the host in case the guest runs with
protected virtualization.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 MAINTAINERS           |  1 +
 lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
 s390x/Makefile        |  1 +
 4 files changed, 117 insertions(+)
 create mode 100644 lib/s390x/malloc_io.c
 create mode 100644 lib/s390x/malloc_io.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 54124f6..89cb01e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -82,6 +82,7 @@ M: Thomas Huth <thuth@redhat.com>
 M: David Hildenbrand <david@redhat.com>
 M: Janosch Frank <frankja@linux.ibm.com>
 R: Cornelia Huck <cohuck@redhat.com>
+R: Pierre Morel <pmorel@linux.ibm.com>
 L: kvm@vger.kernel.org
 L: linux-s390@vger.kernel.org
 F: s390x/*
diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
new file mode 100644
index 0000000..bfe8c6a
--- /dev/null
+++ b/lib/s390x/malloc_io.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * I/O page allocation
+ *
+ * Copyright (c) 2021 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * Using this interface provide host access to the allocated pages in
+ * case the guest is a secure guest.
+ * This is needed for I/O buffers.
+ *
+ */
+#include <libcflat.h>
+#include <asm/page.h>
+#include <asm/uv.h>
+#include <malloc_io.h>
+#include <alloc_page.h>
+#include <asm/facility.h>
+
+static int share_pages(void *p, int count)
+{
+	int i = 0;
+
+	for (i = 0; i < count; i++, p += PAGE_SIZE)
+		if (uv_set_shared((unsigned long)p))
+			return i;
+	return i;
+}
+
+static void unshare_pages(void *p, int count)
+{
+	int i;
+
+	for (i = count; i > 0; i--, p += PAGE_SIZE)
+		uv_remove_shared((unsigned long)p);
+}
+
+void *alloc_io_pages(int size, int flags)
+{
+	int order = (size >> PAGE_SHIFT);
+	void *p;
+	int n;
+
+	assert(size);
+
+	p = alloc_pages_flags(order, AREA_DMA31 | flags);
+	if (!p || !test_facility(158))
+		return p;
+
+	n = share_pages(p, 1 << order);
+	if (n == 1 << order)
+		return p;
+
+	unshare_pages(p, n);
+	free_pages(p);
+	return NULL;
+}
+
+void free_io_pages(void *p, int size)
+{
+	int order = size >> PAGE_SHIFT;
+
+	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
+
+	if (test_facility(158))
+		unshare_pages(p, 1 << order);
+	free_pages(p);
+}
diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
new file mode 100644
index 0000000..494dfe9
--- /dev/null
+++ b/lib/s390x/malloc_io.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * I/O allocations
+ *
+ * Copyright (c) 2021 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ */
+#ifndef _S390X_MALLOC_IO_H_
+#define _S390X_MALLOC_IO_H_
+
+/*
+ * Allocates a page aligned page bound range of contiguous real or
+ * absolute memory in the DMA31 region large enough to contain size
+ * bytes.
+ * If Protected Virtualization facility is present, shares the pages
+ * with the host.
+ * If all the pages for the specified size cannot be reserved,
+ * the function rewinds the partial allocation and a NULL pointer
+ * is returned.
+ *
+ * @size: the minimal size allocated in byte.
+ * @flags: the flags used for the underlying page allocator.
+ *
+ * Errors:
+ *   The allocation will assert the size parameter, will fail if the
+ *   underlying page allocator fail or in the case of protected
+ *   virtualisation if the sharing of the pages fails.
+ *
+ * Returns a pointer to the first page in case of success, NULL otherwise.
+ */
+void *alloc_io_pages(int size, int flags);
+
+/*
+ * Frees a previously memory space allocated by alloc_io_pages.
+ * If Protected Virtualization facility is present, unshares the pages
+ * with the host.
+ * The address must be aligned on a page boundary otherwise an assertion
+ * breaks the program.
+ */
+void free_io_pages(void *p, int size);
+
+#endif /* _S390X_MALLOC_IO_H_ */
diff --git a/s390x/Makefile b/s390x/Makefile
index b079a26..4b6301c 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -63,6 +63,7 @@ cflatobjs += lib/s390x/smp.o
 cflatobjs += lib/s390x/vm.o
 cflatobjs += lib/s390x/css_dump.o
 cflatobjs += lib/s390x/css_lib.o
+cflatobjs += lib/s390x/malloc_io.o
 
 OBJDIRS += lib/s390x
 
-- 
2.17.1

[kvm-unit-tests PATCH v4 3/3] s390x: css: pv: css test adaptation for PV

[Pierre Morel]

We want the tests to automatically work with or without protected
virtualisation.
To do this we need to share the I/O memory with the host.

Let's replace all static allocations with dynamic allocations
to clearly separate shared and private memory.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h     |  3 +--
 lib/s390x/css_lib.c | 28 ++++++++--------------------
 s390x/css.c         | 43 +++++++++++++++++++++++++++++++------------
 3 files changed, 40 insertions(+), 34 deletions(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 221b67c..e3dee9f 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -283,8 +283,7 @@ int css_enable(int schid, int isc);
 
 /* Library functions */
 int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw);
-int start_single_ccw(unsigned int sid, int code, void *data, int count,
-		     unsigned char flags);
+struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags);
 void css_irq_io(void);
 int css_residual_count(unsigned int schid);
 
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index 8e02371..f31098d 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -18,6 +18,7 @@
 #include <asm/time.h>
 #include <asm/arch_def.h>
 
+#include <malloc_io.h>
 #include <css.h>
 
 static struct schib schib;
@@ -202,33 +203,20 @@ int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw)
 	return ssch(sid, &orb);
 }
 
-/*
- * In the future, we want to implement support for CCW chains;
- * for that, we will need to work with ccw1 pointers.
- */
-static struct ccw1 unique_ccw;
-
-int start_single_ccw(unsigned int sid, int code, void *data, int count,
-		     unsigned char flags)
+struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
 {
-	int cc;
-	struct ccw1 *ccw = &unique_ccw;
+	struct ccw1 *ccw;
+
+	ccw = alloc_io_pages(sizeof(*ccw), 0);
+	if (!ccw)
+		return NULL;
 
-	report_prefix_push("start_subchannel");
-	/* Build the CCW chain with a single CCW */
 	ccw->code = code;
 	ccw->flags = flags;
 	ccw->count = count;
 	ccw->data_address = (int)(unsigned long)data;
 
-	cc = start_ccw1_chain(sid, ccw);
-	if (cc) {
-		report(0, "cc = %d", cc);
-		report_prefix_pop();
-		return cc;
-	}
-	report_prefix_pop();
-	return 0;
+	return ccw;
 }
 
 /* wait_and_check_io_completion:
diff --git a/s390x/css.c b/s390x/css.c
index ee3bc83..01378e5 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -17,13 +17,15 @@
 #include <interrupt.h>
 #include <asm/arch_def.h>
 
+#include <malloc_io.h>
 #include <css.h>
+#include <asm/barrier.h>
 
 #define DEFAULT_CU_TYPE		0x3832 /* virtio-ccw */
 static unsigned long cu_type = DEFAULT_CU_TYPE;
 
 static int test_device_sid;
-static struct senseid senseid;
+static struct senseid *senseid;
 
 static void test_enumerate(void)
 {
@@ -57,6 +59,7 @@ static void test_enable(void)
  */
 static void test_sense(void)
 {
+	struct ccw1 *ccw;
 	int ret;
 	int len;
 
@@ -80,11 +83,23 @@ static void test_sense(void)
 
 	lowcore_ptr->io_int_param = 0;
 
-	memset(&senseid, 0, sizeof(senseid));
-	ret = start_single_ccw(test_device_sid, CCW_CMD_SENSE_ID,
-			       &senseid, sizeof(senseid), CCW_F_SLI);
-	if (ret)
+	senseid = alloc_io_pages(sizeof(*senseid), 0);
+	if (!senseid) {
+		report(0, "Allocation of senseid");
+		goto error_senseid;
+	}
+
+	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
+	if (!ccw) {
+		report(0, "Allocation of CCW");
+		goto error_ccw;
+	}
+
+	ret = start_ccw1_chain(test_device_sid, ccw);
+	if (ret) {
+		report(0, "Starting CCW chain");
 		goto error;
+	}
 
 	if (wait_and_check_io_completion(test_device_sid) < 0)
 		goto error;
@@ -97,7 +112,7 @@ static void test_sense(void)
 	if (ret < 0) {
 		report_info("no valid residual count");
 	} else if (ret != 0) {
-		len = sizeof(senseid) - ret;
+		len = sizeof(*senseid) - ret;
 		if (ret && len < CSS_SENSEID_COMMON_LEN) {
 			report(0, "transferred a too short length: %d", ret);
 			goto error;
@@ -105,21 +120,25 @@ static void test_sense(void)
 			report_info("transferred a shorter length: %d", len);
 	}
 
-	if (senseid.reserved != 0xff) {
-		report(0, "transferred garbage: 0x%02x", senseid.reserved);
+	if (senseid->reserved != 0xff) {
+		report(0, "transferred garbage: 0x%02x", senseid->reserved);
 		goto error;
 	}
 
 	report_prefix_pop();
 
 	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
-		    senseid.reserved, senseid.cu_type, senseid.cu_model,
-		    senseid.dev_type, senseid.dev_model);
+		    senseid->reserved, senseid->cu_type, senseid->cu_model,
+		    senseid->dev_type, senseid->dev_model);
 
-	report(senseid.cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
-	       (uint16_t) cu_type, senseid.cu_type);
+	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
+	       (uint16_t)cu_type, senseid->cu_type);
 
 error:
+	free_io_pages(ccw, sizeof(*ccw));
+error_ccw:
+	free_io_pages(senseid, sizeof(*senseid));
+error_senseid:
 	unregister_io_int_func(css_irq_io);
 }
 
-- 
2.17.1

Re: [kvm-unit-tests PATCH v4 1/3] s390x: pv: implement routine to share/unshare memory

[Janosch Frank]

On 1/21/21 10:13 AM, Pierre Morel wrote:
> When communicating with the host we need to share part of
> the memory.
> 
> Let's implement the ultravisor calls for this.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Suggested-by: Janosch Frank <frankja@linux.ibm.com>
> Acked-by: Cornelia Huck <cohuck@redhat.com>
> Acked-by: Thomas Huth <thuth@redhat.com>
> ---
>  lib/s390x/asm/uv.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index 4c2fc48..8400026 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -71,4 +71,42 @@ static inline int uv_call(unsigned long r1, unsigned long r2)
>  	return cc;
>  }
>  
> +static inline int share(unsigned long addr, u16 cmd)
> +{
> +	struct uv_cb_share uvcb = {
> +		.header.cmd = cmd,
> +		.header.len = sizeof(uvcb),
> +		.paddr = addr
> +	};
> +	int cc;
> +
> +	cc = uv_call(0, (u64)&uvcb);
> +	if (!cc && uvcb.header.rc == 0x0001)

s/0x0001/UVC_RC_EXECUTED/


> +		return 0;
> +
> +	report_info("cc %d response code: %04x", cc, uvcb.header.rc);

Will the print have the string UV in it or will I need to guess that a
UV call failed?

I'm wondering if an assert would make more sense, if callers are
interested in the uv rc they will need to write an own share function
anyway.

> +	return -1;
> +}
> +
> +/*
> + * Guest 2 request to the Ultravisor to make a page shared with the
> + * hypervisor for IO.
> + *
> + * @addr: Real or absolute address of the page to be shared
> + */
> +static inline int uv_set_shared(unsigned long addr)
> +{
> +	return share(addr, UVC_CMD_SET_SHARED_ACCESS);
> +}
> +
> +/*
> + * Guest 2 request to the Ultravisor to make a page unshared.
> + *
> + * @addr: Real or absolute address of the page to be unshared
> + */
> +static inline int uv_remove_shared(unsigned long addr)
> +{
> +	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
> +}
> +
>  #endif
> 

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Thomas Huth]

On 21/01/2021 10.13, Pierre Morel wrote:
> To centralize the memory allocation for I/O we define
> the alloc_io_page/free_io_page functions which share the I/O
> memory with the host in case the guest runs with
> protected virtualization.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   MAINTAINERS           |  1 +
>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>   s390x/Makefile        |  1 +
>   4 files changed, 117 insertions(+)
>   create mode 100644 lib/s390x/malloc_io.c
>   create mode 100644 lib/s390x/malloc_io.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 54124f6..89cb01e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -82,6 +82,7 @@ M: Thomas Huth <thuth@redhat.com>
>   M: David Hildenbrand <david@redhat.com>
>   M: Janosch Frank <frankja@linux.ibm.com>
>   R: Cornelia Huck <cohuck@redhat.com>
> +R: Pierre Morel <pmorel@linux.ibm.com>
>   L: kvm@vger.kernel.org
>   L: linux-s390@vger.kernel.org
>   F: s390x/*
> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
> new file mode 100644
> index 0000000..bfe8c6a
> --- /dev/null
> +++ b/lib/s390x/malloc_io.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0

"GPL-2.0" is deprecated according to https://spdx.org/licenses/ ... please 
use "GPL-2.0-only" instead.

> +/*
> + * I/O page allocation
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * Using this interface provide host access to the allocated pages in
> + * case the guest is a secure guest.
> + * This is needed for I/O buffers.
> + *
> + */
> +#include <libcflat.h>
> +#include <asm/page.h>
> +#include <asm/uv.h>
> +#include <malloc_io.h>
> +#include <alloc_page.h>
> +#include <asm/facility.h>
> +
> +static int share_pages(void *p, int count)
> +{
> +	int i = 0;
> +
> +	for (i = 0; i < count; i++, p += PAGE_SIZE)
> +		if (uv_set_shared((unsigned long)p))
> +			return i;

Just a matter of taste, but you could replace the "return i" here also with 
a "break" since you're returning i below anyway.

> +	return i;
> +}
> +
> +static void unshare_pages(void *p, int count)
> +{
> +	int i;
> +
> +	for (i = count; i > 0; i--, p += PAGE_SIZE)
> +		uv_remove_shared((unsigned long)p);
> +}
> +
> +void *alloc_io_pages(int size, int flags)

I still think the naming or size parameter is confusing here. If I read 
something like alloc_io_pages(), I'd expect a "num_pages" parameter. So if 
you want to keep the "size" in bytes, I'd suggest to rename the function to 
"alloc_io_mem" instead.

> +{
> +	int order = (size >> PAGE_SHIFT);

I think this is wrong. According to the description of alloc_pages_flag, it 
allocates "1ull << order" pages.
So you likely want to do this instead here:

         int order = get_order(size >> PAGE_SHIFT);

> +	void *p;
> +	int n;
> +
> +	assert(size);
> +
> +	p = alloc_pages_flags(order, AREA_DMA31 | flags);
> +	if (!p || !test_facility(158))
> +		return p;
> +
> +	n = share_pages(p, 1 << order);
> +	if (n == 1 << order)
> +		return p;
> +
> +	unshare_pages(p, n);
> +	free_pages(p);
> +	return NULL;
> +}
> +
> +void free_io_pages(void *p, int size)
> +{
> +	int order = size >> PAGE_SHIFT;

dito?

> +	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
> +
> +	if (test_facility(158))
> +		unshare_pages(p, 1 << order);
> +	free_pages(p);
> +}
> diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
> new file mode 100644
> index 0000000..494dfe9
> --- /dev/null
> +++ b/lib/s390x/malloc_io.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

GPL-2.0-only please.

> +/*
> + * I/O allocations
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + */
> +#ifndef _S390X_MALLOC_IO_H_
> +#define _S390X_MALLOC_IO_H_
> +
> +/*
> + * Allocates a page aligned page bound range of contiguous real or
> + * absolute memory in the DMA31 region large enough to contain size
> + * bytes.
> + * If Protected Virtualization facility is present, shares the pages
> + * with the host.
> + * If all the pages for the specified size cannot be reserved,
> + * the function rewinds the partial allocation and a NULL pointer
> + * is returned.
> + *
> + * @size: the minimal size allocated in byte.
> + * @flags: the flags used for the underlying page allocator.
> + *
> + * Errors:
> + *   The allocation will assert the size parameter, will fail if the
> + *   underlying page allocator fail or in the case of protected
> + *   virtualisation if the sharing of the pages fails.

I think "virtualization" (with an z) is more common than "virtualisation".

> + *
> + * Returns a pointer to the first page in case of success, NULL otherwise.
> + */
> +void *alloc_io_pages(int size, int flags);
> +
> +/*
> + * Frees a previously memory space allocated by alloc_io_pages.
> + * If Protected Virtualization facility is present, unshares the pages
> + * with the host.
> + * The address must be aligned on a page boundary otherwise an assertion
> + * breaks the program.
> + */
> +void free_io_pages(void *p, int size);
> +
> +#endif /* _S390X_MALLOC_IO_H_ */
> diff --git a/s390x/Makefile b/s390x/Makefile
> index b079a26..4b6301c 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -63,6 +63,7 @@ cflatobjs += lib/s390x/smp.o
>   cflatobjs += lib/s390x/vm.o
>   cflatobjs += lib/s390x/css_dump.o
>   cflatobjs += lib/s390x/css_lib.o
> +cflatobjs += lib/s390x/malloc_io.o
>   
>   OBJDIRS += lib/s390x

  Thomas

Re: [kvm-unit-tests PATCH v4 3/3] s390x: css: pv: css test adaptation for PV

[Thomas Huth]

On 21/01/2021 10.13, Pierre Morel wrote:
> We want the tests to automatically work with or without protected
> virtualisation.
> To do this we need to share the I/O memory with the host.
> 
> Let's replace all static allocations with dynamic allocations
> to clearly separate shared and private memory.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/css.h     |  3 +--
>   lib/s390x/css_lib.c | 28 ++++++++--------------------
>   s390x/css.c         | 43 +++++++++++++++++++++++++++++++------------
>   3 files changed, 40 insertions(+), 34 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Janosch Frank]

On 1/21/21 10:13 AM, Pierre Morel wrote:
> To centralize the memory allocation for I/O we define
> the alloc_io_page/free_io_page functions which share the I/O
> memory with the host in case the guest runs with
> protected virtualization.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  MAINTAINERS           |  1 +
>  lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>  s390x/Makefile        |  1 +
>  4 files changed, 117 insertions(+)
>  create mode 100644 lib/s390x/malloc_io.c
>  create mode 100644 lib/s390x/malloc_io.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 54124f6..89cb01e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -82,6 +82,7 @@ M: Thomas Huth <thuth@redhat.com>
>  M: David Hildenbrand <david@redhat.com>
>  M: Janosch Frank <frankja@linux.ibm.com>
>  R: Cornelia Huck <cohuck@redhat.com>
> +R: Pierre Morel <pmorel@linux.ibm.com>

If you're ok with the amount of mails you'll get then go ahead.
But I think maintainer file changes should always be in a separate patch.

>  L: kvm@vger.kernel.org
>  L: linux-s390@vger.kernel.org
>  F: s390x/*
> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
> new file mode 100644
> index 0000000..bfe8c6a
> --- /dev/null
> +++ b/lib/s390x/malloc_io.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0

I think we wanted to use:
/* SPDX-License-Identifier: GPL-2.0-or-later */

> +/*
> + * I/O page allocation
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * Using this interface provide host access to the allocated pages in
> + * case the guest is a secure guest.
> + * This is needed for I/O buffers.
> + *
> + */
> +#include <libcflat.h>
> +#include <asm/page.h>
> +#include <asm/uv.h>
> +#include <malloc_io.h>
> +#include <alloc_page.h>
> +#include <asm/facility.h>
> +
> +static int share_pages(void *p, int count)
> +{
> +	int i = 0;
> +
> +	for (i = 0; i < count; i++, p += PAGE_SIZE)
> +		if (uv_set_shared((unsigned long)p))
> +			return i;
> +	return i;
> +}
> +
> +static void unshare_pages(void *p, int count)
> +{
> +	int i;
> +
> +	for (i = count; i > 0; i--, p += PAGE_SIZE)
> +		uv_remove_shared((unsigned long)p);
> +}
> +
> +void *alloc_io_pages(int size, int flags)
> +{
> +	int order = (size >> PAGE_SHIFT);
> +	void *p;
> +	int n;
> +
> +	assert(size);
> +
> +	p = alloc_pages_flags(order, AREA_DMA31 | flags);
> +	if (!p || !test_facility(158))
> +		return p;
> +
> +	n = share_pages(p, 1 << order);
> +	if (n == 1 << order)
> +		return p;
> +
> +	unshare_pages(p, n);
> +	free_pages(p);
> +	return NULL;
> +}
> +
> +void free_io_pages(void *p, int size)
> +{
> +	int order = size >> PAGE_SHIFT;
> +
> +	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
> +
> +	if (test_facility(158))
> +		unshare_pages(p, 1 << order);

I have a lib file in the making that will let you check UV features like
test_facility(). When that's ready I'm gonna check for unshare here.

> +	free_pages(p);
> +}
> diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
> new file mode 100644
> index 0000000..494dfe9
> --- /dev/null
> +++ b/lib/s390x/malloc_io.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * I/O allocations
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + */
> +#ifndef _S390X_MALLOC_IO_H_
> +#define _S390X_MALLOC_IO_H_
> +
> +/*
> + * Allocates a page aligned page bound range of contiguous real or
> + * absolute memory in the DMA31 region large enough to contain size
> + * bytes.
> + * If Protected Virtualization facility is present, shares the pages
> + * with the host.
> + * If all the pages for the specified size cannot be reserved,
> + * the function rewinds the partial allocation and a NULL pointer
> + * is returned.
> + *
> + * @size: the minimal size allocated in byte.
> + * @flags: the flags used for the underlying page allocator.
> + *
> + * Errors:
> + *   The allocation will assert the size parameter, will fail if the
> + *   underlying page allocator fail or in the case of protected
> + *   virtualisation if the sharing of the pages fails.
> + *
> + * Returns a pointer to the first page in case of success, NULL otherwise.
> + */
> +void *alloc_io_pages(int size, int flags);
> +
> +/*
> + * Frees a previously memory space allocated by alloc_io_pages.
> + * If Protected Virtualization facility is present, unshares the pages
> + * with the host.
> + * The address must be aligned on a page boundary otherwise an assertion
> + * breaks the program.
> + */
> +void free_io_pages(void *p, int size);
> +
> +#endif /* _S390X_MALLOC_IO_H_ */
> diff --git a/s390x/Makefile b/s390x/Makefile
> index b079a26..4b6301c 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -63,6 +63,7 @@ cflatobjs += lib/s390x/smp.o
>  cflatobjs += lib/s390x/vm.o
>  cflatobjs += lib/s390x/css_dump.o
>  cflatobjs += lib/s390x/css_lib.o
> +cflatobjs += lib/s390x/malloc_io.o
>  
>  OBJDIRS += lib/s390x
>  
> 

Re: [kvm-unit-tests PATCH v4 1/3] s390x: pv: implement routine to share/unshare memory

[Janosch Frank]

On 1/21/21 10:20 AM, Janosch Frank wrote:
> On 1/21/21 10:13 AM, Pierre Morel wrote:
>> When communicating with the host we need to share part of
>> the memory.
>>
>> Let's implement the ultravisor calls for this.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Suggested-by: Janosch Frank <frankja@linux.ibm.com>
>> Acked-by: Cornelia Huck <cohuck@redhat.com>
>> Acked-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  lib/s390x/asm/uv.h | 38 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>
>> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
>> index 4c2fc48..8400026 100644
>> --- a/lib/s390x/asm/uv.h
>> +++ b/lib/s390x/asm/uv.h
>> @@ -71,4 +71,42 @@ static inline int uv_call(unsigned long r1, unsigned long r2)
>>  	return cc;
>>  }
>>  
>> +static inline int share(unsigned long addr, u16 cmd)
>> +{
>> +	struct uv_cb_share uvcb = {
>> +		.header.cmd = cmd,
>> +		.header.len = sizeof(uvcb),
>> +		.paddr = addr
>> +	};
>> +	int cc;
>> +
>> +	cc = uv_call(0, (u64)&uvcb);
>> +	if (!cc && uvcb.header.rc == 0x0001)
> 
> s/0x0001/UVC_RC_EXECUTED/
> 
> 
>> +		return 0;
>> +
>> +	report_info("cc %d response code: %04x", cc, uvcb.header.rc);
> 
> Will the print have the string UV in it or will I need to guess that a
> UV call failed?
> 
> I'm wondering if an assert would make more sense, if callers are
> interested in the uv rc they will need to write an own share function
> anyway.

Ok, I'll take that back. In the following patches you return NULL if the
share for an allocation fails and you check for NULL after every
allocation so this is fine by me.

> 
>> +	return -1;
>> +}
>> +
>> +/*
>> + * Guest 2 request to the Ultravisor to make a page shared with the
>> + * hypervisor for IO.
>> + *
>> + * @addr: Real or absolute address of the page to be shared
>> + */
>> +static inline int uv_set_shared(unsigned long addr)
>> +{
>> +	return share(addr, UVC_CMD_SET_SHARED_ACCESS);
>> +}
>> +
>> +/*
>> + * Guest 2 request to the Ultravisor to make a page unshared.
>> + *
>> + * @addr: Real or absolute address of the page to be unshared
>> + */
>> +static inline int uv_remove_shared(unsigned long addr)
>> +{
>> +	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
>> +}
>> +
>>  #endif
>>
> 

Re: [kvm-unit-tests PATCH v4 1/3] s390x: pv: implement routine to share/unshare memory

[Pierre Morel]

On 1/21/21 10:20 AM, Janosch Frank wrote:
> On 1/21/21 10:13 AM, Pierre Morel wrote:
>> When communicating with the host we need to share part of
>> the memory.
>>
>> Let's implement the ultravisor calls for this.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Suggested-by: Janosch Frank <frankja@linux.ibm.com>
>> Acked-by: Cornelia Huck <cohuck@redhat.com>
>> Acked-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   lib/s390x/asm/uv.h | 38 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
>> index 4c2fc48..8400026 100644
>> --- a/lib/s390x/asm/uv.h
>> +++ b/lib/s390x/asm/uv.h
>> @@ -71,4 +71,42 @@ static inline int uv_call(unsigned long r1, unsigned long r2)
>>   	return cc;
>>   }
>>   
>> +static inline int share(unsigned long addr, u16 cmd)
>> +{
>> +	struct uv_cb_share uvcb = {
>> +		.header.cmd = cmd,
>> +		.header.len = sizeof(uvcb),
>> +		.paddr = addr
>> +	};
>> +	int cc;
>> +
>> +	cc = uv_call(0, (u64)&uvcb);
>> +	if (!cc && uvcb.header.rc == 0x0001)
> 
> s/0x0001/UVC_RC_EXECUTED/

OK

> 
> 
>> +		return 0;
>> +
>> +	report_info("cc %d response code: %04x", cc, uvcb.header.rc);
> 
> Will the print have the string UV in it or will I need to guess that a
> UV call failed?

I will change for a more explicit

> 
> I'm wondering if an assert would make more sense, if callers are
> interested in the uv rc they will need to write an own share function
> anyway.

No need (reported OOB by Janosch)

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Pierre Morel]

On 1/21/21 10:46 AM, Janosch Frank wrote:
> On 1/21/21 10:13 AM, Pierre Morel wrote:
>> To centralize the memory allocation for I/O we define
>> the alloc_io_page/free_io_page functions which share the I/O
>> memory with the host in case the guest runs with
>> protected virtualization.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   MAINTAINERS           |  1 +
>>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>   s390x/Makefile        |  1 +
>>   4 files changed, 117 insertions(+)
>>   create mode 100644 lib/s390x/malloc_io.c
>>   create mode 100644 lib/s390x/malloc_io.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 54124f6..89cb01e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -82,6 +82,7 @@ M: Thomas Huth <thuth@redhat.com>
>>   M: David Hildenbrand <david@redhat.com>
>>   M: Janosch Frank <frankja@linux.ibm.com>
>>   R: Cornelia Huck <cohuck@redhat.com>
>> +R: Pierre Morel <pmorel@linux.ibm.com>
> 
> If you're ok with the amount of mails you'll get then go ahead.
> But I think maintainer file changes should always be in a separate patch.

You are right it was more an attempts to bring the Linux checkpatch to 
be quiet but this would need more changes so.. we can discuss this in a 
separate patch.

> 
>>   L: kvm@vger.kernel.org
>>   L: linux-s390@vger.kernel.org
>>   F: s390x/*
>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>> new file mode 100644
>> index 0000000..bfe8c6a
>> --- /dev/null
>> +++ b/lib/s390x/malloc_io.c
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0
> 
> I think we wanted to use:
> /* SPDX-License-Identifier: GPL-2.0-or-later */

yes


...snip...
>> +
>> +void free_io_pages(void *p, int size)
>> +{
>> +	int order = size >> PAGE_SHIFT;
>> +
>> +	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
>> +
>> +	if (test_facility(158))
>> +		unshare_pages(p, 1 << order);
> 
> I have a lib file in the making that will let you check UV features like
> test_facility(). When that's ready I'm gonna check for unshare here.

OK

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 3/3] s390x: css: pv: css test adaptation for PV

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 5674 bytes --]

On 1/21/21 10:13 AM, Pierre Morel wrote:
> We want the tests to automatically work with or without protected
> virtualisation.
> To do this we need to share the I/O memory with the host.
> 
> Let's replace all static allocations with dynamic allocations
> to clearly separate shared and private memory.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Acked-by: Janosch Frank <frankja@de.ibm.com>

> ---
>  lib/s390x/css.h     |  3 +--
>  lib/s390x/css_lib.c | 28 ++++++++--------------------
>  s390x/css.c         | 43 +++++++++++++++++++++++++++++++------------
>  3 files changed, 40 insertions(+), 34 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 221b67c..e3dee9f 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -283,8 +283,7 @@ int css_enable(int schid, int isc);
>  
>  /* Library functions */
>  int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw);
> -int start_single_ccw(unsigned int sid, int code, void *data, int count,
> -		     unsigned char flags);
> +struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags);
>  void css_irq_io(void);
>  int css_residual_count(unsigned int schid);
>  
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 8e02371..f31098d 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -18,6 +18,7 @@
>  #include <asm/time.h>
>  #include <asm/arch_def.h>
>  
> +#include <malloc_io.h>
>  #include <css.h>
>  
>  static struct schib schib;
> @@ -202,33 +203,20 @@ int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw)
>  	return ssch(sid, &orb);
>  }
>  
> -/*
> - * In the future, we want to implement support for CCW chains;
> - * for that, we will need to work with ccw1 pointers.
> - */
> -static struct ccw1 unique_ccw;
> -
> -int start_single_ccw(unsigned int sid, int code, void *data, int count,
> -		     unsigned char flags)
> +struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>  {
> -	int cc;
> -	struct ccw1 *ccw = &unique_ccw;
> +	struct ccw1 *ccw;
> +
> +	ccw = alloc_io_pages(sizeof(*ccw), 0);
> +	if (!ccw)
> +		return NULL;
>  
> -	report_prefix_push("start_subchannel");
> -	/* Build the CCW chain with a single CCW */
>  	ccw->code = code;
>  	ccw->flags = flags;
>  	ccw->count = count;
>  	ccw->data_address = (int)(unsigned long)data;
>  
> -	cc = start_ccw1_chain(sid, ccw);
> -	if (cc) {
> -		report(0, "cc = %d", cc);
> -		report_prefix_pop();
> -		return cc;
> -	}
> -	report_prefix_pop();
> -	return 0;
> +	return ccw;
>  }
>  
>  /* wait_and_check_io_completion:
> diff --git a/s390x/css.c b/s390x/css.c
> index ee3bc83..01378e5 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -17,13 +17,15 @@
>  #include <interrupt.h>
>  #include <asm/arch_def.h>
>  
> +#include <malloc_io.h>
>  #include <css.h>
> +#include <asm/barrier.h>
>  
>  #define DEFAULT_CU_TYPE		0x3832 /* virtio-ccw */
>  static unsigned long cu_type = DEFAULT_CU_TYPE;
>  
>  static int test_device_sid;
> -static struct senseid senseid;
> +static struct senseid *senseid;
>  
>  static void test_enumerate(void)
>  {
> @@ -57,6 +59,7 @@ static void test_enable(void)
>   */
>  static void test_sense(void)
>  {
> +	struct ccw1 *ccw;
>  	int ret;
>  	int len;
>  
> @@ -80,11 +83,23 @@ static void test_sense(void)
>  
>  	lowcore_ptr->io_int_param = 0;
>  
> -	memset(&senseid, 0, sizeof(senseid));
> -	ret = start_single_ccw(test_device_sid, CCW_CMD_SENSE_ID,
> -			       &senseid, sizeof(senseid), CCW_F_SLI);
> -	if (ret)
> +	senseid = alloc_io_pages(sizeof(*senseid), 0);
> +	if (!senseid) {
> +		report(0, "Allocation of senseid");
> +		goto error_senseid;
> +	}
> +
> +	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
> +	if (!ccw) {
> +		report(0, "Allocation of CCW");
> +		goto error_ccw;
> +	}
> +
> +	ret = start_ccw1_chain(test_device_sid, ccw);
> +	if (ret) {
> +		report(0, "Starting CCW chain");
>  		goto error;
> +	}
>  
>  	if (wait_and_check_io_completion(test_device_sid) < 0)
>  		goto error;
> @@ -97,7 +112,7 @@ static void test_sense(void)
>  	if (ret < 0) {
>  		report_info("no valid residual count");
>  	} else if (ret != 0) {
> -		len = sizeof(senseid) - ret;
> +		len = sizeof(*senseid) - ret;
>  		if (ret && len < CSS_SENSEID_COMMON_LEN) {
>  			report(0, "transferred a too short length: %d", ret);
>  			goto error;
> @@ -105,21 +120,25 @@ static void test_sense(void)
>  			report_info("transferred a shorter length: %d", len);
>  	}
>  
> -	if (senseid.reserved != 0xff) {
> -		report(0, "transferred garbage: 0x%02x", senseid.reserved);
> +	if (senseid->reserved != 0xff) {
> +		report(0, "transferred garbage: 0x%02x", senseid->reserved);
>  		goto error;
>  	}
>  
>  	report_prefix_pop();
>  
>  	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
> -		    senseid.reserved, senseid.cu_type, senseid.cu_model,
> -		    senseid.dev_type, senseid.dev_model);
> +		    senseid->reserved, senseid->cu_type, senseid->cu_model,
> +		    senseid->dev_type, senseid->dev_model);
>  
> -	report(senseid.cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
> -	       (uint16_t) cu_type, senseid.cu_type);
> +	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
> +	       (uint16_t)cu_type, senseid->cu_type);
>  
>  error:
> +	free_io_pages(ccw, sizeof(*ccw));
> +error_ccw:
> +	free_io_pages(senseid, sizeof(*senseid));
> +error_senseid:
>  	unregister_io_int_func(css_irq_io);
>  }
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [kvm-unit-tests PATCH v4 3/3] s390x: css: pv: css test adaptation for PV

[Cornelia Huck]

On Thu, 21 Jan 2021 10:13:12 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We want the tests to automatically work with or without protected
> virtualisation.
> To do this we need to share the I/O memory with the host.
> 
> Let's replace all static allocations with dynamic allocations
> to clearly separate shared and private memory.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  3 +--
>  lib/s390x/css_lib.c | 28 ++++++++--------------------
>  s390x/css.c         | 43 +++++++++++++++++++++++++++++++------------
>  3 files changed, 40 insertions(+), 34 deletions(-)
> 

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Pierre Morel]

On 1/21/21 10:46 AM, Janosch Frank wrote:
> On 1/21/21 10:13 AM, Pierre Morel wrote:
>> To centralize the memory allocation for I/O we define
>> the alloc_io_page/free_io_page functions which share the I/O
>> memory with the host in case the guest runs with
>> protected virtualization.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   MAINTAINERS           |  1 +
>>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>   s390x/Makefile        |  1 +
>>   4 files changed, 117 insertions(+)
>>   create mode 100644 lib/s390x/malloc_io.c
>>   create mode 100644 lib/s390x/malloc_io.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 54124f6..89cb01e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -82,6 +82,7 @@ M: Thomas Huth <thuth@redhat.com>
>>   M: David Hildenbrand <david@redhat.com>
>>   M: Janosch Frank <frankja@linux.ibm.com>
>>   R: Cornelia Huck <cohuck@redhat.com>
>> +R: Pierre Morel <pmorel@linux.ibm.com>
> 
> If you're ok with the amount of mails you'll get then go ahead.
> But I think maintainer file changes should always be in a separate patch.
> 
>>   L: kvm@vger.kernel.org
>>   L: linux-s390@vger.kernel.org
>>   F: s390x/*
>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>> new file mode 100644
>> index 0000000..bfe8c6a
>> --- /dev/null
>> +++ b/lib/s390x/malloc_io.c
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0
> 
> I think we wanted to use:

@Janosch , @Thomas

> /* SPDX-License-Identifier: GPL-2.0-or-later */

or

// SPDX-License-Identifier: GPL-2.0-only

later or only ?

/* or // ?


If both are OK, I will take the Janosch proposition which is in use in 
vm.[ch] and ignore the Linux checkpatch warning.

Just to : Why are you people not using the Linux style code completely 
instead of making new exceptions.

i.e. SPDX license and MAINTAINERS


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 3/3] s390x: css: pv: css test adaptation for PV

[Pierre Morel]

On 1/21/21 1:48 PM, Cornelia Huck wrote:
> On Thu, 21 Jan 2021 10:13:12 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We want the tests to automatically work with or without protected
>> virtualisation.
>> To do this we need to share the I/O memory with the host.
>>
>> Let's replace all static allocations with dynamic allocations
>> to clearly separate shared and private memory.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  3 +--
>>   lib/s390x/css_lib.c | 28 ++++++++--------------------
>>   s390x/css.c         | 43 +++++++++++++++++++++++++++++++------------
>>   3 files changed, 40 insertions(+), 34 deletions(-)
>>
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 3/3] s390x: css: pv: css test adaptation for PV

[Pierre Morel]

On 1/21/21 10:57 AM, Janosch Frank wrote:
> On 1/21/21 10:13 AM, Pierre Morel wrote:
>> We want the tests to automatically work with or without protected
>> virtualisation.
>> To do this we need to share the I/O memory with the host.
>>
>> Let's replace all static allocations with dynamic allocations
>> to clearly separate shared and private memory.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Acked-by: Janosch Frank <frankja@de.ibm.com>

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 3/3] s390x: css: pv: css test adaptation for PV

[Pierre Morel]

On 1/21/21 10:35 AM, Thomas Huth wrote:
> On 21/01/2021 10.13, Pierre Morel wrote:
>> We want the tests to automatically work with or without protected
>> virtualisation.
>> To do this we need to share the I/O memory with the host.
>>
>> Let's replace all static allocations with dynamic allocations
>> to clearly separate shared and private memory.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  3 +--
>>   lib/s390x/css_lib.c | 28 ++++++++--------------------
>>   s390x/css.c         | 43 +++++++++++++++++++++++++++++++------------
>>   3 files changed, 40 insertions(+), 34 deletions(-)
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Pierre Morel]

On 1/21/21 10:46 AM, Janosch Frank wrote:
> On 1/21/21 10:13 AM, Pierre Morel wrote:
>> To centralize the memory allocation for I/O we define
>> the alloc_io_page/free_io_page functions which share the I/O
>> memory with the host in case the guest runs with
>> protected virtualization.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   MAINTAINERS           |  1 +
>>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>   s390x/Makefile        |  1 +
>>   4 files changed, 117 insertions(+)
>>   create mode 100644 lib/s390x/malloc_io.c
>>   create mode 100644 lib/s390x/malloc_io.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 54124f6..89cb01e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -82,6 +82,7 @@ M: Thomas Huth <thuth@redhat.com>
>>   M: David Hildenbrand <david@redhat.com>
>>   M: Janosch Frank <frankja@linux.ibm.com>
>>   R: Cornelia Huck <cohuck@redhat.com>
>> +R: Pierre Morel <pmorel@linux.ibm.com>
> 
> If you're ok with the amount of mails you'll get then go ahead.
> But I think maintainer file changes should always be in a separate patch.
> 
>>   L: kvm@vger.kernel.org
>>   L: linux-s390@vger.kernel.org
>>   F: s390x/*
>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>> new file mode 100644
>> index 0000000..bfe8c6a
>> --- /dev/null
>> +++ b/lib/s390x/malloc_io.c
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0
> 
> I think we wanted to use:
> /* SPDX-License-Identifier: GPL-2.0-or-later */
> 
>> +/*
>> + * I/O page allocation
>> + *
>> + * Copyright (c) 2021 IBM Corp
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + * Using this interface provide host access to the allocated pages in
>> + * case the guest is a secure guest.
>> + * This is needed for I/O buffers.
>> + *
>> + */
>> +#include <libcflat.h>
>> +#include <asm/page.h>
>> +#include <asm/uv.h>
>> +#include <malloc_io.h>
>> +#include <alloc_page.h>
>> +#include <asm/facility.h>
>> +
>> +static int share_pages(void *p, int count)
>> +{
>> +	int i = 0;
>> +
>> +	for (i = 0; i < count; i++, p += PAGE_SIZE)
>> +		if (uv_set_shared((unsigned long)p))
>> +			return i;
>> +	return i;
>> +}
>> +
>> +static void unshare_pages(void *p, int count)
>> +{
>> +	int i;
>> +
>> +	for (i = count; i > 0; i--, p += PAGE_SIZE)
>> +		uv_remove_shared((unsigned long)p);
>> +}
>> +
>> +void *alloc_io_pages(int size, int flags)
>> +{
>> +	int order = (size >> PAGE_SHIFT);
>> +	void *p;
>> +	int n;
>> +
>> +	assert(size);
>> +
>> +	p = alloc_pages_flags(order, AREA_DMA31 | flags);
>> +	if (!p || !test_facility(158))
>> +		return p;
>> +
>> +	n = share_pages(p, 1 << order);
>> +	if (n == 1 << order)
>> +		return p;
>> +
>> +	unshare_pages(p, n);
>> +	free_pages(p);
>> +	return NULL;
>> +}
>> +
>> +void free_io_pages(void *p, int size)
>> +{
>> +	int order = size >> PAGE_SHIFT;
>> +
>> +	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
>> +
>> +	if (test_facility(158))
>> +		unshare_pages(p, 1 << order);
> 
> I have a lib file in the making that will let you check UV features like
> test_facility(). When that's ready I'm gonna check for unshare here.

OK.
On share_pages the rc is enough I suppose.


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Thomas Huth]

On 21/01/2021 14.02, Pierre Morel wrote:
> 
> 
> On 1/21/21 10:46 AM, Janosch Frank wrote:
>> On 1/21/21 10:13 AM, Pierre Morel wrote:
>>> To centralize the memory allocation for I/O we define
>>> the alloc_io_page/free_io_page functions which share the I/O
>>> memory with the host in case the guest runs with
>>> protected virtualization.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   MAINTAINERS           |  1 +
>>>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>>   s390x/Makefile        |  1 +
>>>   4 files changed, 117 insertions(+)
>>>   create mode 100644 lib/s390x/malloc_io.c
>>>   create mode 100644 lib/s390x/malloc_io.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 54124f6..89cb01e 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -82,6 +82,7 @@ M: Thomas Huth <thuth@redhat.com>
>>>   M: David Hildenbrand <david@redhat.com>
>>>   M: Janosch Frank <frankja@linux.ibm.com>
>>>   R: Cornelia Huck <cohuck@redhat.com>
>>> +R: Pierre Morel <pmorel@linux.ibm.com>
>>
>> If you're ok with the amount of mails you'll get then go ahead.
>> But I think maintainer file changes should always be in a separate patch.
>>
>>>   L: kvm@vger.kernel.org
>>>   L: linux-s390@vger.kernel.org
>>>   F: s390x/*
>>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>>> new file mode 100644
>>> index 0000000..bfe8c6a
>>> --- /dev/null
>>> +++ b/lib/s390x/malloc_io.c
>>> @@ -0,0 +1,70 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>
>> I think we wanted to use:
> 
> @Janosch , @Thomas
> 
>> /* SPDX-License-Identifier: GPL-2.0-or-later */
> 
> or
> 
> // SPDX-License-Identifier: GPL-2.0-only
> 
> later or only ?

If it's a new file, it's up to the author. I personally prefer -later, but I 
think IBM's preference is normally -only instead. Please check with your 
colleagues.
Most s390x-related files in the kvm-unit-tests currently use "GPL-2.0-only", 
so that should be ok anyway.

> /* or // ?

I don't mind. // seems to be kernel style for .c files, but so far we've 
only used /* with SPDX in the kvm-unit-tests, so both should be fine, I think.

> Just to : Why are you people not using the Linux style code completely 
> instead of making new exceptions.
> 
> i.e. SPDX license and MAINTAINERS

Actually, I wonder why the Linux documentation still recommends the 
identifiers that are marked as deprecated on the SPDX website. The 
deprecated "GPL-2.0" can be rather confusing, since it IMHO can easily be 
mistaken as "GPL-2.0+", so the newer identifiers are better, indeed.

Not sure what you mean with MAINTAINERS, though.

  Thomas

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Pierre Morel]

On 1/21/21 2:43 PM, Thomas Huth wrote:
> On 21/01/2021 14.02, Pierre Morel wrote:
>>
>>
>> On 1/21/21 10:46 AM, Janosch Frank wrote:
>>> On 1/21/21 10:13 AM, Pierre Morel wrote:
>>>> To centralize the memory allocation for I/O we define
>>>> the alloc_io_page/free_io_page functions which share the I/O
>>>> memory with the host in case the guest runs with
>>>> protected virtualization.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>   MAINTAINERS           |  1 +
>>>>   lib/s390x/malloc_io.c | 70 
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>>>   s390x/Makefile        |  1 +
>>>>   4 files changed, 117 insertions(+)
>>>>   create mode 100644 lib/s390x/malloc_io.c
>>>>   create mode 100644 lib/s390x/malloc_io.h
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 54124f6..89cb01e 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -82,6 +82,7 @@ M: Thomas Huth <thuth@redhat.com>
>>>>   M: David Hildenbrand <david@redhat.com>
>>>>   M: Janosch Frank <frankja@linux.ibm.com>
>>>>   R: Cornelia Huck <cohuck@redhat.com>
>>>> +R: Pierre Morel <pmorel@linux.ibm.com>
>>>
>>> If you're ok with the amount of mails you'll get then go ahead.
>>> But I think maintainer file changes should always be in a separate 
>>> patch.
>>>
>>>>   L: kvm@vger.kernel.org
>>>>   L: linux-s390@vger.kernel.org
>>>>   F: s390x/*
>>>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>>>> new file mode 100644
>>>> index 0000000..bfe8c6a
>>>> --- /dev/null
>>>> +++ b/lib/s390x/malloc_io.c
>>>> @@ -0,0 +1,70 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>
>>> I think we wanted to use:
>>
>> @Janosch , @Thomas
>>
>>> /* SPDX-License-Identifier: GPL-2.0-or-later */
>>
>> or
>>
>> // SPDX-License-Identifier: GPL-2.0-only
>>
>> later or only ?
> 
> If it's a new file, it's up to the author. I personally prefer -later, 
> but I think IBM's preference is normally -only instead. Please check 
> with your colleagues.
> Most s390x-related files in the kvm-unit-tests currently use 
> "GPL-2.0-only", so that should be ok anyway.
> 
>> /* or // ?
> 
> I don't mind. // seems to be kernel style for .c files, but so far we've 
> only used /* with SPDX in the kvm-unit-tests, so both should be fine, I 
> think.
> 
>> Just to : Why are you people not using the Linux style code completely 
>> instead of making new exceptions.
>>
>> i.e. SPDX license and MAINTAINERS
> 
> Actually, I wonder why the Linux documentation still recommends the 
> identifiers that are marked as deprecated on the SPDX website. The 
> deprecated "GPL-2.0" can be rather confusing, since it IMHO can easily 
> be mistaken as "GPL-2.0+", so the newer identifiers are better, indeed.
> 
> Not sure what you mean with MAINTAINERS, though.

Thanks for the explanations :)

For MAINTAINERS, the Linux kernel checkpatch warns that we should use
TABS instead of SPACES between item and names.

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Janosch Frank]

On 1/21/21 2:02 PM, Pierre Morel wrote:
> 
> 
> On 1/21/21 10:46 AM, Janosch Frank wrote:
>> On 1/21/21 10:13 AM, Pierre Morel wrote:
>>> To centralize the memory allocation for I/O we define
>>> the alloc_io_page/free_io_page functions which share the I/O
>>> memory with the host in case the guest runs with
>>> protected virtualization.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   MAINTAINERS           |  1 +
>>>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>>   s390x/Makefile        |  1 +
>>>   4 files changed, 117 insertions(+)
>>>   create mode 100644 lib/s390x/malloc_io.c
>>>   create mode 100644 lib/s390x/malloc_io.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 54124f6..89cb01e 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -82,6 +82,7 @@ M: Thomas Huth <thuth@redhat.com>
>>>   M: David Hildenbrand <david@redhat.com>
>>>   M: Janosch Frank <frankja@linux.ibm.com>
>>>   R: Cornelia Huck <cohuck@redhat.com>
>>> +R: Pierre Morel <pmorel@linux.ibm.com>
>>
>> If you're ok with the amount of mails you'll get then go ahead.
>> But I think maintainer file changes should always be in a separate patch.
>>
>>>   L: kvm@vger.kernel.org
>>>   L: linux-s390@vger.kernel.org
>>>   F: s390x/*
>>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>>> new file mode 100644
>>> index 0000000..bfe8c6a
>>> --- /dev/null
>>> +++ b/lib/s390x/malloc_io.c
>>> @@ -0,0 +1,70 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>
>> I think we wanted to use:
> 
> @Janosch , @Thomas
> 
>> /* SPDX-License-Identifier: GPL-2.0-or-later */
> 
> or
> 
> // SPDX-License-Identifier: GPL-2.0-only
> 
> later or only ?
> 
> /* or // ?
> 
> 
> If both are OK, I will take the Janosch proposition which is in use in 
> vm.[ch] and ignore the Linux checkpatch warning.
> 
> Just to : Why are you people not using the Linux style code completely 
> instead of making new exceptions.
> 
> i.e. SPDX license and MAINTAINERS
> 

s390 also has /* */ style SPDX and GPL2.0+ statements in the kernel...

Since KUT has way less developers the style rules aren't as strict and
currently I see that as an advantage. Following checkpatch down the
cliff is a bad idea in the kernel and for unit tests. It's most often
correct, but not always.

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Thomas Huth]

On 21/01/2021 14.47, Pierre Morel wrote:
[...]
> For MAINTAINERS, the Linux kernel checkpatch warns that we should use
> TABS instead of SPACES between item and names.

Interesting, I wasn't aware of that. I guess it's simply because the 
MAINTAINERS file in kvm-unit-tests is older than the patch that changed the 
checkpatch script in the kernel, and updates to the MAINTAINRS file in k-u-t 
are so seldom that nobody really noticed afterwards.

If it bothers you, feel free to send a patch to fix k-u-t's MAINTAINERS 
file, it might be nice indeed to be aligned with the kernel here again.

  Thomas

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Pierre Morel]

On 1/21/21 2:56 PM, Thomas Huth wrote:
> On 21/01/2021 14.47, Pierre Morel wrote:
> [...]
>> For MAINTAINERS, the Linux kernel checkpatch warns that we should use
>> TABS instead of SPACES between item and names.
> 
> Interesting, I wasn't aware of that. I guess it's simply because the 
> MAINTAINERS file in kvm-unit-tests is older than the patch that changed 
> the checkpatch script in the kernel, and updates to the MAINTAINRS file 
> in k-u-t are so seldom that nobody really noticed afterwards.
> 
> If it bothers you, feel free to send a patch to fix k-u-t's MAINTAINERS 
> file, it might be nice indeed to be aligned with the kernel here again.
> 
>   Thomas
> 
> 

OK, I will do,
Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Pierre Morel]

On 1/21/21 2:48 PM, Janosch Frank wrote:
> On 1/21/21 2:02 PM, Pierre Morel wrote:
>>
>>
>> On 1/21/21 10:46 AM, Janosch Frank wrote:
>>> On 1/21/21 10:13 AM, Pierre Morel wrote:
>>>> To centralize the memory allocation for I/O we define
>>>> the alloc_io_page/free_io_page functions which share the I/O
>>>> memory with the host in case the guest runs with
>>>> protected virtualization.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>    MAINTAINERS           |  1 +
>>>>    lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>>>    lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>>>    s390x/Makefile        |  1 +
>>>>    4 files changed, 117 insertions(+)
>>>>    create mode 100644 lib/s390x/malloc_io.c
>>>>    create mode 100644 lib/s390x/malloc_io.h
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 54124f6..89cb01e 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -82,6 +82,7 @@ M: Thomas Huth <thuth@redhat.com>
>>>>    M: David Hildenbrand <david@redhat.com>
>>>>    M: Janosch Frank <frankja@linux.ibm.com>
>>>>    R: Cornelia Huck <cohuck@redhat.com>
>>>> +R: Pierre Morel <pmorel@linux.ibm.com>
>>>
>>> If you're ok with the amount of mails you'll get then go ahead.
>>> But I think maintainer file changes should always be in a separate patch.
>>>
>>>>    L: kvm@vger.kernel.org
>>>>    L: linux-s390@vger.kernel.org
>>>>    F: s390x/*
>>>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>>>> new file mode 100644
>>>> index 0000000..bfe8c6a
>>>> --- /dev/null
>>>> +++ b/lib/s390x/malloc_io.c
>>>> @@ -0,0 +1,70 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>
>>> I think we wanted to use:
>>
>> @Janosch , @Thomas
>>
>>> /* SPDX-License-Identifier: GPL-2.0-or-later */
>>
>> or
>>
>> // SPDX-License-Identifier: GPL-2.0-only
>>
>> later or only ?
>>
>> /* or // ?
>>
>>
>> If both are OK, I will take the Janosch proposition which is in use in
>> vm.[ch] and ignore the Linux checkpatch warning.
>>
>> Just to : Why are you people not using the Linux style code completely
>> instead of making new exceptions.
>>
>> i.e. SPDX license and MAINTAINERS
>>
> 
> s390 also has /* */ style SPDX and GPL2.0+ statements in the kernel...
> 
> Since KUT has way less developers the style rules aren't as strict and
> currently I see that as an advantage. Following checkpatch down the
> cliff is a bad idea in the kernel and for unit tests. It's most often
> correct, but not always.
> 

Oh OK,
thanks for the explanation,

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/3] s390x: define UV compatible I/O allocation

[Pierre Morel]

On 1/21/21 10:32 AM, Thomas Huth wrote:

...snip...

>> +#include <asm/facility.h>
>> +
>> +static int share_pages(void *p, int count)
>> +{
>> +    int i = 0;
>> +
>> +    for (i = 0; i < count; i++, p += PAGE_SIZE)
>> +        if (uv_set_shared((unsigned long)p))
>> +            return i;
> 
> Just a matter of taste, but you could replace the "return i" here also 
> with a "break" since you're returning i below anyway.

right a single out point is always better.

> 
>> +    return i;
>> +}
>> +
>> +static void unshare_pages(void *p, int count)
>> +{
>> +    int i;
>> +
>> +    for (i = count; i > 0; i--, p += PAGE_SIZE)
>> +        uv_remove_shared((unsigned long)p);
>> +}
>> +
>> +void *alloc_io_pages(int size, int flags)
> 
> I still think the naming or size parameter is confusing here. If I read 
> something like alloc_io_pages(), I'd expect a "num_pages" parameter. So 
> if you want to keep the "size" in bytes, I'd suggest to rename the 
> function to "alloc_io_mem" instead.

OK, I rename the function, allowing the user to keep a simple interface
without having to calculate the page order.

> 
>> +{
>> +    int order = (size >> PAGE_SHIFT);
> 
> I think this is wrong. According to the description of alloc_pages_flag, 
> it allocates "1ull << order" pages.
> So you likely want to do this instead here:
> 
>          int order = get_order(size >> PAGE_SHIFT);

you are absolutely right.

> 
>> +    void *p;
>> +    int n;
>> +
>> +    assert(size);
>> +
>> +    p = alloc_pages_flags(order, AREA_DMA31 | flags);
>> +    if (!p || !test_facility(158))
>> +        return p;
>> +
>> +    n = share_pages(p, 1 << order);
>> +    if (n == 1 << order)
>> +        return p;
>> +
>> +    unshare_pages(p, n);
>> +    free_pages(p);
>> +    return NULL;
>> +}
>> +
>> +void free_io_pages(void *p, int size)
>> +{
>> +    int order = size >> PAGE_SHIFT;
> 
> dito?

yes :(

> 
>> +    assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
>> +
>> +    if (test_facility(158))
>> +        unshare_pages(p, 1 << order);
>> +    free_pages(p);
>> +}
>> diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
>> new file mode 100644
>> index 0000000..494dfe9
>> --- /dev/null
>> +++ b/lib/s390x/malloc_io.h
>> @@ -0,0 +1,45 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> GPL-2.0-only please.

almmost... I use:
/* SPDX-License-Identifier: GPL-2.0-or-later */

as in other files updated by janosch if this is not a problem.

> 
>> +/*
>> + * I/O allocations
>> + *
>> + * Copyright (c) 2021 IBM Corp
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + */
>> +#ifndef _S390X_MALLOC_IO_H_
>> +#define _S390X_MALLOC_IO_H_
>> +
>> +/*
>> + * Allocates a page aligned page bound range of contiguous real or
>> + * absolute memory in the DMA31 region large enough to contain size
>> + * bytes.
>> + * If Protected Virtualization facility is present, shares the pages
>> + * with the host.
>> + * If all the pages for the specified size cannot be reserved,
>> + * the function rewinds the partial allocation and a NULL pointer
>> + * is returned.
>> + *
>> + * @size: the minimal size allocated in byte.
>> + * @flags: the flags used for the underlying page allocator.
>> + *
>> + * Errors:
>> + *   The allocation will assert the size parameter, will fail if the
>> + *   underlying page allocator fail or in the case of protected
>> + *   virtualisation if the sharing of the pages fails.
> 
> I think "virtualization" (with an z) is more common than "virtualisation".

OK


Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen