[PATCH 0/4] KVM: selftests: aarch64: page_fault_test S1PTW related fixes

[PATCH 0/4] KVM: selftests: aarch64: page_fault_test S1PTW related fixes

[Ricardo Koller]

Commit "KVM: arm64: Fix S1PTW handling on RO memslots" changed the way
S1PTW faults were handled by KVM.  Before this fix, KVM treated any fault
due to any S1PTW as a write, even S1PTW that didn't update the PTE.  Some
tests in page_fault_test mistakenly check for this KVM behavior and are
currently failing.  For example, there's a dirty log test that asserts that
a S1PTW without AF or DA results in the PTE page getting dirty.

The first commit fixes the userfaultfd check by relaxing all read vs. write
checks.  The second commit fixes the dirtylog tests by only asserting dirty
bits when the AF bit is set.  The third commit fixes an issue found after
fixing the previous two: the dirty log test was checking for the first page
in the PT region.  Finally, commit "KVM: arm64: Fix S1PTW handling on RO
memslots" allows for having readonly memslots holding page tables, so
commit 4 add tests for it.

Ricardo Koller (4):
  KVM: selftests: aarch64: Relax userfaultfd read vs. write checks
  KVM: selftests: aarch64: Do not default to dirty PTE pages on all
    S1PTWs
  KVM: selftests: aarch64: Fix check of dirty log PT write
  KVM: selftests: aarch64: Test read-only PT memory regions

 .../selftests/kvm/aarch64/page_fault_test.c   | 183 ++++++++++--------
 1 file changed, 101 insertions(+), 82 deletions(-)

-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 1/4] KVM: selftests: aarch64: Relax userfaultfd read vs. write checks

[Ricardo Koller]

Only Stage1 Page table walks (S1PTW) writing a PTE on an unmapped page
should result in a userfaultfd write. However, the userfaultfd tests in
page_fault_test wrongly assert that any S1PTW is a PTE write.

Fix this by relaxing the read vs. write checks in all userfaultfd handlers.
Note that this is also an attempt to focus less on KVM (and userfaultfd)
behavior, and more on architectural behavior. Also note that after commit
"KVM: arm64: Fix S1PTW handling on RO memslots" the userfaultfd fault
(S1PTW with AF on an unmaped PTE page) is actually a read: the translation
fault that comes before the permission fault.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../selftests/kvm/aarch64/page_fault_test.c   | 83 ++++++++-----------
 1 file changed, 34 insertions(+), 49 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/page_fault_test.c b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
index beb944fa6fd4..0dda58766185 100644
--- a/tools/testing/selftests/kvm/aarch64/page_fault_test.c
+++ b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
@@ -304,7 +304,7 @@ static struct uffd_args {
 
 /* Returns true to continue the test, and false if it should be skipped. */
 static int uffd_generic_handler(int uffd_mode, int uffd, struct uffd_msg *msg,
-				struct uffd_args *args, bool expect_write)
+				struct uffd_args *args)
 {
 	uint64_t addr = msg->arg.pagefault.address;
 	uint64_t flags = msg->arg.pagefault.flags;
@@ -313,7 +313,6 @@ static int uffd_generic_handler(int uffd_mode, int uffd, struct uffd_msg *msg,
 
 	TEST_ASSERT(uffd_mode == UFFDIO_REGISTER_MODE_MISSING,
 		    "The only expected UFFD mode is MISSING");
-	ASSERT_EQ(!!(flags & UFFD_PAGEFAULT_FLAG_WRITE), expect_write);
 	ASSERT_EQ(addr, (uint64_t)args->hva);
 
 	pr_debug("uffd fault: addr=%p write=%d\n",
@@ -337,19 +336,14 @@ static int uffd_generic_handler(int uffd_mode, int uffd, struct uffd_msg *msg,
 	return 0;
 }
 
-static int uffd_pt_write_handler(int mode, int uffd, struct uffd_msg *msg)
+static int uffd_pt_handler(int mode, int uffd, struct uffd_msg *msg)
 {
-	return uffd_generic_handler(mode, uffd, msg, &pt_args, true);
+	return uffd_generic_handler(mode, uffd, msg, &pt_args);
 }
 
-static int uffd_data_write_handler(int mode, int uffd, struct uffd_msg *msg)
+static int uffd_data_handler(int mode, int uffd, struct uffd_msg *msg)
 {
-	return uffd_generic_handler(mode, uffd, msg, &data_args, true);
-}
-
-static int uffd_data_read_handler(int mode, int uffd, struct uffd_msg *msg)
-{
-	return uffd_generic_handler(mode, uffd, msg, &data_args, false);
+	return uffd_generic_handler(mode, uffd, msg, &data_args);
 }
 
 static void setup_uffd_args(struct userspace_mem_region *region,
@@ -822,7 +816,7 @@ static void help(char *name)
 	.mem_mark_cmd		= CMD_HOLE_DATA | CMD_HOLE_PT,			\
 	.guest_test_check	= { _CHECK(_with_af), _test_check },		\
 	.uffd_data_handler	= _uffd_data_handler,				\
-	.uffd_pt_handler	= uffd_pt_write_handler,			\
+	.uffd_pt_handler	= uffd_pt_handler,				\
 	.expected_events	= { .uffd_faults = _uffd_faults, },		\
 }
 
@@ -878,7 +872,7 @@ static void help(char *name)
 	.guest_prepare		= { _PREPARE(_access) },			\
 	.guest_test		= _access,					\
 	.uffd_data_handler	= _uffd_data_handler,				\
-	.uffd_pt_handler	= uffd_pt_write_handler,			\
+	.uffd_pt_handler	= uffd_pt_handler,				\
 	.mmio_handler		= _mmio_handler,				\
 	.expected_events	= { .mmio_exits = _mmio_exits,			\
 				    .uffd_faults = _uffd_faults },		\
@@ -892,7 +886,7 @@ static void help(char *name)
 	.mem_mark_cmd		= CMD_HOLE_DATA | CMD_HOLE_PT,			\
 	.guest_test		= _access,					\
 	.uffd_data_handler	= _uffd_data_handler,				\
-	.uffd_pt_handler	= uffd_pt_write_handler,			\
+	.uffd_pt_handler	= uffd_pt_handler,			\
 	.fail_vcpu_run_handler	= fail_vcpu_run_mmio_no_syndrome_handler,	\
 	.expected_events	= { .fail_vcpu_runs = 1,			\
 				    .uffd_faults = _uffd_faults },		\
@@ -933,29 +927,27 @@ static struct test_desc tests[] = {
 	 * (S1PTW).
 	 */
 	TEST_UFFD(guest_read64, with_af, CMD_HOLE_DATA | CMD_HOLE_PT,
-		  uffd_data_read_handler, uffd_pt_write_handler, 2),
-	/* no_af should also lead to a PT write. */
+		  uffd_data_handler, uffd_pt_handler, 2),
 	TEST_UFFD(guest_read64, no_af, CMD_HOLE_DATA | CMD_HOLE_PT,
-		  uffd_data_read_handler, uffd_pt_write_handler, 2),
-	/* Note how that cas invokes the read handler. */
+		  uffd_data_handler, uffd_pt_handler, 2),
 	TEST_UFFD(guest_cas, with_af, CMD_HOLE_DATA | CMD_HOLE_PT,
-		  uffd_data_read_handler, uffd_pt_write_handler, 2),
+		  uffd_data_handler, uffd_pt_handler, 2),
 	/*
 	 * Can't test guest_at with_af as it's IMPDEF whether the AF is set.
 	 * The S1PTW fault should still be marked as a write.
 	 */
 	TEST_UFFD(guest_at, no_af, CMD_HOLE_DATA | CMD_HOLE_PT,
-		  uffd_data_read_handler, uffd_pt_write_handler, 1),
+		  uffd_no_handler, uffd_pt_handler, 1),
 	TEST_UFFD(guest_ld_preidx, with_af, CMD_HOLE_DATA | CMD_HOLE_PT,
-		  uffd_data_read_handler, uffd_pt_write_handler, 2),
+		  uffd_data_handler, uffd_pt_handler, 2),
 	TEST_UFFD(guest_write64, with_af, CMD_HOLE_DATA | CMD_HOLE_PT,
-		  uffd_data_write_handler, uffd_pt_write_handler, 2),
+		  uffd_data_handler, uffd_pt_handler, 2),
 	TEST_UFFD(guest_dc_zva, with_af, CMD_HOLE_DATA | CMD_HOLE_PT,
-		  uffd_data_write_handler, uffd_pt_write_handler, 2),
+		  uffd_data_handler, uffd_pt_handler, 2),
 	TEST_UFFD(guest_st_preidx, with_af, CMD_HOLE_DATA | CMD_HOLE_PT,
-		  uffd_data_write_handler, uffd_pt_write_handler, 2),
+		  uffd_data_handler, uffd_pt_handler, 2),
 	TEST_UFFD(guest_exec, with_af, CMD_HOLE_DATA | CMD_HOLE_PT,
-		  uffd_data_read_handler, uffd_pt_write_handler, 2),
+		  uffd_data_handler, uffd_pt_handler, 2),
 
 	/*
 	 * Try accesses when the data and PT memory regions are both
@@ -980,25 +972,25 @@ static struct test_desc tests[] = {
 	 * fault, and nothing in the dirty log.  Any S1PTW should result in
 	 * a write in the dirty log and a userfaultfd write.
 	 */
