Re: [PATCH v2 06/12] KVM: mark kvm-buses as empty once they were destroyed

2015-03-26 Thread Andre Przywara 
Hi Marcelo,

On 25/03/15 21:59, Marcelo Tosatti wrote:
 On Wed, Mar 25, 2015 at 05:09:13PM +, Marc Zyngier wrote:
 On 23/03/15 15:58, Andre Przywara wrote:
 In kvm_destroy_vm() we call kvm_io_bus_destroy() pretty early,
 especially before calling kvm_arch_destroy_vm(). To avoid
 unregistering devices from the already destroyed bus, let's mark
 the bus with NULL to let other users know it has been destroyed
 already.
 This avoids a crash on a VM shutdown with the VGIC using the
 kvm_io_bus later (the unregistering is in there to be able to roll
 back a faulting init).

 Signed-off-by: Andre Przywara andre.przyw...@arm.com

 That seems sensible, but I don't see why nobody else hits that. What are
 we doing differently?

So I checked the other users of kvm_io_bus, none of them seems to
explicitly unregister the kvm_io_bus devices they registered before if
the VM goes down. They all rely on the whole kvm_io_bus being brought
down eventually, so there is no need for an explicit unregister.
The only point they do unregister is when cleaning up after init fails
in the middle.
So I changed my code to match that behaviour: cleaning up in init and
leaving the VM destruction case alone.
This lets me get rid of that patch and actually removes more code in one
of the following patches.

 It should be valid to call kvm_io_bus_unregister_dev after
 kvm_io_bus_destroy.

AFAICS it's not.
kvm_io_bus_unregister_dev reads like this:
bus = kvm-buses[bus_idx];
r = -ENOENT;
for (i = 0; i  bus-dev_count; i++)
So having the actual bus freed already before would lead to an invalid
dereference. As stated above no-one seems to use it like this at the
moment. Shall I make a (separate) patch to fix that theoretical case?
Setting the pointer values to NULL after kfree() and checking them in
kvm_io_bus_unregister_dev()? What about other functions using the buses?

 
 Are you patching it to handle NULL kvm-buses[bus_idx] ?

No, I handle this is my VGIC clean up function, so that it doesn't try
to call kvm_io_bus_unregister_dev if the bus is already down. But that
will go away with the change mentioned above.

I will check back with Marc how to actually merge those fixes in.

Cheers,
Andre.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/12] KVM: mark kvm-buses as empty once they were destroyed

2015-03-26 Thread Chen, Tiejun 

On 2015/3/27 9:31, Marcelo Tosatti wrote:

On Wed, Mar 25, 2015 at 05:09:13PM +, Marc Zyngier wrote:

On 23/03/15 15:58, Andre Przywara wrote:

In kvm_destroy_vm() we call kvm_io_bus_destroy() pretty early,
especially before calling kvm_arch_destroy_vm(). To avoid
unregistering devices from the already destroyed bus, let's mark
the bus with NULL to let other users know it has been destroyed
already.
This avoids a crash on a VM shutdown with the VGIC using the
kvm_io_bus later (the unregistering is in there to be able to roll
back a faulting init).

Signed-off-by: Andre Przywara andre.przyw...@arm.com


That seems sensible, but I don't see why nobody else hits that. What are
we doing differently?

Otherwise,

Reviewed-by: Marc Zyngier marc.zyng...@arm.com

Paolo, Marcelo, can we have your Ack on this?

Thanks,

M.


---
  virt/kvm/kvm_main.c |4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8c7ab0b..6f164eb 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -604,8 +604,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;


Could we fold this line into a common like,

@@ -596,7 +597,6 @@ static void kvm_destroy_devices(struct kvm *kvm)

 static void kvm_destroy_vm(struct kvm *kvm)
 {
-   int i;
struct mm_struct *mm = kvm-mm;

kvm_arch_sync_events(kvm);
@@ -604,8 +604,7 @@ 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++)
-   kvm_io_bus_destroy(kvm-buses[i]);
+   kvm_destroy_all_io_bus(kvm);
kvm_coalesced_mmio_free(kvm);
 #if defined(CONFIG_MMU_NOTIFIER)  defined(KVM_ARCH_WANT_MMU_NOTIFIER)
mmu_notifier_unregister(kvm-mmu_notifier, kvm-mm);
@@ -2943,6 +2942,16 @@ static void kvm_io_bus_destroy(struct kvm_io_bus 
*bus)

kfree(bus);
 }

