* [Qemu-devel] [PATCH v6] ppc: remove idle_timer logic
@ 2019-07-24 12:47 Shivaprasad G Bhat
2019-07-25 3:34 ` David Gibson
2019-07-25 8:42 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
0 siblings, 2 replies; 3+ messages in thread
From: Shivaprasad G Bhat @ 2019-07-24 12:47 UTC (permalink / raw)
To: qemu-ppc, qemu-devel, david
The KVM_CAP_PPC_IRQ_LEVEL is part of the kernel now since 2.6.37.
Drop the redundant logic which is not excercised on new the kernels anymore.
Exit with error on older kernels.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
v5: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg05301.html
Changes from v5:
- exit with error if KVM_CAP_PPC_IRQ_LEVEL is not there.
target/ppc/kvm.c | 75 ++++--------------------------------------------------
1 file changed, 5 insertions(+), 70 deletions(-)
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 8a06d3171e..4a3f36f0d5 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -56,7 +56,6 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
};
static int cap_interrupt_unset;
-static int cap_interrupt_level;
static int cap_segstate;
static int cap_booke_sregs;
static int cap_ppc_smt;
@@ -87,25 +86,6 @@ static int cap_large_decr;
static uint32_t debug_inst_opcode;
-/*
- * XXX We have a race condition where we actually have a level triggered
- * interrupt, but the infrastructure can't expose that yet, so the guest
- * takes but ignores it, goes to sleep and never gets notified that there's
- * still an interrupt pending.
- *
- * As a quick workaround, let's just wake up again 20 ms after we injected
- * an interrupt. That way we can assure that we're always reinjecting
- * interrupts in case the guest swallowed them.
- */
-static QEMUTimer *idle_timer;
-
-static void kvm_kick_cpu(void *opaque)
-{
- PowerPCCPU *cpu = opaque;
-
- qemu_cpu_kick(CPU(cpu));
-}
-
/*
* Check whether we are running with KVM-PR (instead of KVM-HV). This
* should only be used for fallback tests - generally we should use
@@ -125,7 +105,6 @@ static int kvmppc_get_dec_bits(void);
int kvm_arch_init(MachineState *ms, KVMState *s)
{
cap_interrupt_unset = kvm_check_extension(s, KVM_CAP_PPC_UNSET_IRQ);
- cap_interrupt_level = kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL);
cap_segstate = kvm_check_extension(s, KVM_CAP_PPC_SEGSTATE);
cap_booke_sregs = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_SREGS);
cap_ppc_smt_possible = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
@@ -161,9 +140,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
*/
cap_ppc_pvr_compat = false;
- if (!cap_interrupt_level) {
- fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the "
- "VM to stall at times!\n");
+ if (!kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL)) {
+ fprintf(stderr, "KVM: Host kernel doesn't have level irq capability\n");
+ exit(1);
}
kvm_ppc_register_host_cpu_type(ms);
@@ -491,8 +470,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
return ret;
}
- idle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, kvm_kick_cpu, cpu);
-
switch (cenv->mmu_model) {
case POWERPC_MMU_BOOKE206:
/* This target supports access to KVM's guest TLB */
@@ -1332,7 +1309,7 @@ int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level)
return 0;
}
- if (!kvm_enabled() || !cap_interrupt_unset || !cap_interrupt_level) {
+ if (!kvm_enabled() || !cap_interrupt_unset) {
return 0;
}
@@ -1349,49 +1326,7 @@ int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level)
void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
{
- PowerPCCPU *cpu = POWERPC_CPU(cs);
- CPUPPCState *env = &cpu->env;
- int r;
- unsigned irq;
-
- qemu_mutex_lock_iothread();
-
- /*
- * PowerPC QEMU tracks the various core input pins (interrupt,
- * critical interrupt, reset, etc) in PPC-specific
- * env->irq_input_state.
- */
- if (!cap_interrupt_level &&
- run->ready_for_interrupt_injection &&
- (cs->interrupt_request & CPU_INTERRUPT_HARD) &&
- (env->irq_input_state & (1 << PPC_INPUT_INT)))
- {
- /*
- * For now KVM disregards the 'irq' argument. However, in the
- * future KVM could cache it in-kernel to avoid a heavyweight
- * exit when reading the UIC.
- */
- irq = KVM_INTERRUPT_SET;
-
- trace_kvm_injected_interrupt(irq);
- r = kvm_vcpu_ioctl(cs, KVM_INTERRUPT, &irq);
- if (r < 0) {
- printf("cpu %d fail inject %x\n", cs->cpu_index, irq);
- }
-
- /* Always wake up soon in case the interrupt was level based */
- timer_mod(idle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
- (NANOSECONDS_PER_SECOND / 50));
- }
-
- /*
- * We don't know if there are more interrupts pending after
- * this. However, the guest will return to userspace in the course
- * of handling this one anyways, so we will get a chance to
- * deliver the rest.
- */
-
- qemu_mutex_unlock_iothread();
+ return;
}
MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ppc: remove idle_timer logic
2019-07-24 12:47 [Qemu-devel] [PATCH v6] ppc: remove idle_timer logic Shivaprasad G Bhat
@ 2019-07-25 3:34 ` David Gibson
2019-07-25 8:42 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
1 sibling, 0 replies; 3+ messages in thread
From: David Gibson @ 2019-07-25 3:34 UTC (permalink / raw)
To: Shivaprasad G Bhat; +Cc: qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5737 bytes --]
On Wed, Jul 24, 2019 at 07:47:45AM -0500, Shivaprasad G Bhat wrote:
> The KVM_CAP_PPC_IRQ_LEVEL is part of the kernel now since 2.6.37.
> Drop the redundant logic which is not excercised on new the kernels anymore.
> Exit with error on older kernels.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
Applied, although..
> ---
> v5: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg05301.html
> Changes from v5:
> - exit with error if KVM_CAP_PPC_IRQ_LEVEL is not there.
>
> target/ppc/kvm.c | 75 ++++--------------------------------------------------
> 1 file changed, 5 insertions(+), 70 deletions(-)
>
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 8a06d3171e..4a3f36f0d5 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -56,7 +56,6 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
> };
>
> static int cap_interrupt_unset;
> -static int cap_interrupt_level;
> static int cap_segstate;
> static int cap_booke_sregs;
> static int cap_ppc_smt;
> @@ -87,25 +86,6 @@ static int cap_large_decr;
>
> static uint32_t debug_inst_opcode;
>
> -/*
> - * XXX We have a race condition where we actually have a level triggered
> - * interrupt, but the infrastructure can't expose that yet, so the guest
> - * takes but ignores it, goes to sleep and never gets notified that there's
> - * still an interrupt pending.
> - *
> - * As a quick workaround, let's just wake up again 20 ms after we injected
> - * an interrupt. That way we can assure that we're always reinjecting
> - * interrupts in case the guest swallowed them.
> - */
> -static QEMUTimer *idle_timer;
> -
> -static void kvm_kick_cpu(void *opaque)
> -{
> - PowerPCCPU *cpu = opaque;
> -
> - qemu_cpu_kick(CPU(cpu));
> -}
> -
> /*
> * Check whether we are running with KVM-PR (instead of KVM-HV). This
> * should only be used for fallback tests - generally we should use
> @@ -125,7 +105,6 @@ static int kvmppc_get_dec_bits(void);
> int kvm_arch_init(MachineState *ms, KVMState *s)
> {
> cap_interrupt_unset = kvm_check_extension(s, KVM_CAP_PPC_UNSET_IRQ);
> - cap_interrupt_level = kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL);
> cap_segstate = kvm_check_extension(s, KVM_CAP_PPC_SEGSTATE);
> cap_booke_sregs = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_SREGS);
> cap_ppc_smt_possible = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
> @@ -161,9 +140,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> */
> cap_ppc_pvr_compat = false;
>
> - if (!cap_interrupt_level) {
> - fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the "
> - "VM to stall at times!\n");
> + if (!kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL)) {
> + fprintf(stderr, "KVM: Host kernel doesn't have level irq capability\n");
.. I've changed this to an error_report() instead of a raw fprintf.
> + exit(1);
> }
>
> kvm_ppc_register_host_cpu_type(ms);
> @@ -491,8 +470,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
> return ret;
> }
>
> - idle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, kvm_kick_cpu, cpu);
> -
> switch (cenv->mmu_model) {
> case POWERPC_MMU_BOOKE206:
> /* This target supports access to KVM's guest TLB */
> @@ -1332,7 +1309,7 @@ int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level)
> return 0;
> }
>
> - if (!kvm_enabled() || !cap_interrupt_unset || !cap_interrupt_level) {
> + if (!kvm_enabled() || !cap_interrupt_unset) {
> return 0;
> }
>
> @@ -1349,49 +1326,7 @@ int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level)
>
> void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
> {
> - PowerPCCPU *cpu = POWERPC_CPU(cs);
> - CPUPPCState *env = &cpu->env;
> - int r;
> - unsigned irq;
> -
> - qemu_mutex_lock_iothread();
> -
> - /*
> - * PowerPC QEMU tracks the various core input pins (interrupt,
> - * critical interrupt, reset, etc) in PPC-specific
> - * env->irq_input_state.
> - */
> - if (!cap_interrupt_level &&
> - run->ready_for_interrupt_injection &&
> - (cs->interrupt_request & CPU_INTERRUPT_HARD) &&
> - (env->irq_input_state & (1 << PPC_INPUT_INT)))
> - {
> - /*
> - * For now KVM disregards the 'irq' argument. However, in the
> - * future KVM could cache it in-kernel to avoid a heavyweight
> - * exit when reading the UIC.
> - */
> - irq = KVM_INTERRUPT_SET;
> -
> - trace_kvm_injected_interrupt(irq);
> - r = kvm_vcpu_ioctl(cs, KVM_INTERRUPT, &irq);
> - if (r < 0) {
> - printf("cpu %d fail inject %x\n", cs->cpu_index, irq);
> - }
> -
> - /* Always wake up soon in case the interrupt was level based */
> - timer_mod(idle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> - (NANOSECONDS_PER_SECOND / 50));
> - }
> -
> - /*
> - * We don't know if there are more interrupts pending after
> - * this. However, the guest will return to userspace in the course
> - * of handling this one anyways, so we will get a chance to
> - * deliver the rest.
> - */
> -
> - qemu_mutex_unlock_iothread();
> + return;
> }
>
> MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v6] ppc: remove idle_timer logic
2019-07-24 12:47 [Qemu-devel] [PATCH v6] ppc: remove idle_timer logic Shivaprasad G Bhat
2019-07-25 3:34 ` David Gibson
@ 2019-07-25 8:42 ` Greg Kurz
1 sibling, 0 replies; 3+ messages in thread
From: Greg Kurz @ 2019-07-25 8:42 UTC (permalink / raw)
To: Shivaprasad G Bhat; +Cc: qemu-ppc, qemu-devel, david
On Wed, 24 Jul 2019 07:47:45 -0500
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
You could mention that this code was completely broken and leaking memory on
setups with multiple vCPUs. Not worth fixing since the KVM_CAP_PPC_IRQ_LEVEL
is part... etc...
> The KVM_CAP_PPC_IRQ_LEVEL is part of the kernel now since 2.6.37.
> Drop the redundant logic which is not excercised on new the kernels anymore.
> Exit with error on older kernels.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
> v5: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg05301.html
> Changes from v5:
> - exit with error if KVM_CAP_PPC_IRQ_LEVEL is not there.
>
> target/ppc/kvm.c | 75 ++++--------------------------------------------------
> 1 file changed, 5 insertions(+), 70 deletions(-)
>
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 8a06d3171e..4a3f36f0d5 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -56,7 +56,6 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
> };
>
> static int cap_interrupt_unset;
> -static int cap_interrupt_level;
> static int cap_segstate;
> static int cap_booke_sregs;
> static int cap_ppc_smt;
> @@ -87,25 +86,6 @@ static int cap_large_decr;
>
> static uint32_t debug_inst_opcode;
>
> -/*
> - * XXX We have a race condition where we actually have a level triggered
> - * interrupt, but the infrastructure can't expose that yet, so the guest
> - * takes but ignores it, goes to sleep and never gets notified that there's
> - * still an interrupt pending.
> - *
> - * As a quick workaround, let's just wake up again 20 ms after we injected
> - * an interrupt. That way we can assure that we're always reinjecting
> - * interrupts in case the guest swallowed them.
> - */
> -static QEMUTimer *idle_timer;
> -
> -static void kvm_kick_cpu(void *opaque)
> -{
> - PowerPCCPU *cpu = opaque;
> -
> - qemu_cpu_kick(CPU(cpu));
> -}
> -
> /*
> * Check whether we are running with KVM-PR (instead of KVM-HV). This
> * should only be used for fallback tests - generally we should use
> @@ -125,7 +105,6 @@ static int kvmppc_get_dec_bits(void);
> int kvm_arch_init(MachineState *ms, KVMState *s)
> {
> cap_interrupt_unset = kvm_check_extension(s, KVM_CAP_PPC_UNSET_IRQ);
> - cap_interrupt_level = kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL);
> cap_segstate = kvm_check_extension(s, KVM_CAP_PPC_SEGSTATE);
> cap_booke_sregs = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_SREGS);
> cap_ppc_smt_possible = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
> @@ -161,9 +140,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> */
> cap_ppc_pvr_compat = false;
>
> - if (!cap_interrupt_level) {
> - fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the "
> - "VM to stall at times!\n");
> + if (!kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL)) {
> + fprintf(stderr, "KVM: Host kernel doesn't have level irq capability\n");
error_report() is preferred over fprintf() for logging errors, and don't forget
to drop the trailing "\n" when you convert to error_report().
> + exit(1);
> }
>
> kvm_ppc_register_host_cpu_type(ms);
> @@ -491,8 +470,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
> return ret;
> }
>
> - idle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, kvm_kick_cpu, cpu);
> -
> switch (cenv->mmu_model) {
> case POWERPC_MMU_BOOKE206:
> /* This target supports access to KVM's guest TLB */
> @@ -1332,7 +1309,7 @@ int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level)
> return 0;
> }
>
> - if (!kvm_enabled() || !cap_interrupt_unset || !cap_interrupt_level) {
> + if (!kvm_enabled() || !cap_interrupt_unset) {
It looks like cap_interrupt_unset which is even older (2.6.35) is a good
candidate for cleanup as well. Maybe add a patch for that also when you
repost.
> return 0;
> }
>
> @@ -1349,49 +1326,7 @@ int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level)
>
> void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
> {
> - PowerPCCPU *cpu = POWERPC_CPU(cs);
> - CPUPPCState *env = &cpu->env;
> - int r;
> - unsigned irq;
> -
> - qemu_mutex_lock_iothread();
> -
> - /*
> - * PowerPC QEMU tracks the various core input pins (interrupt,
> - * critical interrupt, reset, etc) in PPC-specific
> - * env->irq_input_state.
> - */
> - if (!cap_interrupt_level &&
> - run->ready_for_interrupt_injection &&
> - (cs->interrupt_request & CPU_INTERRUPT_HARD) &&
> - (env->irq_input_state & (1 << PPC_INPUT_INT)))
> - {
> - /*
> - * For now KVM disregards the 'irq' argument. However, in the
> - * future KVM could cache it in-kernel to avoid a heavyweight
> - * exit when reading the UIC.
> - */
> - irq = KVM_INTERRUPT_SET;
> -
> - trace_kvm_injected_interrupt(irq);
> - r = kvm_vcpu_ioctl(cs, KVM_INTERRUPT, &irq);
> - if (r < 0) {
> - printf("cpu %d fail inject %x\n", cs->cpu_index, irq);
> - }
> -
> - /* Always wake up soon in case the interrupt was level based */
> - timer_mod(idle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> - (NANOSECONDS_PER_SECOND / 50));
> - }
> -
> - /*
> - * We don't know if there are more interrupts pending after
> - * this. However, the guest will return to userspace in the course
> - * of handling this one anyways, so we will get a chance to
> - * deliver the rest.
> - */
> -
> - qemu_mutex_unlock_iothread();
> + return;
> }
>
> MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-07-26 7:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-24 12:47 [Qemu-devel] [PATCH v6] ppc: remove idle_timer logic Shivaprasad G Bhat
2019-07-25 3:34 ` David Gibson
2019-07-25 8:42 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.