public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues
@ 2023-12-12  6:27 Dan Wu
  2023-12-12  6:27 ` [kvm-unit-tests PATCH v1 1/3] x86: Add a common header asyncpf.h Dan Wu
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Dan Wu @ 2023-12-12  6:27 UTC (permalink / raw)
  To: seanjc, pbonzini, kvm; +Cc: xiaoyao.li, dan1.wu

When running asyncpf test, it gets skipped without a clear reason:

    ./asyncpf

    enabling apic
    smp: waiting for 0 APs
    paging enabled
    cr0 = 80010011
    cr3 = 107f000
    cr4 = 20
    install handler
    enable async pf
    alloc memory
    start loop
    end loop
    start loop
    end loop
    SUMMARY: 0 tests
    SKIP asyncpf (0 tests)

The reason is that KVM changed to use interrupt-based 'page-ready' notification
and abandoned #PF-based 'page-ready' notification mechanism. Interrupt-based
'page-ready' notification requires KVM_ASYNC_PF_DELIVERY_AS_INT to be set as well
in MSR_KVM_ASYNC_PF_EN to enable asyncpf.

This series tries to fix the problem by separating two testcases for different mechanisms.

- For old #PF-based notification, changes current asyncpf.c to add CPUID check
  at the beginning. It checks (KVM_FEATURE_ASYNC_PF && !KVM_FEATURE_ASYNC_PF_INT),
  otherwise it gets skipped.

- For new interrupt-based notification, add a new test, asyncpf-int.c, to check
  (KVM_FEATURE_ASYNC_PF && KVM_FEATURE_ASYNC_PF_INT) and implement interrupt-based
  'page-ready' handler.

Dan Wu (3):
  x86: Add a common header asyncpf.h
  x86: Add async page fault int test
  x86/asyncpf: Add CPUID feature bits check to ensure feature is
    available

 ci/cirrus-ci-fedora.yml |   1 +
 lib/x86/processor.h     |   6 ++
 x86/Makefile.common     |   3 +-
 x86/asyncpf.c           |  31 ++++++----
 x86/asyncpf.h           |  23 ++++++++
 x86/asyncpf_int.c       | 127 ++++++++++++++++++++++++++++++++++++++++
 x86/unittests.cfg       |   6 +-
 7 files changed, 185 insertions(+), 12 deletions(-)
 create mode 100644 x86/asyncpf.h
 create mode 100644 x86/asyncpf_int.c

-- 
2.39.3


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [kvm-unit-tests PATCH v1 1/3] x86: Add a common header asyncpf.h
  2023-12-12  6:27 [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues Dan Wu
@ 2023-12-12  6:27 ` Dan Wu
  2023-12-12  6:27 ` [kvm-unit-tests PATCH v1 2/3] x86: Add async page fault int test Dan Wu
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Dan Wu @ 2023-12-12  6:27 UTC (permalink / raw)
  To: seanjc, pbonzini, kvm; +Cc: xiaoyao.li, dan1.wu

Grab the ASYNC PF related bit definitions from asyncpf.c and put them
into asyncpf.h. Add some new definitions and they are going to be used
by asyncpf-int test which will be added later.

Signed-off-by: Dan Wu <dan1.wu@intel.com>
---
 x86/asyncpf.c |  9 +--------
 x86/asyncpf.h | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 8 deletions(-)
 create mode 100644 x86/asyncpf.h

diff --git a/x86/asyncpf.c b/x86/asyncpf.c
index bc515be9..a0bdefcf 100644
--- a/x86/asyncpf.c
+++ b/x86/asyncpf.c
@@ -27,14 +27,7 @@
 #include "libcflat.h"
 #include "vmalloc.h"
 #include <stdint.h>
-
-#define KVM_PV_REASON_PAGE_NOT_PRESENT 1
-#define KVM_PV_REASON_PAGE_READY 2
-
-#define MSR_KVM_ASYNC_PF_EN 0x4b564d02
-
-#define KVM_ASYNC_PF_ENABLED                    (1 << 0)
-#define KVM_ASYNC_PF_SEND_ALWAYS                (1 << 1)
+#include "asyncpf.h"
 
 volatile uint32_t apf_reason __attribute__((aligned(64)));
 char *buf;
diff --git a/x86/asyncpf.h b/x86/asyncpf.h
new file mode 100644
index 00000000..8e4a133e
--- /dev/null
+++ b/x86/asyncpf.h
@@ -0,0 +1,23 @@
+#define KVM_PV_REASON_PAGE_NOT_PRESENT 	1
+#define KVM_PV_REASON_PAGE_READY 2
+
+#define MSR_KVM_ASYNC_PF_EN 	0x4b564d02
+#define MSR_KVM_ASYNC_PF_INT    0x4b564d06
+#define MSR_KVM_ASYNC_PF_ACK    0x4b564d07
+
+#define KVM_ASYNC_PF_ENABLED                    (1 << 0)
+#define KVM_ASYNC_PF_SEND_ALWAYS                (1 << 1)
+#define KVM_ASYNC_PF_DELIVERY_AS_INT            (1 << 3)
+
+#define HYPERVISOR_CALLBACK_VECTOR	0xf3
+
+struct kvm_vcpu_pv_apf_data {
+      /* Used for 'page not present' events delivered via #PF */
+      uint32_t  flags;
+
+      /* Used for 'page ready' events delivered via interrupt notification */
+      uint32_t  token;
+
+      uint8_t  pad[56];
+      uint32_t  enabled;
+} __attribute__((aligned(64)));
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [kvm-unit-tests PATCH v1 2/3] x86: Add async page fault int test
  2023-12-12  6:27 [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues Dan Wu
  2023-12-12  6:27 ` [kvm-unit-tests PATCH v1 1/3] x86: Add a common header asyncpf.h Dan Wu