+static void kvm_destroy_all_io_bus(struct kvm *kvm)
+{
+   int i;
+
+   for (i = 0; i  KVM_NR_BUSES; i++) {
+   kvm_io_bus_destroy(kvm-buses[i]);
+   kvm-buses[i] = NULL;
+   }
+}
+
 static inline int kvm_io_bus_cmp(const struct kvm_io_range *r1,
  const struct kvm_io_range *r2)
 {

Thanks
Tiejun
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/12] KVM: mark kvm-buses as empty once they were destroyed

2015-03-26 Thread Marcelo Tosatti 
On Wed, Mar 25, 2015 at 05:09:13PM +, Marc Zyngier wrote:
 On 23/03/15 15:58, Andre Przywara wrote:
  In kvm_destroy_vm() we call kvm_io_bus_destroy() pretty early,
  especially before calling kvm_arch_destroy_vm(). To avoid
  unregistering devices from the already destroyed bus, let's mark
  the bus with NULL to let other users know it has been destroyed
  already.
  This avoids a crash on a VM shutdown with the VGIC using the
  kvm_io_bus later (the unregistering is in there to be able to roll
  back a faulting init).
  
  Signed-off-by: Andre Przywara andre.przyw...@arm.com
 
 That seems sensible, but I don't see why nobody else hits that. What are
 we doing differently?
 
 Otherwise,
 
 Reviewed-by: Marc Zyngier marc.zyng...@arm.com
 
 Paolo, Marcelo, can we have your Ack on this?
 
 Thanks,
 
   M.
 
  ---
   virt/kvm/kvm_main.c |4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)
  
  diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
  index 8c7ab0b..6f164eb 100644
  --- a/virt/kvm/kvm_main.c
  +++ b/virt/kvm/kvm_main.c
  @@ -604,8 +604,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);
  
 
 
 -- 
 Jazz is not dead. It just smells funny...

Reviewed-by: Marcelo Tosatti mtosa...@redhat.com

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/12] KVM: mark kvm-buses as empty once they were destroyed

2015-03-25 Thread Marc Zyngier 
On 23/03/15 15:58, Andre Przywara wrote:
 In kvm_destroy_vm() we call kvm_io_bus_destroy() pretty early,
 especially before calling kvm_arch_destroy_vm(). To avoid
 unregistering devices from the already destroyed bus, let's mark
 the bus with NULL to let other users know it has been destroyed
 already.
 This avoids a crash on a VM shutdown with the VGIC using the
 kvm_io_bus later (the unregistering is in there to be able to roll
 back a faulting init).
 
 Signed-off-by: Andre Przywara andre.przyw...@arm.com

That seems sensible, but I don't see why nobody else hits that. What are
we doing differently?

Otherwise,

Reviewed-by: Marc Zyngier marc.zyng...@arm.com

Paolo, Marcelo, can we have your Ack on this?

Thanks,

M.

 ---
  virt/kvm/kvm_main.c |4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
 index 8c7ab0b..6f164eb 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -604,8 +604,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);
 


-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/12] KVM: mark kvm-buses as empty once they were destroyed

2015-03-25 Thread Marcelo Tosatti 
On Wed, Mar 25, 2015 at 05:09:13PM +, Marc Zyngier wrote:
 On 23/03/15 15:58, Andre Przywara wrote:
  In kvm_destroy_vm() we call kvm_io_bus_destroy() pretty early,
  especially before calling kvm_arch_destroy_vm(). To avoid
  unregistering devices from the already destroyed bus, let's mark
  the bus with NULL to let other users know it has been destroyed
  already.
  This avoids a crash on a VM shutdown with the VGIC using the
  kvm_io_bus later (the unregistering is in there to be able to roll
  back a faulting init).
  
  Signed-off-by: Andre Przywara andre.przyw...@arm.com
 
 That seems sensible, but I don't see why nobody else hits that. What are
 we doing differently?

It should be valid to call kvm_io_bus_unregister_dev after
kvm_io_bus_destroy.

Are you patching it to handle NULL kvm-buses[bus_idx] ?

 Otherwise,
 
 Reviewed-by: Marc Zyngier marc.zyng...@arm.com
 
 Paolo, Marcelo, can we have your Ack on this?
 
 Thanks,

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 06/12] KVM: mark kvm-buses as empty once they were destroyed

2015-03-23 Thread Andre Przywara 
In kvm_destroy_vm() we call kvm_io_bus_destroy() pretty early,
especially before calling kvm_arch_destroy_vm(). To avoid
unregistering devices from the already destroyed bus, let's mark
the bus with NULL to let other users know it has been destroyed
already.
This avoids a crash on a VM shutdown with the VGIC using the
kvm_io_bus later (the unregistering is in there to be able to roll
back a faulting init).

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
 virt/kvm/kvm_main.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8c7ab0b..6f164eb 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -604,8 +604,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);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html