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

[Peter Xu <peterx@redhat.com>]

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