* [kvm-unit-tests PATCH v2 0/2] s390x: add tests for diag258
@ 2024-09-23 6:26 Nico Boehr
2024-09-23 6:26 ` [kvm-unit-tests PATCH v2 1/2] s390x: add test " Nico Boehr
2024-09-23 6:26 ` [kvm-unit-tests PATCH v2 2/2] s390x: edat: move LC_SIZE to arch_def.h Nico Boehr
0 siblings, 2 replies; 6+ messages in thread
From: Nico Boehr @ 2024-09-23 6:26 UTC (permalink / raw)
To: frankja, imbrenda, thuth; +Cc: kvm, linux-s390
v2:
---
* do not run test under TCG
Add tests for diag258 handling on s390x.
There recently was a bugfix in the kernel:
https://lore.kernel.org/r/20240917151904.74314-2-nrb@linux.ibm.com
This adds tests for it.
Nico Boehr (2):
s390x: add test for diag258
s390x: edat: move LC_SIZE to arch_def.h
lib/s390x/asm/arch_def.h | 1 +
s390x/Makefile | 1 +
s390x/diag258.c | 258 +++++++++++++++++++++++++++++++++++++++
s390x/edat.c | 1 -
s390x/unittests.cfg | 3 +
5 files changed, 263 insertions(+), 1 deletion(-)
create mode 100644 s390x/diag258.c
--
2.46.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [kvm-unit-tests PATCH v2 1/2] s390x: add test for diag258
2024-09-23 6:26 [kvm-unit-tests PATCH v2 0/2] s390x: add tests for diag258 Nico Boehr
@ 2024-09-23 6:26 ` Nico Boehr
2024-09-25 15:34 ` Claudio Imbrenda
2024-09-23 6:26 ` [kvm-unit-tests PATCH v2 2/2] s390x: edat: move LC_SIZE to arch_def.h Nico Boehr
1 sibling, 1 reply; 6+ messages in thread
From: Nico Boehr @ 2024-09-23 6:26 UTC (permalink / raw)
To: frankja, imbrenda, thuth; +Cc: kvm, linux-s390
This adds a test for diag258 (page ref service/async page fault).
There recently was a virtual-real address confusion bug, so we should
test:
- diag258 parameter Rx is a real adress
- crossing the end of RAM with the parameter list yields an addressing
exception
- invalid diagcode in the parameter block yields an specification
exception
- diag258 correctly applies prefixing.
Note that we're just testing error cases as of now.
Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
s390x/Makefile | 1 +
s390x/diag258.c | 258 ++++++++++++++++++++++++++++++++++++++++++++
s390x/unittests.cfg | 3 +
3 files changed, 262 insertions(+)
create mode 100644 s390x/diag258.c
diff --git a/s390x/Makefile b/s390x/Makefile
index 23342bd64f44..66d71351caab 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -44,6 +44,7 @@ tests += $(TEST_DIR)/exittime.elf
tests += $(TEST_DIR)/ex.elf
tests += $(TEST_DIR)/topology.elf
tests += $(TEST_DIR)/sie-dat.elf
+tests += $(TEST_DIR)/diag258.elf
pv-tests += $(TEST_DIR)/pv-diags.elf
pv-tests += $(TEST_DIR)/pv-icptcode.elf
diff --git a/s390x/diag258.c b/s390x/diag258.c
new file mode 100644
index 000000000000..5337534f60d1
--- /dev/null
+++ b/s390x/diag258.c
@@ -0,0 +1,258 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Diag 258: Async Page Fault Handler
+ *
+ * Copyright (c) 2024 IBM Corp
+ *
+ * Authors:
+ * Nico Boehr <nrb@linux.ibm.com>
+ */
+
+#include <libcflat.h>
+#include <asm-generic/barrier.h>
+#include <asm/asm-offsets.h>
+#include <asm/interrupt.h>
+#include <asm/mem.h>
+#include <asm/pgtable.h>
+#include <mmu.h>
+#include <sclp.h>
+#include <vmalloc.h>
+
+static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));
+
+#define __PF_RES_FIELD 0x8000000000000000UL
+
+/* copied from Linux arch/s390/mm/pfault.c */
+struct pfault_refbk {
+ u16 refdiagc;
+ u16 reffcode;
+ u16 refdwlen;
+ u16 refversn;
+ u64 refgaddr;
+ u64 refselmk;
+ u64 refcmpmk;
+ u64 reserved;
+};
+
+uint64_t pfault_token = 0x0123fadec0fe3210UL;
+
+static struct pfault_refbk pfault_init_refbk __attribute__((aligned(8))) = {
+ .refdiagc = 0x258,
+ .reffcode = 0, /* TOKEN */
+ .refdwlen = sizeof(struct pfault_refbk) / sizeof(uint64_t),
+ .refversn = 2,
+ .refgaddr = (u64)&pfault_token,
+ .refselmk = 1UL << 48,
+ .refcmpmk = 1UL << 48,
+ .reserved = __PF_RES_FIELD
+};
+
+static struct pfault_refbk pfault_cancel_refbk __attribute((aligned(8))) = {
+ .refdiagc = 0x258,
+ .reffcode = 1, /* CANCEL */
+ .refdwlen = sizeof(struct pfault_refbk) / sizeof(uint64_t),
+ .refversn = 2,
+ .refgaddr = 0,
+ .refselmk = 0,
+ .refcmpmk = 0,
+ .reserved = 0
+};
+
+static inline int diag258(struct pfault_refbk *refbk)
+{
+ int rc = -1;
+
+ asm volatile(
+ " diag %[refbk],%[rc],0x258\n"
+ : [rc] "+d" (rc)
+ : [refbk] "a" (refbk), "m" (*(refbk))
+ : "cc");
+ return rc;
+}
+
+static void test_priv(void)
+{
+ report_prefix_push("privileged");
+ expect_pgm_int();
+ enter_pstate();
+ diag258(&pfault_init_refbk);
+ check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+ report_prefix_pop();
+}
+
+static void* page_map_outside_real_space(phys_addr_t page_real)
+{
+ pgd_t *root = (pgd_t *)(stctg(1) & PAGE_MASK);
+ void* vaddr = alloc_vpage();
+
+ install_page(root, page_real, vaddr);
+
+ return vaddr;
+}
+
+/*
+ * Verify that the refbk pointer is a real address and not a virtual
+ * address. This is tested by enabling DAT and establishing a mapping
+ * for the refbk that is outside of the bounds of our (guest-)physical
+ * address space.
+ */
+static void test_refbk_real(void)
+{
+ pgd_t *root;
+ struct pfault_refbk *refbk;
+ void *refbk_page;
+
+ report_prefix_push("refbk is real");
+
+ /* Set up virtual memory and allocate a physical page for storing the refbk */
+ setup_vm();
+ refbk_page = alloc_page();
+
+ /* Map refblk page outside of physical memory identity mapping */
+ root = (pgd_t *)(stctg(1) & PAGE_MASK);
+ refbk = page_map_outside_real_space(virt_to_pte_phys(root, refbk_page));
+
+ /* Assert the mapping really is outside identity mapping */
+ report_info("refbk is at 0x%lx", (u64)refbk);
+ report_info("ram size is 0x%lx", get_ram_size());
+ assert((u64)refbk > get_ram_size());
+
+ /* Copy the init refbk to the page */
+ memcpy(refbk, &pfault_init_refbk, sizeof(struct pfault_refbk));
+
+ /* Protect the virtual mapping to avoid diag258 actually doing something */
+ protect_page(refbk, PAGE_ENTRY_I);
+
+ expect_pgm_int();
+ diag258(refbk);
+ check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
+ report_prefix_pop();
+
+ free_page(refbk_page);
+ disable_dat();
+ irq_set_dat_mode(false, 0);
+}
+
+/*
+ * Verify diag258 correctly applies prefixing.
+ */
+static void test_refbk_prefixing(void)
+{
+ uint64_t ry;
+ uint32_t old_prefix;
+ struct pfault_refbk *refbk_in_prefix, *refbk_in_reverse_prefix;
+ const size_t lowcore_offset_for_refbk = offsetof(struct lowcore, pad_0x03a0);
+
+ report_prefix_push("refbk prefixing");
+
+ report_info("refbk at lowcore offset 0x%lx", lowcore_offset_for_refbk);
+
+ assert((unsigned long)&prefix_buf < SZ_2G);
+
+ memcpy(prefix_buf, 0, LC_SIZE);
+
+ /*
+ * After the call to set_prefix() below, this will refer to absolute
+ * address lowcore_offset_for_refbk (reverse prefixing).
+ */
+ refbk_in_reverse_prefix = (struct pfault_refbk *)(&prefix_buf[0] + lowcore_offset_for_refbk);
+
+ /*
+ * After the call to set_prefix() below, this will refer to absolute
+ * address &prefix_buf[0] + lowcore_offset_for_refbk (forward prefixing).
+ */
+ refbk_in_prefix = (struct pfault_refbk *)OPAQUE_PTR(lowcore_offset_for_refbk);
+
+ old_prefix = get_prefix();
+ set_prefix((uint32_t)(uintptr_t)prefix_buf);
+
+ /*
+ * If diag258 would not be applying prefixing on access to
+ * refbk_in_reverse_prefix correctly, it would access absolute address
+ * refbk_in_reverse_prefix (which to us is accessible at real address
+ * refbk_in_prefix).
+ * Make sure it really fails by putting invalid function code
+ * at refbk_in_prefix.
+ */
+ refbk_in_prefix->refdiagc = 0xc0fe;
+
+ /*
+ * Put a valid refbk at refbk_in_reverse_prefix.
+ */
+ memcpy(refbk_in_reverse_prefix, &pfault_init_refbk, sizeof(pfault_init_refbk));
+
+ ry = diag258(refbk_in_reverse_prefix);
+ report(!ry, "real address refbk accessed");
+
+ /*
+ * Activating should have worked. Cancel the activation and expect
+ * return 0. If activation would not have worked, this should return with
+ * 4 (pfault handshaking not active).
+ */
+ ry = diag258(&pfault_cancel_refbk);
+ report(!ry, "handshaking canceled");
+
+ set_prefix(old_prefix);
+
+ report_prefix_pop();
+}
+
+/*
+ * Verify that a refbk exceeding physical memory is not accepted, even
+ * when crossing a frame boundary.
+ */
+static void test_refbk_crossing(void)
+{
+ const size_t bytes_in_last_page = 8;
+ struct pfault_refbk *refbk = (struct pfault_refbk *)(get_ram_size() - bytes_in_last_page);
+
+ report_prefix_push("refbk crossing");
+
+ report_info("refbk is at 0x%lx", (u64)refbk);
+ report_info("ram size is 0x%lx", get_ram_size());
+ assert(sizeof(struct pfault_refbk) > bytes_in_last_page);
+
+ /* Copy bytes_in_last_page bytes of the init refbk to the page */
+ memcpy(refbk, &pfault_init_refbk, bytes_in_last_page);
+
+ expect_pgm_int();
+ diag258(refbk);
+ check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
+ report_prefix_pop();
+}
+
+/*
+ * Verify that a refbk with an invalid refdiagc is not accepted.
+ */
+static void test_refbk_invalid_diagcode(void)
+{
+ struct pfault_refbk refbk = pfault_init_refbk;
+
+ report_prefix_push("invalid refdiagc");
+ refbk.refdiagc = 0xc0fe;
+
+ expect_pgm_int();
+ diag258(&refbk);
+ check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+ report_prefix_pop();
+}
+
+int main(void)
+{
+ report_prefix_push("diag258");
+
+ expect_pgm_int();
+ diag258(&pfault_init_refbk);
+ if (clear_pgm_int() == PGM_INT_CODE_SPECIFICATION) {
+ report_skip("diag258 not supported");
+ } else {
+ test_priv();
+ test_refbk_real();
+ test_refbk_prefixing();
+ test_refbk_crossing();
+ test_refbk_invalid_diagcode();
+ }
+
+ report_prefix_pop();
+ return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 3a9decc932f2..8131ba105d3f 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -392,3 +392,6 @@ file = sie-dat.elf
[pv-attest]
file = pv-attest.elf
+
+[diag258]
+file = diag258.elf
--
2.46.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [kvm-unit-tests PATCH v2 2/2] s390x: edat: move LC_SIZE to arch_def.h
2024-09-23 6:26 [kvm-unit-tests PATCH v2 0/2] s390x: add tests for diag258 Nico Boehr
2024-09-23 6:26 ` [kvm-unit-tests PATCH v2 1/2] s390x: add test " Nico Boehr
@ 2024-09-23 6:26 ` Nico Boehr
2024-09-25 14:37 ` Claudio Imbrenda
1 sibling, 1 reply; 6+ messages in thread
From: Nico Boehr @ 2024-09-23 6:26 UTC (permalink / raw)
To: frankja, imbrenda, thuth; +Cc: kvm, linux-s390
struct lowcore is defined in arch_def.h and LC_SIZE is useful to other
tests as well, therefore move it to arch_def.h.
Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
lib/s390x/asm/arch_def.h | 1 +
s390x/edat.c | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 745a33878de5..5574a45156a9 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -119,6 +119,7 @@ enum address_space {
#define CTL2_GUARDED_STORAGE (63 - 59)
+#define LC_SIZE (2 * PAGE_SIZE)
struct lowcore {
uint8_t pad_0x0000[0x0080 - 0x0000]; /* 0x0000 */
uint32_t ext_int_param; /* 0x0080 */
diff --git a/s390x/edat.c b/s390x/edat.c
index 16138397017c..e664b09d9633 100644
--- a/s390x/edat.c
+++ b/s390x/edat.c
@@ -17,7 +17,6 @@
#define PGD_PAGE_SHIFT (REGION1_SHIFT - PAGE_SHIFT)
-#define LC_SIZE (2 * PAGE_SIZE)
#define VIRT(x) ((void *)((unsigned long)(x) + (unsigned long)mem))
static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));
--
2.46.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] s390x: edat: move LC_SIZE to arch_def.h
2024-09-23 6:26 ` [kvm-unit-tests PATCH v2 2/2] s390x: edat: move LC_SIZE to arch_def.h Nico Boehr
@ 2024-09-25 14:37 ` Claudio Imbrenda
0 siblings, 0 replies; 6+ messages in thread
From: Claudio Imbrenda @ 2024-09-25 14:37 UTC (permalink / raw)
To: Nico Boehr; +Cc: frankja, thuth, kvm, linux-s390
On Mon, 23 Sep 2024 08:26:04 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:
> struct lowcore is defined in arch_def.h and LC_SIZE is useful to other
> tests as well, therefore move it to arch_def.h.
>
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
I find this patch a little weird here, since it's not used for the
previous patch, and doesn't seem to have anything to do with it either.
the patch itself is otherwise good, I'm just a little puzzled.
nonetheless:
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
> lib/s390x/asm/arch_def.h | 1 +
> s390x/edat.c | 1 -
> 2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 745a33878de5..5574a45156a9 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -119,6 +119,7 @@ enum address_space {
>
> #define CTL2_GUARDED_STORAGE (63 - 59)
>
> +#define LC_SIZE (2 * PAGE_SIZE)
> struct lowcore {
> uint8_t pad_0x0000[0x0080 - 0x0000]; /* 0x0000 */
> uint32_t ext_int_param; /* 0x0080 */
> diff --git a/s390x/edat.c b/s390x/edat.c
> index 16138397017c..e664b09d9633 100644
> --- a/s390x/edat.c
> +++ b/s390x/edat.c
> @@ -17,7 +17,6 @@
>
> #define PGD_PAGE_SHIFT (REGION1_SHIFT - PAGE_SHIFT)
>
> -#define LC_SIZE (2 * PAGE_SIZE)
> #define VIRT(x) ((void *)((unsigned long)(x) + (unsigned long)mem))
>
> static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] s390x: add test for diag258
2024-09-23 6:26 ` [kvm-unit-tests PATCH v2 1/2] s390x: add test " Nico Boehr
@ 2024-09-25 15:34 ` Claudio Imbrenda
2024-10-02 14:13 ` Nico Boehr
0 siblings, 1 reply; 6+ messages in thread
From: Claudio Imbrenda @ 2024-09-25 15:34 UTC (permalink / raw)
To: Nico Boehr; +Cc: frankja, thuth, kvm, linux-s390
On Mon, 23 Sep 2024 08:26:03 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:
> This adds a test for diag258 (page ref service/async page fault).
>
> There recently was a virtual-real address confusion bug, so we should
> test:
> - diag258 parameter Rx is a real adress
> - crossing the end of RAM with the parameter list yields an addressing
> exception
> - invalid diagcode in the parameter block yields an specification
> exception
> - diag258 correctly applies prefixing.
>
> Note that we're just testing error cases as of now.
>
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
> s390x/Makefile | 1 +
> s390x/diag258.c | 258 ++++++++++++++++++++++++++++++++++++++++++++
> s390x/unittests.cfg | 3 +
> 3 files changed, 262 insertions(+)
> create mode 100644 s390x/diag258.c
>
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 23342bd64f44..66d71351caab 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -44,6 +44,7 @@ tests += $(TEST_DIR)/exittime.elf
> tests += $(TEST_DIR)/ex.elf
> tests += $(TEST_DIR)/topology.elf
> tests += $(TEST_DIR)/sie-dat.elf
> +tests += $(TEST_DIR)/diag258.elf
>
> pv-tests += $(TEST_DIR)/pv-diags.elf
> pv-tests += $(TEST_DIR)/pv-icptcode.elf
> diff --git a/s390x/diag258.c b/s390x/diag258.c
> new file mode 100644
> index 000000000000..5337534f60d1
> --- /dev/null
> +++ b/s390x/diag258.c
> @@ -0,0 +1,258 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Diag 258: Async Page Fault Handler
> + *
> + * Copyright (c) 2024 IBM Corp
> + *
> + * Authors:
> + * Nico Boehr <nrb@linux.ibm.com>
> + */
> +
> +#include <libcflat.h>
> +#include <asm-generic/barrier.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/mem.h>
> +#include <asm/pgtable.h>
> +#include <mmu.h>
> +#include <sclp.h>
> +#include <vmalloc.h>
> +
> +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));
wait, you are using LC_SIZE in this file... even though it's only
defined in the next patch.
I think you should swap patch 1 and 2
> +
> +#define __PF_RES_FIELD 0x8000000000000000UL
> +
> +/* copied from Linux arch/s390/mm/pfault.c */
> +struct pfault_refbk {
> + u16 refdiagc;
> + u16 reffcode;
> + u16 refdwlen;
> + u16 refversn;
> + u64 refgaddr;
> + u64 refselmk;
> + u64 refcmpmk;
> + u64 reserved;
> +};
> +
> +uint64_t pfault_token = 0x0123fadec0fe3210UL;
> +
> +static struct pfault_refbk pfault_init_refbk __attribute__((aligned(8))) = {
> + .refdiagc = 0x258,
> + .reffcode = 0, /* TOKEN */
> + .refdwlen = sizeof(struct pfault_refbk) / sizeof(uint64_t),
> + .refversn = 2,
> + .refgaddr = (u64)&pfault_token,
> + .refselmk = 1UL << 48,
> + .refcmpmk = 1UL << 48,
> + .reserved = __PF_RES_FIELD
> +};
> +
> +static struct pfault_refbk pfault_cancel_refbk __attribute((aligned(8))) = {
> + .refdiagc = 0x258,
> + .reffcode = 1, /* CANCEL */
> + .refdwlen = sizeof(struct pfault_refbk) / sizeof(uint64_t),
> + .refversn = 2,
> + .refgaddr = 0,
> + .refselmk = 0,
> + .refcmpmk = 0,
> + .reserved = 0
> +};
> +
> +static inline int diag258(struct pfault_refbk *refbk)
> +{
> + int rc = -1;
> +
> + asm volatile(
> + " diag %[refbk],%[rc],0x258\n"
> + : [rc] "+d" (rc)
> + : [refbk] "a" (refbk), "m" (*(refbk))
> + : "cc");
> + return rc;
> +}
> +
> +static void test_priv(void)
> +{
> + report_prefix_push("privileged");
> + expect_pgm_int();
> + enter_pstate();
> + diag258(&pfault_init_refbk);
> + check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> + report_prefix_pop();
> +}
> +
> +static void* page_map_outside_real_space(phys_addr_t page_real)
> +{
> + pgd_t *root = (pgd_t *)(stctg(1) & PAGE_MASK);
> + void* vaddr = alloc_vpage();
> +
> + install_page(root, page_real, vaddr);
> +
> + return vaddr;
> +}
> +
> +/*
> + * Verify that the refbk pointer is a real address and not a virtual
> + * address. This is tested by enabling DAT and establishing a mapping
> + * for the refbk that is outside of the bounds of our (guest-)physical
s/physical/real/ (or absolute)
> + * address space.
> + */
> +static void test_refbk_real(void)
> +{
> + pgd_t *root;
> + struct pfault_refbk *refbk;
> + void *refbk_page;
please reverse Christmas tree
> +
> + report_prefix_push("refbk is real");
> +
> + /* Set up virtual memory and allocate a physical page for storing the refbk */
> + setup_vm();
> + refbk_page = alloc_page();
> +
> + /* Map refblk page outside of physical memory identity mapping */
> + root = (pgd_t *)(stctg(1) & PAGE_MASK);
> + refbk = page_map_outside_real_space(virt_to_pte_phys(root, refbk_page));
> +
> + /* Assert the mapping really is outside identity mapping */
> + report_info("refbk is at 0x%lx", (u64)refbk);
> + report_info("ram size is 0x%lx", get_ram_size());
> + assert((u64)refbk > get_ram_size());
> +
> + /* Copy the init refbk to the page */
> + memcpy(refbk, &pfault_init_refbk, sizeof(struct pfault_refbk));
> +
> + /* Protect the virtual mapping to avoid diag258 actually doing something */
> + protect_page(refbk, PAGE_ENTRY_I);
> +
> + expect_pgm_int();
> + diag258(refbk);
> + check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
> + report_prefix_pop();
> +
> + free_page(refbk_page);
> + disable_dat();
> + irq_set_dat_mode(false, 0);
> +}
> +
> +/*
> + * Verify diag258 correctly applies prefixing.
> + */
> +static void test_refbk_prefixing(void)
> +{
> + uint64_t ry;
> + uint32_t old_prefix;
> + struct pfault_refbk *refbk_in_prefix, *refbk_in_reverse_prefix;
> + const size_t lowcore_offset_for_refbk = offsetof(struct lowcore, pad_0x03a0);
reverse Christmas tree here too
> +
> + report_prefix_push("refbk prefixing");
> +
> + report_info("refbk at lowcore offset 0x%lx", lowcore_offset_for_refbk);
> +
> + assert((unsigned long)&prefix_buf < SZ_2G);
> +
> + memcpy(prefix_buf, 0, LC_SIZE);
> +
> + /*
> + * After the call to set_prefix() below, this will refer to absolute
> + * address lowcore_offset_for_refbk (reverse prefixing).
> + */
> + refbk_in_reverse_prefix = (struct pfault_refbk *)(&prefix_buf[0] + lowcore_offset_for_refbk);
> +
> + /*
> + * After the call to set_prefix() below, this will refer to absolute
> + * address &prefix_buf[0] + lowcore_offset_for_refbk (forward prefixing).
> + */
> + refbk_in_prefix = (struct pfault_refbk *)OPAQUE_PTR(lowcore_offset_for_refbk);
> +
> + old_prefix = get_prefix();
> + set_prefix((uint32_t)(uintptr_t)prefix_buf);
> +
> + /*
> + * If diag258 would not be applying prefixing on access to
> + * refbk_in_reverse_prefix correctly, it would access absolute address
> + * refbk_in_reverse_prefix (which to us is accessible at real address
> + * refbk_in_prefix).
> + * Make sure it really fails by putting invalid function code
> + * at refbk_in_prefix.
> + */
> + refbk_in_prefix->refdiagc = 0xc0fe;
> +
> + /*
> + * Put a valid refbk at refbk_in_reverse_prefix.
> + */
> + memcpy(refbk_in_reverse_prefix, &pfault_init_refbk, sizeof(pfault_init_refbk));
> +
> + ry = diag258(refbk_in_reverse_prefix);
> + report(!ry, "real address refbk accessed");
> +
> + /*
> + * Activating should have worked. Cancel the activation and expect
> + * return 0. If activation would not have worked, this should return with
> + * 4 (pfault handshaking not active).
> + */
> + ry = diag258(&pfault_cancel_refbk);
> + report(!ry, "handshaking canceled");
> +
> + set_prefix(old_prefix);
> +
> + report_prefix_pop();
> +}
it seems like you are only testing the first page of lowcore; can you
expand the test to also test the second page?
> +
> +/*
> + * Verify that a refbk exceeding physical memory is not accepted, even
> + * when crossing a frame boundary.
> + */
> +static void test_refbk_crossing(void)
> +{
> + const size_t bytes_in_last_page = 8;
are there any alignment requirements for the buffer?
if so, that should also be tested (either that a fault is triggered or
that the lowest bytes are ignored, depending on how it is defined to
work)
> + struct pfault_refbk *refbk = (struct pfault_refbk *)(get_ram_size() - bytes_in_last_page);
> +
> + report_prefix_push("refbk crossing");
> +
> + report_info("refbk is at 0x%lx", (u64)refbk);
> + report_info("ram size is 0x%lx", get_ram_size());
> + assert(sizeof(struct pfault_refbk) > bytes_in_last_page);
> +
> + /* Copy bytes_in_last_page bytes of the init refbk to the page */
> + memcpy(refbk, &pfault_init_refbk, bytes_in_last_page);
> +
> + expect_pgm_int();
> + diag258(refbk);
> + check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
> + report_prefix_pop();
> +}
> +
> +/*
> + * Verify that a refbk with an invalid refdiagc is not accepted.
> + */
> +static void test_refbk_invalid_diagcode(void)
> +{
> + struct pfault_refbk refbk = pfault_init_refbk;
> +
> + report_prefix_push("invalid refdiagc");
> + refbk.refdiagc = 0xc0fe;
other testcases in this file depend on invalid codes failing; maybe
move this test up?
> +
> + expect_pgm_int();
> + diag258(&refbk);
> + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> + report_prefix_pop();
> +}
> +
> +int main(void)
> +{
> + report_prefix_push("diag258");
> +
> + expect_pgm_int();
> + diag258(&pfault_init_refbk);
> + if (clear_pgm_int() == PGM_INT_CODE_SPECIFICATION) {
> + report_skip("diag258 not supported");
> + } else {
> + test_priv();
> + test_refbk_real();
should probably go here....
> + test_refbk_prefixing();
> + test_refbk_crossing();
> + test_refbk_invalid_diagcode();
.... this one ^
> + }
> +
> + report_prefix_pop();
> + return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 3a9decc932f2..8131ba105d3f 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -392,3 +392,6 @@ file = sie-dat.elf
>
> [pv-attest]
> file = pv-attest.elf
> +
> +[diag258]
> +file = diag258.elf
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] s390x: add test for diag258
2024-09-25 15:34 ` Claudio Imbrenda
@ 2024-10-02 14:13 ` Nico Boehr
0 siblings, 0 replies; 6+ messages in thread
From: Nico Boehr @ 2024-10-02 14:13 UTC (permalink / raw)
To: Claudio Imbrenda; +Cc: frankja, thuth, kvm, linux-s390
Quoting Claudio Imbrenda (2024-09-25 17:34:52)
> > +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));
>
> wait, you are using LC_SIZE in this file... even though it's only
> defined in the next patch.
>
> I think you should swap patch 1 and 2
will do
[...]
> > +/*
> > + * Verify that the refbk pointer is a real address and not a virtual
> > + * address. This is tested by enabling DAT and establishing a mapping
> > + * for the refbk that is outside of the bounds of our (guest-)physical
>
> s/physical/real/ (or absolute)
Huh? Why? Talking about the size here, so absolute, real and physical are
all equivalent here.
[...]
> > + /*
> > + * Put a valid refbk at refbk_in_reverse_prefix.
> > + */
> > + memcpy(refbk_in_reverse_prefix, &pfault_init_refbk, sizeof(pfault_init_refbk));
> > +
> > + ry = diag258(refbk_in_reverse_prefix);
> > + report(!ry, "real address refbk accessed");
> > +
> > + /*
> > + * Activating should have worked. Cancel the activation and expect
> > + * return 0. If activation would not have worked, this should return with
> > + * 4 (pfault handshaking not active).
> > + */
> > + ry = diag258(&pfault_cancel_refbk);
> > + report(!ry, "handshaking canceled");
> > +
> > + set_prefix(old_prefix);
> > +
> > + report_prefix_pop();
> > +}
>
> it seems like you are only testing the first page of lowcore; can you
> expand the test to also test the second page?
Would you mind leaving this for a future extension?
> > +
> > +/*
> > + * Verify that a refbk exceeding physical memory is not accepted, even
> > + * when crossing a frame boundary.
> > + */
> > +static void test_refbk_crossing(void)
> > +{
> > + const size_t bytes_in_last_page = 8;
>
> are there any alignment requirements for the buffer?
> if so, that should also be tested (either that a fault is triggered or
> that the lowest bytes are ignored, depending on how it is defined to
> work)
There are alignment requirements. Would it be OK to do this in a future
extension?
> > +/*
> > + * Verify that a refbk with an invalid refdiagc is not accepted.
> > + */
> > +static void test_refbk_invalid_diagcode(void)
> > +{
> > + struct pfault_refbk refbk = pfault_init_refbk;
> > +
> > + report_prefix_push("invalid refdiagc");
> > + refbk.refdiagc = 0xc0fe;
>
> other testcases in this file depend on invalid codes failing; maybe
> move this test up?
Yes, thanks, done.
> > +int main(void)
> > +{
> > + report_prefix_push("diag258");
> > +
> > + expect_pgm_int();
> > + diag258(&pfault_init_refbk);
> > + if (clear_pgm_int() == PGM_INT_CODE_SPECIFICATION) {
> > + report_skip("diag258 not supported");
> > + } else {
> > + test_priv();
> > + test_refbk_real();
>
> should probably go here....
test_refbk_real() relies on the invalid diagcode doing nothing, so it
should go *before* that one.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-02 14:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-23 6:26 [kvm-unit-tests PATCH v2 0/2] s390x: add tests for diag258 Nico Boehr
2024-09-23 6:26 ` [kvm-unit-tests PATCH v2 1/2] s390x: add test " Nico Boehr
2024-09-25 15:34 ` Claudio Imbrenda
2024-10-02 14:13 ` Nico Boehr
2024-09-23 6:26 ` [kvm-unit-tests PATCH v2 2/2] s390x: edat: move LC_SIZE to arch_def.h Nico Boehr
2024-09-25 14:37 ` Claudio Imbrenda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox