On Thu, Feb 9, 2012 at 3:02 PM, Alexander Graf <agraf@xxxxxxx> wrote:
>
> On 09.02.2012, at 23:33, Christoffer Dall wrote:
>
>> From: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
>>
>> The kvm_vcpu_kick function performs roughly the same funcitonality on
>> most all architectures, so we shouldn't have separate copies.
>>
>> PowerPC keeps a pointer to interchanging waitqueues on the vcpu_arch
>> structure and to accomodate this special need a
>> __KVM_HAVE_ARCH_VCPU_GET_WQ define and accompanying function
>> kvm_arch_vcpu_wq have been defined. For all other architectures this
>> is a generic inline that just returns &vcpu->wq;
>>
>> This patch applies to Linus' tree on the Linux 3.3-rc1 tag.
>
> Please work against kvm.git for kvm patches :). Makes life easier for everyone involved. It takes a while for patches to land in Linus' tree. I don't think it matters for this patch, but it might in the future.
>
ok
>>
>> Changes since v2:
>> - Restore arch-specific vcpu->cpu assignment to arch-specific code
>> - Set vcpu mode in PowerPC arch-specific kvm code
>>
>> Changes since v1:
>> - Abstact CPU mode check into arch-specific function
>> - Remove redundant vcpu->cpu assignment
>>
>> Signed-off-by: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
>> ---
>> arch/ia64/include/asm/kvm_host.h | 1 +
>> arch/ia64/kvm/kvm-ia64.c | 15 ---------------
>> arch/powerpc/include/asm/kvm_host.h | 6 ++++++
>> arch/powerpc/kvm/book3s_pr.c | 4 ++++
>> arch/powerpc/kvm/booke.c | 2 ++
>> arch/powerpc/kvm/powerpc.c | 17 +++++++----------
>> arch/s390/kvm/kvm-s390.c | 8 ++++++++
>> arch/x86/kvm/x86.c | 16 ++--------------
>> include/linux/kvm_host.h | 9 +++++++++
>> virt/kvm/kvm_main.c | 22 ++++++++++++++++++++++
>> 10 files changed, 61 insertions(+), 39 deletions(-)
>>
>> diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
>> index 2689ee5..06a5e91 100644
>> --- a/arch/ia64/include/asm/kvm_host.h
>> +++ b/arch/ia64/include/asm/kvm_host.h
>> @@ -365,6 +365,7 @@ struct thash_cb {
>> };
>>
>> struct kvm_vcpu_stat {
>> + u32 halt_wakeup;
>> };
>>
>> struct kvm_vcpu_arch {
>> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
>> index 4050520..3f6a3cc 100644
>> --- a/arch/ia64/kvm/kvm-ia64.c
>> +++ b/arch/ia64/kvm/kvm-ia64.c
>> @@ -1849,21 +1849,6 @@ void kvm_arch_hardware_unsetup(void)
>> {
>> }
>>
>> -void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>> -{
>> - int me;
>> - int cpu = vcpu->cpu;
>> -
>> - if (waitqueue_active(&vcpu->wq))
>> - wake_up_interruptible(&vcpu->wq);
>> -
>> - me = get_cpu();
>> - if (cpu != me && (unsigned) cpu < nr_cpu_ids && cpu_online(cpu))
>> - if (!test_and_set_bit(KVM_REQ_KICK, &vcpu->requests))
>> - smp_send_reschedule(cpu);
>> - put_cpu();
>> -}
>> -
>> int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
>> {
>> return __apic_accept_irq(vcpu, irq->vector);
>> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
>> index bf8af5d..b687444 100644
>> --- a/arch/powerpc/include/asm/kvm_host.h
>> +++ b/arch/powerpc/include/asm/kvm_host.h
>> @@ -438,4 +438,10 @@ struct kvm_vcpu_arch {
>> #define KVMPPC_VCPU_BUSY_IN_HOST 1
>> #define KVMPPC_VCPU_RUNNABLE 2
>>
>> +#define __KVM_HAVE_ARCH_VCPU_GET_WQ 1
>> +static inline wait_queue_head *kvm_arch_vcpu_wq(struct kvm_vcpu *vcpu)
>> +{
>> + return vcpu->arch.wqp;
>> +}
>> +
>> #endif /* __POWERPC_KVM_HOST_H__ */
>> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
>> index e2cfb9e..43466aa 100644
>> --- a/arch/powerpc/kvm/book3s_pr.c
>> +++ b/arch/powerpc/kvm/book3s_pr.c
>> @@ -968,6 +968,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
>> if (vcpu->arch.shared->msr & MSR_FP)
>> kvmppc_handle_ext(vcpu, BOOK3S_INTERRUPT_FP_UNAVAIL, MSR_FP);
>>
>> + vcpu->mode = IN_GUEST_MODE;
>
> This is racy. I'll just write a patch myself then :)
>
I couldn't quite figure out the atomicity here, so please fix it for me...
>> +
>> kvm_guest_enter();
>>
>> ret = __kvmppc_vcpu_run(kvm_run, vcpu);
>> @@ -976,6 +978,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
>>
>> local_irq_disable();
>>
>> + vcpu->mode = OUTSIDE_GUEST_MODE;
>> +
>> current->thread.regs->msr = ext_msr;
>>
>> /* Make sure we save the guest FPU/Altivec/VSX state */
>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>> index bb6c988..f909289 100644
>> --- a/arch/powerpc/kvm/booke.c
>> +++ b/arch/powerpc/kvm/booke.c
>> @@ -321,11 +321,13 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
>> return -EINVAL;
>> }
>>
>> + vcpu->mode = IN_GUEST_MODE;
>> local_irq_disable();
>> kvm_guest_enter();
>> ret = __kvmppc_vcpu_run(kvm_run, vcpu);
>> kvm_guest_exit();
>> local_irq_enable();
>> + vcpu->mode = OUTSIDE_GUEST_MODE;
>>
>> return ret;
>> }
>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> index 607fbdf..3b9b5cd 100644
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -42,6 +42,11 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
>> !!(v->arch.pending_exceptions);
>> }
>>
>> +int kvm_arch_vcpu_in_guest_mode(struct kvm_vcpu *vcpu)
>> +{
>> + return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
>> +}
>> +
>> int kvmppc_kvm_pv(struct kvm_vcpu *vcpu)
>> {
>> int nr = kvmppc_get_gpr(vcpu, 11);
>> @@ -311,10 +316,7 @@ static void kvmppc_decrementer_func(unsigned long data)
>>
>> kvmppc_core_queue_dec(vcpu);
>>
>> - if (waitqueue_active(vcpu->arch.wqp)) {
>> - wake_up_interruptible(vcpu->arch.wqp);
>> - vcpu->stat.halt_wakeup++;
>> - }
>> + kvm_vcpu_kick(vcpu);
>> }
>>
>> /*
>> @@ -572,12 +574,7 @@ int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu *vcpu, struct kvm_interrupt *irq)
>>
>> kvmppc_core_queue_external(vcpu, irq);
>>
>> - if (waitqueue_active(vcpu->arch.wqp)) {
>> - wake_up_interruptible(vcpu->arch.wqp);
>> - vcpu->stat.halt_wakeup++;
>> - } else if (vcpu->cpu != -1) {
>> - smp_send_reschedule(vcpu->cpu);
>> - }
>> + kvm_vcpu_kick(vcpu);
>>
>> return 0;
>> }
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index d1c44573..5e2217f 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -380,6 +380,14 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>> return 0;
>> }
>>
>> +int kvm_arch_vcpu_in_guest_mode(struct kvm_vcpu *vcpu)
>> +{
>> + /* kvm common code refers to this, but never calls it */
>> + BUG();
>> + return 0;
>> +}
>> +
>> +
>> static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>> {
>> kvm_s390_vcpu_initial_reset(vcpu);
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 43c4dab..57d13a6 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -6135,21 +6135,9 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>> kvm_cpu_has_interrupt(vcpu));
>> }
>>
>> -void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>> +int kvm_arch_vcpu_in_guest_mode(struct kvm_vcpu *vcpu)
>> {
>> - int me;
>> - int cpu = vcpu->cpu;
>> -
>> - if (waitqueue_active(&vcpu->wq)) {
>> - wake_up_interruptible(&vcpu->wq);
>> - ++vcpu->stat.halt_wakeup;
>> - }
>> -
>> - me = get_cpu();
>> - if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
>> - if (kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE)
>> - smp_send_reschedule(cpu);
>> - put_cpu();
>> + return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
>> }
>>
>> int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu)
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 41973e2..638fa64 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -440,6 +440,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm, struct kvm_memory_slot *memslot,
>> gfn_t gfn);
>>
>> void kvm_vcpu_block(struct kvm_vcpu *vcpu);
>> +void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
>> void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu);
>> void kvm_resched(struct kvm_vcpu *vcpu);
>> void kvm_load_guest_fpu(struct kvm_vcpu *vcpu);
>> @@ -507,6 +508,7 @@ int kvm_arch_hardware_setup(void);
>> void kvm_arch_hardware_unsetup(void);
>> void kvm_arch_check_processor_compat(void *rtn);
>> int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
>> +int kvm_arch_vcpu_in_guest_mode(struct kvm_vcpu *vcpu);
>>
>> void kvm_free_physmem(struct kvm *kvm);
>>
>> @@ -522,6 +524,13 @@ static inline void kvm_arch_free_vm(struct kvm *kvm)
>> }
>> #endif
>>
>> +#ifndef __KVM_HAVE_ARCH_VCPU_GET_WQ
>> +static inline wait_queue_head_t *kvm_arch_vcpu_wq(struct kvm_vcpu *vcpu)
>> +{
>> + return &vcpu->wq;
>> +}
>> +#endif
>> +
>> int kvm_arch_init_vm(struct kvm *kvm);
>> void kvm_arch_destroy_vm(struct kvm *kvm);
>> void kvm_free_all_assigned_devices(struct kvm *kvm);
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 7287bf5..3d72008 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -1581,6 +1581,28 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>> finish_wait(&vcpu->wq, &wait);
>> }
>>
>> +/*
>> + * Kick a sleeping VCPU, or a guest VCPU in guest mode, into host kernel mode.
>> + */
>> +void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>> +{
>> + int me;
>> + int cpu = vcpu->cpu;
>> + wait_queue_head_t *wqp;
>> +
>> + wqp = kvm_arch_vcpu_wq(vcpu);
>> + if (waitqueue_active(wqp)) {
>> + wake_up_interruptible(wqp);
>> + ++vcpu->stat.halt_wakeup;
>> + }
>> +
>> + me = get_cpu();
>> + if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
>> + if (kvm_arch_vcpu_in_guest_mode(vcpu))
>
> Didn't the last version did an atomic cmpxchg here? Also, that check certainly wasn't there for ia64 before. Are you sure it's happy with it? :)
>
>
no, there was no cmpxchg here. I'm not sure why you think it's necessary.
and no, ia64 didn't check for this before, and I'm not sure it's happy
with it, which is why we have these reviews I guess.
how about calling that functions kvm_arch_vcpu_should_kick and
performing the (!test_and_set_bit(KVM_REQ_KICK, &vcpu->requests))
check from ia64 in there and do the vcpu->mode check for other archs?
thanks,
Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
