* [PATCH v2 1/3] KVM: x86: clear bus pointer when destroyed
2017-03-15 8:01 [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper Peter Xu
@ 2017-03-15 8:01 ` Peter Xu
2017-03-21 9:24 ` David Hildenbrand
2017-03-15 8:01 ` [PATCH v2 2/3] KVM: x86: check existance before destroy Peter Xu
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Peter Xu @ 2017-03-15 8:01 UTC (permalink / raw)
To: kvm; +Cc: David Hildenbrand, Paolo Bonzini, peterx,
Radim Krčmář
When releasing the bus, let's clear the bus pointers to mark it out. If
any further device unregister happens on this bus, we know that we're
done if we found the bus being released already.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
virt/kvm/kvm_main.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a17d787..7445566 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -727,8 +727,10 @@ static void kvm_destroy_vm(struct kvm *kvm)
list_del(&kvm->vm_list);
spin_unlock(&kvm_lock);
kvm_free_irq_routing(kvm);
- for (i = 0; i < KVM_NR_BUSES; i++)
+ for (i = 0; i < KVM_NR_BUSES; i++) {
kvm_io_bus_destroy(kvm->buses[i]);
+ kvm->buses[i] = NULL;
+ }
kvm_coalesced_mmio_free(kvm);
#if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)
mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
@@ -3579,6 +3581,14 @@ int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx,
struct kvm_io_bus *new_bus, *bus;
bus = kvm->buses[bus_idx];
+
+ /*
+ * It's possible the bus being released before hand. If so,
+ * we're done here.
+ */
+ if (!bus)
+ return 0;
+
r = -ENOENT;
for (i = 0; i < bus->dev_count; i++)
if (bus->range[i].dev == dev) {
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 1/3] KVM: x86: clear bus pointer when destroyed
2017-03-15 8:01 ` [PATCH v2 1/3] KVM: x86: clear bus pointer when destroyed Peter Xu
@ 2017-03-21 9:24 ` David Hildenbrand
0 siblings, 0 replies; 14+ messages in thread
From: David Hildenbrand @ 2017-03-21 9:24 UTC (permalink / raw)
To: Peter Xu, kvm; +Cc: Paolo Bonzini, Radim Krčmář
On 15.03.2017 09:01, Peter Xu wrote:
> When releasing the bus, let's clear the bus pointers to mark it out. If
> any further device unregister happens on this bus, we know that we're
> done if we found the bus being released already.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> virt/kvm/kvm_main.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a17d787..7445566 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -727,8 +727,10 @@ static void kvm_destroy_vm(struct kvm *kvm)
> list_del(&kvm->vm_list);
> spin_unlock(&kvm_lock);
> kvm_free_irq_routing(kvm);
> - for (i = 0; i < KVM_NR_BUSES; i++)
> + for (i = 0; i < KVM_NR_BUSES; i++) {
> kvm_io_bus_destroy(kvm->buses[i]);
> + kvm->buses[i] = NULL;
> + }
> kvm_coalesced_mmio_free(kvm);
> #if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)
> mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
> @@ -3579,6 +3581,14 @@ int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx,
> struct kvm_io_bus *new_bus, *bus;
>
> bus = kvm->buses[bus_idx];
> +
> + /*
> + * It's possible the bus being released before hand. If so,
> + * we're done here.
> + */
/* the bus might have already been destroyed */ ?
Reviewed-by: David Hildenbrand <david@redhat.com>
> + if (!bus)
> + return 0;
> +
> r = -ENOENT;
> for (i = 0; i < bus->dev_count; i++)
> if (bus->range[i].dev == dev) {
>
--
Thanks,
David
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/3] KVM: x86: check existance before destroy
2017-03-15 8:01 [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper Peter Xu
2017-03-15 8:01 ` [PATCH v2 1/3] KVM: x86: clear bus pointer when destroyed Peter Xu
@ 2017-03-15 8:01 ` Peter Xu
2017-03-21 9:24 ` David Hildenbrand
2017-03-15 8:01 ` [PATCH v2 3/3] KVM: x86: use pic/ioapic destructor when destroy vm Peter Xu
2017-03-15 8:47 ` [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper David Hildenbrand
3 siblings, 1 reply; 14+ messages in thread
From: Peter Xu @ 2017-03-15 8:01 UTC (permalink / raw)
To: kvm; +Cc: David Hildenbrand, Paolo Bonzini, peterx,
Radim Krčmář
Mostly used for split irqchip mode. In that case, these two things are
not inited at all, so no need to release.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/x86/kvm/i8259.c | 3 +++
arch/x86/kvm/ioapic.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 73ea24d..047b17a 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -657,6 +657,9 @@ void kvm_pic_destroy(struct kvm *kvm)
{
struct kvm_pic *vpic = kvm->arch.vpic;
+ if (!vpic)
+ return;
+
kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_master);
kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_slave);
kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_eclr);
diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 6e219e5..289270a 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -635,6 +635,9 @@ void kvm_ioapic_destroy(struct kvm *kvm)
{
struct kvm_ioapic *ioapic = kvm->arch.vioapic;
+ if (!ioapic)
+ return;
+
cancel_delayed_work_sync(&ioapic->eoi_inject);
kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, &ioapic->dev);
kvm->arch.vioapic = NULL;
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 2/3] KVM: x86: check existance before destroy
2017-03-15 8:01 ` [PATCH v2 2/3] KVM: x86: check existance before destroy Peter Xu
@ 2017-03-21 9:24 ` David Hildenbrand
0 siblings, 0 replies; 14+ messages in thread
From: David Hildenbrand @ 2017-03-21 9:24 UTC (permalink / raw)
To: Peter Xu, kvm; +Cc: Paolo Bonzini, Radim Krčmář
On 15.03.2017 09:01, Peter Xu wrote:
> Mostly used for split irqchip mode. In that case, these two things are
> not inited at all, so no need to release.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> arch/x86/kvm/i8259.c | 3 +++
> arch/x86/kvm/ioapic.c | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 73ea24d..047b17a 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -657,6 +657,9 @@ void kvm_pic_destroy(struct kvm *kvm)
> {
> struct kvm_pic *vpic = kvm->arch.vpic;
>
> + if (!vpic)
> + return;
> +
> kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_master);
> kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_slave);
> kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_eclr);
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index 6e219e5..289270a 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -635,6 +635,9 @@ void kvm_ioapic_destroy(struct kvm *kvm)
> {
> struct kvm_ioapic *ioapic = kvm->arch.vioapic;
>
> + if (!ioapic)
> + return;
> +
> cancel_delayed_work_sync(&ioapic->eoi_inject);
> kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, &ioapic->dev);
> kvm->arch.vioapic = NULL;
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] KVM: x86: use pic/ioapic destructor when destroy vm
2017-03-15 8:01 [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper Peter Xu
2017-03-15 8:01 ` [PATCH v2 1/3] KVM: x86: clear bus pointer when destroyed Peter Xu
2017-03-15 8:01 ` [PATCH v2 2/3] KVM: x86: check existance before destroy Peter Xu
@ 2017-03-15 8:01 ` Peter Xu
2017-03-21 9:25 ` David Hildenbrand
2017-03-15 8:47 ` [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper David Hildenbrand
3 siblings, 1 reply; 14+ messages in thread
From: Peter Xu @ 2017-03-15 8:01 UTC (permalink / raw)
To: kvm; +Cc: David Hildenbrand, Paolo Bonzini, peterx,
Radim Krčmář
We have specific destructors for pic/ioapic, we'd better use them when
destroying the VM as well.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/x86/kvm/x86.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1faf620..d30ff49 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8153,8 +8153,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
if (kvm_x86_ops->vm_destroy)
kvm_x86_ops->vm_destroy(kvm);
kvm_iommu_unmap_guest(kvm);
- kfree(kvm->arch.vpic);
- kfree(kvm->arch.vioapic);
+ kvm_pic_destroy(kvm);
+ kvm_ioapic_destroy(kvm);
kvm_free_vcpus(kvm);
kvfree(rcu_dereference_check(kvm->arch.apic_map, 1));
kvm_mmu_uninit_vm(kvm);
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] KVM: x86: use pic/ioapic destructor when destroy vm
2017-03-15 8:01 ` [PATCH v2 3/3] KVM: x86: use pic/ioapic destructor when destroy vm Peter Xu
@ 2017-03-21 9:25 ` David Hildenbrand
0 siblings, 0 replies; 14+ messages in thread
From: David Hildenbrand @ 2017-03-21 9:25 UTC (permalink / raw)
To: Peter Xu, kvm; +Cc: Paolo Bonzini, Radim Krčmář
On 15.03.2017 09:01, Peter Xu wrote:
> We have specific destructors for pic/ioapic, we'd better use them when
> destroying the VM as well.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> arch/x86/kvm/x86.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1faf620..d30ff49 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8153,8 +8153,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> if (kvm_x86_ops->vm_destroy)
> kvm_x86_ops->vm_destroy(kvm);
> kvm_iommu_unmap_guest(kvm);
> - kfree(kvm->arch.vpic);
> - kfree(kvm->arch.vioapic);
> + kvm_pic_destroy(kvm);
> + kvm_ioapic_destroy(kvm);
> kvm_free_vcpus(kvm);
> kvfree(rcu_dereference_check(kvm->arch.apic_map, 1));
> kvm_mmu_uninit_vm(kvm);
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper
2017-03-15 8:01 [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper Peter Xu
` (2 preceding siblings ...)
2017-03-15 8:01 ` [PATCH v2 3/3] KVM: x86: use pic/ioapic destructor when destroy vm Peter Xu
@ 2017-03-15 8:47 ` David Hildenbrand
2017-03-15 8:59 ` Peter Xu
3 siblings, 1 reply; 14+ messages in thread
From: David Hildenbrand @ 2017-03-15 8:47 UTC (permalink / raw)
To: Peter Xu, kvm; +Cc: Paolo Bonzini, Radim Krčmář
Am 15.03.2017 um 09:01 schrieb Peter Xu:
> v2:
> - add one more patch (patch 2 in v2) to check pic/ioapic's
> existance before destroying them
Patch 2+3 without 1 should work, too, right?
>
> A tiny cleanup series for pic/ioapic destruction when vm destroys.
> Please review, Thanks.
>
> Peter Xu (3):
> KVM: x86: clear bus pointer when destroyed
> KVM: x86: check existance before destroy
> KVM: x86: use pic/ioapic destructor when destroy vm
>
> arch/x86/kvm/i8259.c | 3 +++
> arch/x86/kvm/ioapic.c | 3 +++
> arch/x86/kvm/x86.c | 4 ++--
> virt/kvm/kvm_main.c | 12 +++++++++++-
> 4 files changed, 19 insertions(+), 3 deletions(-)
>
--
Thanks,
David
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper
2017-03-15 8:47 ` [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper David Hildenbrand
@ 2017-03-15 8:59 ` Peter Xu
2017-03-15 9:04 ` David Hildenbrand
2017-03-15 11:15 ` David Hildenbrand
0 siblings, 2 replies; 14+ messages in thread
From: Peter Xu @ 2017-03-15 8:59 UTC (permalink / raw)
To: David Hildenbrand; +Cc: kvm, Paolo Bonzini, Radim Krčmář
On Wed, Mar 15, 2017 at 09:47:14AM +0100, David Hildenbrand wrote:
> Am 15.03.2017 um 09:01 schrieb Peter Xu:
> > v2:
> > - add one more patch (patch 2 in v2) to check pic/ioapic's
> > existance before destroying them
>
> Patch 2+3 without 1 should work, too, right?
We may need that? When destroy VM, it's:
kvm_destroy_vm()
foreach (bus) do kvm_io_bus_destroy()
kfree(bus) <--- [1]
kvm_arch_destroy_vm()
kvm_pic_destroy()
kvm_io_bus_unregister_dev() <--- [2]
Here if without patch 1 we'll reference bus->dev_count in [2], while
actually the bus has been freed already in [1]. Thanks,
-- peterx
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper
2017-03-15 8:59 ` Peter Xu
@ 2017-03-15 9:04 ` David Hildenbrand
2017-03-15 11:15 ` David Hildenbrand
1 sibling, 0 replies; 14+ messages in thread
From: David Hildenbrand @ 2017-03-15 9:04 UTC (permalink / raw)
To: Peter Xu; +Cc: kvm, Paolo Bonzini, Radim Krčmář
Am 15.03.2017 um 09:59 schrieb Peter Xu:
> On Wed, Mar 15, 2017 at 09:47:14AM +0100, David Hildenbrand wrote:
>> Am 15.03.2017 um 09:01 schrieb Peter Xu:
>>> v2:
>>> - add one more patch (patch 2 in v2) to check pic/ioapic's
>>> existance before destroying them
>>
>> Patch 2+3 without 1 should work, too, right?
>
> We may need that? When destroy VM, it's:
>
> kvm_destroy_vm()
> foreach (bus) do kvm_io_bus_destroy()
> kfree(bus) <--- [1]
> kvm_arch_destroy_vm()
> kvm_pic_destroy()
> kvm_io_bus_unregister_dev() <--- [2]
>
> Here if without patch 1 we'll reference bus->dev_count in [2], while
> actually the bus has been freed already in [1]. Thanks,
Ah, thanks for the hint!
--
Thanks,
David
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper
2017-03-15 8:59 ` Peter Xu
2017-03-15 9:04 ` David Hildenbrand
@ 2017-03-15 11:15 ` David Hildenbrand
2017-03-16 6:04 ` Peter Xu
1 sibling, 1 reply; 14+ messages in thread
From: David Hildenbrand @ 2017-03-15 11:15 UTC (permalink / raw)
To: Peter Xu; +Cc: kvm, Paolo Bonzini, Radim Krčmář
Am 15.03.2017 um 09:59 schrieb Peter Xu:
> On Wed, Mar 15, 2017 at 09:47:14AM +0100, David Hildenbrand wrote:
>> Am 15.03.2017 um 09:01 schrieb Peter Xu:
>>> v2:
>>> - add one more patch (patch 2 in v2) to check pic/ioapic's
>>> existance before destroying them
>>
>> Patch 2+3 without 1 should work, too, right?
>
> We may need that? When destroy VM, it's:
>
> kvm_destroy_vm()
> foreach (bus) do kvm_io_bus_destroy()
> kfree(bus) <--- [1]
> kvm_arch_destroy_vm()
> kvm_pic_destroy()
> kvm_io_bus_unregister_dev() <--- [2]
>
Wouldn't the natural way be
1. kvm_arch_destroy_vm()
2. foreach (bus) do kvm_io_bus_destroy()
kfree(bus)
Then we wouldn't have to deal with suddenly removed buses. But maybe
that order is of importance here ...
> Here if without patch 1 we'll reference bus->dev_count in [2], while
> actually the bus has been freed already in [1]. Thanks,
>
> -- peterx
>
--
Thanks,
David
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper
2017-03-15 11:15 ` David Hildenbrand
@ 2017-03-16 6:04 ` Peter Xu
2017-03-16 19:32 ` Radim Krčmář
0 siblings, 1 reply; 14+ messages in thread
From: Peter Xu @ 2017-03-16 6:04 UTC (permalink / raw)
To: David Hildenbrand; +Cc: kvm, Paolo Bonzini, Radim Krčmář
On Wed, Mar 15, 2017 at 12:15:24PM +0100, David Hildenbrand wrote:
> Am 15.03.2017 um 09:59 schrieb Peter Xu:
> > On Wed, Mar 15, 2017 at 09:47:14AM +0100, David Hildenbrand wrote:
> >> Am 15.03.2017 um 09:01 schrieb Peter Xu:
> >>> v2:
> >>> - add one more patch (patch 2 in v2) to check pic/ioapic's
> >>> existance before destroying them
> >>
> >> Patch 2+3 without 1 should work, too, right?
> >
> > We may need that? When destroy VM, it's:
> >
> > kvm_destroy_vm()
> > foreach (bus) do kvm_io_bus_destroy()
> > kfree(bus) <--- [1]
> > kvm_arch_destroy_vm()
> > kvm_pic_destroy()
> > kvm_io_bus_unregister_dev() <--- [2]
> >
>
> Wouldn't the natural way be
>
> 1. kvm_arch_destroy_vm()
> 2. foreach (bus) do kvm_io_bus_destroy()
> kfree(bus)
>
> Then we wouldn't have to deal with suddenly removed buses. But maybe
> that order is of importance here ...
I'll leave this question for Paolo/Radim, or anyone knows this better
than me. :-)
-- peterx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper
2017-03-16 6:04 ` Peter Xu
@ 2017-03-16 19:32 ` Radim Krčmář
2017-03-17 6:05 ` Peter Xu
0 siblings, 1 reply; 14+ messages in thread
From: Radim Krčmář @ 2017-03-16 19:32 UTC (permalink / raw)
To: Peter Xu; +Cc: David Hildenbrand, kvm, Paolo Bonzini
2017-03-16 14:04+0800, Peter Xu:
> On Wed, Mar 15, 2017 at 12:15:24PM +0100, David Hildenbrand wrote:
>> Am 15.03.2017 um 09:59 schrieb Peter Xu:
>> > On Wed, Mar 15, 2017 at 09:47:14AM +0100, David Hildenbrand wrote:
>> >> Am 15.03.2017 um 09:01 schrieb Peter Xu:
>> >>> v2:
>> >>> - add one more patch (patch 2 in v2) to check pic/ioapic's
>> >>> existance before destroying them
>> >>
>> >> Patch 2+3 without 1 should work, too, right?
>> >
>> > We may need that? When destroy VM, it's:
>> >
>> > kvm_destroy_vm()
>> > foreach (bus) do kvm_io_bus_destroy()
>> > kfree(bus) <--- [1]
>> > kvm_arch_destroy_vm()
>> > kvm_pic_destroy()
>> > kvm_io_bus_unregister_dev() <--- [2]
>> >
>>
>> Wouldn't the natural way be
>>
>> 1. kvm_arch_destroy_vm()
>> 2. foreach (bus) do kvm_io_bus_destroy()
>> kfree(bus)
>>
>> Then we wouldn't have to deal with suddenly removed buses. But maybe
>> that order is of importance here ...
>
> I'll leave this question for Paolo/Radim, or anyone knows this better
> than me. :-)
I'd like if kvm_destroy_vm() looked like kvm_create_vm() in reverse and
the current order is closer to that.
I don't think there is dependency between those two and we can easily
reverse them in kvm_create_vm(), so go ahead if you get better code out
of it in kvm_arch_destroy_vm(). :)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper
2017-03-16 19:32 ` Radim Krčmář
@ 2017-03-17 6:05 ` Peter Xu
0 siblings, 0 replies; 14+ messages in thread
From: Peter Xu @ 2017-03-17 6:05 UTC (permalink / raw)
To: Radim Krčmář; +Cc: David Hildenbrand, kvm, Paolo Bonzini
On Thu, Mar 16, 2017 at 08:32:14PM +0100, Radim Krčmář wrote:
> 2017-03-16 14:04+0800, Peter Xu:
> > On Wed, Mar 15, 2017 at 12:15:24PM +0100, David Hildenbrand wrote:
> >> Am 15.03.2017 um 09:59 schrieb Peter Xu:
> >> > On Wed, Mar 15, 2017 at 09:47:14AM +0100, David Hildenbrand wrote:
> >> >> Am 15.03.2017 um 09:01 schrieb Peter Xu:
> >> >>> v2:
> >> >>> - add one more patch (patch 2 in v2) to check pic/ioapic's
> >> >>> existance before destroying them
> >> >>
> >> >> Patch 2+3 without 1 should work, too, right?
> >> >
> >> > We may need that? When destroy VM, it's:
> >> >
> >> > kvm_destroy_vm()
> >> > foreach (bus) do kvm_io_bus_destroy()
> >> > kfree(bus) <--- [1]
> >> > kvm_arch_destroy_vm()
> >> > kvm_pic_destroy()
> >> > kvm_io_bus_unregister_dev() <--- [2]
> >> >
> >>
> >> Wouldn't the natural way be
> >>
> >> 1. kvm_arch_destroy_vm()
> >> 2. foreach (bus) do kvm_io_bus_destroy()
> >> kfree(bus)
> >>
> >> Then we wouldn't have to deal with suddenly removed buses. But maybe
> >> that order is of importance here ...
> >
> > I'll leave this question for Paolo/Radim, or anyone knows this better
> > than me. :-)
>
> I'd like if kvm_destroy_vm() looked like kvm_create_vm() in reverse and
> the current order is closer to that.
>
> I don't think there is dependency between those two and we can easily
> reverse them in kvm_create_vm(), so go ahead if you get better code out
> of it in kvm_arch_destroy_vm(). :)
Thanks. :)
IMHO in all cases we can consider having the first two patches, which
should be something nice to have. And if so, I'll prefer patch 3
comparing to reordering of VM init/destroy to avoid any possiblilty of
breakage, until one day we really need to touch them (I guess this
cleanup series is not a good enough reason :). Thanks,
-- peterx
^ permalink raw reply [flat|nested] 14+ messages in thread