Hi Anshul, Thanks for your test. On 07/30/2014 10:31 PM, Anshul Makkar wrote:
> Hi, > > I am testing the cpu-hotunplug patches. I observed that after the > deletion of the cpu with id = x, if I cpu-add the same cpu again id = > x, then qemu exits with the error that file descriptor already exists. Could you please offer the whole reproduce routine? In my test box, we can add a removed cpu with the id. > > On debugging I found that if I give cpu-add <apic-id = x>, then > qemu_kvm_cpu_thread_fn->kvm_init_vcpu is called which sends an IOCTL > (KVM_CREATE_VCPU) to kvm to create a new fd. As the fd already exists > in KVM as we never delete the fd from the kernel and just park it in > separate list, it returns false and QEMU exits. In the above code > flow, no where its being checked if we have the cpu with cpuid = x > available in the parked list and we can reuse it. > > Am I missing something or this bit is yet to be implmented. Yes, it is implemented, in the same way as you mention above, please refer to function kvm_get_vcpu(). Thanks, Gu > > Thanks > Anshul Makkar > > On Fri, Jul 18, 2014 at 4:09 AM, Gu Zheng <guz.f...@cn.fujitsu.com> wrote: >> Hi Anshul, >> On 07/18/2014 12:24 AM, Anshul Makkar wrote: >> >>> Are we not going to introduce new command cpu_del for deleting the cpu ? >>> >>> I couldn't find any patch for addition of cpu_del command. Is this >>> intentional and we intend to use device_del (and similarly device_add) >>> for cpu hot(un)plug or just skipped to be added later. I have the >>> patch for the same which I can release, if the intent is to add this >>> command. >> >> The "device_add/device_del" interface is the approved way to support add/del >> cpu, >> which is also more common and elegant than "cpu_add/del". >> <http://wiki.qemu.org/Features/CPUHotplug> >> so we intend to use device_del rather than the cpu_del. >> And IMO, the cpu_add will be replaced by "device_add" sooner or later. >> >> Thanks, >> Gu >> >>> >>> Thanks >>> Anshul Makkar >>> >>> On Fri, Jul 11, 2014 at 11:59 AM, Gu Zheng <guz.f...@cn.fujitsu.com> wrote: >>>> After ACPI get a signal to eject a vCPU, the vCPU must be >>>> removed from CPU list，before the vCPU really removed, then >>>> release the all related vCPU objects. >>>> But we do not close KVM vcpu fd, just record it into a list, in >>>> order to reuse it. >>>> >>>> Signed-off-by: Chen Fan <chen.fan.f...@cn.fujitsu.com> >>>> Signed-off-by: Gu Zheng <guz.f...@cn.fujitsu.com> >>>> --- >>>> cpus.c | 37 ++++++++++++++++++++++++++++++++ >>>> include/sysemu/kvm.h | 1 + >>>> kvm-all.c | 57 >>>> +++++++++++++++++++++++++++++++++++++++++++++++++- >>>> 3 files changed, 94 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/cpus.c b/cpus.c >>>> index 4dfb889..9a73407 100644 >>>> --- a/cpus.c >>>> +++ b/cpus.c >>>> @@ -786,6 +786,24 @@ void async_run_on_cpu(CPUState *cpu, void >>>> (*func)(void *data), void *data) >>>> qemu_cpu_kick(cpu); >>>> } >>>> >>>> +static void qemu_kvm_destroy_vcpu(CPUState *cpu) >>>> +{ >>>> + CPU_REMOVE(cpu); >>>> + >>>> + if (kvm_destroy_vcpu(cpu) < 0) { >>>> + fprintf(stderr, "kvm_destroy_vcpu failed.\n"); >>>> + exit(1); >>>> + } >>>> + >>>> + object_unparent(OBJECT(cpu)); >>>> +} >>>> + >>>> +static void qemu_tcg_destroy_vcpu(CPUState *cpu) >>>> +{ >>>> + CPU_REMOVE(cpu); >>>> + object_unparent(OBJECT(cpu)); >>>> +} >>>> + >>>> static void flush_queued_work(CPUState *cpu) >>>> { >>>> struct qemu_work_item *wi; >>>> @@ -877,6 +895,11 @@ static void *qemu_kvm_cpu_thread_fn(void *arg) >>>> } >>>> } >>>> qemu_kvm_wait_io_event(cpu); >>>> + if (cpu->exit && !cpu_can_run(cpu)) { >>>> + qemu_kvm_destroy_vcpu(cpu); >>>> + qemu_mutex_unlock(&qemu_global_mutex); >>>> + return NULL; >>>> + } >>>> } >>>> >>>> return NULL; >>>> @@ -929,6 +952,7 @@ static void tcg_exec_all(void); >>>> static void *qemu_tcg_cpu_thread_fn(void *arg) >>>> { >>>> CPUState *cpu = arg; >>>> + CPUState *remove_cpu = NULL; >>>> >>>> qemu_tcg_init_cpu_signals(); >>>> qemu_thread_get_self(cpu->thread); >>>> @@ -961,6 +985,16 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) >>>> } >>>> } >>>> qemu_tcg_wait_io_event(); >>>> + CPU_FOREACH(cpu) { >>>> + if (cpu->exit && !cpu_can_run(cpu)) { >>>> + remove_cpu = cpu; >>>> + break; >>>> + } >>>> + } >>>> + if (remove_cpu) { >>>> + qemu_tcg_destroy_vcpu(remove_cpu); >>>> + remove_cpu = NULL; >>>> + } >>>> } >>>> >>>> return NULL; >>>> @@ -1316,6 +1350,9 @@ static void tcg_exec_all(void) >>>> break; >>>> } >>>> } else if (cpu->stop || cpu->stopped) { >>>> + if (cpu->exit) { >>>> + next_cpu = CPU_NEXT(cpu); >>>> + } >>>> break; >>>> } >>>> } >>>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h >>>> index 174ea36..88e2403 100644 >>>> --- a/include/sysemu/kvm.h >>>> +++ b/include/sysemu/kvm.h >>>> @@ -178,6 +178,7 @@ int kvm_has_intx_set_mask(void); >>>> >>>> int kvm_init_vcpu(CPUState *cpu); >>>> int kvm_cpu_exec(CPUState *cpu); >>>> +int kvm_destroy_vcpu(CPUState *cpu); >>>> >>>> #ifdef NEED_CPU_H >>>> >>>> diff --git a/kvm-all.c b/kvm-all.c >>>> index 3ae30ee..25e2a43 100644 >>>> --- a/kvm-all.c >>>> +++ b/kvm-all.c >>>> @@ -74,6 +74,12 @@ typedef struct KVMSlot >>>> >>>> typedef struct kvm_dirty_log KVMDirtyLog; >>>> >>>> +struct KVMParkedVcpu { >>>> + unsigned long vcpu_id; >>>> + int kvm_fd; >>>> + QLIST_ENTRY(KVMParkedVcpu) node; >>>> +}; >>>> + >>>> struct KVMState >>>> { >>>> KVMSlot *slots; >>>> @@ -108,6 +114,7 @@ struct KVMState >>>> QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) >>>> msi_hashtab[KVM_MSI_HASHTAB_SIZE]; >>>> bool direct_msi; >>>> #endif >>>> + QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus; >>>> }; >>>> >>>> KVMState *kvm_state; >>>> @@ -226,6 +233,53 @@ static int kvm_set_user_memory_region(KVMState *s, >>>> KVMSlot *slot) >>>> return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem); >>>> } >>>> >>>> +int kvm_destroy_vcpu(CPUState *cpu) >>>> +{ >>>> + KVMState *s = kvm_state; >>>> + long mmap_size; >>>> + struct KVMParkedVcpu *vcpu = NULL; >>>> + int ret = 0; >>>> + >>>> + DPRINTF("kvm_destroy_vcpu\n"); >>>> + >>>> + mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0); >>>> + if (mmap_size < 0) { >>>> + ret = mmap_size; >>>> + DPRINTF("KVM_GET_VCPU_MMAP_SIZE failed\n"); >>>> + goto err; >>>> + } >>>> + >>>> + ret = munmap(cpu->kvm_run, mmap_size); >>>> + if (ret < 0) { >>>> + goto err; >>>> + } >>>> + >>>> + vcpu = g_malloc0(sizeof(*vcpu)); >>>> + vcpu->vcpu_id = kvm_arch_vcpu_id(cpu); >>>> + vcpu->kvm_fd = cpu->kvm_fd; >>>> + QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node); >>>> +err: >>>> + return ret; >>>> +} >>>> + >>>> +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) >>>> +{ >>>> + struct KVMParkedVcpu *cpu; >>>> + >>>> + QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) { >>>> + if (cpu->vcpu_id == vcpu_id) { >>>> + int kvm_fd; >>>> + >>>> + QLIST_REMOVE(cpu, node); >>>> + kvm_fd = cpu->kvm_fd; >>>> + g_free(cpu); >>>> + return kvm_fd; >>>> + } >>>> + } >>>> + >>>> + return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id); >>>> +} >>>> + >>>> int kvm_init_vcpu(CPUState *cpu) >>>> { >>>> KVMState *s = kvm_state; >>>> @@ -234,7 +288,7 @@ int kvm_init_vcpu(CPUState *cpu) >>>> >>>> DPRINTF("kvm_init_vcpu\n"); >>>> >>>> - ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)kvm_arch_vcpu_id(cpu)); >>>> + ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu)); >>>> if (ret < 0) { >>>> DPRINTF("kvm_create_vcpu failed\n"); >>>> goto err; >>>> @@ -1404,6 +1458,7 @@ int kvm_init(MachineClass *mc) >>>> #ifdef KVM_CAP_SET_GUEST_DEBUG >>>> QTAILQ_INIT(&s->kvm_sw_breakpoints); >>>> #endif >>>> + QLIST_INIT(&s->kvm_parked_vcpus); >>>> s->vmfd = -1; >>>> s->fd = qemu_open("/dev/kvm", O_RDWR); >>>> if (s->fd == -1) { >>>> -- >>>> 1.7.7 >>>> >>> . >>> >> >> > . >
