[PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper

[PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper

[Peter Xu]

v2:
- add one more patch (patch 2 in v2) to check pic/ioapic's
  existance before destroying them

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(-)

-- 
2.7.4

[PATCH v2 1/3] KVM: x86: clear bus pointer when destroyed

[Peter Xu]

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

[PATCH v2 2/3] KVM: x86: check existance before destroy

[Peter Xu]

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

[PATCH v2 3/3] KVM: x86: use pic/ioapic destructor when destroy vm

[Peter Xu]

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

Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper

[David Hildenbrand]

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

Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper

[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,

-- peterx

Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper

[David Hildenbrand]

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

Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper

[David Hildenbrand]

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

Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper

[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. :-)

-- peterx

Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper

[Radim Krčmář]

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(). :)

Re: [PATCH v2 0/3] KVM: x86: use pic/ioapic destructor when proper

[Peter Xu]

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

Re: [PATCH v2 1/3] KVM: x86: clear bus pointer when destroyed

[David Hildenbrand]

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

Re: [PATCH v2 2/3] KVM: x86: check existance before destroy

[David Hildenbrand]

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

Re: [PATCH v2 3/3] KVM: x86: use pic/ioapic destructor when destroy vm

[David Hildenbrand]

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