-	TEST_UFFD_AND_DIRTY_LOG(guest_read64, with_af, uffd_data_read_handler, 2,
+	TEST_UFFD_AND_DIRTY_LOG(guest_read64, with_af, uffd_data_handler, 2,
 				guest_check_no_write_in_dirty_log),
 	/* no_af should also lead to a PT write. */
-	TEST_UFFD_AND_DIRTY_LOG(guest_read64, no_af, uffd_data_read_handler, 2,
+	TEST_UFFD_AND_DIRTY_LOG(guest_read64, no_af, uffd_data_handler, 2,
 				guest_check_no_write_in_dirty_log),
-	TEST_UFFD_AND_DIRTY_LOG(guest_ld_preidx, with_af, uffd_data_read_handler,
+	TEST_UFFD_AND_DIRTY_LOG(guest_ld_preidx, with_af, uffd_data_handler,
 				2, guest_check_no_write_in_dirty_log),
-	TEST_UFFD_AND_DIRTY_LOG(guest_at, with_af, 0, 1,
+	TEST_UFFD_AND_DIRTY_LOG(guest_at, with_af, uffd_no_handler, 1,
 				guest_check_no_write_in_dirty_log),
-	TEST_UFFD_AND_DIRTY_LOG(guest_exec, with_af, uffd_data_read_handler, 2,
+	TEST_UFFD_AND_DIRTY_LOG(guest_exec, with_af, uffd_data_handler, 2,
 				guest_check_no_write_in_dirty_log),
-	TEST_UFFD_AND_DIRTY_LOG(guest_write64, with_af, uffd_data_write_handler,
+	TEST_UFFD_AND_DIRTY_LOG(guest_write64, with_af, uffd_data_handler,
 				2, guest_check_write_in_dirty_log),
-	TEST_UFFD_AND_DIRTY_LOG(guest_cas, with_af, uffd_data_read_handler, 2,
+	TEST_UFFD_AND_DIRTY_LOG(guest_cas, with_af, uffd_data_handler, 2,
 				guest_check_write_in_dirty_log),
-	TEST_UFFD_AND_DIRTY_LOG(guest_dc_zva, with_af, uffd_data_write_handler,
+	TEST_UFFD_AND_DIRTY_LOG(guest_dc_zva, with_af, uffd_data_handler,
 				2, guest_check_write_in_dirty_log),
 	TEST_UFFD_AND_DIRTY_LOG(guest_st_preidx, with_af,
-				uffd_data_write_handler, 2,
+				uffd_data_handler, 2,
 				guest_check_write_in_dirty_log),
 
 	/*
@@ -1051,22 +1043,15 @@ static struct test_desc tests[] = {
 	 * no userfaultfd write fault. Reads result in userfaultfd getting
 	 * triggered.
 	 */
-	TEST_RO_MEMSLOT_AND_UFFD(guest_read64, 0, 0,
-				 uffd_data_read_handler, 2),
-	TEST_RO_MEMSLOT_AND_UFFD(guest_ld_preidx, 0, 0,
-				 uffd_data_read_handler, 2),
-	TEST_RO_MEMSLOT_AND_UFFD(guest_at, 0, 0,
-				 uffd_no_handler, 1),
-	TEST_RO_MEMSLOT_AND_UFFD(guest_exec, 0, 0,
-				 uffd_data_read_handler, 2),
+	TEST_RO_MEMSLOT_AND_UFFD(guest_read64, 0, 0, uffd_data_handler, 2),
+	TEST_RO_MEMSLOT_AND_UFFD(guest_ld_preidx, 0, 0, uffd_data_handler, 2),
+	TEST_RO_MEMSLOT_AND_UFFD(guest_at, 0, 0, uffd_no_handler, 1),
+	TEST_RO_MEMSLOT_AND_UFFD(guest_exec, 0, 0, uffd_data_handler, 2),
 	TEST_RO_MEMSLOT_AND_UFFD(guest_write64, mmio_on_test_gpa_handler, 1,
-				 uffd_data_write_handler, 2),
-	TEST_RO_MEMSLOT_NO_SYNDROME_AND_UFFD(guest_cas,
-					     uffd_data_read_handler, 2),
-	TEST_RO_MEMSLOT_NO_SYNDROME_AND_UFFD(guest_dc_zva,
-					     uffd_no_handler, 1),
-	TEST_RO_MEMSLOT_NO_SYNDROME_AND_UFFD(guest_st_preidx,
-					     uffd_no_handler, 1),
+				 uffd_data_handler, 2),
+	TEST_RO_MEMSLOT_NO_SYNDROME_AND_UFFD(guest_cas, uffd_data_handler, 2),
+	TEST_RO_MEMSLOT_NO_SYNDROME_AND_UFFD(guest_dc_zva, uffd_no_handler, 1),
+	TEST_RO_MEMSLOT_NO_SYNDROME_AND_UFFD(guest_st_preidx, uffd_no_handler, 1),
 
 	{ 0 }
 };
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH 1/4] KVM: selftests: aarch64: Relax userfaultfd read vs. write checks

[Oliver Upton]

On Tue, Jan 10, 2023 at 02:24:29AM +0000, Ricardo Koller wrote:
> Only Stage1 Page table walks (S1PTW) writing a PTE on an unmapped page
> should result in a userfaultfd write. However, the userfaultfd tests in
> page_fault_test wrongly assert that any S1PTW is a PTE write.
> 
> Fix this by relaxing the read vs. write checks in all userfaultfd handlers.
> Note that this is also an attempt to focus less on KVM (and userfaultfd)
> behavior, and more on architectural behavior. Also note that after commit
> "KVM: arm64: Fix S1PTW handling on RO memslots" the userfaultfd fault
> (S1PTW with AF on an unmaped PTE page) is actually a read: the translation
> fault that comes before the permission fault.

I certainly agree that we cannot make assertions about read v. write
when registering uffd in 'missing' mode. We probably need another test
to assert that we get write faults for hardware AF updates when using
uffd in write protect mode.

--
Thanks,
Oliver

Re: [PATCH 1/4] KVM: selftests: aarch64: Relax userfaultfd read vs. write checks

[Ricardo Koller]

On Mon, Jan 23, 2023 at 11:07:25PM +0000, Oliver Upton wrote:
> On Tue, Jan 10, 2023 at 02:24:29AM +0000, Ricardo Koller wrote:
> > Only Stage1 Page table walks (S1PTW) writing a PTE on an unmapped page
> > should result in a userfaultfd write. However, the userfaultfd tests in
> > page_fault_test wrongly assert that any S1PTW is a PTE write.
> > 
> > Fix this by relaxing the read vs. write checks in all userfaultfd handlers.
> > Note that this is also an attempt to focus less on KVM (and userfaultfd)
> > behavior, and more on architectural behavior. Also note that after commit
> > "KVM: arm64: Fix S1PTW handling on RO memslots" the userfaultfd fault
> > (S1PTW with AF on an unmaped PTE page) is actually a read: the translation
> > fault that comes before the permission fault.
> 
> I certainly agree that we cannot make assertions about read v. write
> when registering uffd in 'missing' mode. We probably need another test
> to assert that we get write faults for hardware AF updates when using
> uffd in write protect mode.

I can do that. Only question, do you prefer having them in this series
with fixes, or another one?

> 
> --
> Thanks,
> Oliver

Re: [PATCH 1/4] KVM: selftests: aarch64: Relax userfaultfd read vs. write checks

[Oliver Upton]

Ricardo,

On Tue, Jan 24, 2023 at 08:17:49AM -0800, Ricardo Koller wrote:
> On Mon, Jan 23, 2023 at 11:07:25PM +0000, Oliver Upton wrote:
> > On Tue, Jan 10, 2023 at 02:24:29AM +0000, Ricardo Koller wrote:
> > > Only Stage1 Page table walks (S1PTW) writing a PTE on an unmapped page
> > > should result in a userfaultfd write. However, the userfaultfd tests in
> > > page_fault_test wrongly assert that any S1PTW is a PTE write.
> > > 
> > > Fix this by relaxing the read vs. write checks in all userfaultfd handlers.
> > > Note that this is also an attempt to focus less on KVM (and userfaultfd)
> > > behavior, and more on architectural behavior. Also note that after commit
> > > "KVM: arm64: Fix S1PTW handling on RO memslots" the userfaultfd fault
> > > (S1PTW with AF on an unmaped PTE page) is actually a read: the translation
> > > fault that comes before the permission fault.
> > 
> > I certainly agree that we cannot make assertions about read v. write
> > when registering uffd in 'missing' mode. We probably need another test
> > to assert that we get write faults for hardware AF updates when using
> > uffd in write protect mode.
> 
> I can do that. Only question, do you prefer having them in this series
> with fixes, or another one?

Oh, don't worry about it for this series as I'd like to grab it sooner
rather than later. Just making a note of some additional improvements to
the test :)

--
Thanks,
Oliver

[PATCH 2/4] KVM: selftests: aarch64: Do not default to dirty PTE pages on all S1PTWs

[Ricardo Koller]

Only Stage1 Page table walks (S1PTW) trying to write into a PTE should
result in the PTE page being dirty in the log.  However, the dirty log
tests in page_fault_test default to treat all S1PTW accesses as writes.
Fix the relevant tests by asserting dirty pages only for S1PTW writes,
which in these tests only applies to when Hardware management of the Access
Flag is enabled.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../selftests/kvm/aarch64/page_fault_test.c   | 93 ++++++++++++-------
 1 file changed, 60 insertions(+), 33 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/page_fault_test.c b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
index 0dda58766185..1a3bb2bd8657 100644
--- a/tools/testing/selftests/kvm/aarch64/page_fault_test.c
+++ b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
@@ -237,6 +237,11 @@ static void guest_check_s1ptw_wr_in_dirty_log(void)
 	GUEST_SYNC(CMD_CHECK_S1PTW_WR_IN_DIRTY_LOG);
 }
 
+static void guest_check_no_s1ptw_wr_in_dirty_log(void)
+{
+	GUEST_SYNC(CMD_CHECK_NO_S1PTW_WR_IN_DIRTY_LOG);
+}
+
 static void guest_exec(void)
 {
 	int (*code)(void) = (int (*)(void))TEST_EXEC_GVA;
@@ -791,7 +796,7 @@ static void help(char *name)
 	.expected_events	= { .uffd_faults = _uffd_faults, },		\
 }
 
-#define TEST_DIRTY_LOG(_access, _with_af, _test_check)				\
+#define TEST_DIRTY_LOG(_access, _with_af, _test_check, _pt_check)		\
 {										\
 	.name			= SCAT3(dirty_log, _access, _with_af),		\
 	.data_memslot_flags	= KVM_MEM_LOG_DIRTY_PAGES,			\
@@ -799,13 +804,12 @@ static void help(char *name)
 	.guest_prepare		= { _PREPARE(_with_af),				\
 				    _PREPARE(_access) },			\
 	.guest_test		= _access,					\
-	.guest_test_check	= { _CHECK(_with_af), _test_check,		\
-				    guest_check_s1ptw_wr_in_dirty_log},		\
+	.guest_test_check	= { _CHECK(_with_af), _test_check, _pt_check },	\
 	.expected_events	= { 0 },					\
 }
 
 #define TEST_UFFD_AND_DIRTY_LOG(_access, _with_af, _uffd_data_handler,		\
-				_uffd_faults, _test_check)			\
+				_uffd_faults, _test_check, _pt_check)		\
 {										\
 	.name			= SCAT3(uffd_and_dirty_log, _access, _with_af),	\
 	.data_memslot_flags	= KVM_MEM_LOG_DIRTY_PAGES,			\
@@ -814,7 +818,7 @@ static void help(char *name)
 				    _PREPARE(_access) },			\
 	.guest_test		= _access,					\
 	.mem_mark_cmd		= CMD_HOLE_DATA | CMD_HOLE_PT,			\
-	.guest_test_check	= { _CHECK(_with_af), _test_check },		\
+	.guest_test_check	= { _CHECK(_with_af), _test_check, _pt_check },	\
 	.uffd_data_handler	= _uffd_data_handler,				\
 	.uffd_pt_handler	= uffd_pt_handler,				\
 	.expected_events	= { .uffd_faults = _uffd_faults, },		\
@@ -953,16 +957,25 @@ static struct test_desc tests[] = {
 	 * Try accesses when the data and PT memory regions are both
 	 * tracked for dirty logging.
 	 */
-	TEST_DIRTY_LOG(guest_read64, with_af, guest_check_no_write_in_dirty_log),
-	/* no_af should also lead to a PT write. */
-	TEST_DIRTY_LOG(guest_read64, no_af, guest_check_no_write_in_dirty_log),
-	TEST_DIRTY_LOG(guest_ld_preidx, with_af, guest_check_no_write_in_dirty_log),
-	TEST_DIRTY_LOG(guest_at, no_af, guest_check_no_write_in_dirty_log),
-	TEST_DIRTY_LOG(guest_exec, with_af, guest_check_no_write_in_dirty_log),
-	TEST_DIRTY_LOG(guest_write64, with_af, guest_check_write_in_dirty_log),
-	TEST_DIRTY_LOG(guest_cas, with_af, guest_check_write_in_dirty_log),
-	TEST_DIRTY_LOG(guest_dc_zva, with_af, guest_check_write_in_dirty_log),
-	TEST_DIRTY_LOG(guest_st_preidx, with_af, guest_check_write_in_dirty_log),
+	TEST_DIRTY_LOG(guest_read64, with_af, guest_check_no_write_in_dirty_log,
+		       guest_check_s1ptw_wr_in_dirty_log),
+	TEST_DIRTY_LOG(guest_read64, no_af, guest_check_no_write_in_dirty_log,
+		       guest_check_no_s1ptw_wr_in_dirty_log),
+	TEST_DIRTY_LOG(guest_ld_preidx, with_af,
+		       guest_check_no_write_in_dirty_log,
+		       guest_check_s1ptw_wr_in_dirty_log),
+	TEST_DIRTY_LOG(guest_at, no_af, guest_check_no_write_in_dirty_log,
+		       guest_check_no_s1ptw_wr_in_dirty_log),
+	TEST_DIRTY_LOG(guest_exec, with_af, guest_check_no_write_in_dirty_log,
+		       guest_check_s1ptw_wr_in_dirty_log),
+	TEST_DIRTY_LOG(guest_write64, with_af, guest_check_write_in_dirty_log,
+		       guest_check_s1ptw_wr_in_dirty_log),
+	TEST_DIRTY_LOG(guest_cas, with_af, guest_check_write_in_dirty_log,
+		       guest_check_s1ptw_wr_in_dirty_log),
+	TEST_DIRTY_LOG(guest_dc_zva, with_af, guest_check_write_in_dirty_log,
+		       guest_check_s1ptw_wr_in_dirty_log),
+	TEST_DIRTY_LOG(guest_st_preidx, with_af, guest_check_write_in_dirty_log,
+		       guest_check_s1ptw_wr_in_dirty_log),
 
 	/*
 	 * Access when the data and PT memory regions are both marked for
@@ -972,27 +985,41 @@ static struct test_desc tests[] = {
 	 * fault, and nothing in the dirty log.  Any S1PTW should result in
 	 * a write in the dirty log and a userfaultfd write.
 	 */
-	TEST_UFFD_AND_DIRTY_LOG(guest_read64, with_af, uffd_data_handler, 2,
-				guest_check_no_write_in_dirty_log),
-	/* no_af should also lead to a PT write. */
-	TEST_UFFD_AND_DIRTY_LOG(guest_read64, no_af, uffd_data_handler, 2,
-				guest_check_no_write_in_dirty_log),
-	TEST_UFFD_AND_DIRTY_LOG(guest_ld_preidx, with_af, uffd_data_handler,
-				2, guest_check_no_write_in_dirty_log),
+	TEST_UFFD_AND_DIRTY_LOG(guest_read64, with_af,
+				uffd_data_handler, 2,
+				guest_check_no_write_in_dirty_log,
+				guest_check_s1ptw_wr_in_dirty_log),
+	TEST_UFFD_AND_DIRTY_LOG(guest_read64, no_af,
+				uffd_data_handler, 2,
+				guest_check_no_write_in_dirty_log,
+				guest_check_no_s1ptw_wr_in_dirty_log),
+	TEST_UFFD_AND_DIRTY_LOG(guest_ld_preidx, with_af,
+				uffd_data_handler,
+				2, guest_check_no_write_in_dirty_log,
+				guest_check_s1ptw_wr_in_dirty_log),
 	TEST_UFFD_AND_DIRTY_LOG(guest_at, with_af, uffd_no_handler, 1,
-				guest_check_no_write_in_dirty_log),
-	TEST_UFFD_AND_DIRTY_LOG(guest_exec, with_af, uffd_data_handler, 2,
-				guest_check_no_write_in_dirty_log),
-	TEST_UFFD_AND_DIRTY_LOG(guest_write64, with_af, uffd_data_handler,
-				2, guest_check_write_in_dirty_log),
-	TEST_UFFD_AND_DIRTY_LOG(guest_cas, with_af, uffd_data_handler, 2,
-				guest_check_write_in_dirty_log),
-	TEST_UFFD_AND_DIRTY_LOG(guest_dc_zva, with_af, uffd_data_handler,
-				2, guest_check_write_in_dirty_log),
+				guest_check_no_write_in_dirty_log,
+				guest_check_s1ptw_wr_in_dirty_log),
+	TEST_UFFD_AND_DIRTY_LOG(guest_exec, with_af,
+				uffd_data_handler, 2,
+				guest_check_no_write_in_dirty_log,
+				guest_check_s1ptw_wr_in_dirty_log),
+	TEST_UFFD_AND_DIRTY_LOG(guest_write64, with_af,
+				uffd_data_handler,
+				2, guest_check_write_in_dirty_log,
+				guest_check_s1ptw_wr_in_dirty_log),
+	TEST_UFFD_AND_DIRTY_LOG(guest_cas, with_af,
+				uffd_data_handler, 2,
+				guest_check_write_in_dirty_log,
+				guest_check_s1ptw_wr_in_dirty_log),
+	TEST_UFFD_AND_DIRTY_LOG(guest_dc_zva, with_af,
+				uffd_data_handler,
+				2, guest_check_write_in_dirty_log,
+				guest_check_s1ptw_wr_in_dirty_log),
 	TEST_UFFD_AND_DIRTY_LOG(guest_st_preidx, with_af,
 				uffd_data_handler, 2,
-				guest_check_write_in_dirty_log),
-
+				guest_check_write_in_dirty_log,
+				guest_check_s1ptw_wr_in_dirty_log),
 	/*
 	 * Try accesses when the data memory region is marked read-only
 	 * (with KVM_MEM_READONLY). Writes with a syndrome result in an
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 3/4] KVM: selftests: aarch64: Fix check of dirty log PT write

[Ricardo Koller]

The dirty log checks are mistakenly testing the first page in the page
table (PT) memory region instead of the page holding the test data page
PTE.  This wasn't an issue before commit "KVM: arm64: Fix handling of S1PTW
S2 fault on RO memslots" as all PT pages (including the first page) were
treated as writes.

Fix the page_fault_test dirty logging tests by checking for the right page:
the one for the PTE of the data test page.

Fixes: a4edf25b3e25 ("KVM: selftests: aarch64: Add dirty logging tests into page_fault_test")
Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 tools/testing/selftests/kvm/aarch64/page_fault_test.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/page_fault_test.c b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
index 1a3bb2bd8657..2e2178a7d0d8 100644
--- a/tools/testing/selftests/kvm/aarch64/page_fault_test.c
+++ b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
@@ -470,9 +470,12 @@ static bool handle_cmd(struct kvm_vm *vm, int cmd)
 {
 	struct userspace_mem_region *data_region, *pt_region;
 	bool continue_test = true;
+	uint64_t pte_gpa, pte_pg;
 
 	data_region = vm_get_mem_region(vm, MEM_REGION_TEST_DATA);
 	pt_region = vm_get_mem_region(vm, MEM_REGION_PT);
+	pte_gpa = addr_hva2gpa(vm, virt_get_pte_hva(vm, TEST_GVA));
+	pte_pg = (pte_gpa - pt_region->region.guest_phys_addr) / getpagesize();
 
 	if (cmd == CMD_SKIP_TEST)
 		continue_test = false;
@@ -485,13 +488,13 @@ static bool handle_cmd(struct kvm_vm *vm, int cmd)
 		TEST_ASSERT(check_write_in_dirty_log(vm, data_region, 0),
 			    "Missing write in dirty log");
 	if (cmd & CMD_CHECK_S1PTW_WR_IN_DIRTY_LOG)
-		TEST_ASSERT(check_write_in_dirty_log(vm, pt_region, 0),
+		TEST_ASSERT(check_write_in_dirty_log(vm, pt_region, pte_pg),
 			    "Missing s1ptw write in dirty log");
 	if (cmd & CMD_CHECK_NO_WRITE_IN_DIRTY_LOG)
 		TEST_ASSERT(!check_write_in_dirty_log(vm, data_region, 0),
 			    "Unexpected write in dirty log");
 	if (cmd & CMD_CHECK_NO_S1PTW_WR_IN_DIRTY_LOG)
-		TEST_ASSERT(!check_write_in_dirty_log(vm, pt_region, 0),
+		TEST_ASSERT(!check_write_in_dirty_log(vm, pt_region, pte_pg),
 			    "Unexpected s1ptw write in dirty log");
 
 	return continue_test;
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 4/4] KVM: selftests: aarch64: Test read-only PT memory regions

[Ricardo Koller]

Extend the read-only memslot tests in page_fault_test to test read-only PT
(Page table) memslots. Note that this was not allowed before commit "KVM:
arm64: Fix handling of S1PTW S2 fault on RO memslots" as all S1PTW faults
were treated as writes which resulted in an (unrecoverable) exception
inside the guest.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../selftests/kvm/aarch64/page_fault_test.c        | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/page_fault_test.c b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
index 2e2178a7d0d8..2f81d68e876c 100644
--- a/tools/testing/selftests/kvm/aarch64/page_fault_test.c
+++ b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
@@ -831,6 +831,7 @@ static void help(char *name)
 {										\
 	.name			= SCAT3(ro_memslot, _access, _with_af),		\
 	.data_memslot_flags	= KVM_MEM_READONLY,				\
+	.pt_memslot_flags	= KVM_MEM_READONLY,				\
 	.guest_prepare		= { _PREPARE(_access) },			\
 	.guest_test		= _access,					\
 	.mmio_handler		= _mmio_handler,				\
@@ -841,6 +842,7 @@ static void help(char *name)
 {										\
 	.name			= SCAT2(ro_memslot_no_syndrome, _access),	\
 	.data_memslot_flags	= KVM_MEM_READONLY,				\
+	.pt_memslot_flags	= KVM_MEM_READONLY,				\
 	.guest_test		= _access,					\
 	.fail_vcpu_run_handler	= fail_vcpu_run_mmio_no_syndrome_handler,	\
 	.expected_events	= { .fail_vcpu_runs = 1 },			\
@@ -851,7 +853,7 @@ static void help(char *name)
 {										\
 	.name			= SCAT3(ro_memslot, _access, _with_af),		\
 	.data_memslot_flags	= KVM_MEM_READONLY | KVM_MEM_LOG_DIRTY_PAGES,	\
-	.pt_memslot_flags	= KVM_MEM_LOG_DIRTY_PAGES,			\
+	.pt_memslot_flags	= KVM_MEM_READONLY | KVM_MEM_LOG_DIRTY_PAGES,	\
 	.guest_prepare		= { _PREPARE(_access) },			\
 	.guest_test		= _access,					\
 	.guest_test_check	= { _test_check },				\
@@ -863,7 +865,7 @@ static void help(char *name)
 {										\
 	.name			= SCAT2(ro_memslot_no_syn_and_dlog, _access),	\
 	.data_memslot_flags	= KVM_MEM_READONLY | KVM_MEM_LOG_DIRTY_PAGES,	\
-	.pt_memslot_flags	= KVM_MEM_LOG_DIRTY_PAGES,			\
+	.pt_memslot_flags	= KVM_MEM_READONLY | KVM_MEM_LOG_DIRTY_PAGES,	\
 	.guest_test		= _access,					\
 	.guest_test_check	= { _test_check },				\
 	.fail_vcpu_run_handler	= fail_vcpu_run_mmio_no_syndrome_handler,	\
@@ -875,6 +877,7 @@ static void help(char *name)
 {										\
 	.name			= SCAT2(ro_memslot_uffd, _access),		\
 	.data_memslot_flags	= KVM_MEM_READONLY,				\
+	.pt_memslot_flags	= KVM_MEM_READONLY,				\
 	.mem_mark_cmd		= CMD_HOLE_DATA | CMD_HOLE_PT,			\
 	.guest_prepare		= { _PREPARE(_access) },			\
 	.guest_test		= _access,					\
@@ -890,6 +893,7 @@ static void help(char *name)
 {										\
 	.name			= SCAT2(ro_memslot_no_syndrome, _access),	\
 	.data_memslot_flags	= KVM_MEM_READONLY,				\
+	.pt_memslot_flags	= KVM_MEM_READONLY,				\
 	.mem_mark_cmd		= CMD_HOLE_DATA | CMD_HOLE_PT,			\
 	.guest_test		= _access,					\
 	.uffd_data_handler	= _uffd_data_handler,				\
@@ -1024,7 +1028,7 @@ static struct test_desc tests[] = {
 				guest_check_write_in_dirty_log,
 				guest_check_s1ptw_wr_in_dirty_log),
 	/*
-	 * Try accesses when the data memory region is marked read-only
+	 * Access when both the PT and data regions are marked read-only
 	 * (with KVM_MEM_READONLY). Writes with a syndrome result in an
 	 * MMIO exit, writes with no syndrome (e.g., CAS) result in a
 	 * failed vcpu run, and reads/execs with and without syndroms do
@@ -1040,7 +1044,7 @@ static struct test_desc tests[] = {
 	TEST_RO_MEMSLOT_NO_SYNDROME(guest_st_preidx),
 
 	/*
-	 * Access when both the data region is both read-only and marked
+	 * The PT and data regions are both read-only and marked
 	 * for dirty logging at the same time. The expected result is that
 	 * for writes there should be no write in the dirty log. The
 	 * readonly handling is the same as if the memslot was not marked
@@ -1065,7 +1069,7 @@ static struct test_desc tests[] = {
 						  guest_check_no_write_in_dirty_log),
 
 	/*
-	 * Access when the data region is both read-only and punched with
+	 * The PT and data regions are both read-only and punched with
 	 * holes tracked with userfaultfd.  The expected result is the
 	 * union of both userfaultfd and read-only behaviors. For example,
 	 * write accesses result in a userfaultfd write fault and an MMIO
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH 4/4] KVM: selftests: aarch64: Test read-only PT memory regions

[Oliver Upton]

On Tue, Jan 10, 2023 at 02:24:32AM +0000, Ricardo Koller wrote:
> Extend the read-only memslot tests in page_fault_test to test read-only PT
> (Page table) memslots. Note that this was not allowed before commit "KVM:
> arm64: Fix handling of S1PTW S2 fault on RO memslots" as all S1PTW faults
> were treated as writes which resulted in an (unrecoverable) exception
> inside the guest.

Do we need an additional test that the guest gets nuked if TCR_EL1.HA =
0b1 and AF is clear in one of the stage-1 PTEs?

> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../selftests/kvm/aarch64/page_fault_test.c        | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/page_fault_test.c b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
> index 2e2178a7d0d8..2f81d68e876c 100644
> --- a/tools/testing/selftests/kvm/aarch64/page_fault_test.c
> +++ b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
> @@ -831,6 +831,7 @@ static void help(char *name)
>  {										\
>  	.name			= SCAT3(ro_memslot, _access, _with_af),		\

Does the '_with_af' actually belong here? The macro doesn't take such a
parameter. AFAICT the access flag is already set in all S1 PTEs for this
case and TCR_EL1.HA = 0b0.

>  	.data_memslot_flags	= KVM_MEM_READONLY,				\
> +	.pt_memslot_flags	= KVM_MEM_READONLY,				\
>  	.guest_prepare		= { _PREPARE(_access) },			\
>  	.guest_test		= _access,					\
>  	.mmio_handler		= _mmio_handler,				\

--
Thanks,
Oliver

Re: [PATCH 4/4] KVM: selftests: aarch64: Test read-only PT memory regions

[Ricardo Koller]

On Mon, Jan 23, 2023 at 11:36:52PM +0000, Oliver Upton wrote:
> On Tue, Jan 10, 2023 at 02:24:32AM +0000, Ricardo Koller wrote:
> > Extend the read-only memslot tests in page_fault_test to test read-only PT
> > (Page table) memslots. Note that this was not allowed before commit "KVM:
> > arm64: Fix handling of S1PTW S2 fault on RO memslots" as all S1PTW faults
> > were treated as writes which resulted in an (unrecoverable) exception
> > inside the guest.
> 
> Do we need an additional test that the guest gets nuked if TCR_EL1.HA =
> 0b1 and AF is clear in one of the stage-1 PTEs?
> 

That should be easy to add. The only issue is whether that's also a case
of checking for very specific KVM behavior that could change in the
future. It's unlikely, but what if KVM emulated the page table walker
behavior to give userspace all the info it needed to fix the fault.
Although it's so unlikely that I think I will add the exception check.

> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  .../selftests/kvm/aarch64/page_fault_test.c        | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/aarch64/page_fault_test.c b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
> > index 2e2178a7d0d8..2f81d68e876c 100644
> > --- a/tools/testing/selftests/kvm/aarch64/page_fault_test.c
> > +++ b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
> > @@ -831,6 +831,7 @@ static void help(char *name)
> >  {										\
> >  	.name			= SCAT3(ro_memslot, _access, _with_af),		\
> 
> Does the '_with_af' actually belong here? The macro doesn't take such a
> parameter. AFAICT the access flag is already set in all S1 PTEs for this
> case and TCR_EL1.HA = 0b0.

Good catch. That name would have been very confusing when debugging.

> 
> >  	.data_memslot_flags	= KVM_MEM_READONLY,				\
> > +	.pt_memslot_flags	= KVM_MEM_READONLY,				\
> >  	.guest_prepare		= { _PREPARE(_access) },			\
> >  	.guest_test		= _access,					\
> >  	.mmio_handler		= _mmio_handler,				\
> 
> --
> Thanks,
> Oliver

Re: [PATCH 4/4] KVM: selftests: aarch64: Test read-only PT memory regions

[Oliver Upton]

On Tue, Jan 24, 2023 at 08:26:02AM -0800, Ricardo Koller wrote:
> On Mon, Jan 23, 2023 at 11:36:52PM +0000, Oliver Upton wrote:
> > On Tue, Jan 10, 2023 at 02:24:32AM +0000, Ricardo Koller wrote:
> > > Extend the read-only memslot tests in page_fault_test to test read-only PT
> > > (Page table) memslots. Note that this was not allowed before commit "KVM:
> > > arm64: Fix handling of S1PTW S2 fault on RO memslots" as all S1PTW faults
> > > were treated as writes which resulted in an (unrecoverable) exception
> > > inside the guest.
> > 
> > Do we need an additional test that the guest gets nuked if TCR_EL1.HA =
> > 0b1 and AF is clear in one of the stage-1 PTEs?
> > 
> 
> That should be easy to add. The only issue is whether that's also a case
> of checking for very specific KVM behavior that could change in the
> future.

From the perspective of the guest I believe this to match the
architecture. An external abort is appropriate if the hardware update to
a descriptor failed.

I believe that the current implementation of this in KVM is slightly
wrong, though. AFAICT, we encode the abort with an FSC of 0x10, which
indicates an SEA occurred outside of a table walk. The other nuance of
reporting SEAs due to a TTW is that the FSC encodes the level at which
the external abort occurred. Nonetheless, I think we can hide behind
R_BGPQR of DDI0487I.a and always encode a level of 0:

"""
  If a synchronous External abort is generated due to a TLB or
  intermediate TLB caching structure, including parity or ECC errors,
  then all of the following are permitted:
   - If the PE cannot precisely determine the translation stage at which
     the error occurred, then it is reported and prioritized as a stage 1
     fault.
   - If the PE cannot precisely determine the lookup level at which the
     error occurred, then the lookup level is reported and prioritized
     as one of the following:
     - The lowest-numbered lookup level that could have caused the error.
     - If the PE cannot determine any information about the lookup level,
     then level 0.
"""

Thoughts?

--
Thanks,
Oliver

Re: [PATCH 4/4] KVM: selftests: aarch64: Test read-only PT memory regions

[Marc Zyngier]

On Tue, 24 Jan 2023 19:54:57 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Jan 24, 2023 at 08:26:02AM -0800, Ricardo Koller wrote:
> > On Mon, Jan 23, 2023 at 11:36:52PM +0000, Oliver Upton wrote:
> > > On Tue, Jan 10, 2023 at 02:24:32AM +0000, Ricardo Koller wrote:
> > > > Extend the read-only memslot tests in page_fault_test to test read-only PT
> > > > (Page table) memslots. Note that this was not allowed before commit "KVM:
> > > > arm64: Fix handling of S1PTW S2 fault on RO memslots" as all S1PTW faults
> > > > were treated as writes which resulted in an (unrecoverable) exception
> > > > inside the guest.
> > > 
> > > Do we need an additional test that the guest gets nuked if TCR_EL1.HA =
> > > 0b1 and AF is clear in one of the stage-1 PTEs?
> > > 
> > 
> > That should be easy to add. The only issue is whether that's also a case
> > of checking for very specific KVM behavior that could change in the
> > future.
> 
> From the perspective of the guest I believe this to match the
> architecture. An external abort is appropriate if the hardware update to
> a descriptor failed.
> 
> I believe that the current implementation of this in KVM is slightly
> wrong, though. AFAICT, we encode the abort with an FSC of 0x10, which
> indicates an SEA occurred outside of a table walk. The other nuance of
> reporting SEAs due to a TTW is that the FSC encodes the level at which
> the external abort occurred. Nonetheless, I think we can hide behind
> R_BGPQR of DDI0487I.a and always encode a level of 0:
> 
> """
>   If a synchronous External abort is generated due to a TLB or
>   intermediate TLB caching structure, including parity or ECC errors,
>   then all of the following are permitted:
>    - If the PE cannot precisely determine the translation stage at which
>      the error occurred, then it is reported and prioritized as a stage 1
>      fault.
>    - If the PE cannot precisely determine the lookup level at which the
>      error occurred, then the lookup level is reported and prioritized
>      as one of the following:
>      - The lowest-numbered lookup level that could have caused the error.
>      - If the PE cannot determine any information about the lookup level,
>      then level 0.
> """
> 
> Thoughts?

Indeed, the abort injection has always been on the dodgy side of
things. I remember Christoffer and I writing this, saying that it was
something we'd have to eventually fix. 10 years down the line, this
code is, unsurprisingly, still dodgy.

My vote would be to slightly extend the API to take a set of
KVM-specific flags to give context to the injection helpers (such as
SEA during a TTW), and bring the KVM behaviour in line with the
architecture.

Reporting 0 in the FSC is probably OK, but we should also be able to
determine which level this fails at:

- Sample FAR_EL2[55] to derive which TTBR this translates from (n)
- From TCR_EL1.{TnSZ,TGn}, you can determine the number of levels

There is a bunch of tables for this in the ARM ARM, and it is possible
to come up with a decent formula that encompass all the possible
combinations.

But as I said, 0 is probably fine... ;-)

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 4/4] KVM: selftests: aarch64: Test read-only PT memory regions

[Ricardo Koller]

On Wed, Jan 25, 2023 at 12:26:01PM +0000, Marc Zyngier wrote:
> On Tue, 24 Jan 2023 19:54:57 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Tue, Jan 24, 2023 at 08:26:02AM -0800, Ricardo Koller wrote:
> > > On Mon, Jan 23, 2023 at 11:36:52PM +0000, Oliver Upton wrote:
> > > > On Tue, Jan 10, 2023 at 02:24:32AM +0000, Ricardo Koller wrote:
> > > > > Extend the read-only memslot tests in page_fault_test to test read-only PT
> > > > > (Page table) memslots. Note that this was not allowed before commit "KVM:
> > > > > arm64: Fix handling of S1PTW S2 fault on RO memslots" as all S1PTW faults
> > > > > were treated as writes which resulted in an (unrecoverable) exception
> > > > > inside the guest.
> > > > 
> > > > Do we need an additional test that the guest gets nuked if TCR_EL1.HA =
> > > > 0b1 and AF is clear in one of the stage-1 PTEs?
> > > > 
> > > 
> > > That should be easy to add. The only issue is whether that's also a case
> > > of checking for very specific KVM behavior that could change in the
> > > future.
> > 
> > From the perspective of the guest I believe this to match the
> > architecture. An external abort is appropriate if the hardware update to
> > a descriptor failed.
> > 
> > I believe that the current implementation of this in KVM is slightly
> > wrong, though. AFAICT, we encode the abort with an FSC of 0x10, which
> > indicates an SEA occurred outside of a table walk. The other nuance of
> > reporting SEAs due to a TTW is that the FSC encodes the level at which
> > the external abort occurred. Nonetheless, I think we can hide behind
> > R_BGPQR of DDI0487I.a and always encode a level of 0:
> > 
> > """
> >   If a synchronous External abort is generated due to a TLB or
> >   intermediate TLB caching structure, including parity or ECC errors,
> >   then all of the following are permitted:
> >    - If the PE cannot precisely determine the translation stage at which
> >      the error occurred, then it is reported and prioritized as a stage 1
> >      fault.
> >    - If the PE cannot precisely determine the lookup level at which the
> >      error occurred, then the lookup level is reported and prioritized
> >      as one of the following:
> >      - The lowest-numbered lookup level that could have caused the error.
> >      - If the PE cannot determine any information about the lookup level,
> >      then level 0.
> > """
> > 
> > Thoughts?
> 
> Indeed, the abort injection has always been on the dodgy side of
> things. I remember Christoffer and I writing this, saying that it was
> something we'd have to eventually fix. 10 years down the line, this
> code is, unsurprisingly, still dodgy.
> 
> My vote would be to slightly extend the API to take a set of
> KVM-specific flags to give context to the injection helpers (such as
> SEA during a TTW), and bring the KVM behaviour in line with the
> architecture.
> 
> Reporting 0 in the FSC is probably OK, but we should also be able to
> determine which level this fails at:
> 
> - Sample FAR_EL2[55] to derive which TTBR this translates from (n)
> - From TCR_EL1.{TnSZ,TGn}, you can determine the number of levels
> 
> There is a bunch of tables for this in the ARM ARM, and it is possible
> to come up with a decent formula that encompass all the possible
> combinations.
> 
> But as I said, 0 is probably fine... ;-)
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
> 

Thank you both.

So, what about the following? I can send a series after this that
includes a KVM fix to report level 0 in the FSC in this S1PTW case, and
an extra test that checks that the exception comes with some sane values
(like a sane level in the FSC). Then, getting the actual lookup level
can be added as an improvement (with less priority than the first fix).

Ricardo

Re: [PATCH 4/4] KVM: selftests: aarch64: Test read-only PT memory regions

[Marc Zyngier]

On Wed, 25 Jan 2023 14:02:18 +0000,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Wed, Jan 25, 2023 at 12:26:01PM +0000, Marc Zyngier wrote:
> > On Tue, 24 Jan 2023 19:54:57 +0000,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > On Tue, Jan 24, 2023 at 08:26:02AM -0800, Ricardo Koller wrote:
> > > > On Mon, Jan 23, 2023 at 11:36:52PM +0000, Oliver Upton wrote:
> > > > > On Tue, Jan 10, 2023 at 02:24:32AM +0000, Ricardo Koller wrote:
> > > > > > Extend the read-only memslot tests in page_fault_test to test read-only PT
> > > > > > (Page table) memslots. Note that this was not allowed before commit "KVM:
> > > > > > arm64: Fix handling of S1PTW S2 fault on RO memslots" as all S1PTW faults
> > > > > > were treated as writes which resulted in an (unrecoverable) exception
> > > > > > inside the guest.
> > > > > 
> > > > > Do we need an additional test that the guest gets nuked if TCR_EL1.HA =
> > > > > 0b1 and AF is clear in one of the stage-1 PTEs?
> > > > > 
> > > > 
> > > > That should be easy to add. The only issue is whether that's also a case
> > > > of checking for very specific KVM behavior that could change in the
> > > > future.
> > > 
> > > From the perspective of the guest I believe this to match the
> > > architecture. An external abort is appropriate if the hardware update to
> > > a descriptor failed.
> > > 
> > > I believe that the current implementation of this in KVM is slightly
> > > wrong, though. AFAICT, we encode the abort with an FSC of 0x10, which
> > > indicates an SEA occurred outside of a table walk. The other nuance of
> > > reporting SEAs due to a TTW is that the FSC encodes the level at which
> > > the external abort occurred. Nonetheless, I think we can hide behind
> > > R_BGPQR of DDI0487I.a and always encode a level of 0:
> > > 
> > > """
> > >   If a synchronous External abort is generated due to a TLB or
> > >   intermediate TLB caching structure, including parity or ECC errors,
> > >   then all of the following are permitted:
> > >    - If the PE cannot precisely determine the translation stage at which
> > >      the error occurred, then it is reported and prioritized as a stage 1
> > >      fault.
> > >    - If the PE cannot precisely determine the lookup level at which the
> > >      error occurred, then the lookup level is reported and prioritized
> > >      as one of the following:
> > >      - The lowest-numbered lookup level that could have caused the error.
> > >      - If the PE cannot determine any information about the lookup level,
> > >      then level 0.
> > > """
> > > 
> > > Thoughts?
> > 
> > Indeed, the abort injection has always been on the dodgy side of
> > things. I remember Christoffer and I writing this, saying that it was
> > something we'd have to eventually fix. 10 years down the line, this
> > code is, unsurprisingly, still dodgy.
> > 
> > My vote would be to slightly extend the API to take a set of
> > KVM-specific flags to give context to the injection helpers (such as
> > SEA during a TTW), and bring the KVM behaviour in line with the
> > architecture.
> > 
> > Reporting 0 in the FSC is probably OK, but we should also be able to
> > determine which level this fails at:
> > 
> > - Sample FAR_EL2[55] to derive which TTBR this translates from (n)
> > - From TCR_EL1.{TnSZ,TGn}, you can determine the number of levels
> > 
> > There is a bunch of tables for this in the ARM ARM, and it is possible
> > to come up with a decent formula that encompass all the possible
> > combinations.
> > 
> > But as I said, 0 is probably fine... ;-)
> > 
> > 	M.
> > 
> > -- 
> > Without deviation from the norm, progress is not possible.
> > 
> 
> Thank you both.
> 
> So, what about the following? I can send a series after this that
> includes a KVM fix to report level 0 in the FSC in this S1PTW case, and
> an extra test that checks that the exception comes with some sane values
> (like a sane level in the FSC). Then, getting the actual lookup level
> can be added as an improvement (with less priority than the first fix).

Works for me. You could also fold the level-0 fix in this series, and
only add the lookup level fix later, if ever.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/4] KVM: selftests: aarch64: page_fault_test S1PTW related fixes

[Oliver Upton]

On Tue, Jan 10, 2023 at 02:24:28AM +0000, Ricardo Koller wrote:
> Commit "KVM: arm64: Fix S1PTW handling on RO memslots" changed the way
> S1PTW faults were handled by KVM.

I understand that this commit wasn't in Linus' tree at the time you sent
these patches, could you please attribute it as:

  commit 406504c7b040 ("KVM: arm64: Fix S1PTW handling on RO memslots")

in v2?

--
Thanks,
Oliver

Re: [PATCH 0/4] KVM: selftests: aarch64: page_fault_test S1PTW related fixes

[Oliver Upton]

On Mon, Jan 23, 2023 at 11:41:57PM +0000, Oliver Upton wrote:
> On Tue, Jan 10, 2023 at 02:24:28AM +0000, Ricardo Koller wrote:
> > Commit "KVM: arm64: Fix S1PTW handling on RO memslots" changed the way
> > S1PTW faults were handled by KVM.
> 
> I understand that this commit wasn't in Linus' tree at the time you sent
> these patches, could you please attribute it as:
> 
>   commit 406504c7b040 ("KVM: arm64: Fix S1PTW handling on RO memslots")
> 
> in v2?

Sorry, I was a bit terse. What I mean is to update all of the commit
messages to reflect the suggestion.

--
Thanks,
Oliver

Re: [PATCH 0/4] KVM: selftests: aarch64: page_fault_test S1PTW related fixes

[Ricardo Koller]

On Mon, Jan 23, 2023 at 11:43:19PM +0000, Oliver Upton wrote:
> On Mon, Jan 23, 2023 at 11:41:57PM +0000, Oliver Upton wrote:
> > On Tue, Jan 10, 2023 at 02:24:28AM +0000, Ricardo Koller wrote:
> > > Commit "KVM: arm64: Fix S1PTW handling on RO memslots" changed the way
> > > S1PTW faults were handled by KVM.
> > 
> > I understand that this commit wasn't in Linus' tree at the time you sent
> > these patches, could you please attribute it as:
> > 
> >   commit 406504c7b040 ("KVM: arm64: Fix S1PTW handling on RO memslots")
> > 
> > in v2?
> 
> Sorry, I was a bit terse. What I mean is to update all of the commit
> messages to reflect the suggestion.

Sounds good, will do. I was betting on having a v2 by then (should have
mentioned it in the cover letter).

> 
> --
> Thanks,
> Oliver