@ 2023-12-12  6:27 ` Dan Wu
  2023-12-12  6:27 ` [kvm-unit-tests PATCH v1 3/3] x86/asyncpf: Add CPUID feature bits check to ensure feature is available Dan Wu
  2023-12-12 15:17 ` [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues Sean Christopherson
  3 siblings, 0 replies; 8+ messages in thread
From: Dan Wu @ 2023-12-12  6:27 UTC (permalink / raw)
  To: seanjc, pbonzini, kvm; +Cc: xiaoyao.li, dan1.wu

KVM switched to use interrupt for 'page ready' APF event since Linux v5.10 and
the legacy mechanism using #PF was deprecated. Add a new test for interrupt
based 'page ready' APF event delivery.

Signed-off-by: Dan Wu <dan1.wu@intel.com>
Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
The test is based on asyncpf.c and simplifies implementation.
---
 ci/cirrus-ci-fedora.yml |   1 +
 lib/x86/processor.h     |   6 ++
 x86/Makefile.common     |   3 +-
 x86/asyncpf_int.c       | 127 ++++++++++++++++++++++++++++++++++++++++
 x86/unittests.cfg       |   4 ++
 5 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 x86/asyncpf_int.c

diff --git a/ci/cirrus-ci-fedora.yml b/ci/cirrus-ci-fedora.yml
index 918c9a36..52cb10c6 100644
--- a/ci/cirrus-ci-fedora.yml
+++ b/ci/cirrus-ci-fedora.yml
@@ -22,6 +22,7 @@ fedora_task:
     - ./run_tests.sh
         access
         asyncpf
+        asyncpf_int
         debug
         emulator
         ept
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 44f4fd1e..1a0f1243 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -263,6 +263,12 @@ static inline bool is_intel(void)
 #define	X86_FEATURE_ARCH_CAPABILITIES	(CPUID(0x7, 0, EDX, 29))
 #define	X86_FEATURE_PKS			(CPUID(0x7, 0, ECX, 31))
 
+/*
+ * KVM defined leafs
+ */
+#define	KVM_FEATURE_ASYNC_PF		(CPUID(0x40000001, 0, EAX, 4))
+#define	KVM_FEATURE_ASYNC_PF_INT	(CPUID(0x40000001, 0, EAX, 14))
+
 /*
  * Extended Leafs, a.k.a. AMD defined
  */
diff --git a/x86/Makefile.common b/x86/Makefile.common
index 4ae9a557..c4b309e3 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -90,7 +90,8 @@ tests-common = $(TEST_DIR)/vmexit.$(exe) $(TEST_DIR)/tsc.$(exe) \
                $(TEST_DIR)/emulator.$(exe) \
                $(TEST_DIR)/eventinj.$(exe) \
                $(TEST_DIR)/smap.$(exe) \
-               $(TEST_DIR)/umip.$(exe)
+               $(TEST_DIR)/umip.$(exe) \
+               $(TEST_DIR)/asyncpf_int.$(exe)
 
 # The following test cases are disabled when building EFI tests because they
 # use absolute addresses in their inline assembly code, which cannot compile
diff --git a/x86/asyncpf_int.c b/x86/asyncpf_int.c
new file mode 100644
index 00000000..84268f6b
--- /dev/null
+++ b/x86/asyncpf_int.c
@@ -0,0 +1,127 @@
+/*
+ * Async PF Int test. For the test to actually do anything it needs to be started
+ * in memory cgroup with 512M of memory and with more than 1G memory provided
+ * to the guest.
+ *
+ * To identify the cgroup version on Linux:
+ * stat -fc %T /sys/fs/cgroup/
+ *
+ * If the output is tmpfs, your system is using cgroup v1:
+ * To create cgroup do as root:
+ * mkdir /dev/cgroup
+ * mount -t cgroup none -omemory /dev/cgroup
+ * chmod a+rxw /dev/cgroup/
+ * From a shell you will start qemu from:
+ * mkdir /dev/cgroup/1
+ * echo $$ >  /dev/cgroup/1/tasks
+ * echo 512M > /dev/cgroup/1/memory.limit_in_bytes
+ *
+ * If the output is cgroup2fs, your system is using cgroup v2:
+ * mkdir /sys/fs/cgroup/cg1
+ * echo $$ >  /sys/fs/cgroup/cg1/cgroup.procs
+ * echo 512M > /sys/fs/cgroup/cg1/memory.max
+ *
+ */
+#include "x86/processor.h"
+#include "x86/apic.h"
+#include "x86/isr.h"
+#include "x86/vm.h"
+#include "alloc.h"
+#include "vmalloc.h"
+#include "asyncpf.h"
+
+struct kvm_vcpu_pv_apf_data apf_reason;
+
+char *buf;
+void* virt;
+volatile uint64_t  i;
+volatile uint64_t phys;
+volatile uint32_t saved_token;
+volatile uint32_t asyncpf_num;
+
+static inline uint32_t get_and_clear_apf_reason(void)
+{
+	uint32_t r = apf_reason.flags;
+	apf_reason.flags = 0;
+	return r;
+}
+
+static void handle_interrupt(isr_regs_t *regs)
+{
+	uint32_t apf_token = apf_reason.token;
+
+	apf_reason.token = 0;
+	wrmsr(MSR_KVM_ASYNC_PF_ACK, 1);
+
+	if (apf_token == 0xffffffff) {
+		report_pass("Wakeup all, got token 0x%x", apf_token);
+	} else if (apf_token == saved_token) {
+		asyncpf_num++;
+		install_pte(phys_to_virt(read_cr3()), 1, virt, phys | PT_PRESENT_MASK | PT_WRITABLE_MASK, 0);
+		phys = 0;
+	} else {
+		report_fail("unexpected async pf int token 0x%x", apf_token);
+	}
+
+	eoi();
+}
+
+static void handle_pf(struct ex_regs *r)
+{
+	virt = (void*)((ulong)(buf+i) & ~(PAGE_SIZE-1));
+	uint32_t reason = get_and_clear_apf_reason();
+	switch (reason) {
+	case 0:
+		report_fail("unexpected #PF at %#lx", read_cr2());
+		exit(report_summary());
+	case KVM_PV_REASON_PAGE_NOT_PRESENT:
+		phys = virt_to_pte_phys(phys_to_virt(read_cr3()), virt);
+		install_pte(phys_to_virt(read_cr3()), 1, virt, phys, 0);
+		write_cr3(read_cr3());
+		saved_token = read_cr2();
+		while (phys) {
+			safe_halt(); /* enables irq */
+		}
+		break;
+	default:
+		report_fail("unexpected async pf with reason 0x%x", reason);
+		exit(report_summary());
+	}
+}
+
+#define MEM (1ull*1024*1024*1024)
+
+int main(int ac, char **av)
+{
+	if (!this_cpu_has(KVM_FEATURE_ASYNC_PF)) {
+		report_skip("KVM_FEATURE_ASYNC_PF is not supported\n");
+		return report_summary();
+	}
+
+	if (!this_cpu_has(KVM_FEATURE_ASYNC_PF_INT)) {
+		report_skip("KVM_FEATURE_ASYNC_PF_INT is not supported\n");
+		return report_summary();
+	}
+
+	setup_vm();
+
+	handle_exception(PF_VECTOR, handle_pf);
+	handle_irq(HYPERVISOR_CALLBACK_VECTOR, handle_interrupt);
+	memset(&apf_reason, 0, sizeof(apf_reason));
+
+	wrmsr(MSR_KVM_ASYNC_PF_INT, HYPERVISOR_CALLBACK_VECTOR);
+	wrmsr(MSR_KVM_ASYNC_PF_EN, virt_to_phys((void*)&apf_reason) |
+		KVM_ASYNC_PF_SEND_ALWAYS | KVM_ASYNC_PF_ENABLED | KVM_ASYNC_PF_DELIVERY_AS_INT);
+
+	buf = malloc(MEM);
+	sti();
+
+	/* access a lot of memory to make host swap it out */
+	for (i = 0; i < MEM; i += 4096)
+		buf[i] = 1;
+
+	cli();
+	report(asyncpf_num > 0, "get %d async pf events ('page not present' #PF event with matched "
+		"'page ready' interrupt event )", asyncpf_num);
+	return report_summary();
+}
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 3fe59449..8735ba34 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -174,6 +174,10 @@ extra_params = -cpu max
 file = asyncpf.flat
 extra_params = -m 2048
 
+[asyncpf_int]
+file = asyncpf_int.flat
+extra_params = -cpu host -m 2048
+
 [emulator]
 file = emulator.flat
 
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [kvm-unit-tests PATCH v1 3/3] x86/asyncpf: Add CPUID feature bits check to ensure feature is available
  2023-12-12  6:27 [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues Dan Wu
  2023-12-12  6:27 ` [kvm-unit-tests PATCH v1 1/3] x86: Add a common header asyncpf.h Dan Wu
  2023-12-12  6:27 ` [kvm-unit-tests PATCH v1 2/3] x86: Add async page fault int test Dan Wu
@ 2023-12-12  6:27 ` Dan Wu
  2023-12-12 15:17 ` [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues Sean Christopherson
  3 siblings, 0 replies; 8+ messages in thread
From: Dan Wu @ 2023-12-12  6:27 UTC (permalink / raw)
  To: seanjc, pbonzini, kvm; +Cc: xiaoyao.li, dan1.wu

Asyncpf test only works for #PF-based 'page-ready' notification. This case
becomes invalid for KVM that starts to enumerate interrupt-based 'page-ready'
notification.

Add CPUID feature check at the beginning, and skip the test when KVM_FEATURE_ASYNC_PF
is not available or it enumerates KVM_FEATURE_ASYNC_PF_INT.

To run this test, add the QEMU option "-cpu host" to check CPUID, since
KVM_FEATURE_ASYNC_PF_INT can't be detected without "-cpu host".

Update the usage of how to setup cgroup for different cgroup versions.

Signed-off-by: Dan Wu <dan1.wu@intel.com>
Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Unlike patch #2, this patch retains the original test without reducing the memory
access round from 2 to 1. If anyone thinks it worthes a go, please call out.
---
 x86/asyncpf.c     | 22 ++++++++++++++++++++--
 x86/unittests.cfg |  2 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/x86/asyncpf.c b/x86/asyncpf.c
index a0bdefcf..e2cf9934 100644
--- a/x86/asyncpf.c
+++ b/x86/asyncpf.c
@@ -1,18 +1,26 @@
 /*
  * Async PF test. For the test to actually do anything it needs to be started
- * in memory cgroup with 512M of memory and with more then 1G memory provided
+ * in memory cgroup with 512M of memory and with more than 1G memory provided
  * to the guest.
  *
+ * To identify the cgroup version on Linux:
+ * stat -fc %T /sys/fs/cgroup/
+ *
+ * If the output is tmpfs, your system is using cgroup v1:
  * To create cgroup do as root:
  * mkdir /dev/cgroup
  * mount -t cgroup none -omemory /dev/cgroup
  * chmod a+rxw /dev/cgroup/
- *
  * From a shell you will start qemu from:
  * mkdir /dev/cgroup/1
  * echo $$ >  /dev/cgroup/1/tasks
  * echo 512M > /dev/cgroup/1/memory.limit_in_bytes
  *
+ * If the output is cgroup2fs, your system is using cgroup v2:
+ * mkdir /sys/fs/cgroup/cg1
+ * echo $$ >  /sys/fs/cgroup/cg1/cgroup.procs
+ * echo 512M > /sys/fs/cgroup/cg1/memory.max
+ *
  */
 #include "x86/msr.h"
 #include "x86/processor.h"
@@ -79,6 +87,16 @@ static void pf_isr(struct ex_regs *r)
 
 int main(int ac, char **av)
 {
+	if (!this_cpu_has(KVM_FEATURE_ASYNC_PF)) {
+		report_skip("KVM_FEATURE_ASYNC_PF is not supported\n");
+		return report_summary();
+	}
+
+	if (this_cpu_has(KVM_FEATURE_ASYNC_PF_INT)) {
+		report_skip("interrupt-based page-ready event is enumerated, use asyncpf-int instead.\n");
+		return report_summary();
+	}
+
 	int loop = 2;
 
 	setup_vm();
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 8735ba34..94a5c7c7 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -172,7 +172,7 @@ extra_params = -cpu max
 
 [asyncpf]
 file = asyncpf.flat
-extra_params = -m 2048
+extra_params = -cpu host -m 2048
 
 [asyncpf_int]
 file = asyncpf_int.flat
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues
  2023-12-12  6:27 [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues Dan Wu
                   ` (2 preceding siblings ...)
  2023-12-12  6:27 ` [kvm-unit-tests PATCH v1 3/3] x86/asyncpf: Add CPUID feature bits check to ensure feature is available Dan Wu
@ 2023-12-12 15:17 ` Sean Christopherson
  2023-12-13  1:36   ` Wu, Dan1
  3 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2023-12-12 15:17 UTC (permalink / raw)
  To: Dan Wu; +Cc: pbonzini, kvm, xiaoyao.li

On Tue, Dec 12, 2023, Dan Wu wrote:
> When running asyncpf test, it gets skipped without a clear reason:
> 
>     ./asyncpf
> 
>     enabling apic
>     smp: waiting for 0 APs
>     paging enabled
>     cr0 = 80010011
>     cr3 = 107f000
>     cr4 = 20
>     install handler
>     enable async pf
>     alloc memory
>     start loop
>     end loop
>     start loop
>     end loop
>     SUMMARY: 0 tests
>     SKIP asyncpf (0 tests)
> 
> The reason is that KVM changed to use interrupt-based 'page-ready' notification
> and abandoned #PF-based 'page-ready' notification mechanism. Interrupt-based
> 'page-ready' notification requires KVM_ASYNC_PF_DELIVERY_AS_INT to be set as well
> in MSR_KVM_ASYNC_PF_EN to enable asyncpf.
> 
> This series tries to fix the problem by separating two testcases for different mechanisms.
> 
> - For old #PF-based notification, changes current asyncpf.c to add CPUID check
>   at the beginning. It checks (KVM_FEATURE_ASYNC_PF && !KVM_FEATURE_ASYNC_PF_INT),
>   otherwise it gets skipped.
> 
> - For new interrupt-based notification, add a new test, asyncpf-int.c, to check
>   (KVM_FEATURE_ASYNC_PF && KVM_FEATURE_ASYNC_PF_INT) and implement interrupt-based
>   'page-ready' handler.

Using #PF to deliver page-ready is completely dead, no?  Unless I'm mistaken, let's
just drop the existing support and replace it with the interrupted-based mechanism.
I see no reason to continue maintaining the old crud.  If someone wants to verify
an old, broken KVM, then they can use the old version of KUT.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues
  2023-12-12 15:17 ` [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues Sean Christopherson
@ 2023-12-13  1:36   ` Wu, Dan1
  2023-12-13 18:20     ` Sean Christopherson
  0 siblings, 1 reply; 8+ messages in thread
From: Wu, Dan1 @ 2023-12-13  1:36 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: pbonzini, kvm, xiaoyao.li


On 12/12/2023 11:17 PM, Sean Christopherson wrote:
> On Tue, Dec 12, 2023, Dan Wu wrote:
>> When running asyncpf test, it gets skipped without a clear reason:
>>
>>      ./asyncpf
>>
>>      enabling apic
>>      smp: waiting for 0 APs
>>      paging enabled
>>      cr0 = 80010011
>>      cr3 = 107f000
>>      cr4 = 20
>>      install handler
>>      enable async pf
>>      alloc memory
>>      start loop
>>      end loop
>>      start loop
>>      end loop
>>      SUMMARY: 0 tests
>>      SKIP asyncpf (0 tests)
>>
>> The reason is that KVM changed to use interrupt-based 'page-ready' notification
>> and abandoned #PF-based 'page-ready' notification mechanism. Interrupt-based
>> 'page-ready' notification requires KVM_ASYNC_PF_DELIVERY_AS_INT to be set as well
>> in MSR_KVM_ASYNC_PF_EN to enable asyncpf.
>>
>> This series tries to fix the problem by separating two testcases for different mechanisms.
>>
>> - For old #PF-based notification, changes current asyncpf.c to add CPUID check
>>    at the beginning. It checks (KVM_FEATURE_ASYNC_PF && !KVM_FEATURE_ASYNC_PF_INT),
>>    otherwise it gets skipped.
>>
>> - For new interrupt-based notification, add a new test, asyncpf-int.c, to check
>>    (KVM_FEATURE_ASYNC_PF && KVM_FEATURE_ASYNC_PF_INT) and implement interrupt-based
>>    'page-ready' handler.
> Using #PF to deliver page-ready is completely dead, no?  Unless I'm mistaken, let's
> just drop the existing support and replace it with the interrupted-based mechanism.
> I see no reason to continue maintaining the old crud.  If someone wants to verify
> an old, broken KVM, then they can use the old version of  KUT.

Yes, since Linux v5.10 the feature asyncpf is deprecated.

So, just drop asyncpf.c and add asyncpf_int.c is enough, right?

>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues
  2023-12-13  1:36   ` Wu, Dan1
@ 2023-12-13 18:20     ` Sean Christopherson
  2023-12-14  2:32       ` Wu, Dan1
  0 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2023-12-13 18:20 UTC (permalink / raw)
  To: Dan1 Wu; +Cc: pbonzini, kvm, xiaoyao.li

On Wed, Dec 13, 2023, Dan1 Wu wrote:
> 
> On 12/12/2023 11:17 PM, Sean Christopherson wrote:
> > On Tue, Dec 12, 2023, Dan Wu wrote:
> > > When running asyncpf test, it gets skipped without a clear reason:
> > > 
> > >      ./asyncpf
> > > 
> > >      enabling apic
> > >      smp: waiting for 0 APs
> > >      paging enabled
> > >      cr0 = 80010011
> > >      cr3 = 107f000
> > >      cr4 = 20
> > >      install handler
> > >      enable async pf
> > >      alloc memory
> > >      start loop
> > >      end loop
> > >      start loop
> > >      end loop
> > >      SUMMARY: 0 tests
> > >      SKIP asyncpf (0 tests)
> > > 
> > > The reason is that KVM changed to use interrupt-based 'page-ready' notification
> > > and abandoned #PF-based 'page-ready' notification mechanism. Interrupt-based
> > > 'page-ready' notification requires KVM_ASYNC_PF_DELIVERY_AS_INT to be set as well
> > > in MSR_KVM_ASYNC_PF_EN to enable asyncpf.
> > > 
> > > This series tries to fix the problem by separating two testcases for different mechanisms.
> > > 
> > > - For old #PF-based notification, changes current asyncpf.c to add CPUID check
> > >    at the beginning. It checks (KVM_FEATURE_ASYNC_PF && !KVM_FEATURE_ASYNC_PF_INT),
> > >    otherwise it gets skipped.
> > > 
> > > - For new interrupt-based notification, add a new test, asyncpf-int.c, to check
> > >    (KVM_FEATURE_ASYNC_PF && KVM_FEATURE_ASYNC_PF_INT) and implement interrupt-based
> > >    'page-ready' handler.
> > Using #PF to deliver page-ready is completely dead, no?  Unless I'm mistaken, let's
> > just drop the existing support and replace it with the interrupted-based mechanism.
> > I see no reason to continue maintaining the old crud.  If someone wants to verify
> > an old, broken KVM, then they can use the old version of  KUT.
> 
> Yes, since Linux v5.10 the feature asyncpf is deprecated.
> 
> So, just drop asyncpf.c and add asyncpf_int.c is enough, right?

I would rather not add asyncpf_int.c, and instead keep asyncpf.c and modify it to
use ASYNC_PF_INT.  It _might_ be a bit more churn, but modifying the existing code
instead of dropping in a new file will better preserve the history, and may also
allow for finer grained patches (not sure on that one).

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues
  2023-12-13 18:20     ` Sean Christopherson
@ 2023-12-14  2:32       ` Wu, Dan1
  0 siblings, 0 replies; 8+ messages in thread
From: Wu, Dan1 @ 2023-12-14  2:32 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: pbonzini, kvm, xiaoyao.li


On 12/14/2023 2:20 AM, Sean Christopherson wrote:
> On Wed, Dec 13, 2023, Dan1 Wu wrote:
>> On 12/12/2023 11:17 PM, Sean Christopherson wrote:
>>> On Tue, Dec 12, 2023, Dan Wu wrote:
>>>> When running asyncpf test, it gets skipped without a clear reason:
>>>>
>>>>       ./asyncpf
>>>>
>>>>       enabling apic
>>>>       smp: waiting for 0 APs
>>>>       paging enabled
>>>>       cr0 = 80010011
>>>>       cr3 = 107f000
>>>>       cr4 = 20
>>>>       install handler
>>>>       enable async pf
>>>>       alloc memory
>>>>       start loop
>>>>       end loop
>>>>       start loop
>>>>       end loop
>>>>       SUMMARY: 0 tests
>>>>       SKIP asyncpf (0 tests)
>>>>
>>>> The reason is that KVM changed to use interrupt-based 'page-ready' notification
>>>> and abandoned #PF-based 'page-ready' notification mechanism. Interrupt-based
>>>> 'page-ready' notification requires KVM_ASYNC_PF_DELIVERY_AS_INT to be set as well
>>>> in MSR_KVM_ASYNC_PF_EN to enable asyncpf.
>>>>
>>>> This series tries to fix the problem by separating two testcases for different mechanisms.
>>>>
>>>> - For old #PF-based notification, changes current asyncpf.c to add CPUID check
>>>>     at the beginning. It checks (KVM_FEATURE_ASYNC_PF && !KVM_FEATURE_ASYNC_PF_INT),
>>>>     otherwise it gets skipped.
>>>>
>>>> - For new interrupt-based notification, add a new test, asyncpf-int.c, to check
>>>>     (KVM_FEATURE_ASYNC_PF && KVM_FEATURE_ASYNC_PF_INT) and implement interrupt-based
>>>>     'page-ready' handler.
>>> Using #PF to deliver page-ready is completely dead, no?  Unless I'm mistaken, let's
>>> just drop the existing support and replace it with the interrupted-based mechanism.
>>> I see no reason to continue maintaining the old crud.  If someone wants to verify
>>> an old, broken KVM, then they can use the old version of  KUT.
>> Yes, since Linux v5.10 the feature asyncpf is deprecated.
>>
>> So, just drop asyncpf.c and add asyncpf_int.c is enough, right?
> I would rather not add asyncpf_int.c, and instead keep asyncpf.c and modify it to
> use ASYNC_PF_INT.  It _might_ be a bit more churn, but modifying the existing code
> instead of dropping in a new file will better preserve the history, and may also
> allow for finer grained patches (not sure on that one).

ok, I will modify it in the next version. Thanks for your review.


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-12-14  2:32 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-12  6:27 [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues Dan Wu
2023-12-12  6:27 ` [kvm-unit-tests PATCH v1 1/3] x86: Add a common header asyncpf.h Dan Wu
2023-12-12  6:27 ` [kvm-unit-tests PATCH v1 2/3] x86: Add async page fault int test Dan Wu
2023-12-12  6:27 ` [kvm-unit-tests PATCH v1 3/3] x86/asyncpf: Add CPUID feature bits check to ensure feature is available Dan Wu
2023-12-12 15:17 ` [kvm-unit-tests PATCH v1 0/3] x86: fix async page fault issues Sean Christopherson
2023-12-13  1:36   ` Wu, Dan1
2023-12-13 18:20     ` Sean Christopherson
2023-12-14  2:32       ` Wu, Dan1

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox