* [kvm-unit-tests PATCH v3 0/3] s390x: css: pv: css test adaptation for PV
@ 2021-01-19 19:52 Pierre Morel
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 1/3] s390x: pv: implement routine to share/unshare memory Pierre Morel
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Pierre Morel @ 2021-01-19 19:52 UTC (permalink / raw)
To: kvm
Cc: linux-s390, frankja, david, thuth, cohuck, imbrenda, drjones, pbonzini
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.
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.
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
lib/s390x/asm/uv.h | 38 ++++++++++++++++++++++++++++++++
lib/s390x/css.h | 3 +--
lib/s390x/css_lib.c | 28 +++++++-----------------
lib/s390x/malloc_io.c | 50 +++++++++++++++++++++++++++++++++++++++++++
lib/s390x/malloc_io.h | 18 ++++++++++++++++
s390x/Makefile | 1 +
s390x/css.c | 35 ++++++++++++++++++++----------
7 files changed, 140 insertions(+), 33 deletions(-)
create mode 100644 lib/s390x/malloc_io.c
create mode 100644 lib/s390x/malloc_io.h
--
2.17.1
changelog:
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [kvm-unit-tests PATCH v3 1/3] s390x: pv: implement routine to share/unshare memory
2021-01-19 19:52 [kvm-unit-tests PATCH v3 0/3] s390x: css: pv: css test adaptation for PV Pierre Morel
@ 2021-01-19 19:52 ` Pierre Morel
2021-01-20 10:48 ` Thomas Huth
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 2/3] s390x: define UV compatible I/O allocation Pierre Morel
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 3/3] s390x: css: pv: css test adaptation for PV Pierre Morel
2 siblings, 1 reply; 13+ messages in thread
From: Pierre Morel @ 2021-01-19 19:52 UTC (permalink / raw)
To: kvm
Cc: linux-s390, frankja, david, thuth, cohuck, imbrenda, drjones, pbonzini
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>
---
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..1242336 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [kvm-unit-tests PATCH v3 2/3] s390x: define UV compatible I/O allocation
2021-01-19 19:52 [kvm-unit-tests PATCH v3 0/3] s390x: css: pv: css test adaptation for PV Pierre Morel
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 1/3] s390x: pv: implement routine to share/unshare memory Pierre Morel
@ 2021-01-19 19:52 ` Pierre Morel
2021-01-20 11:01 ` Thomas Huth
2021-01-20 12:25 ` Claudio Imbrenda
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 3/3] s390x: css: pv: css test adaptation for PV Pierre Morel
2 siblings, 2 replies; 13+ messages in thread
From: Pierre Morel @ 2021-01-19 19:52 UTC (permalink / raw)
To: kvm
Cc: linux-s390, frankja, david, thuth, cohuck, imbrenda, drjones, pbonzini
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>
---
lib/s390x/malloc_io.c | 50 +++++++++++++++++++++++++++++++++++++++++++
lib/s390x/malloc_io.h | 18 ++++++++++++++++
s390x/Makefile | 1 +
3 files changed, 69 insertions(+)
create mode 100644 lib/s390x/malloc_io.c
create mode 100644 lib/s390x/malloc_io.h
diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
new file mode 100644
index 0000000..2a946e0
--- /dev/null
+++ b/lib/s390x/malloc_io.c
@@ -0,0 +1,50 @@
+/*
+ * I/O page allocation
+ *
+ * Copyright (c) 2021 IBM Corp
+ *
+ * Authors:
+ * Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ *
+ * 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>
+
+void *alloc_io_page(int size)
+{
+ void *p;
+
+ assert(size <= PAGE_SIZE);
+
+ p = alloc_pages_flags(1, AREA_DMA31);
+ if (!p)
+ return NULL;
+ memset(p, 0, PAGE_SIZE);
+
+ if (!test_facility(158))
+ return p;
+
+ if (uv_set_shared((unsigned long)p) == 0)
+ return p;
+
+ free_pages(p);
+ return NULL;
+}
+
+void free_io_page(void *p)
+{
+ if (test_facility(158))
+ uv_remove_shared((unsigned long)p);
+ free_pages(p);
+}
diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
new file mode 100644
index 0000000..f780191
--- /dev/null
+++ b/lib/s390x/malloc_io.h
@@ -0,0 +1,18 @@
+/*
+ * I/O allocations
+ *
+ * Copyright (c) 2021 IBM Corp
+ *
+ * Authors:
+ * Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+#ifndef _S390X_MALLOC_IO_H_
+#define _S390X_MALLOC_IO_H_
+
+void *alloc_io_page(int size);
+void free_io_page(void *p);
+
+#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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [kvm-unit-tests PATCH v3 3/3] s390x: css: pv: css test adaptation for PV
2021-01-19 19:52 [kvm-unit-tests PATCH v3 0/3] s390x: css: pv: css test adaptation for PV Pierre Morel
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 1/3] s390x: pv: implement routine to share/unshare memory Pierre Morel
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 2/3] s390x: define UV compatible I/O allocation Pierre Morel
@ 2021-01-19 19:52 ` Pierre Morel
2021-01-20 12:03 ` Thomas Huth
2 siblings, 1 reply; 13+ messages in thread
From: Pierre Morel @ 2021-01-19 19:52 UTC (permalink / raw)
To: kvm
Cc: linux-s390, frankja, david, thuth, cohuck, imbrenda, drjones, pbonzini
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 | 35 ++++++++++++++++++++++++-----------
3 files changed, 33 insertions(+), 33 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..6a0a0ec 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_page(sizeof(*ccw));
+ 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..4b0b6b1 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,9 +83,15 @@ 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);
+ senseid = alloc_io_page(sizeof(*senseid));
+ if (!senseid)
+ goto error_senseid;
+
+ ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
+ if (!ccw)
+ goto error_ccw;
+
+ ret = start_ccw1_chain(test_device_sid, ccw);
if (ret)
goto error;
@@ -97,7 +106,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 +114,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_page(ccw);
+error_ccw:
+ free_io_page(senseid);
+error_senseid:
unregister_io_int_func(css_irq_io);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [kvm-unit-tests PATCH v3 1/3] s390x: pv: implement routine to share/unshare memory
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 1/3] s390x: pv: implement routine to share/unshare memory Pierre Morel
@ 2021-01-20 10:48 ` Thomas Huth
2021-01-20 12:16 ` Pierre Morel
2021-01-20 16:48 ` Claudio Imbrenda
0 siblings, 2 replies; 13+ messages in thread
From: Thomas Huth @ 2021-01-20 10:48 UTC (permalink / raw)
To: Pierre Morel, kvm
Cc: linux-s390, frankja, david, cohuck, imbrenda, drjones, pbonzini
On 19/01/2021 20.52, 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>
> ---
> 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..1242336 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))
You can drop the innermost parentheses.
> + 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
When is it real, and when is it absolute?
> + */
> +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
dito
> + */
> +static inline int uv_remove_shared(unsigned long addr)
> +{
> + return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
> +}
> +
> #endif
Apart from the nits:
Acked-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [kvm-unit-tests PATCH v3 2/3] s390x: define UV compatible I/O allocation
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 2/3] s390x: define UV compatible I/O allocation Pierre Morel
@ 2021-01-20 11:01 ` Thomas Huth
2021-01-20 12:39 ` Pierre Morel
2021-01-20 12:25 ` Claudio Imbrenda
1 sibling, 1 reply; 13+ messages in thread
From: Thomas Huth @ 2021-01-20 11:01 UTC (permalink / raw)
To: Pierre Morel, kvm
Cc: linux-s390, frankja, david, cohuck, imbrenda, drjones, pbonzini
On 19/01/2021 20.52, 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>
> ---
> lib/s390x/malloc_io.c | 50 +++++++++++++++++++++++++++++++++++++++++++
> lib/s390x/malloc_io.h | 18 ++++++++++++++++
> s390x/Makefile | 1 +
> 3 files changed, 69 insertions(+)
> create mode 100644 lib/s390x/malloc_io.c
> create mode 100644 lib/s390x/malloc_io.h
>
> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
> new file mode 100644
> index 0000000..2a946e0
> --- /dev/null
> +++ b/lib/s390x/malloc_io.c
> @@ -0,0 +1,50 @@
> +/*
> + * I/O page allocation
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + * Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2.
Janosch recently started to introduce SPDX identifieres to the s390x code,
so I think it would be good to use them here, too.
> + * 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>
> +
> +void *alloc_io_page(int size)
> +{
> + void *p;
> +
> + assert(size <= PAGE_SIZE);
Apart from the assert() statement, the size parameter seems to be completely
unused. It's also weird to have the function named alloc_something_page()
and then have a parameter that takes bytes. Thus I'd suggest to either drop
the size parameter completely, or to rename the function to alloc_io_mem and
then to alloc multiple pages below in case the size is bigger than
PAGE_SIZE. Or maybe even to name the function alloc_io_pages and then use
"int num_pages" as a parameter, allowing to allocate multiple pages at once?
> +
> + p = alloc_pages_flags(1, AREA_DMA31);
> + if (!p)
> + return NULL;
> + memset(p, 0, PAGE_SIZE);
> +
> + if (!test_facility(158))
> + return p;
> +
> + if (uv_set_shared((unsigned long)p) == 0)
> + return p;
> +
> + free_pages(p);
> + return NULL;
> +}
> +
> +void free_io_page(void *p)
> +{
> + if (test_facility(158))
> + uv_remove_shared((unsigned long)p);
> + free_pages(p);
> +}
> diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
> new file mode 100644
> index 0000000..f780191
> --- /dev/null
> +++ b/lib/s390x/malloc_io.h
> @@ -0,0 +1,18 @@
> +/*
> + * I/O allocations
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + * Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2.
> + */
Please also add SPDX license information here.
Thomas
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [kvm-unit-tests PATCH v3 3/3] s390x: css: pv: css test adaptation for PV
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 3/3] s390x: css: pv: css test adaptation for PV Pierre Morel
@ 2021-01-20 12:03 ` Thomas Huth
2021-01-21 9:07 ` Pierre Morel
0 siblings, 1 reply; 13+ messages in thread
From: Thomas Huth @ 2021-01-20 12:03 UTC (permalink / raw)
To: Pierre Morel, kvm
Cc: linux-s390, frankja, david, cohuck, imbrenda, drjones, pbonzini
On 19/01/2021 20.52, 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>
> ---
[...]
> diff --git a/s390x/css.c b/s390x/css.c
> index ee3bc83..4b0b6b1 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,9 +83,15 @@ 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);
> + senseid = alloc_io_page(sizeof(*senseid));
Would it make sense to move the above alloc_io_page into the ccw_alloc()
function, too?
> + if (!senseid)
> + goto error_senseid;
> +
> + ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
> + if (!ccw)
> + goto error_ccw;
> +
> + ret = start_ccw1_chain(test_device_sid, ccw);
> if (ret)
> goto error;
I think you should add a "report(0, ...)" or report_abort() in front of all
three gotos above - otherwise the problems might go unnoticed.
> @@ -97,7 +106,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 +114,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_page(ccw);
> +error_ccw:
> + free_io_page(senseid);
> +error_senseid:
> unregister_io_int_func(css_irq_io);
> }
>
>
Thomas
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [kvm-unit-tests PATCH v3 1/3] s390x: pv: implement routine to share/unshare memory
2021-01-20 10:48 ` Thomas Huth
@ 2021-01-20 12:16 ` Pierre Morel
2021-01-20 16:48 ` Claudio Imbrenda
1 sibling, 0 replies; 13+ messages in thread
From: Pierre Morel @ 2021-01-20 12:16 UTC (permalink / raw)
To: Thomas Huth, kvm
Cc: linux-s390, frankja, david, cohuck, imbrenda, drjones, pbonzini
On 1/20/21 11:48 AM, Thomas Huth wrote:
> On 19/01/2021 20.52, 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>
>> ---
>> 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..1242336 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))
>
> You can drop the innermost parentheses.
OK.
>
>> + 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
>
> When is it real, and when is it absolute?
It only depends on the prefixing, the call can use both.
>
>> + */
>> +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
>
> dito
>
>> + */
>> +static inline int uv_remove_shared(unsigned long addr)
>> +{
>> + return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
>> +}
>> +
>> #endif
>
> Apart from the nits:
> Acked-by: Thomas Huth <thuth@redhat.com>
>
Thanks thomas for reviewing.
Regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [kvm-unit-tests PATCH v3 2/3] s390x: define UV compatible I/O allocation
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 2/3] s390x: define UV compatible I/O allocation Pierre Morel
2021-01-20 11:01 ` Thomas Huth
@ 2021-01-20 12:25 ` Claudio Imbrenda
2021-01-20 12:41 ` Pierre Morel
1 sibling, 1 reply; 13+ messages in thread
From: Claudio Imbrenda @ 2021-01-20 12:25 UTC (permalink / raw)
To: Pierre Morel
Cc: kvm, linux-s390, frankja, david, thuth, cohuck, drjones, pbonzini
On Tue, 19 Jan 2021 20:52:23 +0100
Pierre Morel <pmorel@linux.ibm.com> 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>
> ---
> lib/s390x/malloc_io.c | 50
> +++++++++++++++++++++++++++++++++++++++++++ lib/s390x/malloc_io.h |
> 18 ++++++++++++++++ s390x/Makefile | 1 +
> 3 files changed, 69 insertions(+)
> create mode 100644 lib/s390x/malloc_io.c
> create mode 100644 lib/s390x/malloc_io.h
>
> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
> new file mode 100644
> index 0000000..2a946e0
> --- /dev/null
> +++ b/lib/s390x/malloc_io.c
> @@ -0,0 +1,50 @@
> +/*
> + * I/O page allocation
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + * Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify
> it
> + * under the terms of the GNU General Public License version 2.
> + *
> + * 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>
> +
> +void *alloc_io_page(int size)
> +{
> + void *p;
> +
> + assert(size <= PAGE_SIZE);
I agree with Thomas here, remove size, or use it as a page count or
page order.
> + p = alloc_pages_flags(1, AREA_DMA31);
you are allocating 2 pages here...
> + if (!p)
> + return NULL;
> + memset(p, 0, PAGE_SIZE);
...and then clearing only one
but since you did not specify FLAG_DONTZERO, the page has been cleared
already by the allocator
> +
> + if (!test_facility(158))
> + return p;
> +
> + if (uv_set_shared((unsigned long)p) == 0)
> + return p;
> +
> + free_pages(p);
> + return NULL;
> +}
> +
> +void free_io_page(void *p)
> +{
> + if (test_facility(158))
> + uv_remove_shared((unsigned long)p);
> + free_pages(p);
> +}
> diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
> new file mode 100644
> index 0000000..f780191
> --- /dev/null
> +++ b/lib/s390x/malloc_io.h
> @@ -0,0 +1,18 @@
> +/*
> + * I/O allocations
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + * Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify
> it
> + * under the terms of the GNU General Public License version 2.
> + */
> +#ifndef _S390X_MALLOC_IO_H_
> +#define _S390X_MALLOC_IO_H_
> +
> +void *alloc_io_page(int size);
> +void free_io_page(void *p);
> +
> +#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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [kvm-unit-tests PATCH v3 2/3] s390x: define UV compatible I/O allocation
2021-01-20 11:01 ` Thomas Huth
@ 2021-01-20 12:39 ` Pierre Morel
0 siblings, 0 replies; 13+ messages in thread
From: Pierre Morel @ 2021-01-20 12:39 UTC (permalink / raw)
To: Thomas Huth, kvm
Cc: linux-s390, frankja, david, cohuck, imbrenda, drjones, pbonzini
On 1/20/21 12:01 PM, Thomas Huth wrote:
> On 19/01/2021 20.52, 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>
>> ---
>> lib/s390x/malloc_io.c | 50 +++++++++++++++++++++++++++++++++++++++++++
>> lib/s390x/malloc_io.h | 18 ++++++++++++++++
>> s390x/Makefile | 1 +
>> 3 files changed, 69 insertions(+)
>> create mode 100644 lib/s390x/malloc_io.c
>> create mode 100644 lib/s390x/malloc_io.h
>>
>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>> new file mode 100644
>> index 0000000..2a946e0
>> --- /dev/null
>> +++ b/lib/s390x/malloc_io.c
>> @@ -0,0 +1,50 @@
>> +/*
>> + * I/O page allocation
>> + *
>> + * Copyright (c) 2021 IBM Corp
>> + *
>> + * Authors:
>> + * Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2.
>
> Janosch recently started to introduce SPDX identifieres to the s390x
> code, so I think it would be good to use them here, too.
>
>> + * 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>
>> +
>> +void *alloc_io_page(int size)
>> +{
>> + void *p;
>> +
>> + assert(size <= PAGE_SIZE);
>
> Apart from the assert() statement, the size parameter seems to be
> completely unused. It's also weird to have the function named
right.
> alloc_something_page() and then have a parameter that takes bytes. Thus
> I'd suggest to either drop the size parameter completely, or to rename
> the function to alloc_io_mem and then to alloc multiple pages below in
> case the size is bigger than PAGE_SIZE. Or maybe even to name the
> function alloc_io_pages and then use "int num_pages" as a parameter,
> allowing to allocate multiple pages at once?
OK, may bet using order as with the alloc_pages_flags would be fine.
Then I will need a new flag in the pages.
>
>> +
>> + p = alloc_pages_flags(1, AREA_DMA31);
humm 0 here (Claudio)
>> + if (!p)
>> + return NULL;
>> + memset(p, 0, PAGE_SIZE);
>> +
>> + if (!test_facility(158))
>> + return p;
>> +
>> + if (uv_set_shared((unsigned long)p) == 0)
>> + return p;
>> +
>> + free_pages(p);
>> + return NULL;
>> +}
>> +
>> +void free_io_page(void *p)
>> +{
>> + if (test_facility(158))
>> + uv_remove_shared((unsigned long)p);
>> + free_pages(p);
>> +}
>> diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
>> new file mode 100644
>> index 0000000..f780191
>> --- /dev/null
>> +++ b/lib/s390x/malloc_io.h
>> @@ -0,0 +1,18 @@
>> +/*
>> + * I/O allocations
>> + *
>> + * Copyright (c) 2021 IBM Corp
>> + *
>> + * Authors:
>> + * Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2.
>> + */
> Please also add SPDX license information here.
Will do.
Thanks for reviewing,
Pierre
>
> Thomas
>
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [kvm-unit-tests PATCH v3 2/3] s390x: define UV compatible I/O allocation
2021-01-20 12:25 ` Claudio Imbrenda
@ 2021-01-20 12:41 ` Pierre Morel
0 siblings, 0 replies; 13+ messages in thread
From: Pierre Morel @ 2021-01-20 12:41 UTC (permalink / raw)
To: Claudio Imbrenda
Cc: kvm, linux-s390, frankja, david, thuth, cohuck, drjones, pbonzini
On 1/20/21 1:25 PM, Claudio Imbrenda wrote:
> On Tue, 19 Jan 2021 20:52:23 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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>
>> ---
>> lib/s390x/malloc_io.c | 50
>> +++++++++++++++++++++++++++++++++++++++++++ lib/s390x/malloc_io.h |
>> 18 ++++++++++++++++ s390x/Makefile | 1 +
>> 3 files changed, 69 insertions(+)
>> create mode 100644 lib/s390x/malloc_io.c
>> create mode 100644 lib/s390x/malloc_io.h
>>
>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>> new file mode 100644
>> index 0000000..2a946e0
>> --- /dev/null
>> +++ b/lib/s390x/malloc_io.c
>> @@ -0,0 +1,50 @@
>> +/*
>> + * I/O page allocation
>> + *
>> + * Copyright (c) 2021 IBM Corp
>> + *
>> + * Authors:
>> + * Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + * This code is free software; you can redistribute it and/or modify
>> it
>> + * under the terms of the GNU General Public License version 2.
>> + *
>> + * 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>
>> +
>> +void *alloc_io_page(int size)
>> +{
>> + void *p;
>> +
>> + assert(size <= PAGE_SIZE);
>
> I agree with Thomas here, remove size, or use it as a page count or
> page order.
>
>> + p = alloc_pages_flags(1, AREA_DMA31);
>
> you are allocating 2 pages here...
humm, yes, forgot to change this as I changed to your interface.
Thanks for the reviewing,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [kvm-unit-tests PATCH v3 1/3] s390x: pv: implement routine to share/unshare memory
2021-01-20 10:48 ` Thomas Huth
2021-01-20 12:16 ` Pierre Morel
@ 2021-01-20 16:48 ` Claudio Imbrenda
1 sibling, 0 replies; 13+ messages in thread
From: Claudio Imbrenda @ 2021-01-20 16:48 UTC (permalink / raw)
To: Thomas Huth
Cc: Pierre Morel, kvm, linux-s390, frankja, david, cohuck, drjones, pbonzini
On Wed, 20 Jan 2021 11:48:52 +0100
Thomas Huth <thuth@redhat.com> wrote:
[...]
> > +/*
> > + * 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
>
> When is it real, and when is it absolute?
as far as we are concerned, it's unpredictable
this means that a guest should avoid sharing any prefix (or
reverse prefix) pages.
> > + */
> > +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
>
> dito
same
> > + */
> > +static inline int uv_remove_shared(unsigned long addr)
> > +{
> > + return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
> > +}
> > +
> > #endif
>
> Apart from the nits:
> Acked-by: Thomas Huth <thuth@redhat.com>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [kvm-unit-tests PATCH v3 3/3] s390x: css: pv: css test adaptation for PV
2021-01-20 12:03 ` Thomas Huth
@ 2021-01-21 9:07 ` Pierre Morel
0 siblings, 0 replies; 13+ messages in thread
From: Pierre Morel @ 2021-01-21 9:07 UTC (permalink / raw)
To: Thomas Huth, kvm
Cc: linux-s390, frankja, david, cohuck, imbrenda, drjones, pbonzini
On 1/20/21 1:03 PM, Thomas Huth wrote:
> On 19/01/2021 20.52, 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>
>> ---
> [...]
>> diff --git a/s390x/css.c b/s390x/css.c
>> index ee3bc83..4b0b6b1 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,9 +83,15 @@ 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);
>> + senseid = alloc_io_page(sizeof(*senseid));
>
> Would it make sense to move the above alloc_io_page into the ccw_alloc()
> function, too?
If the goal is to have all allocations inside the ccw_alloc(),
I don't think so, we may have an already allocated buffer for which we
want to pass the address without any allocation inside ccw_alloc() to
reuse the same buffer.
>
>> + if (!senseid)
>> + goto error_senseid;
>> +
>> + ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid),
>> CCW_F_SLI);
>> + if (!ccw)
>> + goto error_ccw;
>> +
>> + ret = start_ccw1_chain(test_device_sid, ccw);
>> if (ret)
>> goto error;
>
> I think you should add a "report(0, ...)" or report_abort() in front of
> all three gotos above - otherwise the problems might go unnoticed.
Yes, right, I will do this,
Thanks.
Pierre
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-01-21 9:09 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-19 19:52 [kvm-unit-tests PATCH v3 0/3] s390x: css: pv: css test adaptation for PV Pierre Morel
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 1/3] s390x: pv: implement routine to share/unshare memory Pierre Morel
2021-01-20 10:48 ` Thomas Huth
2021-01-20 12:16 ` Pierre Morel
2021-01-20 16:48 ` Claudio Imbrenda
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 2/3] s390x: define UV compatible I/O allocation Pierre Morel
2021-01-20 11:01 ` Thomas Huth
2021-01-20 12:39 ` Pierre Morel
2021-01-20 12:25 ` Claudio Imbrenda
2021-01-20 12:41 ` Pierre Morel
2021-01-19 19:52 ` [kvm-unit-tests PATCH v3 3/3] s390x: css: pv: css test adaptation for PV Pierre Morel
2021-01-20 12:03 ` Thomas Huth
2021-01-21 9:07 ` Pierre Morel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).