Re: [PATCH V10 01/18] perf/core: Use static_call to optimize perf_guest_info_callbacks
On 8/27/2021 3:59 AM, Sean Christopherson wrote:
TL;DR: Please don't merge this patch, it's broken and is also built on a shoddy
foundation that I would like to fix.
Hi Sean,Peter, Paolo
I will send out an V11 which drops this patch since it's buggy, and Sean
is working on fix this.
Does this sound good?
Thanks,
Zhu Lingshan
On Fri, Aug 06, 2021, Zhu Lingshan wrote:
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 464917096e73..e466fc8176e1 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6489,9 +6489,18 @@ static void perf_pending_event(struct irq_work *entry)
*/
struct perf_guest_info_callbacks *perf_guest_cbs;
+/* explicitly use __weak to fix duplicate symbol error */
+void __weak arch_perf_update_guest_cbs(void)
+{
+}
+
int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
{
+ if (WARN_ON_ONCE(perf_guest_cbs))
+ return -EBUSY;
+
perf_guest_cbs = cbs;
+ arch_perf_update_guest_cbs();
This is horribly broken, it fails to cleanup the static calls when KVM
unregisters
the callbacks, which happens when the vendor module, e.g. kvm_intel, is
unloaded.
The explosion doesn't happen until 'kvm' is unloaded because the functions are
implemented in 'kvm', i.e. the use-after-free is deferred a bit.
BUG: unable to handle page fault for address: a011bb90
#PF: supervisor instruction fetch in kernel mode
#PF: error_code(0x0010) - not-present page
PGD 6211067 P4D 6211067 PUD 6212063 PMD 102b99067 PTE 0
Oops: 0010 [#1] PREEMPT SMP
CPU: 0 PID: 1047 Comm: rmmod Not tainted 5.14.0-rc2+ #460
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
RIP: 0010:0xa011bb90
Code: Unable to access opcode bytes at RIP 0xa011bb66.
Call Trace:
? perf_misc_flags+0xe/0x50
? perf_prepare_sample+0x53/0x6b0
? perf_event_output_forward+0x67/0x160
? kvm_clock_read+0x14/0x30
? kvm_sched_clock_read+0x5/0x10
? sched_clock_cpu+0xd/0xd0
? __perf_event_overflow+0x52/0xf0
? handle_pmi_common+0x1f2/0x2d0
? __flush_tlb_all+0x30/0x30
? intel_pmu_handle_irq+0xcf/0x410
? nmi_handle+0x5/0x260
? perf_event_nmi_handler+0x28/0x50
? nmi_handle+0xc7/0x260
? lock_release+0x2b0/0x2b0
? default_do_nmi+0x6b/0x170
? exc_nmi+0x103/0x130
? end_repeat_nmi+0x16/0x1f
? lock_release+0x2b0/0x2b0
? lock_release+0x2b0/0x2b0
? lock_release+0x2b0/0x2b0
Modules linked in: irqbypass [last unloaded: kvm]
Even more fun, the existing perf_guest_cbs framework is also broken, though it's
much harder to get it to fail, and probably impossible to get it to fail without
some help. The issue is that perf_guest_cbs is global, which means that it can
be nullified by KVM (during module unload) while the callbacks are being
accessed
by a PMI handler on a different CPU.
The bug has escaped notice because all dererfences of perf_guest_cbs follow the
same "perf_guest_cbs && perf_guest_cbs->is_in_guest()" pattern, and AFAICT the
compiler never reload perf_guest_cbs in this sequence. The compiler does reload
perf_guest_cbs for any future dereferences, but the ->is_in_guest() guard all
but
guarantees the PMI handler will win the race, e.g. to nullify perf_guest_cbs,
KVM has to completely exit the guest and teardown down all VMs before it can be
unloaded.
But with a help, e.g. RAED_ONCE(perf_guest_cbs), unloading kvm_intel can trigger
a NULL pointer derference, e.g. this tweak
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 1eb45139fcc6..202e5ad97f82 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2954,7 +2954,7 @@ unsigned long perf_misc_flags(struct pt_regs *regs)
{
int misc = 0;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+ if (READ_ONCE(perf_guest_cbs) &&
READ_ONCE(perf_guest_cbs)->is_in_guest()) {
if (perf_guest_cbs->is_user_mode())
misc |= PERF_RECORD_MISC_GUEST_USER;
else
while spamming module load/unload leads to:
BUG: kernel NULL pointer dereference, address:
#PF: supervisor read access in kernel mode
#PF: error_code(0x) - not-present page
PGD 0 P4D 0
Oops: [#1] PREEMPT SMP
CPU: 6 PID: 1825 Comm: stress Not tainted 5.14.0-rc2+ #459
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
RIP: 0010:perf_misc_flags+0x1c/0x70
Call Trace:
perf_prepare_sample+0x53/0x6b0
perf_event_output_forward+0x67/0x160
__perf_event_overflow+0x52/0xf0
handle_pmi_common+0x207/0x300
intel_pmu_handle_irq+0xcf/0x410
perf_event_nmi_handler+0x28/0x50
nmi_handle+0xc7/0x260
default_do_nmi+0x6b/0x170
exc_nmi+0x103/0x130
asm_exc_nmi+0x76/0xbf
The good news is that I have a series th
[PATCH V10 01/18] perf/core: Use static_call to optimize perf_guest_info_callbacks
From: Like Xu
For "struct perf_guest_info_callbacks", the two fields "is_in_guest"
and "is_user_mode" are replaced with a new multiplexed member named
"state", and the "get_guest_ip" field will be renamed to "get_ip".
For arm64, xen and kvm/x86, the application of DEFINE_STATIC_CALL_RET0
could make all that perf_guest_cbs stuff suck less. For arm, csky, nds32,
and riscv, just applied some renamed refactoring.
Cc: Will Deacon
Cc: Marc Zyngier
Cc: Guo Ren
Cc: Nick Hu
Cc: Paul Walmsley
Cc: Boris Ostrovsky
Cc: linux-arm-ker...@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-c...@vger.kernel.org
Cc: linux-ri...@lists.infradead.org
Cc: xen-de...@lists.xenproject.org
Suggested-by: Peter Zijlstra (Intel)
Original-by: Peter Zijlstra (Intel)
Signed-off-by: Like Xu
Signed-off-by: Zhu Lingshan
Reviewed-by: Boris Ostrovsky
Acked-by: Peter Zijlstra (Intel)
---
arch/arm/kernel/perf_callchain.c | 16 +++-
arch/arm64/kernel/perf_callchain.c | 29 +-
arch/arm64/kvm/perf.c | 22 -
arch/csky/kernel/perf_callchain.c | 4 +--
arch/nds32/kernel/perf_event_cpu.c | 16 +++-
arch/riscv/kernel/perf_callchain.c | 4 +--
arch/x86/events/core.c | 39 --
arch/x86/events/intel/core.c | 7 +++---
arch/x86/include/asm/kvm_host.h| 2 +-
arch/x86/kvm/pmu.c | 2 +-
arch/x86/kvm/x86.c | 37 +++-
arch/x86/xen/pmu.c | 33 ++---
include/linux/perf_event.h | 12 ++---
kernel/events/core.c | 9 +++
14 files changed, 144 insertions(+), 88 deletions(-)
diff --git a/arch/arm/kernel/perf_callchain.c b/arch/arm/kernel/perf_callchain.c
index 3b69a76d341e..1ce30f86d6c7 100644
--- a/arch/arm/kernel/perf_callchain.c
+++ b/arch/arm/kernel/perf_callchain.c
@@ -64,7 +64,7 @@ perf_callchain_user(struct perf_callchain_entry_ctx *entry,
struct pt_regs *regs
{
struct frame_tail __user *tail;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+ if (perf_guest_cbs && perf_guest_cbs->state()) {
/* We don't support guest os callchain now */
return;
}
@@ -100,7 +100,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
*entry, struct pt_regs *re
{
struct stackframe fr;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+ if (perf_guest_cbs && perf_guest_cbs->state()) {
/* We don't support guest os callchain now */
return;
}
@@ -111,8 +111,8 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
*entry, struct pt_regs *re
unsigned long perf_instruction_pointer(struct pt_regs *regs)
{
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest())
- return perf_guest_cbs->get_guest_ip();
+ if (perf_guest_cbs && perf_guest_cbs->state())
+ return perf_guest_cbs->get_ip();
return instruction_pointer(regs);
}
@@ -120,9 +120,13 @@ unsigned long perf_instruction_pointer(struct pt_regs
*regs)
unsigned long perf_misc_flags(struct pt_regs *regs)
{
int misc = 0;
+ unsigned int state = 0;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
- if (perf_guest_cbs->is_user_mode())
+ if (perf_guest_cbs)
+ state = perf_guest_cbs->state();
+
+ if (perf_guest_cbs && state) {
+ if (state & PERF_GUEST_USER)
misc |= PERF_RECORD_MISC_GUEST_USER;
else
misc |= PERF_RECORD_MISC_GUEST_KERNEL;
diff --git a/arch/arm64/kernel/perf_callchain.c
b/arch/arm64/kernel/perf_callchain.c
index 4a72c2727309..1b344e23fd2f 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -5,6 +5,7 @@
* Copyright (C) 2015 ARM Limited
*/
#include
+#include
#include
#include
@@ -99,10 +100,25 @@ compat_user_backtrace(struct compat_frame_tail __user
*tail,
}
#endif /* CONFIG_COMPAT */
+DEFINE_STATIC_CALL_RET0(arm64_guest_state, *(perf_guest_cbs->state));
+DEFINE_STATIC_CALL_RET0(arm64_guest_get_ip, *(perf_guest_cbs->get_ip));
+
+void arch_perf_update_guest_cbs(void)
+{
+ static_call_update(arm64_guest_state, (void *)&__static_call_return0);
+ static_call_update(arm64_guest_get_ip, (void *)&__static_call_return0);
+
+ if (perf_guest_cbs && perf_guest_cbs->state)
+ static_call_update(arm64_guest_state, perf_guest_cbs->state);
+
+ if (perf_guest_cbs && perf_guest_cbs->get_ip)
+ static_call_update(arm64_guest_get_ip, perf_guest_cbs->get_ip);
+}
+
void perf_callchain_user(struct perf_callchain_entry_ctx
[PATCH V9 01/18] perf/core: Use static_call to optimize perf_guest_info_callbacks
From: Like Xu
For "struct perf_guest_info_callbacks", the two fields "is_in_guest"
and "is_user_mode" are replaced with a new multiplexed member named
"state", and the "get_guest_ip" field will be renamed to "get_ip".
For arm64, xen and kvm/x86, the application of DEFINE_STATIC_CALL_RET0
could make all that perf_guest_cbs stuff suck less. For arm, csky, nds32,
and riscv, just applied some renamed refactoring.
Cc: Will Deacon
Cc: Marc Zyngier
Cc: Guo Ren
Cc: Nick Hu
Cc: Paul Walmsley
Cc: Boris Ostrovsky
Cc: linux-arm-ker...@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-c...@vger.kernel.org
Cc: linux-ri...@lists.infradead.org
Cc: xen-de...@lists.xenproject.org
Suggested-by: Peter Zijlstra (Intel)
Original-by: Peter Zijlstra (Intel)
Signed-off-by: Like Xu
Signed-off-by: Zhu Lingshan
Reviewed-by: Boris Ostrovsky
---
arch/arm/kernel/perf_callchain.c | 16 +++-
arch/arm64/kernel/perf_callchain.c | 29 +-
arch/arm64/kvm/perf.c | 22 -
arch/csky/kernel/perf_callchain.c | 4 +--
arch/nds32/kernel/perf_event_cpu.c | 16 +++-
arch/riscv/kernel/perf_callchain.c | 4 +--
arch/x86/events/core.c | 39 --
arch/x86/events/intel/core.c | 7 +++---
arch/x86/include/asm/kvm_host.h| 2 +-
arch/x86/kvm/pmu.c | 2 +-
arch/x86/kvm/x86.c | 37 +++-
arch/x86/xen/pmu.c | 33 ++---
include/linux/perf_event.h | 12 ++---
kernel/events/core.c | 9 +++
14 files changed, 144 insertions(+), 88 deletions(-)
diff --git a/arch/arm/kernel/perf_callchain.c b/arch/arm/kernel/perf_callchain.c
index 3b69a76d341e..1ce30f86d6c7 100644
--- a/arch/arm/kernel/perf_callchain.c
+++ b/arch/arm/kernel/perf_callchain.c
@@ -64,7 +64,7 @@ perf_callchain_user(struct perf_callchain_entry_ctx *entry,
struct pt_regs *regs
{
struct frame_tail __user *tail;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+ if (perf_guest_cbs && perf_guest_cbs->state()) {
/* We don't support guest os callchain now */
return;
}
@@ -100,7 +100,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
*entry, struct pt_regs *re
{
struct stackframe fr;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+ if (perf_guest_cbs && perf_guest_cbs->state()) {
/* We don't support guest os callchain now */
return;
}
@@ -111,8 +111,8 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
*entry, struct pt_regs *re
unsigned long perf_instruction_pointer(struct pt_regs *regs)
{
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest())
- return perf_guest_cbs->get_guest_ip();
+ if (perf_guest_cbs && perf_guest_cbs->state())
+ return perf_guest_cbs->get_ip();
return instruction_pointer(regs);
}
@@ -120,9 +120,13 @@ unsigned long perf_instruction_pointer(struct pt_regs
*regs)
unsigned long perf_misc_flags(struct pt_regs *regs)
{
int misc = 0;
+ unsigned int state = 0;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
- if (perf_guest_cbs->is_user_mode())
+ if (perf_guest_cbs)
+ state = perf_guest_cbs->state();
+
+ if (perf_guest_cbs && state) {
+ if (state & PERF_GUEST_USER)
misc |= PERF_RECORD_MISC_GUEST_USER;
else
misc |= PERF_RECORD_MISC_GUEST_KERNEL;
diff --git a/arch/arm64/kernel/perf_callchain.c
b/arch/arm64/kernel/perf_callchain.c
index 4a72c2727309..1b344e23fd2f 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -5,6 +5,7 @@
* Copyright (C) 2015 ARM Limited
*/
#include
+#include
#include
#include
@@ -99,10 +100,25 @@ compat_user_backtrace(struct compat_frame_tail __user
*tail,
}
#endif /* CONFIG_COMPAT */
+DEFINE_STATIC_CALL_RET0(arm64_guest_state, *(perf_guest_cbs->state));
+DEFINE_STATIC_CALL_RET0(arm64_guest_get_ip, *(perf_guest_cbs->get_ip));
+
+void arch_perf_update_guest_cbs(void)
+{
+ static_call_update(arm64_guest_state, (void *)&__static_call_return0);
+ static_call_update(arm64_guest_get_ip, (void *)&__static_call_return0);
+
+ if (perf_guest_cbs && perf_guest_cbs->state)
+ static_call_update(arm64_guest_state, perf_guest_cbs->state);
+
+ if (perf_guest_cbs && perf_guest_cbs->get_ip)
+ static_call_update(arm64_guest_get_ip, perf_guest_cbs->get_ip);
+}
+
void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
Re: [PATCH V8 01/18] perf/core: Use static_call to optimize perf_guest_info_callbacks
On 7/21/2021 7:57 PM, Like Xu wrote: On 16/7/2021 4:53 pm, Zhu Lingshan wrote: + } else if (xenpmu_data->pmu.r.regs.cpl & 3) oh, my typo, will fix in V9 Thanks Lingshan, serious for this version ? arch/x86/xen/pmu.c:438:9: error: expected identifier or ‘(’ before ‘return’ 438 | return state; | ^~ arch/x86/xen/pmu.c:439:1: error: expected identifier or ‘(’ before ‘}’ token 439 | } | ^ arch/x86/xen/pmu.c: In function ‘xen_guest_state’: arch/x86/xen/pmu.c:436:9: error: control reaches end of non-void function [-Werror=return-type] 436 | } | ^ cc1: some warnings being treated as errors + state |= PERF_GUEST_USER; } ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH V8 01/18] perf/core: Use static_call to optimize perf_guest_info_callbacks
On 7/21/2021 7:57 PM, Like Xu wrote: On 16/7/2021 4:53 pm, Zhu Lingshan wrote: + } else if (xenpmu_data->pmu.r.regs.cpl & 3) Lingshan, serious for this version ? arch/x86/xen/pmu.c:438:9: error: expected identifier or ‘(’ before ‘return’ 438 | return state; | ^~ arch/x86/xen/pmu.c:439:1: error: expected identifier or ‘(’ before ‘}’ token 439 | } | ^ arch/x86/xen/pmu.c: In function ‘xen_guest_state’: arch/x86/xen/pmu.c:436:9: error: control reaches end of non-void function [-Werror=return-type] 436 | } | ^ cc1: some warnings being treated as errors + state |= PERF_GUEST_USER; } forgot to enable XEN build in .config, V9 fixes this will come soon ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH V8 01/18] perf/core: Use static_call to optimize perf_guest_info_callbacks
From: Like Xu
For "struct perf_guest_info_callbacks", the two fields "is_in_guest"
and "is_user_mode" are replaced with a new multiplexed member named
"state", and the "get_guest_ip" field will be renamed to "get_ip".
For arm64, xen and kvm/x86, the application of DEFINE_STATIC_CALL_RET0
could make all that perf_guest_cbs stuff suck less. For arm, csky, nds32,
and riscv, just applied some renamed refactoring.
Cc: Will Deacon
Cc: Marc Zyngier
Cc: Guo Ren
Cc: Nick Hu
Cc: Paul Walmsley
Cc: Boris Ostrovsky
Cc: linux-arm-ker...@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-c...@vger.kernel.org
Cc: linux-ri...@lists.infradead.org
Cc: xen-de...@lists.xenproject.org
Suggested-by: Peter Zijlstra (Intel)
Original-by: Peter Zijlstra (Intel)
Signed-off-by: Like Xu
Signed-off-by: Zhu Lingshan
Reviewed-by: Boris Ostrovsky
---
arch/arm/kernel/perf_callchain.c | 16 +++-
arch/arm64/kernel/perf_callchain.c | 29 +-
arch/arm64/kvm/perf.c | 22 -
arch/csky/kernel/perf_callchain.c | 4 +--
arch/nds32/kernel/perf_event_cpu.c | 16 +++-
arch/riscv/kernel/perf_callchain.c | 4 +--
arch/x86/events/core.c | 39 --
arch/x86/events/intel/core.c | 7 +++---
arch/x86/include/asm/kvm_host.h| 2 +-
arch/x86/kvm/pmu.c | 2 +-
arch/x86/kvm/x86.c | 37 +++-
arch/x86/xen/pmu.c | 32 ++--
include/linux/perf_event.h | 12 ++---
kernel/events/core.c | 9 +++
14 files changed, 144 insertions(+), 87 deletions(-)
diff --git a/arch/arm/kernel/perf_callchain.c b/arch/arm/kernel/perf_callchain.c
index 3b69a76d341e..1ce30f86d6c7 100644
--- a/arch/arm/kernel/perf_callchain.c
+++ b/arch/arm/kernel/perf_callchain.c
@@ -64,7 +64,7 @@ perf_callchain_user(struct perf_callchain_entry_ctx *entry,
struct pt_regs *regs
{
struct frame_tail __user *tail;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+ if (perf_guest_cbs && perf_guest_cbs->state()) {
/* We don't support guest os callchain now */
return;
}
@@ -100,7 +100,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
*entry, struct pt_regs *re
{
struct stackframe fr;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+ if (perf_guest_cbs && perf_guest_cbs->state()) {
/* We don't support guest os callchain now */
return;
}
@@ -111,8 +111,8 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
*entry, struct pt_regs *re
unsigned long perf_instruction_pointer(struct pt_regs *regs)
{
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest())
- return perf_guest_cbs->get_guest_ip();
+ if (perf_guest_cbs && perf_guest_cbs->state())
+ return perf_guest_cbs->get_ip();
return instruction_pointer(regs);
}
@@ -120,9 +120,13 @@ unsigned long perf_instruction_pointer(struct pt_regs
*regs)
unsigned long perf_misc_flags(struct pt_regs *regs)
{
int misc = 0;
+ unsigned int state = 0;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
- if (perf_guest_cbs->is_user_mode())
+ if (perf_guest_cbs)
+ state = perf_guest_cbs->state();
+
+ if (perf_guest_cbs && state) {
+ if (state & PERF_GUEST_USER)
misc |= PERF_RECORD_MISC_GUEST_USER;
else
misc |= PERF_RECORD_MISC_GUEST_KERNEL;
diff --git a/arch/arm64/kernel/perf_callchain.c
b/arch/arm64/kernel/perf_callchain.c
index 4a72c2727309..1b344e23fd2f 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -5,6 +5,7 @@
* Copyright (C) 2015 ARM Limited
*/
#include
+#include
#include
#include
@@ -99,10 +100,25 @@ compat_user_backtrace(struct compat_frame_tail __user
*tail,
}
#endif /* CONFIG_COMPAT */
+DEFINE_STATIC_CALL_RET0(arm64_guest_state, *(perf_guest_cbs->state));
+DEFINE_STATIC_CALL_RET0(arm64_guest_get_ip, *(perf_guest_cbs->get_ip));
+
+void arch_perf_update_guest_cbs(void)
+{
+ static_call_update(arm64_guest_state, (void *)&__static_call_return0);
+ static_call_update(arm64_guest_get_ip, (void *)&__static_call_return0);
+
+ if (perf_guest_cbs && perf_guest_cbs->state)
+ static_call_update(arm64_guest_state, perf_guest_cbs->state);
+
+ if (perf_guest_cbs && perf_guest_cbs->get_ip)
+ static_call_update(arm64_guest_get_ip, perf_guest_cbs->get_ip);
+}
+
void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
Re: [PATCH V7 01/18] perf/core: Use static_call to optimize perf_guest_info_callbacks
On 7/2/2021 7:22 PM, Peter Zijlstra wrote:
On Tue, Jun 22, 2021 at 05:42:49PM +0800, Zhu Lingshan wrote:
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 8f71dd72ef95..c71af4cfba9b 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -90,6 +90,27 @@ DEFINE_STATIC_CALL_NULL(x86_pmu_pebs_aliases,
*x86_pmu.pebs_aliases);
*/
DEFINE_STATIC_CALL_RET0(x86_pmu_guest_get_msrs, *x86_pmu.guest_get_msrs);
+DEFINE_STATIC_CALL_RET0(x86_guest_state, *(perf_guest_cbs->state));
+DEFINE_STATIC_CALL_RET0(x86_guest_get_ip, *(perf_guest_cbs->get_ip));
+DEFINE_STATIC_CALL_RET0(x86_guest_handle_intel_pt_intr,
*(perf_guest_cbs->handle_intel_pt_intr));
+
+void arch_perf_update_guest_cbs(void)
+{
+ static_call_update(x86_guest_state, (void *)&__static_call_return0);
+ static_call_update(x86_guest_get_ip, (void *)&__static_call_return0);
+ static_call_update(x86_guest_handle_intel_pt_intr, (void
*)&__static_call_return0);
+
+ if (perf_guest_cbs && perf_guest_cbs->state)
+ static_call_update(x86_guest_state, perf_guest_cbs->state);
+
+ if (perf_guest_cbs && perf_guest_cbs->get_ip)
+ static_call_update(x86_guest_get_ip, perf_guest_cbs->get_ip);
+
+ if (perf_guest_cbs && perf_guest_cbs->handle_intel_pt_intr)
+ static_call_update(x86_guest_handle_intel_pt_intr,
+ perf_guest_cbs->handle_intel_pt_intr);
+}
Coding style wants { } on that last if().
will fix these coding style issues in V8
Thanks!
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH V7 01/18] perf/core: Use static_call to optimize perf_guest_info_callbacks
Thanks Boris, I will fix this in V8
On 6/23/2021 1:31 AM, Boris Ostrovsky wrote:
On 6/22/21 5:38 AM, Zhu Lingshan wrote:
-static int xen_is_user_mode(void)
-{
- const struct xen_pmu_data *xenpmu_data = get_xenpmu_data();
+ state |= PERF_GUEST_ACTIVE;
- if (!xenpmu_data) {
- pr_warn_once("%s: pmudata not initialized\n", __func__);
- return 0;
+ if (xenpmu_data->pmu.pmu_flags & PMU_SAMPLE_PV) {
+ if (xenpmu_data->pmu.pmu_flags & PMU_SAMPLE_USER)
+ state |= PERF_GUEST_USER;
+ } else {
+ if (!!(xenpmu_data->pmu.r.regs.cpl & 3))
+ state |= PERF_GUEST_USER;
Please drop "!!", it's not needed here. And use "else if".
With that, for Xen bits:
Reviewed-by: Boris Ostrovsky
-boris
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH V7 01/18] perf/core: Use static_call to optimize perf_guest_info_callbacks
From: Like Xu
For "struct perf_guest_info_callbacks", the two fields "is_in_guest"
and "is_user_mode" are replaced with a new multiplexed member named
"state", and the "get_guest_ip" field will be renamed to "get_ip".
For arm64, xen and kvm/x86, the application of DEFINE_STATIC_CALL_RET0
could make all that perf_guest_cbs stuff suck less. For arm, csky, nds32,
and riscv, just applied some renamed refactoring.
Cc: Will Deacon
Cc: Marc Zyngier
Cc: Guo Ren
Cc: Nick Hu
Cc: Paul Walmsley
Cc: Boris Ostrovsky
Cc: linux-arm-ker...@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-c...@vger.kernel.org
Cc: linux-ri...@lists.infradead.org
Cc: xen-de...@lists.xenproject.org
Suggested-by: Peter Zijlstra (Intel)
Original-by: Peter Zijlstra (Intel)
Signed-off-by: Like Xu
Signed-off-by: Zhu Lingshan
---
arch/arm/kernel/perf_callchain.c | 16 -
arch/arm64/kernel/perf_callchain.c | 29 ++-
arch/arm64/kvm/perf.c | 22 -
arch/csky/kernel/perf_callchain.c | 4 ++--
arch/nds32/kernel/perf_event_cpu.c | 16 -
arch/riscv/kernel/perf_callchain.c | 4 ++--
arch/x86/events/core.c | 38 --
arch/x86/events/intel/core.c | 7 +++---
arch/x86/include/asm/kvm_host.h| 2 +-
arch/x86/kvm/pmu.c | 2 +-
arch/x86/kvm/x86.c | 37 -
arch/x86/xen/pmu.c | 33 +++---
include/linux/perf_event.h | 12 ++
kernel/events/core.c | 9 +++
14 files changed, 144 insertions(+), 87 deletions(-)
diff --git a/arch/arm/kernel/perf_callchain.c b/arch/arm/kernel/perf_callchain.c
index 3b69a76d341e..1ce30f86d6c7 100644
--- a/arch/arm/kernel/perf_callchain.c
+++ b/arch/arm/kernel/perf_callchain.c
@@ -64,7 +64,7 @@ perf_callchain_user(struct perf_callchain_entry_ctx *entry,
struct pt_regs *regs
{
struct frame_tail __user *tail;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+ if (perf_guest_cbs && perf_guest_cbs->state()) {
/* We don't support guest os callchain now */
return;
}
@@ -100,7 +100,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
*entry, struct pt_regs *re
{
struct stackframe fr;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+ if (perf_guest_cbs && perf_guest_cbs->state()) {
/* We don't support guest os callchain now */
return;
}
@@ -111,8 +111,8 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
*entry, struct pt_regs *re
unsigned long perf_instruction_pointer(struct pt_regs *regs)
{
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest())
- return perf_guest_cbs->get_guest_ip();
+ if (perf_guest_cbs && perf_guest_cbs->state())
+ return perf_guest_cbs->get_ip();
return instruction_pointer(regs);
}
@@ -120,9 +120,13 @@ unsigned long perf_instruction_pointer(struct pt_regs
*regs)
unsigned long perf_misc_flags(struct pt_regs *regs)
{
int misc = 0;
+ unsigned int state = 0;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
- if (perf_guest_cbs->is_user_mode())
+ if (perf_guest_cbs)
+ state = perf_guest_cbs->state();
+
+ if (perf_guest_cbs && state) {
+ if (state & PERF_GUEST_USER)
misc |= PERF_RECORD_MISC_GUEST_USER;
else
misc |= PERF_RECORD_MISC_GUEST_KERNEL;
diff --git a/arch/arm64/kernel/perf_callchain.c
b/arch/arm64/kernel/perf_callchain.c
index 88ff471b0bce..5df2bd5d12ba 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -5,6 +5,7 @@
* Copyright (C) 2015 ARM Limited
*/
#include
+#include
#include
#include
@@ -99,10 +100,25 @@ compat_user_backtrace(struct compat_frame_tail __user
*tail,
}
#endif /* CONFIG_COMPAT */
+DEFINE_STATIC_CALL_RET0(arm64_guest_state, *(perf_guest_cbs->state));
+DEFINE_STATIC_CALL_RET0(arm64_guest_get_ip, *(perf_guest_cbs->get_ip));
+
+void arch_perf_update_guest_cbs(void)
+{
+ static_call_update(arm64_guest_state, (void *)&__static_call_return0);
+ static_call_update(arm64_guest_get_ip, (void *)&__static_call_return0);
+
+ if (perf_guest_cbs && perf_guest_cbs->state)
+ static_call_update(arm64_guest_state, perf_guest_cbs->state);
+
+ if (perf_guest_cbs && perf_guest_cbs->get_ip)
+ static_call_update(arm64_guest_get_ip, perf_guest_cbs->get_ip);
+}
+
void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
struct
[PATCH V7 01/18] perf/core: Use static_call to optimize perf_guest_info_callbacks
From: Like Xu
For "struct perf_guest_info_callbacks", the two fields "is_in_guest"
and "is_user_mode" are replaced with a new multiplexed member named
"state", and the "get_guest_ip" field will be renamed to "get_ip".
For arm64, xen and kvm/x86, the application of DEFINE_STATIC_CALL_RET0
could make all that perf_guest_cbs stuff suck less. For arm, csky, nds32,
and riscv, just applied some renamed refactoring.
Cc: Will Deacon
Cc: Marc Zyngier
Cc: Guo Ren
Cc: Nick Hu
Cc: Paul Walmsley
Cc: Boris Ostrovsky
Cc: linux-arm-ker...@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-c...@vger.kernel.org
Cc: linux-ri...@lists.infradead.org
Cc: xen-de...@lists.xenproject.org
Suggested-by: Peter Zijlstra (Intel)
Original-by: Peter Zijlstra (Intel)
Signed-off-by: Like Xu
Signed-off-by: Zhu Lingshan
---
arch/arm/kernel/perf_callchain.c | 16 -
arch/arm64/kernel/perf_callchain.c | 29 ++-
arch/arm64/kvm/perf.c | 22 -
arch/csky/kernel/perf_callchain.c | 4 ++--
arch/nds32/kernel/perf_event_cpu.c | 16 -
arch/riscv/kernel/perf_callchain.c | 4 ++--
arch/x86/events/core.c | 38 --
arch/x86/events/intel/core.c | 7 +++---
arch/x86/include/asm/kvm_host.h| 2 +-
arch/x86/kvm/pmu.c | 2 +-
arch/x86/kvm/x86.c | 37 -
arch/x86/xen/pmu.c | 33 +++---
include/linux/perf_event.h | 12 ++
kernel/events/core.c | 9 +++
14 files changed, 144 insertions(+), 87 deletions(-)
diff --git a/arch/arm/kernel/perf_callchain.c b/arch/arm/kernel/perf_callchain.c
index 3b69a76d341e..1ce30f86d6c7 100644
--- a/arch/arm/kernel/perf_callchain.c
+++ b/arch/arm/kernel/perf_callchain.c
@@ -64,7 +64,7 @@ perf_callchain_user(struct perf_callchain_entry_ctx *entry,
struct pt_regs *regs
{
struct frame_tail __user *tail;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+ if (perf_guest_cbs && perf_guest_cbs->state()) {
/* We don't support guest os callchain now */
return;
}
@@ -100,7 +100,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
*entry, struct pt_regs *re
{
struct stackframe fr;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+ if (perf_guest_cbs && perf_guest_cbs->state()) {
/* We don't support guest os callchain now */
return;
}
@@ -111,8 +111,8 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
*entry, struct pt_regs *re
unsigned long perf_instruction_pointer(struct pt_regs *regs)
{
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest())
- return perf_guest_cbs->get_guest_ip();
+ if (perf_guest_cbs && perf_guest_cbs->state())
+ return perf_guest_cbs->get_ip();
return instruction_pointer(regs);
}
@@ -120,9 +120,13 @@ unsigned long perf_instruction_pointer(struct pt_regs
*regs)
unsigned long perf_misc_flags(struct pt_regs *regs)
{
int misc = 0;
+ unsigned int state = 0;
- if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
- if (perf_guest_cbs->is_user_mode())
+ if (perf_guest_cbs)
+ state = perf_guest_cbs->state();
+
+ if (perf_guest_cbs && state) {
+ if (state & PERF_GUEST_USER)
misc |= PERF_RECORD_MISC_GUEST_USER;
else
misc |= PERF_RECORD_MISC_GUEST_KERNEL;
diff --git a/arch/arm64/kernel/perf_callchain.c
b/arch/arm64/kernel/perf_callchain.c
index 88ff471b0bce..5df2bd5d12ba 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -5,6 +5,7 @@
* Copyright (C) 2015 ARM Limited
*/
#include
+#include
#include
#include
@@ -99,10 +100,25 @@ compat_user_backtrace(struct compat_frame_tail __user
*tail,
}
#endif /* CONFIG_COMPAT */
+DEFINE_STATIC_CALL_RET0(arm64_guest_state, *(perf_guest_cbs->state));
+DEFINE_STATIC_CALL_RET0(arm64_guest_get_ip, *(perf_guest_cbs->get_ip));
+
+void arch_perf_update_guest_cbs(void)
+{
+ static_call_update(arm64_guest_state, (void *)&__static_call_return0);
+ static_call_update(arm64_guest_get_ip, (void *)&__static_call_return0);
+
+ if (perf_guest_cbs && perf_guest_cbs->state)
+ static_call_update(arm64_guest_state, perf_guest_cbs->state);
+
+ if (perf_guest_cbs && perf_guest_cbs->get_ip)
+ static_call_update(arm64_guest_get_ip, perf_guest_cbs->get_ip);
+}
+
void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
struct
Re: [PATCH] Revert "irqbypass: do not start cons/prod when failed connect"
On 5/10/2021 6:00 PM, Marc Zyngier wrote: On Mon, 10 May 2021 09:32:54 +0100, "Zhu, Lingshan" wrote: On 5/10/2021 3:09 PM, Zhu, Lingshan wrote: On 5/10/2021 12:34 PM, Jason Wang wrote: 在 2021/5/10 上午11:00, Zhu, Lingshan 写道: On 5/10/2021 10:43 AM, Jason Wang wrote: 在 2021/5/8 下午3:11, Zhu Lingshan 写道: This reverts commit a979a6aa009f3c99689432e0cdb5402a4463fb88. The reverted commit may cause VM freeze on arm64 platform. Because on arm64 platform, stop a consumer will suspend the VM, the VM will freeze without a start consumer Signed-off-by: Zhu Lingshan Acked-by: Jason Wang Please resubmit with the formal process of stable (stable-kernel-rules.rst). sure, I will re-submit it to stable kernel once it is merged into Linus tree. Thanks I think it's better to resubmit (option 1), see how stable-kernel-rules.rst said: "" :ref:`option_1` is **strongly** preferred, is the easiest and most common. :ref:`option_2` and :ref:`option_3` are more useful if the patch isn't deemed worthy at the time it is applied to a public git tree (for instance, because it deserves more regression testing first). """ Thanks OK, works for me, I will add cc stable, and resubmit it soon Thanks! I just seeMarc has already added "Cc: sta...@vger.kernel.org", and he would take the patch in his tree, so I think no need to resend. That's fine, I can fix things up myself and queue the fix for -rc2. Thanks Marc! Thanks, M. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] Revert "irqbypass: do not start cons/prod when failed connect"
On 5/10/2021 3:09 PM, Zhu, Lingshan wrote:
On 5/10/2021 12:34 PM, Jason Wang wrote:
在 2021/5/10 上午11:00, Zhu, Lingshan 写道:
On 5/10/2021 10:43 AM, Jason Wang wrote:
在 2021/5/8 下午3:11, Zhu Lingshan 写道:
This reverts commit a979a6aa009f3c99689432e0cdb5402a4463fb88.
The reverted commit may cause VM freeze on arm64 platform.
Because on arm64 platform, stop a consumer will suspend the VM,
the VM will freeze without a start consumer
Signed-off-by: Zhu Lingshan
Acked-by: Jason Wang
Please resubmit with the formal process of stable
(stable-kernel-rules.rst).
sure, I will re-submit it to stable kernel once it is merged into
Linus tree.
Thanks
I think it's better to resubmit (option 1), see how
stable-kernel-rules.rst said:
""
:ref:`option_1` is **strongly** preferred, is the easiest and most
common.
:ref:`option_2` and :ref:`option_3` are more useful if the patch
isn't deemed
worthy at the time it is applied to a public git tree (for instance,
because
it deserves more regression testing first).
"""
Thanks
OK, works for me, I will add cc stable, and resubmit it soon
Thanks!
I just seeMarc has already added "Cc: sta...@vger.kernel.org", and he
would take the patch in his tree,
so I think no need to resend.
Thanks,
Zhu Lingshan
Thanks
---
virt/lib/irqbypass.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c
index c9bb3957f58a..28fda42e471b 100644
--- a/virt/lib/irqbypass.c
+++ b/virt/lib/irqbypass.c
@@ -40,21 +40,17 @@ static int __connect(struct
irq_bypass_producer *prod,
if (prod->add_consumer)
ret = prod->add_consumer(prod, cons);
- if (ret)
- goto err_add_consumer;
-
- ret = cons->add_producer(cons, prod);
- if (ret)
- goto err_add_producer;
+ if (!ret) {
+ ret = cons->add_producer(cons, prod);
+ if (ret && prod->del_consumer)
+ prod->del_consumer(prod, cons);
+ }
if (cons->start)
cons->start(cons);
if (prod->start)
prod->start(prod);
-err_add_producer:
- if (prod->del_consumer)
- prod->del_consumer(prod, cons);
-err_add_consumer:
+
return ret;
}
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] Revert "irqbypass: do not start cons/prod when failed connect"
On 5/10/2021 12:34 PM, Jason Wang wrote:
在 2021/5/10 上午11:00, Zhu, Lingshan 写道:
On 5/10/2021 10:43 AM, Jason Wang wrote:
在 2021/5/8 下午3:11, Zhu Lingshan 写道:
This reverts commit a979a6aa009f3c99689432e0cdb5402a4463fb88.
The reverted commit may cause VM freeze on arm64 platform.
Because on arm64 platform, stop a consumer will suspend the VM,
the VM will freeze without a start consumer
Signed-off-by: Zhu Lingshan
Acked-by: Jason Wang
Please resubmit with the formal process of stable
(stable-kernel-rules.rst).
sure, I will re-submit it to stable kernel once it is merged into
Linus tree.
Thanks
I think it's better to resubmit (option 1), see how
stable-kernel-rules.rst said:
""
:ref:`option_1` is **strongly** preferred, is the easiest and most
common.
:ref:`option_2` and :ref:`option_3` are more useful if the patch isn't
deemed
worthy at the time it is applied to a public git tree (for instance,
because
it deserves more regression testing first).
"""
Thanks
OK, works for me, I will add cc stable, and resubmit it soon
Thanks!
Thanks
---
virt/lib/irqbypass.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c
index c9bb3957f58a..28fda42e471b 100644
--- a/virt/lib/irqbypass.c
+++ b/virt/lib/irqbypass.c
@@ -40,21 +40,17 @@ static int __connect(struct irq_bypass_producer
*prod,
if (prod->add_consumer)
ret = prod->add_consumer(prod, cons);
- if (ret)
- goto err_add_consumer;
-
- ret = cons->add_producer(cons, prod);
- if (ret)
- goto err_add_producer;
+ if (!ret) {
+ ret = cons->add_producer(cons, prod);
+ if (ret && prod->del_consumer)
+ prod->del_consumer(prod, cons);
+ }
if (cons->start)
cons->start(cons);
if (prod->start)
prod->start(prod);
-err_add_producer:
- if (prod->del_consumer)
- prod->del_consumer(prod, cons);
-err_add_consumer:
+
return ret;
}
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] Revert "irqbypass: do not start cons/prod when failed connect"
On 5/10/2021 10:43 AM, Jason Wang wrote:
在 2021/5/8 下午3:11, Zhu Lingshan 写道:
This reverts commit a979a6aa009f3c99689432e0cdb5402a4463fb88.
The reverted commit may cause VM freeze on arm64 platform.
Because on arm64 platform, stop a consumer will suspend the VM,
the VM will freeze without a start consumer
Signed-off-by: Zhu Lingshan
Acked-by: Jason Wang
Please resubmit with the formal process of stable
(stable-kernel-rules.rst).
sure, I will re-submit it to stable kernel once it is merged into Linus
tree.
Thanks
Thanks
---
virt/lib/irqbypass.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c
index c9bb3957f58a..28fda42e471b 100644
--- a/virt/lib/irqbypass.c
+++ b/virt/lib/irqbypass.c
@@ -40,21 +40,17 @@ static int __connect(struct irq_bypass_producer
*prod,
if (prod->add_consumer)
ret = prod->add_consumer(prod, cons);
- if (ret)
- goto err_add_consumer;
-
- ret = cons->add_producer(cons, prod);
- if (ret)
- goto err_add_producer;
+ if (!ret) {
+ ret = cons->add_producer(cons, prod);
+ if (ret && prod->del_consumer)
+ prod->del_consumer(prod, cons);
+ }
if (cons->start)
cons->start(cons);
if (prod->start)
prod->start(prod);
-err_add_producer:
- if (prod->del_consumer)
- prod->del_consumer(prod, cons);
-err_add_consumer:
+
return ret;
}
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: Question on guest enable msi fail when using GICv4/4.1
On 5/8/2021 9:51 AM, Jason Wang wrote: 在 2021/5/8 上午1:36, Marc Zyngier 写道: On Fri, 07 May 2021 12:02:57 +0100, Marc Zyngier wrote: On Fri, 07 May 2021 10:58:23 +0100, Shaokun Zhang wrote: Hi Marc, Thanks for your quick reply. On 2021/5/7 17:03, Marc Zyngier wrote: On Fri, 07 May 2021 06:57:04 +0100, Shaokun Zhang wrote: [This letter comes from Nianyao Tang] Hi, Using GICv4/4.1 and msi capability, guest vf driver requires 3 vectors and enable msi, will lead to guest stuck. Stuck how? Guest serial does not response anymore and guest network shutdown. Qemu gets number of interrupts from Multiple Message Capable field set by guest. This field is aligned to a power of 2(if a function requires 3 vectors, it initializes it to 2). So I guess this is a MultiMSI device with 4 vectors, right? Yes, it can support maximum of 32 msi interrupts, and vf driver only use 3 msi. However, guest driver just sends 3 mapi-cmd to vits and 3 ite entries is recorded in host. Vfio initializes msi interrupts using the number of interrupts 4 provide by qemu. When it comes to the 4th msi without ite in vits, in irq_bypass_register_producer, producer and consumer will __connect fail, due to find_ite fail, and do not resume guest. Let me rephrase this to check that I understand it: - The device has 4 vectors - The guest only create mappings for 3 of them - VFIO calls kvm_vgic_v4_set_forwarding() for each vector - KVM doesn't have a mapping for the 4th vector and returns an error - VFIO disable this 4th vector Is that correct? If yes, I don't understand why that impacts the guest at all. From what I can see, vfio_msi_set_vector_signal() just prints a message on the console and carries on. function calls: --> vfio_msi_set_vector_signal --> irq_bypass_register_producer -->__connect in __connect, add_producer finally calls kvm_vgic_v4_set_forwarding and fails to get the 4th mapping. When add_producer fail, it does not call cons->start, calls kvm_arch_irq_bypass_start and then kvm_arm_resume_guest. [+Eric, who wrote the irq_bypass infrastructure.] Ah, so the guest is actually paused, not in a livelock situation (which is how I interpreted "stuck"). I think we should handle this case gracefully, as there should be no expectation that the guest will be using this interrupt. Given that VFIO seems to be pretty unfazed when a producer fails, I'm temped to do the same thing and restart the guest. Also, __disconnect doesn't care about errors, so why should __connect have this odd behaviour? Can you please try this? It is completely untested (and I think the del_consumer call is odd, which is why I've also dropped it). Eric, what do you think? Adding Zhu, Jason, MST to the party. It all seems to be caused by this commit: commit a979a6aa009f3c99689432e0cdb5402a4463fb88 Author: Zhu Lingshan Date: Fri Jul 31 14:55:33 2020 +0800 irqbypass: do not start cons/prod when failed connect If failed to connect, there is no need to start consumer nor producer. Signed-off-by: Zhu Lingshan Suggested-by: Jason Wang Link: https://lore.kernel.org/r/20200731065533.4144-7-lingshan@intel.com Signed-off-by: Michael S. Tsirkin Zhu, I'd really like to understand why you think it is OK not to restart consumer and producers when a connection has failed to be established between the two? My bad, I didn't check ARM code but it's not easy to infer that the cons->start/stop is not a per consumer specific operation but a global one like VM halting/resuming. Hi Marc, I will send out a patch to revert this commit as Jason suggested. Thanks In the case of KVM/arm64, this results in the guest being forever suspended and never resumed. That's obviously not an acceptable regression, as there is a number of benign reasons for a connect to fail. Let's revert this commit. Thanks Thanks, M. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH] Revert "irqbypass: do not start cons/prod when failed connect"
This reverts commit a979a6aa009f3c99689432e0cdb5402a4463fb88.
The reverted commit may cause VM freeze on arm64 platform.
Because on arm64 platform, stop a consumer will suspend the VM,
the VM will freeze without a start consumer
Signed-off-by: Zhu Lingshan
---
virt/lib/irqbypass.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c
index c9bb3957f58a..28fda42e471b 100644
--- a/virt/lib/irqbypass.c
+++ b/virt/lib/irqbypass.c
@@ -40,21 +40,17 @@ static int __connect(struct irq_bypass_producer *prod,
if (prod->add_consumer)
ret = prod->add_consumer(prod, cons);
- if (ret)
- goto err_add_consumer;
-
- ret = cons->add_producer(cons, prod);
- if (ret)
- goto err_add_producer;
+ if (!ret) {
+ ret = cons->add_producer(cons, prod);
+ if (ret && prod->del_consumer)
+ prod->del_consumer(prod, cons);
+ }
if (cons->start)
cons->start(cons);
if (prod->start)
prod->start(prod);
-err_add_producer:
- if (prod->del_consumer)
- prod->del_consumer(prod, cons);
-err_add_consumer:
+
return ret;
}
--
2.27.0
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
