| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 15 Dec 2011 15:48:38 +0900
From: Takuya Yoshikawa <yoshikawa.takuya@....ntt.co.jp>
To: Liu Ping Fan <kernelfans@...il.com>
CC: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, avi@...hat.com,
aliguori@...ibm.com, gleb@...hat.com, mtosatti@...hat.com,
jan.kiszka@....de
Subject: Re: [PATCH v4] kvm: make vcpu life cycle separated from kvm instance
(2011/12/15 13:28), Liu Ping Fan wrote:
> From: Liu Ping Fan<pingfank@...ux.vnet.ibm.com>
>
> Currently, vcpu can be destructed only when kvm instance destroyed.
> Change this to vcpu's destruction before kvm instance, so vcpu MUST
> and CAN be destroyed before kvm's destroy.
Could you explain why this change is needed here?
Would be helpful for those, including me, who will read the commit later.
>
> Signed-off-by: Liu Ping Fan<pingfank@...ux.vnet.ibm.com>
> ---
...
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index cac4746..f275b8c 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -50,25 +50,28 @@ static void pic_unlock(struct kvm_pic *s)
> {
> bool wakeup = s->wakeup_needed;
> struct kvm_vcpu *vcpu, *found = NULL;
> - int i;
> + struct kvm *kvm = s->kvm;
>
> s->wakeup_needed = false;
>
> spin_unlock(&s->lock);
>
> if (wakeup) {
> - kvm_for_each_vcpu(i, vcpu, s->kvm) {
> + rcu_read_lock();
> + kvm_for_each_vcpu(vcpu, kvm)
> if (kvm_apic_accept_pic_intr(vcpu)) {
> found = vcpu;
> break;
> }
> - }
>
> - if (!found)
> + if (!found) {
> + rcu_read_unlock();
> return;
> + }
>
> kvm_make_request(KVM_REQ_EVENT, found);
> kvm_vcpu_kick(found);
> + rcu_read_unlock();
> }
> }
How about this? (just about stylistic issues)
if (!wakeup)
return;
rcu_read_lock();
kvm_for_each_vcpu(vcpu, kvm)
if (kvm_apic_accept_pic_intr(vcpu)) {
found = vcpu;
break;
}
if (!found)
goto out;
kvm_make_request(KVM_REQ_EVENT, found);
kvm_vcpu_kick(found);
out:
rcu_read_unlock();
...
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
...
> +void kvm_arch_vcpu_zap(struct work_struct *work)
> +{
> + struct kvm_vcpu *vcpu = container_of(work, struct kvm_vcpu,
> + zap_work);
> + struct kvm *kvm = vcpu->kvm;
>
> - atomic_set(&kvm->online_vcpus, 0);
> - mutex_unlock(&kvm->lock);
> + kvm_clear_async_pf_completion_queue(vcpu);
> + kvm_unload_vcpu_mmu(vcpu);
> + kvm_arch_vcpu_free(vcpu);
> + kvm_put_kvm(kvm);
> }
zap is really a good name for this?
...
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index d526231..733de1c 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -19,6 +19,7 @@
> #include<linux/slab.h>
> #include<linux/rcupdate.h>
> #include<linux/ratelimit.h>
> +#include<linux/atomic.h>
> #include<asm/signal.h>
>
> #include<linux/kvm.h>
> @@ -113,6 +114,10 @@ enum {
>
> struct kvm_vcpu {
> struct kvm *kvm;
> + atomic_t refcount;
> + struct list_head list;
> + struct rcu_head head;
> + struct work_struct zap_work;
How about adding some comments?
zap_work is not at all self explanatory, IMO.
> #ifdef CONFIG_PREEMPT_NOTIFIERS
> struct preempt_notifier preempt_notifier;
> #endif
> @@ -241,9 +246,9 @@ struct kvm {
> u32 bsp_vcpu_id;
> struct kvm_vcpu *bsp_vcpu;
> #endif
> - struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
> + struct list_head vcpus;
> atomic_t online_vcpus;
> - int last_boosted_vcpu;
> + struct kvm_vcpu *last_boosted_vcpu;
> struct list_head vm_list;
> struct mutex lock;
> struct kvm_io_bus *buses[KVM_NR_BUSES];
> @@ -290,17 +295,15 @@ struct kvm {
> #define kvm_printf(kvm, fmt ...) printk(KERN_DEBUG fmt)
> #define vcpu_printf(vcpu, fmt...) kvm_printf(vcpu->kvm, fmt)
>
> -static inline struct kvm_vcpu *kvm_get_vcpu(struct kvm *kvm, int i)
> -{
> - smp_rmb();
> - return kvm->vcpus[i];
> -}
> +struct kvm_vcpu *kvm_vcpu_get(struct kvm_vcpu *vcpu);
> +void kvm_vcpu_put(struct kvm_vcpu *vcpu);
> +void kvm_arch_vcpu_zap(struct work_struct *work);
> +
> +#define kvm_for_each_vcpu(vcpu, kvm) \
> + list_for_each_entry_rcu(vcpu,&kvm->vcpus, list)
Is this macro really worth it?
_rcu shows readers important information, I think.
>
> -#define kvm_for_each_vcpu(idx, vcpup, kvm) \
> - for (idx = 0; \
> - idx< atomic_read(&kvm->online_vcpus)&& \
> - (vcpup = kvm_get_vcpu(kvm, idx)) != NULL; \
> - idx++)
> +#define kvm_for_each_vcpu_continue(vcpu, kvm) \
> + list_for_each_entry_continue_rcu(vcpu,&kvm->vcpus, list)
Same here.
Why do you want to hide _rcu from readers?
Takuya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists